Support 'Narrow to current compose box recipient'. #1194

srdeotarse · 2022-04-10T16:26:28Z

What does this PR do?
This PR adds support to 'Narrow to current compose box recipient' after sending a message outside current narrow.
Shortcut - ctrl+.

Fixes #1182

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

Tested?

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

Commit flow

Notes & Questions

Currently I have used shortcut - . instead of ctrl+. as ctrl+. doesn't seem to work in my local development environment (WSL2 Windows 10)

Interactions

Visual changes

neiljp

@srdeotarse This is a good start, thanks! 👍

This does appear to suffer from ctrl . not working for me too, which may be an issue with terminals in general. I'd suggest meta . (ie. alt .) in the meantime, unless/until we can find out why it is failing.

There is an issue where exiting compose seems to trigger going to all messages which we need to resolve before this can be merged. This appears to only happen if the new action is triggered and then the compose is cancelled/closed (or sent and then cancelled/closed).

Tests would also be good if possible.

neiljp · 2022-04-10T17:34:51Z

docs/hotkeys.md

@@ -54,6 +54,7 @@
 |Show/hide Emoji picker popup for current message|<kbd>:</kbd>|
 |Narrow to the stream of the current message|<kbd>s</kbd>|
 |Narrow to the topic of the current message|<kbd>S</kbd>|
+|Narrow to compose box message recipient.|<kbd>.</kbd>|


Good use of the script? :)

Definitely, you just have to change the key in keys.py and run the script to update hotkeys.md instead of changing manually. Really helpful.

neiljp · 2022-04-10T17:42:27Z

zulipterminal/helper.py

+            "Message is sent outside of current narrow. Press 'Ctrl .' to narrow to message recipient. {}".format(
+                NARROW_TO_MESSAGE_RECIPIENT,
+            )


We should programmatically fetch the key and substitute it in here.

I'm not sure about the use of the arrow here.

You had mentioned an UI element being proposed in #design > button to go to conversation. Should that arrow like symbol appear in compose box when messaging outside current narrow?

So, if we change the shortcut for narrowing to current message recipient, the key in the message should also change. Right?

The arrow issue is now at #1203.

zulipterminal/ui_tools/boxes.py

zulipbot · 2022-04-11T00:01:32Z

Hello @zulip/server-hotkeys members, this pull request was labeled with the "area: hotkeys" label, so you may want to check it out!

neiljp

@srdeotarse Thanks for the improved iteration, though it's still not quite performing right for me.

We've discussed this a little in the stream which should help with the next iteration.

You mentioned the hotkeys script; we don't lint for this and I'm not sure we have an issue for that, if you want to take that up. It would be useful in CI (and locally) to ensure that any changes are synchronized, and could be part of make fix too.

zulipterminal/ui_tools/boxes.py

zulipterminal/helper.py

zulipterminal/config/keys.py

neiljp · 2022-04-14T06:32:59Z

tests/helper/test_helper.py

-            "Message is sent outside of current narrow."
+            "Message is sent outside of current narrow. Press {} to narrow to message recipient. {}".format(
+                KEY_BINDINGS["NARROW_MESSAGE_RECIPIENT"]["keys"][0],
+                NARROW_TO_MESSAGE_RECIPIENT,


As per discussion in the stream, let's skip the icon for now; I think it'd be a representation of the key somehow if we did, a little like we have [P].

Again, this is now #1203

neiljp

@srdeotarse If we assume #1201 is reasonable, this PR seems functional 🎉

I've noted a few outstanding points, but otherwise this is close to merging.

We could add a test for the UI function, but the code is rather simple and UI code can be challenging to test and fragile.

neiljp · 2022-04-15T19:52:30Z

zulipterminal/config/keys.py

@@ -164,6 +164,11 @@ class KeyBinding(TypedDict, total=False):
        'help_text': 'Narrow to the topic of the current message',
        'key_category': 'msg_actions',
    }),
