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.

Sign up for GitHub

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

Jump to bottom

Make the ALL_MESSAGES command work globally #1512

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Make the ALL_MESSAGES command work globally #1512

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0bf31b7
refactor: ui_tools/helper: Add new enum Search Status.
Niloth-p Jun 6, 2024
4f15cac
views/boxes: Reset search only after verifying search status.
Niloth-p Jun 6, 2024
2525a79
views: Make the ALL_MESSAGES command work globally.
Niloth-p Jun 6, 2024
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
5 changes: 4 additions & 1 deletion tests/ui/test_ui_tools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

from zulipterminal.config.keys import keys_for_command, primary_key_for_command
from zulipterminal.config.symbols import STATUS_ACTIVE
from zulipterminal.helper import powerset
from zulipterminal.helper import SearchStatus, powerset
from zulipterminal.ui_tools.views import (
SIDE_PANELS_MOUSE_SCROLL_LINES,
LeftColumnView,
Expand Down Expand Up @@ -565,6 +565,7 @@ def test_keypress_CLEAR_SEARCH(self, mocker, stream_view, key, widget_size):
mocker.patch.object(stream_view, "set_focus")
mocker.patch(VIEWS + ".urwid.Frame.keypress")
mocker.patch.object(stream_view.stream_search_box, "reset_search_text")
stream_view.search_status = SearchStatus.FILTERED
stream_view.streams_btn_list = ["FOO", "foo", "fan", "boo", "BOO"]
stream_view.focus_index_before_search = 3

Expand Down Expand Up @@ -731,6 +732,7 @@ def test_keypress_CLEAR_SEARCH(self, mocker, topic_view, key, widget_size):
mocker.patch(VIEWS + ".TopicsView.set_focus")
mocker.patch(VIEWS + ".urwid.Frame.keypress")
mocker.patch.object(topic_view.topic_search_box, "reset_search_text")
topic_view.search_status = SearchStatus.FILTERED
topic_view.topics_btn_list = ["FOO", "foo", "fan", "boo", "BOO"]
topic_view.focus_index_before_search = 3

Expand Down Expand Up @@ -1112,6 +1114,7 @@ def test_keypress_CLEAR_SEARCH(self, right_col_view, mocker, key, widget_size):
mocker.patch(VIEWS + ".RightColumnView.set_focus")
mocker.patch(VIEWS + ".RightColumnView.set_body")
mocker.patch.object(right_col_view.user_search, "reset_search_text")
right_col_view.search_status = SearchStatus.FILTERED
right_col_view.users_btn_list = []

right_col_view.keypress(size, key)
Expand Down
6 changes: 3 additions & 3 deletions tests/ui_tools/test_boxes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
STREAM_MARKER_WEB_PUBLIC,
)
from zulipterminal.config.ui_mappings import StreamAccessType
from zulipterminal.helper import Index, MinimalUserData
from zulipterminal.helper import Index, MinimalUserData, SearchStatus
from zulipterminal.ui_tools.boxes import (
MAX_MESSAGE_LENGTH_CONFIRMATION_POPUP,
PanelSearchBox,
Expand Down Expand Up @@ -1903,8 +1903,8 @@ def test_keypress_ENTER(
size = widget_size(panel_search_box)
panel_search_box.panel_view.view.controller.is_in_editor_mode = lambda: True
panel_search_box.panel_view.log = log
empty_search = not log
panel_search_box.panel_view.empty_search = empty_search
search_status = SearchStatus.FILTERED if log else SearchStatus.EMPTY
panel_search_box.panel_view.search_status = search_status
panel_search_box.set_caption("")
panel_search_box.edit_text = "key words"

Expand Down
7 changes: 7 additions & 0 deletions zulipterminal/helper.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import time
from collections import defaultdict
from contextlib import contextmanager
from enum import Enum
from functools import partial, wraps
from itertools import chain, combinations
from re import ASCII, MULTILINE, findall, match
Expand Down Expand Up @@ -49,6 +50,12 @@
StreamAccessType = Literal["public", "private", "web-public"]


class SearchStatus(Enum):
DEFAULT = 0
FILTERED = 1
EMPTY = 2
Comment on lines +53 to +56
Copy link
Collaborator

Choose a reason for hiding this comment

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

The enum is an improvement overall 👍 It acknowledges that there are actually three distinct states for the search status in the code.

Re implementation in this commit structure:

  • this commit tracks it, but doesn't do anything with the extra state (cf. we track empty so we can display something different)
  • this commit could migrate to an enum, then extend the enum in another commit, when you need to track the other state?
  • it's strange in general seeing new asserts for FILTERED, but fewer changes for EMPTY, and none for DEFAULT; doing the split as per the above bullet would make it clearer where new state tracking was added and extra asserts added, vs renaming.

For the enum element names:

  • DEFAULT is OK, but in terms of a search-status, it's not as clear as it could be if that's search-on or search-off. UNFILTERED is maybe too close to FILTERED, but is closer to the meaning of it?
  • EMPTY is rather more FILTERED_EMPTY? We could end up in a state where it's "UNFILTERED_EMPTY" (DEFAULT_EMPTY), but I don't think the code covers that case?

Re location, helper.py is OK, though I'd prefer we reduce elements in here since 'helper' is such a generic name :)


Comment on lines +53 to +57
Copy link
Member

Choose a reason for hiding this comment

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

This is a really interesting approach!

I think it would be great to include a docstring here as well, explaining this a bit?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't add any because none of the other classes in helper.py have docstrings.
What aspect do you think this could be more clear about? We could rename it too.

Copy link
Member

Choose a reason for hiding this comment

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

Seems fair. I was just thinking it might be confusing for people looking at the code later, so a docstring would help clarify it. However, now that you mention it, it might not be necessary.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good naming > docstrings/comments, but it could be helpful if the naming can't be easily improved - and it's good not to block on getting perfect names ;)


class StreamData(TypedDict):
name: str
id: int
Expand Down
6 changes: 5 additions & 1 deletion zulipterminal/ui_tools/boxes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
)
from zulipterminal.config.ui_mappings import STREAM_ACCESS_TYPE
from zulipterminal.helper import (
SearchStatus,
asynch,
format_string,
match_emoji,
Expand Down Expand Up @@ -1043,7 +1044,10 @@ def keypress(self, size: urwid_Size, key: str) -> Optional[str]:
# Don't call 'Esc' when inside a popup search-box.
if not self.panel_view.view.controller.is_any_popup_open():
self.panel_view.keypress(size, primary_key_for_command("CLEAR_SEARCH"))
elif is_command_key("EXECUTE_SEARCH", key) and not self.panel_view.empty_search:
elif (
is_command_key("EXECUTE_SEARCH", key)
and self.panel_view.search_status != SearchStatus.EMPTY
):
self.panel_view.view.controller.exit_editor_mode()
self.set_caption([("filter_results", " Search Results "), " "])
self.panel_view.set_focus("body")
Expand Down
43 changes: 31 additions & 12 deletions zulipterminal/ui_tools/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
)
from zulipterminal.config.ui_sizes import LEFT_WIDTH
from zulipterminal.helper import (
SearchStatus,
TidiedUserInfo,
asynch,
match_emoji,
Expand Down Expand Up @@ -335,7 +336,7 @@ def __init__(self, streams_btn_list: List[Any], view: Any) -> None:
),
)
self.search_lock = threading.Lock()
self.empty_search = False
self.search_status = SearchStatus.DEFAULT

@asynch
def update_streams(self, search_box: Any, new_text: str) -> None:
Expand All @@ -352,7 +353,11 @@ def update_streams(self, search_box: Any, new_text: str) -> None:
)[0]

streams_display_num = len(streams_display)
self.empty_search = streams_display_num == 0
self.search_status = (
SearchStatus.EMPTY
if streams_display_num == 0
else SearchStatus.FILTERED
)

# Add a divider to separate pinned streams from the rest.
pinned_stream_names = [
Expand All @@ -371,7 +376,7 @@ def update_streams(self, search_box: Any, new_text: str) -> None:
streams_display.insert(first_unpinned_index, StreamsViewDivider())

self.log.clear()
if not self.empty_search:
if self.search_status == SearchStatus.FILTERED:
self.log.extend(streams_display)
else:
self.log.extend([self.stream_search_box.search_error])
Expand Down Expand Up @@ -404,6 +409,7 @@ def keypress(self, size: urwid_Size, key: str) -> Optional[str]:
self.log.extend(self.streams_btn_list)
self.set_focus("body")
self.log.set_focus(self.focus_index_before_search)
self.search_status = SearchStatus.DEFAULT
self.view.controller.update_screen()
return key
return super().keypress(size, key)
Expand Down Expand Up @@ -436,7 +442,7 @@ def __init__(
header=self.header_list,
)
self.search_lock = threading.Lock()
self.empty_search = False
Copy link

Choose a reason for hiding this comment

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

I think it's not on this PR to solve this, but it's easy to notice here: It's unfortunate that there's a lot of duplication between all the different views. A similar logic change repeated three times.

I'm currently not sure what could be a good way to refactor this. Maybe a class that knows the view state, handles the keypresses, and forwards back to the view what should happen based on some interface.

self.search_status = SearchStatus.DEFAULT

def _focus_position_for_topic_name(self) -> int:
saved_topic_state = self.view.saved_topic_in_stream_id(
Expand All @@ -461,10 +467,14 @@ def update_topics(self, search_box: Any, new_text: str) -> None:
for topic in self.topics_btn_list.copy()
if new_text in topic.topic_name.lower()
]
self.empty_search = len(topics_to_display) == 0
self.search_status = (
SearchStatus.EMPTY
if len(topics_to_display) == 0
else SearchStatus.FILTERED
)

self.log.clear()
if not self.empty_search:
if self.search_status == SearchStatus.FILTERED:
self.log.extend(topics_to_display)
else:
self.log.extend([self.topic_search_box.search_error])
Expand Down Expand Up @@ -524,6 +534,7 @@ def keypress(self, size: urwid_Size, key: str) -> Optional[str]:
self.log.extend(self.topics_btn_list)
self.set_focus("body")
self.log.set_focus(self.focus_index_before_search)
self.search_status = SearchStatus.DEFAULT
self.view.controller.update_screen()
return key
return super().keypress(size, key)
Expand Down Expand Up @@ -665,7 +676,7 @@ def __init__(self, view: Any) -> None:

self.allow_update_user_list = True
self.search_lock = threading.Lock()
self.empty_search = False
self.search_status = SearchStatus.DEFAULT
super().__init__(self.users_view(), header=search_box)

@asynch
Expand Down Expand Up @@ -706,10 +717,12 @@ def update_user_list(
else:
users_display = users

self.empty_search = len(users_display) == 0
self.search_status = (
SearchStatus.EMPTY if len(users_display) == 0 else SearchStatus.FILTERED
)

# FIXME Update log directly?
Copy link

Choose a reason for hiding this comment

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

I'm wondering if this FIXME is now obsolete. I'm not sure on the idea behind that comment, but going towards an Enum seems to not allow updating log directly anymore anyways.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not certain but imo it should be fine.
This enum merely replaces the boolean that was already being used there, so it shouldn't add any new restrictions?

I assume that the comment (added in this commit) means moving some of the logic from RightColumnView to UsersView which holds log. And this should be fine, as we already support TopicsView and StreamsView with the new enum, instead of the LeftColumnView. So, it would probably require just moving the enum to a different class.

if not self.empty_search:
if self.search_status != SearchStatus.EMPTY:
self.body = self.users_view(users_display)
else:
self.body = UsersView(
Expand Down Expand Up @@ -765,6 +778,7 @@ def keypress(self, size: urwid_Size, key: str) -> Optional[str]:
self.body = UsersView(self.view.controller, self.users_btn_list)
self.set_body(self.body)
self.set_focus("body")
self.search_status = SearchStatus.DEFAULT
self.view.controller.update_screen()
return key
elif is_command_key("GO_LEFT", key):
Expand Down Expand Up @@ -2027,7 +2041,7 @@ def __init__(
search_box = urwid.Pile(
[self.emoji_search, urwid.Divider(SECTION_DIVIDER_LINE)]
)
self.empty_search = False
self.search_status = SearchStatus.DEFAULT
self.search_lock = threading.Lock()
super().__init__(
controller,
Expand Down Expand Up @@ -2073,10 +2087,14 @@ def update_emoji_list(
else:
self.emojis_display = self.emoji_buttons

self.empty_search = len(self.emojis_display) == 0
self.search_status = (
SearchStatus.EMPTY
if len(self.emojis_display) == 0
else SearchStatus.FILTERED
)

body_content = self.emojis_display
if self.empty_search:
if self.search_status == SearchStatus.EMPTY:
body_content = [self.emoji_search.search_error]

self.contents["body"] = (
Expand Down Expand Up @@ -2150,5 +2168,6 @@ def keypress(self, size: urwid_Size, key: str) -> str:
self.emoji_search.reset_search_text()
self.controller.exit_editor_mode()
self.controller.exit_popup()
self.search_status = SearchStatus.DEFAULT
return key
return super().keypress(size, key)