Panel Search Box UI Enhancements #1060

Rohitth007 · 2021-06-29T06:40:50Z

This PR deals UI enhancements related to display and reattempting search.
While reattempting search:

The caption sets to empty string
The previous edit_text does not get erased so that users can edit typos.

Display:

Aligns the text inside search box with the rest of the panel
Displays edit text on new line, which reads clearer and helps render long searches properly

Rohitth007 · 2021-06-29T09:15:58Z

The last commit fixes a bug caused by mouse clicking on search, but I'm not entirely sure about the solution.

preetmishra

@Rohitth007 Thanks for your work. This seems to work well functionally. 👍

That said, I would like to mention a couple of points related to the commits:

Given that we label all other types of changes in the version control, feature changes do not need labels like enhancement, feature, etc (it is implicit). You could take a look at the recently merged commit guidelines in the README.
We could improve the commit titles and description. Ideally, we should be able to identify what the commit is doing just by reading the commit title. Generic titles (e.g.: "Improve UI") don't give enough context. Commit descriptions should aim to give an overview than explain what the code is doing literally.

Other than that, the PR looks good overall. I have left a few minor in-line comments. :)

preetmishra · 2021-07-03T12:18:00Z

zulipterminal/ui_tools/boxes.py

@@ -1726,7 +1726,7 @@ def keypress(self, size: urwid_Size, key: str) -> Optional[str]:
            self.panel_view.keypress(size, "esc")
        elif is_command_key("ENTER", key) and not self.panel_view.empty_search:
            self.panel_view.view.controller.exit_editor_mode()
-            self.set_caption([("filter_results", "Search Results"), " "])
+            self.set_caption([("filter_results", " Search Results"), " "])


I think adding " " as another item would look better as this currently applies the styling to Search results as well. :)

I definitely agree with this. Generally the extra space looks good though 👍

I think it look weird that way, having spaces on both sides is another option which looks better. (See below)

preetmishra · 2021-07-03T12:22:19Z

zulipterminal/ui_tools/views.py

@@ -367,7 +367,7 @@ def keypress(self, size: urwid_Size, key: str) -> Optional[str]:
        if is_command_key("SEARCH_STREAMS", key):
            _, self.focus_index_before_search = self.log.get_focus()
            self.set_focus("header")
-            self.stream_search_box.set_caption("")
+            self.stream_search_box.set_caption(" ")


What do you think about adding an extra " " in a method that overrides urwid's set_caption method in PanelSearchBox instead?

I was instead thinking, during the Panel refactor I could group this and 1 other common line into a function call enter_search_mode inside PanelSearchBox

This would be a good function to add during the first commit - it avoids spreading general set_caption calls through the code, which may also be used for other things, as well as providing a central place for this kind of change - and it's easy to grep for a unique name :)

@preetmishra @neiljp Previously, the other common line that I was talking about was enter_editor_mode but now that I see that enter_editor_mode would be removed after the keypress refactor and set_caption would be called only once anyway after the Panel refactor, I don't think this needs to be moved into PanelSearchBox using enter_search_mode or similar.

preetmishra · 2021-07-03T12:26:32Z

zulipterminal/ui_tools/boxes.py

@@ -1726,7 +1726,7 @@ def keypress(self, size: urwid_Size, key: str) -> Optional[str]:
            self.panel_view.keypress(size, "esc")
        elif is_command_key("ENTER", key) and not self.panel_view.empty_search:
            self.panel_view.view.controller.exit_editor_mode()
-            self.set_caption([("filter_results", " Search Results"), " "])
+            self.set_caption([("filter_results", " Search Results\n"), " > "])


Let's wait for everybody's views on this in the stream.

Agreed - also if this is separate to other changes that are less controversial then having it last make it easier to merge the others first.

preetmishra · 2021-07-03T12:36:21Z

zulipterminal/ui_tools/views.py

@@ -368,6 +368,7 @@ def keypress(self, size: urwid_Size, key: str) -> Optional[str]:
            _, self.focus_index_before_search = self.log.get_focus()
            self.set_focus("header")
            self.stream_search_box.set_caption(" ")
+            self.view.controller.enter_editor_mode_with(self.stream_search_box)


