Render Spoilers in ZT #1529

rsashank · 2024-07-02T04:27:56Z

What does this PR do, and why?

Add support for rendering spoilers in messages:

Add styles for the spoiler header in themes.
Add spoiler processing to soup2markup.
Trim spoiler content to 10 characters, appending ... to ensure the table doesn't break.
Add SpoilerButton class, append spoiler buttons to the message info view.
Add SpoilerView class to view spoiler content.
Esc and Enter returns to the message info popup; i returns to the message view.

External discussion & connections

Discussed in #zulip-terminal in Spoilers! #T688 #T1061 #T1173 #T1529
Fully fixes Add support for spoilers #688
Partially fixes issue #
Builds upon previous unmerged work in PR WIP: Feature: Add Spoiler support #1061
Is a follow-up to work in PR #
Requires merge of PR #
Merge will enable work on #

How did you test this?

Manually - Behavioral changes
Manually - Visual changes
Adapting existing automated tests
Adding automated tests for new behavior (or missing tests)
Existing automated tests should already cover this (only a refactor of tested code)

Self-review checklist for each commit

It is a minimal coherent idea
It has a commit summary following the documented style (title & body)
It has a commit summary describing the motivation and reasoning for the change
It individually passes linting and tests
It contains test additions for any new behavior
It flows clearly from a previous branch commit, and/or prepares for the next commit

Visual changes

Rendered Message	Message Information Popup	Spoiler View

rsashank · 2024-07-02T04:29:22Z

Still working on this; I've created a draft PR if anyone wants to give feedback on it so far. :)

This commit: * adds styles for spoiler header in themes * adds markup entry in `soup2markup` for spoilers. Tests added. Co-authored-by: Preet Mishra <[email protected]> Co-authored-by: Ezio-Sarthak <[email protected]>

This commit handles content metadata in `soup2markup` for spoilers. Test amended. Co-authored-by: Preet Mishra <[email protected]> Co-authored-by: Ezio-Sarthak <[email protected]>

This commit adds popup to show hidden spoiler content. Tests added. Co-authored-by: Preet Mishra <[email protected]> Co-authored-by: Ezio-Sarthak <[email protected]>

Tests amended. Co-authored-by: Preet Mishra <[email protected]> Co-authored-by: Ezio-Sarthak <[email protected]>

This commit adds a button that would allow user to view the spoiler content, i.e., onclick of button is SpoilerView popup. Tests added. Co-authored-by: Preet Mishra <[email protected]> Co-authored-by: Ezio-Sarthak <[email protected]>

This commit appends the SpoilerButton to message info view if present. Co-authored-by: Preet Mishra <[email protected]> Co-authored-by: Ezio-Sarthak <[email protected]>

This commit: * Pass message info data to SpoilerButton and show_spoiler method. * Pass message info data to SpoilerView and return to message info view popup upon `Esc` and `Enter`. * Extract spoiler buttons processing to a create_spoiler_buttons. Tests amended.

This commit sets show_footlink to False for links in spoiler blocks.

This commit: * Adds a boolean value spoiler_link to metadata, indicating whether a link is part of spoiler content. * Adds [spoiler] to link caption if spoiler_link is True. Fixes zulip#688.

neiljp · 2024-07-16T23:48:49Z

zulipterminal/ui_tools/messages.py

+                # Limit to the first 10 characters and append "..."
+                if len(processed_header_text) > 10:
+                    processed_header_text = processed_header_text[:10] + "..."


If you have a literal like 10, it's almost always useful to give it a name, particularly when it's referenced in multiple locations like here. Other than being descriptive, it ensures everything gets updated at the same time, which reduces one bug location.

Niloth-p · 2024-07-18T15:12:58Z

I tested this.
It looks pretty cool. I really like the green "Spoiler:" text in zt_dark and zt_blue.
Looks fine in gruvbox_dark. I tried testing the light themes, but I gave up when my eyes started hurting.
I see that you've chosen to use one of the existing colors for all the themes, did you try out any other colors?

When quoting spoilers, the lines are offset (as expected).
Just mentioning this because I didn't notice it elsewhere in CZO or in the previous PR's comments.

And it gets more offset with more levels of depth, but that's probably quite a rare use case.
The quoted spoilers are also shown in the msg info view, so that's great.

Do we want to consider adding any formatting to the Spoiler View, to separate the header and the content? It looks quite fine now, no doubt, I'm only thinking about aesthetics.

But the "up/down scrolls" in the header felt a bit out of place to me. I appreciate sticking to the convention. But, could we update to include that only when not all of the spoiler is visible, and not show it otherwise?

Multiple spoilers, message links, opening/closing work fine.
Handles empty spoilers just like the webapp does, which is displaying an empty message 🤔. This is good for the PR. But I'm surprised. Maybe, as a followup, we can check for empty spoilers/quotes/links/code blocks/etc before sending messages. Not sure if there are discussions elsewhere regarding this.

Will review the code soon.

Niloth-p

Lgtm.

All the passing around of arguments makes this quite bulky, I've opened #1537 which was discussed during #1455 and should help significantly simplify these PRs.

It would help if the commit descriptions of the 2nd and the 5th commits were re-written.
I think you can remove the commit description of the 6th and the 8th commits.

I didn't look into the header character count limits.

Niloth-p · 2024-07-18T14:23:39Z

tests/ui_tools/test_popups.py

+        self.spoiler_view.keypress(size, key)
+        assert not self.controller.exit_popup.called
+
+    @pytest.mark.parametrize("key", {*keys_for_command("EXIT_POPUP")})


Suggested change

@pytest.mark.parametrize("key", {*keys_for_command("EXIT_POPUP")})

@pytest.mark.parametrize("key", keys_for_command("EXIT_POPUP"))

Niloth-p · 2024-07-18T14:28:33Z

tests/ui_tools/test_popups.py

+        self,
+        mocker: MockerFixture,
+        widget_size: Callable[[Widget], urwid_Size],
+        navigation_key_expected_key_pair: Tuple[str, str] = ("ENTER", "ENTER"),


I assume the intention is to clarify the variables, but is this really required for this test? Can't we just use a single variable?

Niloth-p · 2024-07-18T14:33:26Z

zulipterminal/core.py

