keymapping: Add new 'a' key for all messages. #805

adwait-thattey · 2020-10-05T05:09:20Z

A New Keymapping 'ALL_MESSAGES' is added.
This maps to 'a' key.

This replaces the 'GO_BACK' ('ESC') key currently
being used for displaying all messages.

Currently both 'GO_BACK' and 'ALL_MESSAGES' work.

Fixes #804

Signed-off-by: Adwait Thattey coderdude1999@gmail.com

Screenshot:

adwait-thattey · 2020-10-05T05:10:42Z

@neiljp As written above, both 'Esc' and 'a' are working right now and will display all messages. Please let me know if this needs to change.

Also, I could not find if any tests need to be added or updated.

neiljp

@adwait-thattey Thanks for working on this :) This gets a good way there according to the original issue 👍

While this does keep the original behavior, this is by hardcoding it as an option, whereas it is more flexible if we can rely upon extending the keybindings, ie. adding to the set of keys for a command.

I should note that there is limited space next to each 'button' to show one shortcut key, otherwise we could show both keys, which is why it's fine for the help and the [a] to differ.

The other thing mentioned in the issue is adding this to the README, as we have these hotkeys listed there too.

Regarding tests, this passes ones we currently have. If you do want to investigate improving them with this, you could check if there are any hard-coded 'esc' in the tests as that could indicate how they could be broadened to use your new ALL_MESSAGES token.

zulipterminal/config/keys.py

adwait-thattey · 2020-10-05T11:58:57Z

@neiljp I made the changes.
There might be one issue. As the keys are a set, in the top-left buttons, both 'a' or 'esc' can appear depending on which gets popped.

This may cause some indentation issues...

In all my test runs, always 'esc' appeared. But I don't think we can rely on that?
Is this fine?

neiljp · 2020-10-06T01:23:34Z

Just to note here: I responded elsewhere today that we should likely move to a list for the external interface for the keys, for more reproducible behavior generally - this would be a good commit to have before the existing one.

adwait-thattey · 2020-10-06T04:35:04Z

Yes. On it

adwait-thattey · 2020-10-08T19:35:22Z

@neiljp I have added 2 commits.
The first one changes behaviour to return a sorted list from the keys_for_command method and modifies tests by changing types there too

2nd one adds the 'a' key

neiljp

@adwait-thattey This looks good 👍 I've left some comments inline.

There's one mention of set in another test (docstring) which you could update.

My only other concern is that we do use keys_for_command in many places (git grep keys_for_command) and that something may fall through the gaps.

Once you've updated the commits, I'd suggest checking out gitlint and our style in git log, for matching our existing style, though we could also do this before merging.

neiljp · 2020-10-08T19:53:51Z

zulipterminal/config/keys.py

@@ -324,12 +324,12 @@ def is_command_key(command: str, key: str) -> bool:
        raise InvalidCommand(command)


-def keys_for_command(command: str) -> Set[str]:


In the commit text it would be useful to describe why this is useful.

The changes in this commit do start to resolve the ordering issue, but only via sorting - I suspect we'll want to follow up this PR with a larger migration of the internal data to allow specifying eg. the first entry in the keymap to be one that is shown in the UI.

zulipterminal/config/keys.py

zulipterminal/ui_tools/buttons.py

This is needed as set can return an arbitary element on pop() but in some instances we may require a particular key. Appropriate test methods are also modified

A New Keymapping 'ALL_MESSAGES' is added. This maps to 'a' key. This replaces the 'GO_BACK' ('ESC') key currently being used for displaying all messages. Currently both 'GO_BACK' and 'ALL_MESSAGES' work. Fixes #804

adwait-thattey · 2020-10-09T07:14:44Z

@neiljp I made the changes.
I searched through all the instances of keys_for_command but couldn't find any other place that may need change.
While many actions have multiple keys attached to them, only "All_MESSAGES" is used with pop(), needing to change to pop(0).

pop is used in many other places and I wonder if they will need to be changed to pop(0) too... considering if more than 1 keys are added to those actions later

neiljp · 2020-10-09T20:17:34Z

@adwait-thattey Thanks! I've merged these two commits with minor changes to the commit text 🎉
They're visible at 17ee342 and the previous commit.