I understand what the code does but I suspect that the commit description isn't accurate.

I've made some changes and can be found at #1072

preetmishra · 2021-07-03T12:39:16Z

zulipterminal/ui.py

@@ -208,8 +208,7 @@ def keypress(self, size: urwid_Box, key: str) -> Optional[str]:
            or is_command_key("PRIVATE_MESSAGE", key)
        ):
            self.body.focus_col = 1
-            self.middle_column.keypress(size, key)
-            return key
+            return self.middle_column.keypress(size, key)


Instead of minor changes, we could point out what the code does exactly in the commit title. :)

I've made some changes and can be found at #1072

zulipterminal/ui_tools/boxes.py

neiljp

@Rohitth007 You have a range of changes here - design changes (one needing discussion), general refactoring (towards panel simplification/combination), and a mouse bugfix. I'm not sure if they're all connected or dependent - though you may be changing the same files of course - but having them in the same PR isn't going to help this get merged very fast as each aspect has a range of different discussions required, and it makes reviewing more complex and time-consuming.

My impression is that the first 2-3 could be merged fairly quickly with a few changes and some simplification to the commit text - we could discuss writing styles in the ZT stream or the ZT GSoC one, or maybe PMs (but fewer people can contribute that way).

The refactoring is good to see too, but unless the changes are tightly coupled with the others it may be easier to have a separate PR for it, and for the mouse bugfix separately.

neiljp · 2021-07-04T01:28:08Z

zulipterminal/ui_tools/boxes.py

@@ -1726,7 +1726,7 @@ def keypress(self, size: urwid_Size, key: str) -> Optional[str]:
            self.panel_view.keypress(size, "esc")
        elif is_command_key("ENTER", key) and not self.panel_view.empty_search:
            self.panel_view.view.controller.exit_editor_mode()
-            self.set_caption([("filter_results", "Search Results"), " "])
+            self.set_caption([("filter_results", " Search Results"), " "])


I definitely agree with this. Generally the extra space looks good though 👍

neiljp · 2021-07-04T01:31:33Z

zulipterminal/ui_tools/views.py

@@ -367,7 +367,7 @@ def keypress(self, size: urwid_Size, key: str) -> Optional[str]:
        if is_command_key("SEARCH_STREAMS", key):
            _, self.focus_index_before_search = self.log.get_focus()
            self.set_focus("header")
-            self.stream_search_box.set_caption("")
+            self.stream_search_box.set_caption(" ")


This would be a good function to add during the first commit - it avoids spreading general set_caption calls through the code, which may also be used for other things, as well as providing a central place for this kind of change - and it's easy to grep for a unique name :)

neiljp · 2021-07-04T01:32:52Z

zulipterminal/ui_tools/boxes.py

@@ -1726,7 +1726,7 @@ def keypress(self, size: urwid_Size, key: str) -> Optional[str]:
            self.panel_view.keypress(size, "esc")
        elif is_command_key("ENTER", key) and not self.panel_view.empty_search:
            self.panel_view.view.controller.exit_editor_mode()
-            self.set_caption([("filter_results", " Search Results"), " "])
+            self.set_caption([("filter_results", " Search Results\n"), " > "])


Agreed - also if this is separate to other changes that are less controversial then having it last make it easier to merge the others first.

Rohitth007 · 2021-07-08T09:13:24Z

@preetmishra @neiljp I have split the PR as discussed and also dropped the mouse-fix commit as I think it would automatically get resolved with the keypress refactor. This PR only contains the Panel Search UI improvement features. (3 commits)

This commit erases `caption` and keeps `edit_text` while reattempting a search. `edit_text` was being erased every time because `search_text` had to be erased, hence keeping the `edit_text` becomes a problem. To solve this, the default `search_text` was used in caption instead and `edit_text` was set to ''. Removing the line that empties text in ui.py gives the feature we want. `set_content('')` is used so that now, when a user presses `q` for a second search the caption becomes empty. This makes for a cleaner UI while searching by removing caption `Search Results` when typing. Tests updated.

This commit aligns the text in PanelSearchBox to keep consistency with rest of the text in the panel. Tests updated.

This commits displays `edit_text` on a newline to differentiate clearly what is searched for, add aesthetic appeal by using `\n > ` and better render long `edit_text`s. Tests updated.

