New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #131: Wrong messages order #142

Merged
merged 3 commits into from Aug 23, 2016

Conversation

Projects
None yet
4 participants
@kunall17
Collaborator

kunall17 commented Aug 21, 2016

Currently this was adding messages before another MessageHeaderParent now this thing is fixed and comparing ID's now!

Commit 2ef8d48: Shows all the messages of today at the bottom as new messages even if the app pointer is greater than this messageId.

The code used in creating espresso test were very useful I used them to debug the order + if they are included in the correct messageHeaders.

I have created a temporary patch for espresso code just for debugging purpose of this branch what this does is it invokes both the methods to check the message order according to ID's and second one checks if the messages are inserted below the correct messageHeader.
This works if we go to overflow menu and press the Refresh menuItem.
If throws some errors in the log then it means there are still bugs! else it will print the information about the Id's and headerParents ID's

@smarx

This comment has been minimized.

smarx commented Aug 21, 2016

Automated message from Dropbox CLA bot

@kunall17, it looks like you've already signed the Dropbox CLA. Thanks!

@kunall17 kunall17 changed the title from Fixes #132: Wrong messages order to Fixes #131: Wrong messages order Aug 22, 2016

@niftynei

This comment has been minimized.

Contributor

niftynei commented Aug 22, 2016

Hey @kunall17, this seems to work for messages sent today, but any past dates are out of order across message thread boundaries.

In this screenshot, dates are out of order.
screenshot_20160822-143554

In this screenshot, times are out of order.
screenshot_20160822-143655

@koziodigitalinc

This comment has been minimized.

Contributor

koziodigitalinc commented Aug 22, 2016

@niftynei Yeah this is what my concern was while reviewing this pull request. I pulled down the branch to examine the the code.

@kunall17

This comment has been minimized.

Collaborator

kunall17 commented Aug 22, 2016

Will look into this!

@kunall17

This comment has been minimized.

Collaborator

kunall17 commented Aug 22, 2016

This thing even confused me, well the branch is working fine and does what it is coded to be!

In the second screenshot you can see the first message which is at "Aug 20, 06:25" has been added.
After that a new message (timestamp "Aug 20, 06:30") with different messageHeader has been added now after that the two messages at "Aug 20, 08:34" have been added to same old messageHeader this way the old messages are displayed, which is causing the confusion!

@niftynei

This comment has been minimized.

Contributor

niftynei commented Aug 22, 2016

hi @kunall17. thanks for looking into this. the coded functionality, even if working as intended, is incorrect. The order which messages are displayed should exactly mirror that of the web app -- where all messages are globally ordered by their timestamp, or ID. Right now the app re-arranges messages by 'topic' -- putting them out of order 'globally'.

@kunall17

This comment has been minimized.

Collaborator

kunall17 commented Aug 23, 2016

@niftynei Fixed this branch, now this has the same message order as the web UI.
Sorry for the confusion!

@niftynei niftynei merged commit 192414d into zulip:master Aug 23, 2016

@niftynei

This comment has been minimized.

Contributor

niftynei commented Aug 23, 2016

Thanks @kunall17 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment