Display 'Message sent outside current narrow' when narrowing from all_messages and all_PMs. #1204

srdeotarse · 2022-04-17T04:21:30Z

Follow up PR #1194

What does this PR do?
This PR displays Message sent outside current narrow even if narrowing from all_messages or all_PMs

Tested?

Manually
Existing tests (adapted, if necessary)
New tests added (for any new behavior)
Passed linting & tests (each commit)

Commit flow

Notes & Questions

Interactions

CZO - https://chat.zulip.org/#narrow/stream/206-zulip-terminal/topic/.E2.9C.94.20Support.20'Narrow.20to.20current.20compose.20box.20recipient'.20.23T1194

Cases for showing success message - #1194 (comment)

Visual changes

srdeotarse · 2022-04-17T05:12:05Z

@zulipbot add "PR needs review".

neiljp · 2022-04-17T05:49:22Z

From #1194:

However, my remaining query here is whether we want to customize this logic/message a little further:
- in all-messages narrow, we never see this, but may still want to go to the conversation
- in stream (or all-PM) narrows which hold the message, we may also still want to go to the conversation
- if a message is outside the narrow, we always want to show this

I may not have been clear in this comment, but what I meant is that the two messages are not necessarily correlated since the conditions vary:

we still want the 'outside of narrow' behavior to stay the same
we want to also show the hint if not fully narrowed, ie. what the shortcut would do (though this could become too much, we'll see)

neiljp · 2022-04-17T05:52:30Z

zulipterminal/helper.py

@@ -664,7 +660,6 @@ def check_narrow_and_notify(
 def notify_if_message_sent_outside_narrow(
    message: Composition, controller: Any
 ) -> None:
-    current_narrow = controller.model.narrow


This appears a valid one-off tidy-up, but if it's not related to the bulk of the code then it's good to note it in the commit text body. More tidies => maybe it belongs in it's own commit.

srdeotarse · 2022-04-21T13:48:43Z

@zulipbot add "PR needs review".

neiljp · 2022-04-21T21:36:15Z

@srdeotarse Left messages in stream for discussion. Also please address comments in a PR via changes or comments, or discuss in stream and reach a conclusion, before requesting review again :)

Also applicable to title_write_box and to_write_box.

neiljp

@srdeotarse The extra commit here looks interesting, but I'd be inclined to split it out into another PR since it's really extending the "random" help to be more contextual.

The first commit doesn't quite address one of my original points; I referenced that inline, and it likely requires the test structure to be slightly updated to indicate the output. Hopefully you'll find that easier after working on that other PR.

neiljp · 2022-05-15T23:14:16Z

zulipterminal/ui_tools/boxes.py

+        self.check_stream_topic_edit_txt(self.check_stream_topic_edit_txt_flag)
+
+    @asynch
+    def check_stream_topic_edit_txt(


Minor perhaps, but this handles private cases too.

neiljp · 2022-05-15T23:15:37Z

zulipterminal/ui_tools/boxes.py

+            ("footer", "Help(?): "),
+            ("footer_contrast", f" {narrow_key} "),
+            ("footer", " Narrow to conversation."),


This styling (and below) is extracted from the other location. It would be better to use a common functionality.

neiljp · 2022-05-15T23:17:39Z

zulipterminal/ui_tools/boxes.py

+            ("footer_contrast", f" {send_key} "),
+            ("footer", " Send a message."),
+        ]
+        while check_stream_topic_edit_txt_flag:


We don't want to use a busy loop for this, if we do take this approach. This sends the zulip-term CPU usage to 100%+.

An urwid on-change signal may be better, though note that while potentially helpful, alt . will not (or shouldn't) do anything if the recipients aren't valid, so we can avoid showing the notice in that case.

neiljp · 2022-05-15T23:28:37Z

zulipterminal/helper.py

-        and current_narrow != outer_narrow
-        and current_narrow != inner_narrow
-    ):
+    if current_narrow != inner_narrow:


This is the correct condition for the narrowing hint, but please see #1204 (comment)

There are two separate conditions; one handles the 'outside of narrow' part of the message (doesn't need updating), another separate one handles the showing of the second (hint) part.

neiljp · 2022-05-15T23:52:02Z

tests/helper/test_helper.py

+            True,
            id="all_private__pm__not_notified",


We may need to update either the test or the ids, to represent what we're expecting here.

At the very least, this change is now inconsistent: True => notified :)

However, there are different aspects being notified here, so a boolean doesn't fully encapsulate the results any more.

zulipbot · 2023-02-24T21:17:42Z

Heads up @srdeotarse, we just merged some commits that conflict with the changes you 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.

zulipbot added the size: S [Automatic label added by zulipbot] label Apr 17, 2022

srdeotarse changed the title ~~Follow up - Support 'Narrow to current compose box recipient'.~~ Display 'Message sent outside current narrow' when narrowing from all_messages and all_PMs. Apr 17, 2022

zulipbot added the PR needs review PR requires feedback to proceed label Apr 17, 2022

neiljp reviewed Apr 17, 2022

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 Apr 17, 2022

srdeotarse force-pushed the issue-1182-followup-support-Narrow-to-current-compose-box-recipient branch from f4df59c to c4bba4f Compare April 20, 2022 16:25

zulipbot added the PR needs review PR requires feedback to proceed label Apr 21, 2022

neiljp removed the PR needs review PR requires feedback to proceed label Apr 21, 2022

srdeotarse force-pushed the issue-1182-followup-support-Narrow-to-current-compose-box-recipient branch from c4bba4f to e31d5af Compare May 5, 2022 17:13

zulipbot added size: M [Automatic label added by zulipbot] and removed size: S [Automatic label added by zulipbot] labels May 5, 2022

srdeotarse added the PR needs review PR requires feedback to proceed label May 5, 2022

helper: Show msg sent outside current narrow for all_msgs and all PMs.

455f2d7

srdeotarse force-pushed the issue-1182-followup-support-Narrow-to-current-compose-box-recipient branch from e31d5af to 4c48ed0 Compare May 11, 2022 17:59

zulipbot added size: L [Automatic label added by zulipbot] and removed size: M [Automatic label added by zulipbot] labels May 11, 2022

boxes: Show narrow to conversation when stream_write_box is not empty.

af52470

Also applicable to title_write_box and to_write_box.

srdeotarse force-pushed the issue-1182-followup-support-Narrow-to-current-compose-box-recipient branch from 4c48ed0 to af52470 Compare May 11, 2022 18:19

neiljp reviewed May 15, 2022

View reviewed changes

neiljp removed the PR needs review PR requires feedback to proceed label May 15, 2022

zulipbot added the has conflicts label Feb 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display 'Message sent outside current narrow' when narrowing from all_messages and all_PMs. #1204

Display 'Message sent outside current narrow' when narrowing from all_messages and all_PMs. #1204

srdeotarse commented Apr 17, 2022

srdeotarse commented Apr 17, 2022

neiljp commented Apr 17, 2022

neiljp Apr 17, 2022

srdeotarse commented Apr 21, 2022

neiljp commented Apr 21, 2022 •

edited

Loading

neiljp left a comment

neiljp May 15, 2022

neiljp May 15, 2022

neiljp May 15, 2022

neiljp May 15, 2022

neiljp May 15, 2022

zulipbot commented Feb 24, 2023

Display 'Message sent outside current narrow' when narrowing from all_messages and all_PMs. #1204

Are you sure you want to change the base?

Display 'Message sent outside current narrow' when narrowing from all_messages and all_PMs. #1204

Conversation

srdeotarse commented Apr 17, 2022

srdeotarse commented Apr 17, 2022

neiljp commented Apr 17, 2022

neiljp Apr 17, 2022

Choose a reason for hiding this comment

srdeotarse commented Apr 21, 2022

neiljp commented Apr 21, 2022 • edited Loading

neiljp left a comment

Choose a reason for hiding this comment

neiljp May 15, 2022

Choose a reason for hiding this comment

neiljp May 15, 2022

Choose a reason for hiding this comment

neiljp May 15, 2022

Choose a reason for hiding this comment

neiljp May 15, 2022

Choose a reason for hiding this comment

neiljp May 15, 2022

Choose a reason for hiding this comment

zulipbot commented Feb 24, 2023

neiljp commented Apr 21, 2022 •

edited

Loading