Skip to content
This repository has been archived by the owner on Jul 25, 2024. It is now read-only.

Show unread counts and add bankruptcy option. #430

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

Sam1301
Copy link
Member

@Sam1301 Sam1301 commented Mar 3, 2017

Summary of changes

  • shows unread counts

    • real-time sync with server using update_message_flags event
    • UI changes
  • bankruptcy menu option

    • fetch all messages in batches of 200
  • fetch 1000 messages on start-up (related discussion Fix null streams #283 (comment))

@timabbott
Copy link
Member

So the main potential downside of fetching 1000 messages on startup is performance when on a slow network; to what extent have you tested that? It might actually be fine with compression, but we should be sure :)

@Sam1301
Copy link
Member Author

Sam1301 commented Mar 3, 2017

I tested the difference between fetching 100 and 1000 messages. Here are the results:
For 100 messages (current behaviour)
for100msgs

For 1000 messages
for_1000msgs

these results were obtained for a 35Mbps connection... i think we could fetch 1000 messages in batches of 200 or so and display them simultaneously?

@timabbott
Copy link
Member

So I think with a 35Mbps connection, it'll be fine either way. For this, we want to test with a relatively slow connection.

We could do something simpler than doing 5 batches of 200, like fetch 200 at first (to show something) and then fetch the next 800. The thing that's really important to optimize here is the time-to-first-interaction, but there's not much reason to do 5 rounds over 2.

@Sam1301
Copy link
Member Author

Sam1301 commented Mar 3, 2017

ok got it, I'll update the pr with fetching 200 first and displaying it. Following this, in the background 800 messages can be fetched and added to the bottom of the list. During this time, the user will be able to interact with those 200 messages in the list. These 800 messages loaded in background could probably be fetched in a single call. Would that be fine?

@timabbott
Copy link
Member

Yep, fetching 800 in one call makes sense. FWIW the webapp does roughly that with 400 and 1000 as the parameters, so it's a pretty similar algorithm.

@@ -30,7 +30,7 @@
private static final String DATABASE_NAME = "zulip-%s.db";
// any time you make changes to your database objects, you may have to
// increase the database version
private static final int DATABASE_VERSION = 8;
private static final int DATABASE_VERSION = 9;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Database version is already updated here b596202

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saketkumar95 we need to update the database version whenever we make changes to the existing database model so that the existing tables are dropped and new tables with updated schema are created.

android:paddingBottom="16dp"
android:paddingLeft="16dp"
android:paddingRight="16dp"
android:paddingTop="16dp"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better if you put this all in dimens.xml. :)

@saketkumar
Copy link
Collaborator

@Sam1301 Really nice work! :) Resolve the merge conflicts too. 👍

@zulipbot
Copy link
Member

zulipbot commented May 4, 2017

@Sam1301, your pull request has developed a merge conflict! Please review the most recent commit (f791888) for conflicting changes and resolve your pull request's merge conflicts.

@zulipbot
Copy link
Member

zulipbot commented May 7, 2017

@Sam1301, your pull request has developed a merge conflict! Please review the most recent commit (470ade3) for conflicting changes and resolve your pull request's merge conflicts.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants