model: Add footer notification for messages. #824

zee-bit · 2020-11-13T12:40:20Z

This will set a footer text notifying the user everytime
he sends or edits a message that is outside the current
narrow. This will give the user a pointer as to why the
message disappeared from the screen.

Fixes #781.

neiljp

@zee-bit Thanks for giving this issue a go!

I just tried this out and it doesn't seem to work quite as I'd expect, in that it's not triggered in at least this example case:

narrow to test here > zt1
compose and send a message to test here > zt2

Note that this shows a message like we want to see, in the webapp.

It may be worth checking the webapp logic to confirm sending from/to all-messages/PMs/mentioned/streams/topics narrows.

You've left quite a lot of debugging statements in there - unless it's relevant to discussion points, it's better to leave those out in the version you push to a PR - you can always keep it in a branch locally and add it back in on a local branch, in a separate commit.

neiljp · 2020-11-13T17:14:56Z

zulipterminal/model.py

+                if not append_to_stream:
+                    self.controller.view.set_footer_text(
+                        'Message sent outside of current narrow', 3)


This is roughly the response we want to give to the user, but note that since this method handles all incoming new-message events then this will also be triggered by other people sending messages outside of the current narrow.

You could try updating the code here, or have it more directly use the methods which send the message to the server. They should subsequently trigger this method, so it's just doing a check at a different place.

I have updated the code, to fix the above issue as well as extended it for footer notification outside of streams. I have also amended the tests for model and now make check is working fine. I haven't removed debugging statements yet. But I'll do it as soon as the changes are finalized.

I didn't hook up to the methods which send message directly to the server, but I have the code for that in a separate branch locally. I can push that code here if this method does not work for you.

neiljp · 2020-11-13T17:17:08Z

zulipterminal/model.py

+                view = self.controller.view
+                print("event=>", event, flush=True)
+                if (len(self.narrow) == 2
+                    and self.narrow[1][0] == 'topic'
+                        and old_subject != new_subject):
+                    view.set_footer_text("Subject changed", 3)


Is this a different issue you're working on? If so, best to keep it to a separate commit. Unless it can't be separated, it also isolates which code change was required to fix the issue you're referring to in the commit message.

No, it's for the same issue. This was for the case when a message's subject is edited outside of the current narrow. So the message disappears from the current narrow and is shifted to some other narrow. But, I think the logic behind this is also incorrect. I'll have to think more on this. Any pointers that you would like to give?

This is sort of the same issue, but I think it would be fine to first work on showing a message when sending to a different narrow than the current one, then work on what happens with editing as a follow-up.

I think in both cases the focus is on what the active user does within this ZT session, so it's likely best to hook into the messages to the server, rather than events from it.

neiljp

@zee-bit This is definitely in the right area of the code now, but note that narrows are in general much more complex than this - including, for example, if I write a PM and I'm in a stream narrow (or vice versa).

Rather than spreading this logic around, I'd suggest making a method which you can test independently and then use in all the locations where we send/update. This could be useful for reasoning about other cases too.

The other aspect you've not considered here is error-handling - we shouldn't show anything if the message is not sent/updated successfully.

zee-bit · 2021-01-17T20:43:33Z

@zulipbot add "PR needs review"
@zulipbot remove "PR needs update"

neiljp

@zee-bit This seems to work mostly fine, though there are edge cases in terms of narrows that behave unexpectedly, which you might clarify with tests. I'm also wondering if a longer footer duration may be wise, though this may be simplified if we can use footer message 'themes'?

As per my message in zulip-terminal, #924 may be relevant, though don't feel a need to base this PR on it.

neiljp · 2021-02-15T00:31:01Z

zulipterminal/helper.py

