feature: Add Reactions to messages. #913

zee-bit · 2021-02-07T21:45:38Z

This PR packs a critical feature, that supports adding general reactions to messages. Few highlights of this feature:

A new EmojiPicker popup that activates from : hotkey.
Search for any emoji in the list through p hotkey.
A reaction count beside emoji's that users have reacted with.
Emoji list sorted in decreasing order of reaction count, and ascending order of emoji names(if reaction count is same).
A CHECK_MARK that indicates emoji's that the current user has reacted with.
Mouse support allows toggling reactions and scrolling through mouse events.

Working:

Thanks to @kaustubh-nair for his initial work in #707. I was able to pull some of his changes here, which simplified my work.

zulipterminal/ui_tools/buttons.py

zulipterminal/config/keys.py

zee-bit · 2021-02-10T06:23:08Z

@Ezio-Sarthak Thanks for your review. I'll update this PR after making the desired changes. 👍

Apart, from the code, I also request you to please check out the functionality, if you haven't already and report any bugs/anomalies you find, here or in CZO. This would be really helpful. 😅

zulipterminal/ui_tools/views.py

zee-bit · 2021-02-28T19:54:07Z

@zulipbot add "PR needs review"

zulipterminal/model.py

zulipterminal/ui_tools/views.py

mkp6781 · 2021-03-24T14:03:33Z

@zee-bit I'm leaving a few thoughts that came to my mind regarding the commit messages and commit structure.

1st commit: The commit title might need to be rephrased because we intend to "use" the emoji data in self.active_emoji_data. If I am not wrong, I think we use the term "fetch" in connection with obtaining data from the server, which is not the case here.

2nd commit[minor]: First letter of the commit message can be capitalized. :)

3rd commit: This commit has too many changes which makes it tough to be reviewed. So, I think it would be better if we can split it into 2 or 3 commits. Maybe the first one can define EmojiButton, the second one could define EmojiPickerView, and a 3rd one could add support for keys to display the pop up view.

Hope this helps. I am yet to go through the remaining 4 commits.

Ezio-Sarthak

@zee-bit I continue to love this feature, hence decided to give more focussed feedback 👍.

Thanks for splitting the commits, it helped me a lot in understanding the whole story in the PR :)