Rohitth007 · 2021-07-08T15:54:05Z

The rest of the commits can be found at #1072.

zulipbot · 2021-07-15T05:50:45Z

Heads up @Rohitth007, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.

neiljp · 2021-07-15T05:53:42Z

@Rohitth007 I just merged the first two of these separately since they are a definite improvement 🎉

However, there's been little feedback or testing of the other change, which could possibly be more convincing accompanied by other changes - such as adding hotkey hints, using a horizontal bar of color to indicate results, and whether we want a search 'prompt' (the >).

There is also scope for unifying some parts of the message search UI, at least in terms of how it is presented, after this change.

neiljp · 2024-02-04T02:38:57Z

I'm marking this as a PR completion candidate, but this will likely need some rounds of iteration to generate a consistent design, or perhaps a proposal and discussion beforehand to eg. consider how hotkeys currently work and how to represent that.

The main hotkey hinting issue here is how once we enter a search state then users get no hints (at a minimum of) how to

exit the search
edit the search

So, dynamically showing the appropriate keys would be useful for that.

For the multi-line layout, my previous concern was re the search UI jumping; I'm not sure there's a way around that.

Re a 'prompt' (like the >), at this point I'd be mostly concerned whether it looks like part of the search text. Styling that, or having a indent with a space or two, may be sufficient and clean.

Note that some of the original PR here has been merged already, but was discussed at #zulip-terminal>Panel search box UI.

zulipbot added the size: L [Automatic label added by zulipbot] label Jun 29, 2021

Rohitth007 added enhancement New feature or request PR needs review PR requires feedback to proceed area: UI General user interface update labels Jun 29, 2021

Rohitth007 changed the title ~~Panel Search Box UI Enhancement~~ Panel Search Box UI Enhancements Jun 29, 2021

Rohitth007 added the bug Something isn't working label Jun 29, 2021

preetmishra reviewed Jul 3, 2021

View reviewed changes

preetmishra 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 Jul 3, 2021

Rohitth007 mentioned this pull request Jul 3, 2021

[WIP] Refactor keypress to follow urwid rules #1063

Open

neiljp reviewed Jul 4, 2021

View reviewed changes

Rohitth007 force-pushed the panel-search-ui branch 3 times, most recently from 9e26a49 to 26ba08b Compare July 8, 2021 09:11

Rohitth007 added 3 commits July 8, 2021 21:02

boxes/views: Align text in PanelSearchBox.

e4faa3b

This commit aligns the text in PanelSearchBox to keep consistency with rest of the text in the panel. Tests updated.

boxes: Display edit_text on newline.

85193aa

This commits displays `edit_text` on a newline to differentiate clearly what is searched for, add aesthetic appeal by using `\n > ` and better render long `edit_text`s. Tests updated.

Rohitth007 force-pushed the panel-search-ui branch from 26ba08b to 85193aa Compare July 8, 2021 15:32

Rohitth007 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 Jul 8, 2021

zulipbot added the has conflicts label Jul 15, 2021

neiljp added further discussion required Discuss this on #zulip-terminal on chat.zulip.org and removed PR needs review PR requires feedback to proceed labels Jul 15, 2021

neiljp force-pushed the main branch from f9f483a to 793e73d Compare December 15, 2021 21:30

neiljp added the PR completion candidate PR with reviews that may unblock merging label Feb 4, 2024

neiljp mentioned this pull request Apr 22, 2024

Globally refocus after search #1049

Open

Uh oh!

Panel Search Box UI Enhancements #1060

Are you sure you want to change the base?

Panel Search Box UI Enhancements #1060

Uh oh!

Conversation

Rohitth007 commented Jun 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rohitth007 commented Jun 29, 2021

Uh oh!

preetmishra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

neiljp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rohitth007 commented Jul 8, 2021

Uh oh!

Rohitth007 commented Jul 8, 2021

Uh oh!

zulipbot commented Jul 15, 2021

Uh oh!

neiljp commented Jul 15, 2021

Uh oh!

neiljp commented Feb 4, 2024

Uh oh!

Uh oh!

Rohitth007 commented Jun 29, 2021 •

edited

Loading