Fix message updating bugs due to found_newest #711

kaustubh-nair · 2020-07-07T15:04:15Z

This PR fixes an update message bug which is caused by two different sources.

When a stream is not fully loaded, Model.found_newest is set toFalse. Now if a message is received on other streams which are fully loaded, it won't be appended to the view due to found_newest state. This is fixed by maintaining found_newest for each narrow.
On switching between narrows, found_newest is set to False in the Controller. This obviously prevents appending of new messagea in the narrow, unless down is pressed.

Similar bugs are present (now fixed) for sending messages, since they too use the same Model._handle_message_event for adding messages to the view.

neiljp

@kaustubh-nair This has the potential to be a good follow-up to the fix-with-unfortunate-side-effect of #670, and seems to resolve the problem we discussed in PMs 👍 I'd like others to test this too, to ensure we've fixed the bug, though the code looks like a good solution.

It would be useful if you could describe PRs a little more when submitting, as particularly with no corresponding issue then it's not going to be clear what it resolves or why, without diving into the commits. This is a little more writing, but it needn't be too much, and it helps give an overview. For 1-commit PRs, github puts in the commit text (which can always be adjusted to note extra points!) - similarly for multi-commit PRs consider reviewing the commit text(s) you have and giving a brief summary.

zulipterminal/model.py

neiljp · 2020-07-07T21:51:14Z

zulipterminal/model.py

@@ -741,7 +743,8 @@ def _handle_message_event(self, event: Event) -> None:
        if 'read' not in message['flags']:
            set_count([message['id']], self.controller, 1)