I see that we'd prefer to include the refactors in the initial commits, though we'd also try keeping it close to the one for which we made this refactor. The refactor commit for PopUpView (2nd commit) and the introduction of EmojiPickerView (7th commit) seems to be quite a gap. (I'm not sure if there were changes based on this refactor between the commits)

I like the intent of using CHECK_MARK for checking if a user has reacted with an emoji or not. This should be a good v0. We might also explore changing the background of emoji count for the same if that makes sense.

tests/model/test_model.py

zulipterminal/model.py

tests/ui_tools/test_buttons.py

zulipterminal/ui_tools/buttons.py

zulipterminal/ui_tools/boxes.py

zulipterminal/ui_tools/views.py

zulipterminal/model.py

Ezio-Sarthak · 2021-04-30T13:09:26Z

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

neiljp

@zee-bit After merging those model commits I rebased and pushed back the others for convenience. We discussed some points in the stream, and I've left some more inline.

The bugfix seems like a good early commit on the basis that the header/footer shouldn't scroll, so isn't related to fixing the behavior here but can be done first instead. With that and a few other changes as discussed I'm less concerned about the structure/flow at this point and looking forward to this 👍

zulipterminal/core.py

neiljp · 2021-08-11T20:55:08Z

zulipterminal/ui_tools/boxes.py

+        if not fixed_search_caption:
+            super().__init__(caption="", edit_text=self.search_text)


This is not being used anywhere? Everything defaults to True?

Ahh... This was changed recently when we decided to remove the search hint when the user presses p again. If we go by the current approach i.e. disappearing search hint, then I don't think we need this commit(?), but if we want to show the search caption when the user presses p, then this will be useful.

I am still convinced that we dont need to show the search hint when the user presses p, so I've dropped this commit and moved the "initial focus in the search-box" part to the emoji picker commit earlier.

tests/ui_tools/test_buttons.py

neiljp · 2021-08-11T21:02:13Z

tests/ui_tools/test_buttons.py

@@ -297,6 +303,7 @@ def test_keypress_emoji_button(
            controller=controller,
            emoji_unit=("+1", "1f44d", ["thumbs_up", "like"]),
            message=message_fixture,
+            reaction_count=1,


Do we know enough here to assert on the reaction count after?

We can't assert on the reaction_count currently, because of the below lambda functions. I can instead add case'es to the test in the initial EmojiButton commit and then assert on the reaction_count in this one, but that won't be "just a" keypress check anymore?

I've added tests for checking reaction_count as well. Let me know if that looks complex. :)

zulipterminal/ui_tools/buttons.py

zulipterminal/ui_tools/views.py

zulipterminal/ui_tools/buttons.py

tests/ui_tools/test_popups.py

This commit creates an EmojiButton class in buttons.py that will be used to define button objects for each emoji to contain their properties and can be passed to emoji picker view to render them. Tests added.

This commit adds hotkeys for opening/closing of emoji picker popup, and searching for emojis from the popup. The current proposed key for opening/closing emoji popup is ':'(colon), and for searching a particular emoji from the popup is 'p'. docs/hotkeys.md regenerated.

A new pop-up EmojiPickerView will be triggered on pressing ':' key on a message. This view will be used to search and select the emoji that a user wants to toggle (react/unreact with). The selection will be applied only after exiting the popup. This currently only provides a UI for adjusting the thumbs-up/+1 emoji, as currently achievable via `+`, but will be extended in future. A minimal search-box implemented on top of PanelSearchBox is added, which can be triggered on pressing 'p' key from Emoji picker view. The search logic will be added in subsequent commits, however currently only one emoji is in the list. Tests added. Co-authored-by: Zeeshan <equbalzeeshan@gmail.com>

This commit completes the logic of searching for emojis from the EmojiPickerView on pressing 'p' key. The search is kept dynamic for faster reacting to messages. The emoji list now also displays the full list of emoji's present in model.active_emoji_data. A few bugs related to displaying empty emoji list and searching in general have also been fixed. Tests added and amended.

A new reaction_count field is added to the EmojiButton class to keep in check the number of reactions with a particular emoji on a message. The EmojiPicker view also shows the count of reactions of a particular emoji beside emoji_name, and the list is sorted in descending order of emoji_count, then in ascending order of emoji_names if the count is equal. Tests amended.

This commit adds support for handling mouse_events from the emoji picker view, so that users can toggle reactions and scroll the view using their mouse. The mouse_event looks for left-click for toggling message reactions and up/down scroll to scroll through the list of emoji's from the picker. The commit is kept at the last since having mouse support is one of our least priorities for now, and providing basic functions bug-free should always come first. Tests added.

neiljp · 2021-08-17T05:32:23Z

@zee-bit Thanks for your work taking this to the finishing line, including the already-merged commits 👍
I'm merging now that the CI has passed 🎉

This took a little longer to review than I'd hoped, and things may be simpler to read if we make a few changes later, but this is a great feature and one that we've been looking forward to for some time :) The simpler logic we achieved at the end has definitely helped with readability 👍

I know there is plenty else to be getting on with, but it'd be great to discuss follow-up ideas in the stream - I know we discussed a few already, and we can perhaps write up a tracking issue to summarize afterwards. Another that came to mind is if one applies multiple reactions, then the changes already made aren't shown during subsequent searches, so 'saving' the reactions (esc/:) can cause unexpected results - maybe we could keep showing the to-be-changed reactions during a search?

zulipbot added the size: XL [Automatic label added by zulipbot] label Feb 7, 2021

Ezio-Sarthak reviewed Feb 10, 2021

View reviewed changes

zulipterminal/ui_tools/buttons.py Outdated Show resolved Hide resolved

Ezio-Sarthak reviewed Feb 10, 2021

View reviewed changes

zulipterminal/ui_tools/buttons.py Outdated Show resolved Hide resolved

Ezio-Sarthak reviewed Feb 10, 2021

View reviewed changes

zulipterminal/config/keys.py Show resolved Hide resolved

Ezio-Sarthak reviewed Feb 10, 2021

View reviewed changes

zulipterminal/ui_tools/views.py Outdated Show resolved Hide resolved

zee-bit force-pushed the MessageReaction-#T559 branch from b6b305a to 158ac4e Compare February 11, 2021 12:01

Ezio-Sarthak mentioned this pull request Feb 15, 2021

Wrong rendering of reaction removal if multiple already present #606

Open

zee-bit force-pushed the MessageReaction-#T559 branch 3 times, most recently from 179d6c3 to 4ad9330 Compare February 23, 2021 18:50

zee-bit changed the title ~~[WIP] feature: Add Reactions to messages.~~ feature: Add Reactions to messages. Feb 28, 2021

zulipbot added the PR needs review PR requires feedback to proceed label Feb 28, 2021

zee-bit force-pushed the MessageReaction-#T559 branch from 56d424f to 8bd8cbe Compare March 2, 2021 12:18

zee-bit force-pushed the MessageReaction-#T559 branch from 8bd8cbe to 75f93ce Compare March 20, 2021 15:37

mkp6781 reviewed Mar 24, 2021

View reviewed changes

zulipterminal/model.py Outdated Show resolved Hide resolved

mkp6781 reviewed Mar 24, 2021

View reviewed changes

zulipterminal/ui_tools/views.py Outdated Show resolved Hide resolved

zee-bit force-pushed the MessageReaction-#T559 branch from 75f93ce to 102cc1e Compare April 30, 2021 10:38

Ezio-Sarthak suggested changes Apr 30, 2021

View reviewed changes

zulipbot 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 30, 2021

zee-bit force-pushed the MessageReaction-#T559 branch from 102cc1e to e7c8c9f Compare May 1, 2021 09:04

zee-bit force-pushed the MessageReaction-#T559 branch from e7c8c9f to ade3ceb Compare May 25, 2021 12:41

zee-bit force-pushed the MessageReaction-#T559 branch 3 times, most recently from 5b51253 to fa5df75 Compare June 12, 2021 15:18

zee-bit removed the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Jun 12, 2021

neiljp reviewed Aug 11, 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 Aug 11, 2021

zee-bit force-pushed the MessageReaction-#T559 branch 2 times, most recently from e3c70db to 0b2d253 Compare August 12, 2021 22:25

zee-bit 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 Aug 12, 2021

zee-bit force-pushed the MessageReaction-#T559 branch 2 times, most recently from 881562d to 0654ff9 Compare August 14, 2021 20:16

neiljp reviewed Aug 15, 2021

View reviewed changes

zulipterminal/ui_tools/buttons.py Outdated Show resolved Hide resolved

zulipterminal/ui_tools/buttons.py Outdated Show resolved Hide resolved

zee-bit force-pushed the MessageReaction-#T559 branch from 0654ff9 to 9841acb Compare August 15, 2021 09:09

neiljp reviewed Aug 16, 2021

View reviewed changes

tests/ui_tools/test_popups.py Outdated Show resolved Hide resolved

zee-bit force-pushed the MessageReaction-#T559 branch from 9841acb to 3af2bd1 Compare August 16, 2021 06:45

zee-bit and others added 6 commits August 16, 2021 22:16

buttons: Add EmojiButton class for creating emoji button objects.

a7211dd

This commit creates an EmojiButton class in buttons.py that will be used to define button objects for each emoji to contain their properties and can be passed to emoji picker view to render them. Tests added.

neiljp force-pushed the MessageReaction-#T559 branch from 3af2bd1 to dc77603 Compare August 17, 2021 05:22

neiljp added this to the Next Release milestone Aug 17, 2021

neiljp added area: UI General user interface update missing feature: user A missing feature for all users, present in another Zulip client and removed further discussion required Discuss this on #zulip-terminal on chat.zulip.org PR needs review PR requires feedback to proceed labels Aug 17, 2021

neiljp merged commit 19f1396 into zulip:main Aug 17, 2021

This was referenced Aug 17, 2021

Support adding general reactions to messages #559

Closed

WIP: Add reactions to messages #707

Closed

zee-bit mentioned this pull request Aug 17, 2021

TRACKING: Emoji Picker popup improvements #1121

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: Add Reactions to messages. #913

feature: Add Reactions to messages. #913

zee-bit commented Feb 7, 2021 •

edited

Loading

zee-bit commented Feb 10, 2021

zee-bit commented Feb 28, 2021

mkp6781 commented Mar 24, 2021

Ezio-Sarthak left a comment

Ezio-Sarthak commented Apr 30, 2021

neiljp left a comment

neiljp Aug 11, 2021

zee-bit Aug 12, 2021 •

edited

Loading

zee-bit Aug 12, 2021

neiljp Aug 11, 2021

zee-bit Aug 12, 2021 •

edited

Loading

zee-bit Aug 12, 2021

neiljp commented Aug 17, 2021

		if not fixed_search_caption:
		super().__init__(caption="", edit_text=self.search_text)

feature: Add Reactions to messages. #913

feature: Add Reactions to messages. #913

Conversation

zee-bit commented Feb 7, 2021 • edited Loading

zee-bit commented Feb 10, 2021

zee-bit commented Feb 28, 2021

mkp6781 commented Mar 24, 2021

Ezio-Sarthak left a comment

Choose a reason for hiding this comment

Ezio-Sarthak commented Apr 30, 2021

neiljp left a comment

Choose a reason for hiding this comment

neiljp Aug 11, 2021

Choose a reason for hiding this comment

zee-bit Aug 12, 2021 • edited Loading

Choose a reason for hiding this comment

zee-bit Aug 12, 2021

Choose a reason for hiding this comment

neiljp Aug 11, 2021

Choose a reason for hiding this comment

zee-bit Aug 12, 2021 • edited Loading

Choose a reason for hiding this comment

zee-bit Aug 12, 2021

Choose a reason for hiding this comment

neiljp commented Aug 17, 2021

zee-bit commented Feb 7, 2021 •

edited

Loading

zee-bit Aug 12, 2021 •

edited

Loading

zee-bit Aug 12, 2021 •

edited

Loading