-
-
Notifications
You must be signed in to change notification settings - Fork 259
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
6b7eb6b
59b9ab9
16cff3b
7fdf686
1c85d9d
76fafdf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -26,6 +26,7 @@ | |
PopUpView, | ||
StreamInfoView, | ||
StreamMembersView, | ||
TopicInfoView, | ||
UserInfoView, | ||
) | ||
from zulipterminal.urwid_types import urwid_Size | ||
|
@@ -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"), | ||
], | ||
) | ||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I initially considered using I can change it back to use Urwid Docs
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
|
@@ -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, | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
|
There was a problem hiding this comment.
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.