Sets current narrow marker for empty narrows. #278

sumanthvrao · 2019-01-26T11:32:16Z

This PR address setting labels in narrow's with no previous message. A dummy message is created for such narrows with Welcome-Bot as the author and a suitable content, This message gets discarded when the first message arrives. Further, it contains tests for the added features.

In streams with no previous messages.
In PM with no previous messages.

Fixes : #259, #266

neiljp

@sumanthvrao I've left some preliminary thoughts, though this is mostly on coding style rather than the approach to fixing the problem - though I do see it works in practice, based on manual testing.

I know you mentioned this doesn't work for muted streams, but it also doesn't for other types of general empty narrows, including eg. PMs (select a user in the right-hand list, for example).

I'm wondering if these changes can be split into smaller parts (and commits) which can then be connected in a later commit?

You might find adding some tests would make it clearer to yourself (and to reviewers) what features you're adding, as you can eg. add a parameter and a test for that function with the parameter, often alone in one commit.

I'm thinking we should show a 'special message' (ie. message 'content') rather than an empty message with a header, if we go this route; what do you think? (also @amanagr and anyone else)

zulipterminal/core.py

zulipterminal/ui_tools/boxes.py

zulipterminal/ui_tools/utils.py

.gitignore

neiljp

This is quite a large PR and most is in one commit, which makes it difficult to understand in one go. I've left some comments, but with some refactoring and addition of the features in multiple commits it should be easier to understand.

.gitignore

zulipterminal/ui_tools/utils.py

zulipterminal/model.py

zulipterminal/ui_tools/boxes.py

zulipterminal/ui_tools/utils.py

neiljp

Thanks for doing the work on splitting this - it's a lot more readable post-splitting 👍 If you can place the refactoring before the features/fixes, and keep the test changes with each code change, that will make this even more easily reviewable - ie. make the 'story' clearer :)

Note that you can have multiple lines in the text after the commit title, so you can eg. discuss the what/why you changed in one paragraph, and then have a blank line before another sentence, if those things are not related (like that you included test changes).

mypy.ini

CHANGELOG.md

zulipterminal/ui_tools/boxes.py

tests/ui/test_ui_tools.py

tests/model/test_model.py

zulipterminal/ui_tools/boxes.py

neiljp

@sumanthvrao Returning to this PR I remember how complex & involved it is!

I've done some manual testing, which uncovered that s or S from a private dummy message goes to all messages, which seems unexpected!

I'm a little unsure of the (new?) last commit (Prevent narrows from being marked 'M'); it looks like addressing a symptom (forcing the count up), rather than the problem (not increasing it in the first place). I'm not precisely sure what triggers this case (the commit message isn't entirely obvious) - is this set_count being called by Model.set_message_ids_as_read? If so, can we just fix the problem at source, which is probably in MessageView.read_message?

The commit flow looks pretty good, though I'm marginally wary of the 2nd commit being present before the parameters are added to create_msg_box_list, though I think we can't actually trigger that in the code at that point, perhaps? Either way, it seems strange adding an invalid call to a function before the function is updated to allow the extra parameters. Do you see my concern?

zulipterminal/ui_tools/utils.py

neiljp · 2019-04-24T23:31:45Z

zulipterminal/ui_tools/views.py

@@ -87,6 +90,9 @@ def load_new_messages(self, anchor: int) -> None:
        else:
            last_message = None

+        # This for the dummy message
+        if current_ids == set():
+            return


While the first part of this commit seems straightforward, I'm not sure this either solves anything major (I commented it out and couldn't easily trigger an exception) or is logically quite right?

The check here is after fetching messages, and we're just examining the current_ids, which could be empty if the index hasn't records for any stream/topic/etc.

As such, if we do include this check, I think this should definitely use the same approach as the first part - check for a log and look at the top message id.

I did try commenting it and it did not produce an exceptions. On the other hand, I seem to have forgotten why this was added in the first place. To be on the safe side, I followed the third approach here, mainly to rewrite the code similar to the first part - check for log; check for top message in the log; and check if it is a dummy message. Thanks for this 👍

tests/ui/test_ui_tools.py

neiljp · 2019-04-25T00:46:41Z

tests/ui/test_utils.py

-    return_value = create_msg_box_list(model, messages, focus_msg_id)
+    return_value = create_msg_box_list(model, messages, focus_msg_id,
+                                       stream_details=stream_details,
+                                       pm_details=pm_details)
    assert len(return_value) == len_w_list


Are there any extra asserts here that would be suitable, given the change in the test? We could maybe check for calls to MessageBox, which would test assumptions about the empty narrows?

