Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support (un)resolving topics #1544

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 48 additions & 1 deletion tests/ui_tools/test_popups.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
from pytest_mock import MockerFixture
from urwid import Columns, Pile, Text, Widget

from zulipterminal.api_types import Message
from zulipterminal.api_types import RESOLVED_TOPIC_PREFIX, Message
from zulipterminal.config.keys import is_command_key, keys_for_command
from zulipterminal.config.ui_mappings import EDIT_MODE_CAPTIONS
from zulipterminal.helper import CustomProfileData, TidiedUserInfo
Expand All @@ -26,6 +26,7 @@
PopUpView,
StreamInfoView,
StreamMembersView,
TopicInfoView,
UserInfoView,
)
from zulipterminal.urwid_types import urwid_Size
Expand Down Expand Up @@ -1604,6 +1605,52 @@ def test_checkbox_toggle_visual_notification(
toggle_visual_notify_status.assert_called_once_with(stream_id)


class TestTopicInfoView:
@pytest.fixture(autouse=True)
def mock_external_classes(
self, mocker: MockerFixture, general_stream: Dict[str, Any], topics: List[str]
) -> None:
self.controller = mocker.Mock()

mocker.patch.object(
self.controller, "maximum_popup_dimensions", return_value=(64, 64)
)
mocker.patch(LISTWALKER, return_value=[])
self.stream_id = general_stream["stream_id"]
self.topic = topics[0]
self.controller.model.stream_dict = {self.stream_id: general_stream}

self.topic_info_view = TopicInfoView(
self.controller, self.stream_id, self.topic
)

@pytest.mark.parametrize(
"topic_name, expected_topic_button_label",
[
("hi!", "Resolve Topic"),
(f"{RESOLVED_TOPIC_PREFIX}" + "hi!", "Unresolve Topic"),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Unless you plan to extract the 'hi!', this can be a combined f-string without the addition.

],
)
def test_topic_button_label(
self, topic_name: str, expected_topic_button_label: str
) -> None:
topic_info_view = TopicInfoView(self.controller, self.stream_id, topic_name)
assert (
topic_info_view.resolve_topic_setting_button_label
== expected_topic_button_label
)

def test_toggle_resolve_status(self) -> None:
resolve_button = self.topic_info_view.widgets[-1]
resolve_button._emit("click")

self.controller.model.toggle_topic_resolve_status.assert_called_once_with(
stream_id=self.stream_id, topic_name=self.topic
)

self.controller.exit_popup.assert_called_once()
Comment on lines +1643 to +1651
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like you added this extra test, and the one above this?

I'm not sure about using _emit here, at least compared to our normal use of keypress, but that should be fine - what inspired _emit?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially considered using keypress, but assumed the Enter key essentially uses ACTIVATE which triggers a click event in the context of an Urwid Button, just a NIT but thought it would be straightforward to use _emit.

I can change it back to use keypress with ACTIVATE_BUTTON

Urwid Docs

Send ‘click’ signal on ‘activate’ command.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine, though I'll leave this comment unresolved since it may be useful to take this to a discussion of how best to integrate urwid and our own key handling further in this way.



class TestStreamMembersView:
@pytest.fixture(autouse=True)
def mock_external_classes(self, mocker: MockerFixture) -> None:
Expand Down
62 changes: 61 additions & 1 deletion zulipterminal/ui_tools/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
import urwid
from typing_extensions import Literal

from zulipterminal.api_types import EditPropagateMode, Message
from zulipterminal.api_types import RESOLVED_TOPIC_PREFIX, EditPropagateMode, Message
from zulipterminal.config.keys import (
HELP_CATEGORIES,
KEY_BINDINGS,
Expand All @@ -25,6 +25,7 @@
COLUMN_TITLE_BAR_LINE,
PINNED_STREAMS_DIVIDER,
SECTION_DIVIDER_LINE,
STREAM_TOPIC_SEPARATOR,
)
from zulipterminal.config.ui_mappings import (
BOT_TYPE_BY_ID,
Expand Down Expand Up @@ -1569,6 +1570,65 @@ def keypress(self, size: urwid_Size, key: str) -> str:
return super().keypress(size, key)


class TopicInfoView(PopUpView):
def __init__(self, controller: Any, stream_id: int, topic: str) -> None:
self.stream_id = stream_id
self.controller = controller
stream = controller.model.stream_dict[stream_id]
self.topic_name = topic
stream_name = stream["name"]

title = f"{stream_name} {STREAM_TOPIC_SEPARATOR} {self.topic_name}"

topic_info_content: PopUpViewTableContent = []

popup_width, column_widths = self.calculate_table_widths(
topic_info_content, len(title)
)

if self.topic_name.startswith(RESOLVED_TOPIC_PREFIX):
self.resolve_topic_setting_button_label = "Unresolve Topic"
else:
self.resolve_topic_setting_button_label = "Resolve Topic"
resolve_topic_setting = urwid.Button(
self.resolve_topic_setting_button_label,
self.toggle_resolve_status,
)

curs_pos = len(self.resolve_topic_setting_button_label) + 1
# This shifts the cursor present over the first character of
# resolve_topic_button_setting label to last character + 1 so that it isn't
# visible

resolve_topic_setting._w = urwid.AttrMap(
urwid.SelectableIcon(
self.resolve_topic_setting_button_label, cursor_position=curs_pos
),
None,
"selected",
)

# Manual because calculate_table_widths does not support buttons.
# Add 4 to button label to accommodate the buttons itself.
popup_width = max(
popup_width,
len(resolve_topic_setting.label) + 4,
)

self.widgets = self.make_table_with_categories(
topic_info_content, column_widths
)

self.widgets.append(resolve_topic_setting)
super().__init__(controller, self.widgets, "TOPIC_INFO", popup_width, title)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This TOPIC_INFO has no meaning here yet. Not sure of the ordering; let's leave it for now.


def toggle_resolve_status(self, args: Any) -> None:
self.controller.model.toggle_topic_resolve_status(
stream_id=self.stream_id, topic_name=self.topic_name
)
self.controller.exit_popup()


class MsgInfoView(PopUpView):
def __init__(
self,
Expand Down