@@ -651,6 +651,28 @@ def display_error_if_present(response: Dict[str, Any], controller: Any
        controller.view.set_footer_text(response['msg'], 3)


+def notify_if_message_sent_outside_narrow(request: Dict[str, Any],


request doesn't really have any meaning in this context.

How about message?

That's good, though the type will presumably be a Composition now?

For that, we would need to type-annotate our request dict(in model) to appropriate Composition?

I tried doing this, but it seems like MyPy doesn't allow converting existing dict to TypedDict(see mypy issue #8890). Is there any other way of doing so?

EDIT: I figured out another way - by annotating the request dict when creating it, before passing it to the API, and adding total=False param in StreamComposition because the stream_id field should not be added to the request.

tests/helper/test_helper.py

zulipterminal/helper.py

zulipterminal/model.py

This commit generalizes the code for footer notification when a message is sent/edited outside current narrow. This will give the user a pointer as to why the message disappeared from the screen. The code is hooked to the part where message request is sent to the server, hence preventing the extra check for user_id being same as message_sender_id. Tests amended. Fixes zulip#781.

This commit refactors the request dict's type to one of Composition class. Previously, the type of request was Dict[str, Any] which was flexible when it comes type-checking. This change narrows the scope of type-checking, since the Composition class uses TypedDict.

zee-bit · 2021-02-17T20:01:56Z

@zulipbot add "PR needs review"
@zulipbot remove "PR needs update"

zulipbot · 2021-02-18T06:53:35Z

Heads up @zee-bit, 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/main branch and resolve your pull request's merge conflicts accordingly.

neiljp · 2021-02-18T07:56:39Z

@zee-bit Merged manually as 3260e3a after making a few minor adjustments 🎉
(as agreed in the stream)

zulipbot added size: S [Automatic label added by zulipbot] enhancement New feature or request labels Nov 13, 2020

neiljp reviewed Nov 13, 2020

View reviewed changes

zee-bit force-pushed the issue_781 branch 2 times, most recently from d5444ed to 18982fa Compare November 17, 2020 06:47

neiljp added the PR needs review PR requires feedback to proceed label Nov 21, 2020

zee-bit force-pushed the issue_781 branch from 18982fa to 833b3ac Compare December 19, 2020 06:18

zulipbot added size: M [Automatic label added by zulipbot] and removed size: S [Automatic label added by zulipbot] labels Dec 19, 2020

zee-bit force-pushed the issue_781 branch 4 times, most recently from 2b77369 to 13f3f76 Compare January 8, 2021 16:34

neiljp reviewed Jan 12, 2021

View reviewed changes

neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels Jan 12, 2021

zee-bit force-pushed the issue_781 branch from 13f3f76 to 4b595cc Compare January 17, 2021 19:22

zulipbot added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Jan 17, 2021

zee-bit force-pushed the issue_781 branch from 4b595cc to fd30d4a Compare January 19, 2021 06:35

zulipbot added size: L [Automatic label added by zulipbot] and removed size: M [Automatic label added by zulipbot] labels Jan 19, 2021

zee-bit force-pushed the issue_781 branch 2 times, most recently from 8c22b1b to 52ead7c Compare January 28, 2021 08:01

neiljp added this to the Release after next milestone Jan 28, 2021

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

neiljp mentioned this pull request Jan 31, 2021

refactor:Extract function for message being in a narrow #902

Merged

zee-bit closed this Feb 13, 2021

zee-bit force-pushed the issue_781 branch from 52ead7c to 01fc24a Compare February 13, 2021 13:20

zulipbot added size: XS [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels Feb 13, 2021

zee-bit reopened this Feb 13, 2021

zulipbot added size: L [Automatic label added by zulipbot] and removed size: XS [Automatic label added by zulipbot] labels Feb 13, 2021

neiljp reviewed Feb 15, 2021

View reviewed changes

neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels Feb 15, 2021

zee-bit added 2 commits February 18, 2021 01:03

zee-bit force-pushed the issue_781 branch from c510ebc to 3624c33 Compare February 17, 2021 19:53

zulipbot added size: XL [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels Feb 17, 2021

zulipbot added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Feb 17, 2021

zulipbot added the has conflicts label Feb 18, 2021

neiljp closed this Feb 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

model: Add footer notification for messages. #824

model: Add footer notification for messages. #824

zee-bit commented Nov 13, 2020

neiljp left a comment

neiljp Nov 13, 2020

zee-bit Nov 14, 2020 •

edited

Loading

neiljp Nov 13, 2020

zee-bit Nov 14, 2020

neiljp Nov 14, 2020

neiljp left a comment

zee-bit commented Jan 17, 2021

neiljp left a comment

neiljp Feb 15, 2021

zee-bit Feb 16, 2021

neiljp Feb 17, 2021

zee-bit Feb 17, 2021 •

edited

Loading

zee-bit commented Feb 17, 2021

zulipbot commented Feb 18, 2021

neiljp commented Feb 18, 2021

		@@ -651,6 +651,28 @@ def display_error_if_present(response: Dict[str, Any], controller: Any
		controller.view.set_footer_text(response['msg'], 3)


		def notify_if_message_sent_outside_narrow(request: Dict[str, Any],

model: Add footer notification for messages. #824

model: Add footer notification for messages. #824

Conversation

zee-bit commented Nov 13, 2020

neiljp left a comment

Choose a reason for hiding this comment

neiljp Nov 13, 2020

Choose a reason for hiding this comment

zee-bit Nov 14, 2020 • edited Loading

Choose a reason for hiding this comment

neiljp Nov 13, 2020

Choose a reason for hiding this comment

zee-bit Nov 14, 2020

Choose a reason for hiding this comment

neiljp Nov 14, 2020

Choose a reason for hiding this comment

neiljp left a comment

Choose a reason for hiding this comment

zee-bit commented Jan 17, 2021

neiljp left a comment

Choose a reason for hiding this comment

neiljp Feb 15, 2021

Choose a reason for hiding this comment

zee-bit Feb 16, 2021

Choose a reason for hiding this comment

neiljp Feb 17, 2021

Choose a reason for hiding this comment

zee-bit Feb 17, 2021 • edited Loading

Choose a reason for hiding this comment

zee-bit commented Feb 17, 2021

zulipbot commented Feb 18, 2021

neiljp commented Feb 18, 2021

zee-bit Nov 14, 2020 •

edited

Loading

zee-bit Feb 17, 2021 •

edited

Loading