+    ('NARROW_MESSAGE_RECIPIENT', {
+        'keys': ['meta .'],
+        'help_text': 'Narrow to compose box message recipient.',


Note that other help_text doesn't end with a .

We may want to lint for this somewhere.

zulipterminal/helper.py

neiljp · 2022-04-15T20:04:01Z

zulipterminal/helper.py

+        key = primary_key_for_command("NARROW_MESSAGE_RECIPIENT")
+
+        controller.report_success(
+            f"Message is sent outside of current narrow. Press [{key}] to narrow to conversation."


If we keep this message this long, I'd welcome this being displayed a little longer (eg. the 6 we used in the other PR).

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

To address these combinations we may want to split this messaging change out into a followup commit and/or PR, since the functionality itself will be present and consistent with the help menu and documentation already in the rest of this commit. This is an extra hint, and it'd be good to handle it well.

neiljp · 2022-04-15T20:13:36Z

zulipterminal/ui_tools/boxes.py

@@ -773,6 +773,27 @@ def keypress(self, size: urwid_Size, key: str) -> Optional[str]:
                if self.msg_edit_state is not None:
                    self.keypress(size, primary_key_for_command("GO_BACK"))
                    assert self.msg_edit_state is None
+        elif is_command_key("NARROW_MESSAGE_RECIPIENT", key):
+            if not self.to_write_box:


These branches would be more readable using compose_box_status.

srdeotarse · 2022-04-16T05:38:49Z

@zulipbot add "PR needs review".

Fixes zulip#1182.

neiljp · 2022-04-17T02:18:59Z

@srdeotarse Thanks for updating this 👍 I adjusted the other conditional to use compose_box_status and will be merging shortly. This is going to be very helpful day to day 🎉

Follow up PR zulip#1194

These unrelated aspects being tied together was not an issue previously, when narrowing was always done outside of compose. However, with integration of zulip#1194 as 2195904 this is no longer the case, so this is required to ensure that narrowing does not prematurely end editor mode, since otherwise container widgets would steal keypresses. This may be improved later through further refactoring of the keypress handling.

These unrelated aspects being tied together was not an issue previously, when narrowing was always done outside of compose. However, with integration of #1194 as 2195904 this is no longer the case, so this is required to ensure that narrowing does not prematurely end editor mode, since otherwise container widgets would steal keypresses. This may be improved later through further refactoring of the keypress handling.

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

srdeotarse force-pushed the issue-1182-support-Narrow-to-current-compose-box-recipient branch from d4a0229 to 91e6939 Compare April 10, 2022 16:27

neiljp reviewed Apr 11, 2022

View reviewed changes

neiljp added area: hotkeys PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Apr 11, 2022

srdeotarse force-pushed the issue-1182-support-Narrow-to-current-compose-box-recipient branch from 91e6939 to 7689c1d Compare April 11, 2022 10:06

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

neiljp reviewed Apr 14, 2022

View reviewed changes

zulipbot added PR needs review PR requires feedback to proceed and removed PR needs review PR requires feedback to proceed labels Apr 14, 2022

srdeotarse force-pushed the issue-1182-support-Narrow-to-current-compose-box-recipient branch from 7689c1d to fbc1bb2 Compare April 15, 2022 18:18

neiljp mentioned this pull request Apr 15, 2022

views: Avoid sending GO_BACK to duplicate child widgets unnecessarily. #1201

Merged

4 tasks

neiljp reviewed Apr 15, 2022

View reviewed changes

srdeotarse force-pushed the issue-1182-support-Narrow-to-current-compose-box-recipient branch 3 times, most recently from c7270f7 to c6a0423 Compare April 16, 2022 03:55

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

helper/boxes/keys/hotkeys: Narrow to current compose box recipient.

1ad6e3a

Fixes zulip#1182.

neiljp force-pushed the issue-1182-support-Narrow-to-current-compose-box-recipient branch from c6a0423 to 1ad6e3a Compare April 17, 2022 02:17

neiljp added PR ready to be merged PR has been reviewed & is ready to be merged and removed PR needs review PR requires feedback to proceed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Apr 17, 2022

neiljp added this to the Next Release milestone Apr 17, 2022

neiljp merged commit 2195904 into zulip:main Apr 17, 2022

neiljp mentioned this pull request Apr 17, 2022

Show hint to narrow to compose recipients when it differs (before sending) #1203

Open

srdeotarse added a commit to srdeotarse/zulip-terminal that referenced this pull request Apr 17, 2022

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

f4df59c

Follow up PR zulip#1194

srdeotarse mentioned this pull request Apr 17, 2022

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

Open

4 tasks

neiljp mentioned this pull request Apr 18, 2022

bugfix: core: Do not exit editor mode when narrowing. #1206

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support 'Narrow to current compose box recipient'. #1194

Support 'Narrow to current compose box recipient'. #1194

srdeotarse commented Apr 10, 2022

neiljp left a comment

neiljp Apr 10, 2022

srdeotarse Apr 11, 2022 •

edited

Loading

neiljp Apr 10, 2022

srdeotarse Apr 11, 2022

srdeotarse Apr 11, 2022

neiljp Apr 17, 2022

zulipbot commented Apr 11, 2022

neiljp left a comment

neiljp Apr 14, 2022

neiljp Apr 17, 2022

neiljp left a comment

neiljp Apr 15, 2022

neiljp Apr 15, 2022

neiljp Apr 15, 2022

srdeotarse commented Apr 16, 2022

neiljp commented Apr 17, 2022

Support 'Narrow to current compose box recipient'. #1194

Support 'Narrow to current compose box recipient'. #1194

Conversation

srdeotarse commented Apr 10, 2022

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srdeotarse Apr 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zulipbot commented Apr 11, 2022

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srdeotarse commented Apr 16, 2022

neiljp commented Apr 17, 2022

srdeotarse Apr 11, 2022 •

edited

Loading