@@ -481,6 +482,11 @@ def report_warning(
        """
        self.view.set_footer_text(text, "task:warning", duration)

+    def show_spoiler(self, content: str) -> None:
+        self.show_pop_up(
+            SpoilerView(self, "Spoiler (up/down scrolls)", content), "area:msg"


I appreciate the convention. But, this suggestion of "up/down scrolls" looks out of place for most spoiler use cases. Could we update to include that only when not all of the spoiler is visible, and not show it otherwise?

Niloth-p · 2024-07-19T11:30:35Z

zulipterminal/ui_tools/messages.py

+                if len(content_contents) == 1 and content_contents[0] == "\n":
+                    content.contents.pop(0)
+
+                # FIXME: Do not soup2markup content in the MessageBox as it


The comment seems outdated.

Niloth-p · 2024-07-19T12:21:10Z

zulipterminal/ui_tools/messages.py

+                # Spoiler content
+                content = element.find(class_="spoiler-content")
+
+                # Remove surrounding newlines.


Let's change the variable name?
This code seems to make several non-trivial and effective assumptions about the data being passed to it through spoiler-content. Personally, I think improving the readability of this block can help.

Spoiler contents without headers - use the default heading 'Spoiler' which looks fine.

Spoiler headers without content - show empty spoiler content - should these be considered spoilers?
But that's also in the same ballpark as allowing empty spoiler messages, so it's consistent.

Niloth-p · 2024-07-19T12:37:26Z

tests/ui_tools/test_buttons.py

+    ) -> None:
+        self.selectable_icon = mocker.patch(MODULE + ".urwid.SelectableIcon")
+
+        # The method update_widget() is called in SpoilerButton's init.


I'm not sure about this suggestion.
But, can we mix these 2 test functions since they're both having the same line of code tested? Just different assertions.
Removing the mocking of update_widget in the test_init, and adding a parameter set with None should work?

Niloth-p · 2024-07-19T12:47:04Z

zulipterminal/ui_tools/views.py

            if text:
                caption = f"{link_index}: {text}\n{link}"
+                if spoiler_link:


I was thinking of several different approaches like prepending the spoiler keyword to the text, then removing it later from text, or using negative integers to communicate spoiler links or such, but yeah, they're all worse solutions than passing around an extra bool value.

With #1537, this should become simplified, as you won't have to add the 'bool' type in so many different places. Even temporarily, you could just create a type and update it in one place.

Also, I only noticed later that most of the bulk comes from the 'bool' type and not the bool value. So, this should be fine after a bit of refactoring.

I didn't look deeply into this, hoping you can just tell me, do both message_links and topic_links necessarily need to be of the same type?

mounilKshah · 2024-07-27T18:45:00Z

Thanks @rsashank for working on this. I tried running this on my local and it worked fine. Apart from the suggestions already given, I don't have much to add for the current iteration of the PR

zulipbot added the size: XL [Automatic label added by zulipbot] label Jul 2, 2024

rsashank force-pushed the spoilers branch from 0ae1e06 to 7ccda67 Compare July 2, 2024 04:28

rsashank force-pushed the spoilers branch 12 times, most recently from da87797 to 79f395b Compare July 12, 2024 04:05

rsashank marked this pull request as ready for review July 12, 2024 04:05

rsashank changed the title ~~WIP: Spoilers~~ Render Spoilers in ZT Jul 12, 2024

rsashank added area: message rendering PR needs buddy review PR needs mentor review area: popup: message labels Jul 12, 2024

rsashank force-pushed the spoilers branch from 79f395b to c38bb42 Compare July 12, 2024 04:13

rsashank and others added 8 commits July 12, 2024 00:26

boxes/messages: Add support for spoiler header markup in MessageBox.

7d0b85c

This commit: * adds styles for spoiler header in themes * adds markup entry in `soup2markup` for spoilers. Tests added. Co-authored-by: Preet Mishra <[email protected]> Co-authored-by: Ezio-Sarthak <[email protected]>

messages/views: Extract spoiler content in a variable in MessageBox.

555dfdd

This commit handles content metadata in `soup2markup` for spoilers. Test amended. Co-authored-by: Preet Mishra <[email protected]> Co-authored-by: Ezio-Sarthak <[email protected]>

core/views: Add SpoilerView class and corresponding show_* function.

6d11f72

This commit adds popup to show hidden spoiler content. Tests added. Co-authored-by: Preet Mishra <[email protected]> Co-authored-by: Ezio-Sarthak <[email protected]>

core/messages/views: Pass spoiler data to MsgInfoView popup helpers.

a577cef

Tests amended. Co-authored-by: Preet Mishra <[email protected]> Co-authored-by: Ezio-Sarthak <[email protected]>

views: Show SpoilerButton in message info view if present.

7469e69

This commit appends the SpoilerButton to message info view if present. Co-authored-by: Preet Mishra <[email protected]> Co-authored-by: Ezio-Sarthak <[email protected]>

messages: Hide footlink for links in spoiler blocks.

8c13265

This commit sets show_footlink to False for links in spoiler blocks.

buttons/core/messages/view: Add spoiler_link to metadata.

6f7fbdf

This commit: * Adds a boolean value spoiler_link to metadata, indicating whether a link is part of spoiler content. * Adds [spoiler] to link caption if spoiler_link is True. Fixes zulip#688.

rsashank force-pushed the spoilers branch from c38bb42 to 6f7fbdf Compare July 12, 2024 04:26

rsashank added the feedback wanted label Jul 16, 2024

neiljp reviewed Jul 16, 2024

View reviewed changes

Niloth-p mentioned this pull request Jul 19, 2024

Group message data #1537

Open

18 tasks

Niloth-p reviewed Jul 19, 2024

View reviewed changes

Niloth-p removed the PR needs buddy review label Jul 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Render Spoilers in ZT #1529

Render Spoilers in ZT #1529

rsashank commented Jul 2, 2024 •

edited

Loading

rsashank commented Jul 2, 2024

neiljp Jul 16, 2024

Niloth-p commented Jul 18, 2024

Niloth-p left a comment

Niloth-p Jul 18, 2024

Niloth-p Jul 18, 2024

Niloth-p Jul 18, 2024

Niloth-p Jul 19, 2024

Niloth-p Jul 19, 2024

Niloth-p Jul 19, 2024

Niloth-p Jul 19, 2024

mounilKshah commented Jul 27, 2024

	@pytest.mark.parametrize("key", {*keys_for_command("EXIT_POPUP")})
	@pytest.mark.parametrize("key", keys_for_command("EXIT_POPUP"))

Render Spoilers in ZT #1529

Are you sure you want to change the base?

Render Spoilers in ZT #1529

Conversation

rsashank commented Jul 2, 2024 • edited Loading

What does this PR do, and why?

External discussion & connections

How did you test this?

Self-review checklist for each commit

Visual changes

rsashank commented Jul 2, 2024

Choose a reason for hiding this comment

Niloth-p commented Jul 18, 2024

Niloth-p 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

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

mounilKshah commented Jul 27, 2024

rsashank commented Jul 2, 2024 •

edited

Loading