I did add another assert here to check the number of times MessageBox is called. Further, tests have been added in previous commits specifically in test_main_view_generates_dummy_message which tests for assumptions in empty narrow.

tests/model/test_model.py

zulipterminal/model.py

sumanthvrao · 2019-05-27T11:48:01Z

@neiljp Thanks for patiently reviewing this again. I apologise for the delay. Here is a small CHANGELOG from the previous iteration:

's' and 'S' for dummy messages have been handled.
Extra assert has been added for commit Add primary features to Zulip-Terminal. #1 tests.
Extra assert to commit UI: Fix distracted borders due to UTF-8 encoding of messages. #4, to test no of times MessageBox is called.
Re-worded the code surrounding load_new_messages for dummy messages.
Added Ids for test - test_append_message_with_dummy_previous_message
The 'Append Message' is now much simpler. (model: Fix append message event for streams. #362, test_model: Add tests for append message event for streams. #364, model: Fix append message event for PMs. #365)
The last commit is a fix for a bug which can be reproduced by sending a message:

Login as Iago from web-app.
Login as Cordelia from ZT.
Ensure there is no convo between Cordelia and Iago. Dummy message should be present
Send a message from webapp to Cordelia.
Observe 'M' being marked on 'All Messages', 'All Private Messages', 'Iago user'.

Need clarification on :
Change requested for commit #2

The commit flow looks pretty good, though I'm marginally wary of the 2nd commit  ...

sumanthvrao · 2019-06-07T16:50:29Z

@neiljp I rebased this and resolved the conflicts. Its now good for a review. :)

Dummy messages are displayed in narrows with no previous messages. The action of HotKeys like 'r', 'c', 'z', 'quote', 'reactions' on dummy messages are different, as they don't have any defined 'context' (without a real message). Tests added.

This is necessary in boxes & buttons which act with the associated narrowing methods in core.py This addition is made use of in successive commit.

We send specific information through the function that makes accessing parameters more easier when building dummy message; Added parameters to create_msg_box function definition.

Handles 'Up' and 'Down' keys on dummy message; Otherwise it would raise an exception as there are neither old/new messages above/below the dummy message.

We use the passed parameters (recipient details, stream details) while building a dummy message with 'author' as Welcome Bot, 'topic' as the stream description. Tests added.

This commit handles the logic of replacing the dummy message when an incoming message, meant for the same narrow appears. * Ensure dummy message for stream and user PM is replaced when the first message in those respective narrows appear. * Ensure group PM messages do not replace dummy message in user PM narrow. * Ensure dummy message for self-message narrow is replaced. Tests added.

We mark new PMs which replace the dummy message as being 'unread'. This avoids marking 'All messages', 'All Private' and other PM narrows with 'M'.

zulipbot · 2020-04-17T00:46:29Z

Heads up @sumanthvrao, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

zulipbot added size: L bug Something isn't working labels Jan 26, 2019

sumanthvrao changed the title ~~Newstream~~ Sets current stream marker for empty streams. Jan 26, 2019

sumanthvrao force-pushed the newstream branch from 4efc9cc to 67c0123 Compare January 26, 2019 14:16

neiljp reviewed Jan 26, 2019

View reviewed changes

sumanthvrao commented Jan 27, 2019

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

sumanthvrao force-pushed the newstream branch 2 times, most recently from 3574948 to 333ddf0 Compare January 28, 2019 01:15

sumanthvrao changed the title ~~Sets current stream marker for empty streams.~~ [WIP] Sets current stream marker for empty streams. Jan 28, 2019

sumanthvrao force-pushed the newstream branch from 333ddf0 to 87835fb Compare January 31, 2019 02:37

sumanthvrao changed the title ~~[WIP] Sets current stream marker for empty streams.~~ [WIP] Sets current narrow marker for empty narrows. Jan 31, 2019

sumanthvrao force-pushed the newstream branch 3 times, most recently from a9a7398 to 3174de5 Compare January 31, 2019 18:05

zulipbot added size: XL and removed size: L labels Jan 31, 2019

sumanthvrao force-pushed the newstream branch 2 times, most recently from 814eb2f to 2b0b628 Compare February 1, 2019 15:01

sumanthvrao changed the title ~~[WIP] Sets current narrow marker for empty narrows.~~ Sets current narrow marker for empty narrows. Feb 1, 2019

neiljp reviewed Feb 3, 2019

View reviewed changes

sumanthvrao force-pushed the newstream branch 9 times, most recently from d2a11ae to 24ff1a8 Compare February 7, 2019 02:46

neiljp reviewed Feb 7, 2019

View reviewed changes

neiljp reviewed Apr 25, 2019