neiljp · 2020-10-09T20:23:42Z

@adwait-thattey As I mentioned, a good follow-up would be to migrate the system to take a standard element (first, or last?) from lists of keys for each command (and document that in the file), and then ensure that the UI treats that in the correct way.

adwait-thattey · 2020-10-10T04:19:23Z

@neiljp Thanks a lot :)

As I mentioned, a good follow-up would be to migrate the system to take a standard element (first, or last?) from lists of keys for each command (and document that in the file), and then ensure that the UI treats that in the correct way.

Yes, I will try this. Should I open another issue for this?

neiljp · 2020-10-10T14:45:00Z

@adwait-thattey If you plan to work on it, you can just open a PR when you're ready :)

adwait-thattey · 2020-10-11T14:30:15Z

@neiljp I opened a followup PR #806 and made some comments there.
Please check
Thanks :)

neiljp reviewed Oct 5, 2020

View reviewed changes

zulipterminal/config/keys.py Outdated Show resolved Hide resolved

neiljp added area: documentation Requires changes in documentation enhancement New feature or request PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Oct 6, 2020

zulipbot added the size: S [Automatic label added by zulipbot] label Oct 8, 2020

neiljp removed the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Oct 8, 2020

neiljp reviewed Oct 8, 2020

View reviewed changes

neiljp added the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Oct 8, 2020

adwait-thattey added 2 commits October 9, 2020 12:37

keymapping: Return type of keys_for_command changed to sorted list.

ea2240a

This is needed as set can return an arbitary element on pop() but in some instances we may require a particular key. Appropriate test methods are also modified

keymapping: Add 'a' key for all messages.

286747d

A New Keymapping 'ALL_MESSAGES' is added. This maps to 'a' key. This replaces the 'GO_BACK' ('ESC') key currently being used for displaying all messages. Currently both 'GO_BACK' and 'ALL_MESSAGES' work. Fixes #804

neiljp added hacktoberfest-accepted This PR is considered valid for Hacktoberfest :) and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Oct 9, 2020

neiljp closed this Oct 9, 2020

neiljp added this to the Next Release milestone Oct 9, 2020

adwait-thattey mentioned this pull request Oct 11, 2020

keymapping: replace pop with prime_key method #806

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

keymapping: Add new 'a' key for all messages. #805

keymapping: Add new 'a' key for all messages. #805

adwait-thattey commented Oct 5, 2020

adwait-thattey commented Oct 5, 2020

neiljp left a comment

adwait-thattey commented Oct 5, 2020 •

edited

Loading

neiljp commented Oct 6, 2020

adwait-thattey commented Oct 6, 2020

adwait-thattey commented Oct 8, 2020

neiljp left a comment

neiljp Oct 8, 2020

adwait-thattey commented Oct 9, 2020

neiljp commented Oct 9, 2020

neiljp commented Oct 9, 2020

adwait-thattey commented Oct 10, 2020

neiljp commented Oct 10, 2020

adwait-thattey commented Oct 11, 2020

		@@ -324,12 +324,12 @@ def is_command_key(command: str, key: str) -> bool:
		raise InvalidCommand(command)


		def keys_for_command(command: str) -> Set[str]:

keymapping: Add new 'a' key for all messages. #805

keymapping: Add new 'a' key for all messages. #805

Conversation

adwait-thattey commented Oct 5, 2020

adwait-thattey commented Oct 5, 2020

neiljp left a comment

Choose a reason for hiding this comment

adwait-thattey commented Oct 5, 2020 • edited Loading

neiljp commented Oct 6, 2020

adwait-thattey commented Oct 6, 2020

adwait-thattey commented Oct 8, 2020

neiljp left a comment

Choose a reason for hiding this comment

neiljp Oct 8, 2020

Choose a reason for hiding this comment

adwait-thattey commented Oct 9, 2020

neiljp commented Oct 9, 2020

neiljp commented Oct 9, 2020

adwait-thattey commented Oct 10, 2020

neiljp commented Oct 10, 2020

adwait-thattey commented Oct 11, 2020

adwait-thattey commented Oct 5, 2020 •

edited

Loading