-        if hasattr(self.controller, 'view') and self.found_newest:
+        if(hasattr(self.controller, 'view')


Spacing?

You did this elsewhere; are you trying to avoid the condition and code block aligning?

I forgot the spacing.
Code block aligning?

See how and and if get aligned if you add the space.

if (hasattr(self.controller, 'view') and self.found_newest[repr(self.narrow)]): if self.msg_list.log:

The alignment triggers a linting error which could be fixed with either:

if (hasattr(self.controller, 'view') and self.found_newest[repr(self.narrow)]): if self.msg_list.log:

or:

if ( hasattr(self.controller, 'view') and self.found_newest[repr(self.narrow)] ): if self.msg_list.log:

Ah, I didn't know that thanks 👍

neiljp · 2020-07-07T22:06:00Z

zulipterminal/core.py

@@ -187,7 +187,7 @@ def _narrow_to(self, button: Any, anchor: Optional[int],
        if already_narrowed:
            return

-        self.model.found_newest = False
+        self.model.found_newest[repr(narrow)] = False


I understand this is just an update to existing code, but it would be great if we could treat found_newest as an attribute private to the Model; if we could do that refactor first, this commit would also be smaller and cleaner.

Directly setting data in another class like this is something I'm always open to removing, as it typically leads to simplification and removes the need to check between classes when updating code - and this is a small repo!

preetmishra

@kaustubh-nair Thanks for pushing these improvements. I tested the changes locally and it does seem to resolve the bug that we had discussed in the PM. Moreover, the per narrow found_newest logic seems reasonable. 👍

Other than what @neiljp has pointed in his review, the changes look good to me.

zulipterminal/model.py

neiljp

@kaustubh-nair You have roughly the right idea about the refactor, but I think we can simplify it by just calling the new method (or setting values directly) inside the model.

Thanks for updating the PR text. I think it's correct to clarify that the unread count goes up, but the message(s) isn't (aren't) added to the message list?

zulipterminal/model.py

neiljp · 2020-07-08T23:02:42Z

tests/model/test_model.py

@@ -652,7 +652,7 @@ def test__stream_info_from_subscriptions(self, initial_data, streams,

    def test__handle_message_event_with_Falsey_log(self, mocker,
                                                   model, message_fixture):
-        model.found_newest = True
+        model.set_found_newest(True)


Note my other comment about whether we need this function, but it may be worth investigating clarifying these tests, perhaps adding notes why these are set, or changing the test names to be clearer.

neiljp · 2020-07-08T23:38:21Z

zulipterminal/model.py

@@ -141,6 +141,9 @@ def is_search_narrow(self) -> bool:
        """
        return 'search' in [subnarrow[0] for subnarrow in self.narrow]

+    def set_found_newest(self, found_newest: bool) -> None:
+        self.found_newest = found_newest


When requiring a method to access data, or just hiding it in an object/class, I find it useful to have an underscore prefix. That both gives a visual indication elsewhere if we're touching something we shouldn't be (ie. potentially hacky code), but also in making this kind of transition it's a variable rename, so anything that depended upon Model.found_newest would fail with a name error.

sumanthvrao

@kaustubh-nair This is a much-appreciated fix! Thanks. 👌

Left a couple of comments but besides that, I tested manually and don't encounter the original issue anymore.

tests/model/test_model.py

zulipterminal/model.py

kaustubh-nair · 2020-07-12T17:16:20Z

Thanks for the review @sumanthvrao - PR updated

neiljp

@kaustubh-nair This took a while to review, but as you know there were multiple bugs - and it's great to have the ways to reproduce the bugs prior to those commits! This seems to be working great, thanks for following this up! If you can tidy up your commit titles (areas, ie files), which could also be simplified if two commits were reordered, this will be good to go 👍 (there are also some typos in the commit text)

neiljp · 2020-07-15T01:41:04Z

zulipterminal/core.py

@@ -159,7 +159,7 @@ def search_messages(self, text: str) -> None:
        self.model.index['search'].clear()
        self.model.set_search_narrow(text)

-        self.model.found_newest = False
+        self.model._have_last_message = False


This commit claims just to be model, but it also affects core... however if you switch the order of the next commit and this one, you'll remove the core lines first and this commit will be just fine (and smaller)

neiljp · 2020-07-15T01:42:04Z

zulipterminal/core.py

@@ -159,7 +159,6 @@ def search_messages(self, text: str) -> None:
        self.model.index['search'].clear()
        self.model.set_search_narrow(text)

-        self.model._have_last_message = False


See my related comment, but you didn't update the commit title here either - though if you reorder this will become just core:

tests/model/test_model.py

repr() is safer to use for generating string representations since it guarantees that it will yield an object with the same value when passed to eval(). It is designed to be unambiguous, while str() is designed for readability. str() leads to problems while using quotes, datetime objects, etc. Here, the string representation of model.narrow is used for storing the pointer in the index. There is no visible change in the string representation, but this refactor ensure future bugs should not arise due to this.

If found_newest is set to False on changing narrow, then new messages will never be appending to that narrow. This is prevented by the check in Model._handle_message_event(), whose purpose is to prevent the appending of messages if the latest one isn't loaded yet for pagination. The only way that found_newest gets reset is in Model.get_messages() where its value is returned by the server. To reproduce this (now fixed) bug, * Open stream A. * Open stream B. * Open stream A. * Receive messages in stream A - which now won't be displayed.

Rename found_newest to _have_last_message which is more descriptive. Also, an underscore is included to indicate it is a private attribute.

Currently, _have_last_message is not stored per-narrow. This leads to message loading bugs when switching between narrows, where the previous narrow was not fully loaded. Tests amended. To reproduce this (now fixed) bug, * Go to stream A which has been fully loaded. (By pressing `down`) _have_last_message is set to True * Go to stream B and press `down` but B is still not fully loaded. Hence, _have_last_message is set to False * Go to stream A and receive messages - now won't be added to the stream view.

kaustubh-nair · 2020-07-15T16:53:36Z

Updated 👍

neiljp · 2020-07-15T22:22:55Z

@kaustubh-nair Thanks for your attention to this, it will be good to have these bugs fixed :+1 Merging now 🎉

zulipbot added the size: L [Automatic label added by zulipbot] label Jul 7, 2020

neiljp added this to the 0.5.2 milestone Jul 7, 2020

neiljp requested review from preetmishra and sumanthvrao July 7, 2020 22:16

neiljp reviewed Jul 7, 2020

View reviewed changes

preetmishra reviewed Jul 8, 2020

View reviewed changes

kaustubh-nair force-pushed the update-message-found-newest-bug branch from 47bede5 to 1d1f2ef Compare July 8, 2020 15:28

kaustubh-nair commented Jul 8, 2020

View reviewed changes

zulipterminal/model.py Outdated Show resolved Hide resolved

kaustubh-nair force-pushed the update-message-found-newest-bug branch from 1d1f2ef to 2fcefce Compare July 8, 2020 16:15

neiljp reviewed Jul 8, 2020

View reviewed changes

kaustubh-nair force-pushed the update-message-found-newest-bug branch 4 times, most recently from 210b55a to d77d227 Compare July 10, 2020 16:39

zulipbot added size: M [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels Jul 10, 2020

kaustubh-nair changed the title ~~Store found_newest for each narrow~~ Fix message updating bugs due to found_newest Jul 10, 2020

kaustubh-nair force-pushed the update-message-found-newest-bug branch from d77d227 to 8b4f797 Compare July 11, 2020 06:48

sumanthvrao reviewed Jul 12, 2020

View reviewed changes

tests/model/test_model.py Outdated Show resolved Hide resolved

zulipterminal/model.py Outdated Show resolved Hide resolved

kaustubh-nair force-pushed the update-message-found-newest-bug branch from 8b4f797 to f95f0c8 Compare July 12, 2020 17:15

kaustubh-nair added the PR needs review PR requires feedback to proceed label Jul 13, 2020

neiljp reviewed Jul 15, 2020

View reviewed changes

neiljp removed the PR needs review PR requires feedback to proceed label Jul 15, 2020

kaustubh-nair added 4 commits July 15, 2020 21:31

model: refactor: Rename Model.found_newest to a private attribute.

d01ec43

Rename found_newest to _have_last_message which is more descriptive. Also, an underscore is included to indicate it is a private attribute.

kaustubh-nair force-pushed the update-message-found-newest-bug branch 2 times, most recently from a277d75 to 541dc69 Compare July 15, 2020 16:48

kaustubh-nair added the PR needs review PR requires feedback to proceed label Jul 15, 2020

neiljp merged commit 374df5a into zulip:master Jul 15, 2020

Uh oh!

Fix message updating bugs due to found_newest #711

Fix message updating bugs due to found_newest #711

Uh oh!

Conversation

kaustubh-nair commented Jul 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neiljp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

preetmishra Jul 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

preetmishra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

neiljp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sumanthvrao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kaustubh-nair commented Jul 12, 2020

Uh oh!

neiljp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kaustubh-nair commented Jul 15, 2020

Uh oh!

neiljp commented Jul 15, 2020

Uh oh!

Uh oh!

kaustubh-nair commented Jul 7, 2020 •

edited

Loading

preetmishra Jul 8, 2020 •

edited

Loading