View reviewed changes

sumanthvrao force-pushed the newstream branch 2 times, most recently from 8d831c9 to ca49966 Compare May 27, 2019 10:49

sumanthvrao force-pushed the newstream branch from ca49966 to c069779 Compare May 27, 2019 11:48

sumanthvrao force-pushed the newstream branch from c069779 to 06e5b77 Compare June 7, 2019 16:49

sumanthvrao force-pushed the newstream branch from 06e5b77 to 30ffb0c Compare July 8, 2019 12:02

neiljp mentioned this pull request Aug 3, 2019

Ensure all-messages narrow is not empty via is-private/unmuted-streams #446

Draft

zulipbot added the has conflicts label Aug 13, 2019

sumanthvrao force-pushed the newstream branch from 30ffb0c to 3caa5d7 Compare August 15, 2019 05:51

zulipbot removed the has conflicts label Aug 15, 2019

sumanthvrao added 5 commits August 15, 2019 12:13

boxes/buttons: Add user_name field to UserButton.

abc2f03

This is necessary in boxes & buttons which act with the associated narrowing methods in core.py This addition is made use of in successive commit.

core/utils: Changed function calls to create_msg_box.

225af29

We send specific information through the function that makes accessing parameters more easier when building dummy message; Added parameters to create_msg_box function definition.

views: Handles up and down keys on dummy messages.

d7e635a

Handles 'Up' and 'Down' keys on dummy message; Otherwise it would raise an exception as there are neither old/new messages above/below the dummy message.

utils: Build dummy message using parameters passed to create_msg_box.

a2b5738

We use the passed parameters (recipient details, stream details) while building a dummy message with 'author' as Welcome Bot, 'topic' as the stream description. Tests added.

sumanthvrao force-pushed the newstream branch from 3caa5d7 to 73f00d0 Compare August 15, 2019 06:44

sumanthvrao and others added 2 commits August 15, 2019 12:23

helper: Prevent narrows from being marked 'M'.

c9932a4

We mark new PMs which replace the dummy message as being 'unread'. This avoids marking 'All messages', 'All Private' and other PM narrows with 'M'.

sumanthvrao force-pushed the newstream branch from 73f00d0 to c9932a4 Compare August 15, 2019 06:56

sumanthvrao mentioned this pull request Aug 15, 2019

refactor: core: Use message.recipients_emails during narrow_to_user. #461

Closed

zulipbot added the has conflicts label Apr 17, 2020

neiljp mentioned this pull request Apr 21, 2020

Handle narrowing to empty streams in a better way #607

Open

neiljp mentioned this pull request Jun 20, 2020

[WIP] panel_search_box: Enter on empty string now cancels the search. #699

Closed

neiljp mentioned this pull request Jul 22, 2020

Invisibile selection bar on exiting search #580

Open

Base automatically changed from master to main January 30, 2021 20:30

neiljp mentioned this pull request May 1, 2021

Improve dynamic message loading behavior #1009

Open

neiljp force-pushed the main branch from f9f483a to 793e73d Compare December 15, 2021 21:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sets current narrow marker for empty narrows. #278

Sets current narrow marker for empty narrows. #278

sumanthvrao commented Jan 26, 2019 •

edited

neiljp left a comment

neiljp left a comment

neiljp left a comment

neiljp left a comment

neiljp Apr 24, 2019

sumanthvrao May 23, 2019

neiljp Apr 25, 2019

sumanthvrao May 23, 2019 •

edited

sumanthvrao commented May 27, 2019 •

edited

sumanthvrao commented Jun 7, 2019

zulipbot commented Apr 17, 2020

Sets current narrow marker for empty narrows. #278

Are you sure you want to change the base?

Sets current narrow marker for empty narrows. #278

Conversation

sumanthvrao commented Jan 26, 2019 • edited

neiljp left a comment

Choose a reason for hiding this comment

neiljp left a comment

Choose a reason for hiding this comment

neiljp left a comment

Choose a reason for hiding this comment

neiljp left a comment

Choose a reason for hiding this comment

neiljp Apr 24, 2019

Choose a reason for hiding this comment

sumanthvrao May 23, 2019

Choose a reason for hiding this comment

neiljp Apr 25, 2019

Choose a reason for hiding this comment

sumanthvrao May 23, 2019 • edited

Choose a reason for hiding this comment

sumanthvrao commented May 27, 2019 • edited

sumanthvrao commented Jun 7, 2019

zulipbot commented Apr 17, 2020

sumanthvrao commented Jan 26, 2019 •

edited

sumanthvrao May 23, 2019 •

edited

sumanthvrao commented May 27, 2019 •

edited