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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

bugfix: Left strip search text to avoid empty search results #700

Closed
wants to merge 2 commits into from
Closed

bugfix: Left strip search text to avoid empty search results #700

wants to merge 2 commits into from

Conversation

preetmishra
Copy link
Member

This amends PanelSearchBox's update functions to avoid empty search results for spaces.

@neiljp Thanks for reporting the bug in #699 (review). 馃憤

@neiljp neiljp mentioned this pull request Jun 23, 2020
Closed
@neiljp
Copy link
Collaborator

neiljp commented Jun 23, 2020

@preetmishra I'd like to get #699 fixed in a similar way, as I commented there, but this looks reasonable as a v0 in terms of the specific issue I mentioned there, from a quick test.

However, I just gave this a try and for at least the stream list we seem to scroll it even with a space. Is there a different way to fix this that would resolve that too, do you think? Or is that a different bug?

@zulipbot zulipbot added size: M and removed size: S labels Jun 24, 2020
@preetmishra
Copy link
Member Author

@neiljp Thanks for the review.

Re scrolling issue, I gather it is a different bug. I have added a fix, in a separate commit, to resolve this.

@preetmishra preetmishra mentioned this pull request Jun 30, 2020
Merged
@preetmishra
Copy link
Member Author

Note: The stream/topic list doesn't get updated when we completely clear out/backspace an active search as well. This might feel a bit unintuitive but this behaviour already exists in the user search.

Would we want to eradicate this behaviour from all of the PanelSearchBox's update functions or leave them as they are?

@neiljp neiljp added the PR needs review PR requires feedback to proceed label Jul 7, 2020
@neiljp
Copy link
Collaborator

neiljp commented Jul 9, 2020

@preetmishra The behavior of the user search has been what I consider buggy for some time - eg. p backspace doesn't return to an empty case. That this PR seems to have a regression like the user list for the other (panel) searches, makes this worse. I would hope that deleting back to nothing resets to show all the options, as otherwise I don't know what the 'global' list might look like. Admittedly if you have so many entries that you can't see them at all, this doesn't help, but I can't see much of a reason to not reset the display if the search is empty?

Another aspect that this appears to fail on is that while the left-stripped search strings do find results, <space> <enter> doesn't cancel the search, ie. the search string is treated as meaningful, like before @meelanp 's commit. That is, <enter> exits the search, but <space><enter> does not, which seems strange if <space> doesn't mean anything for searches, at least at the left.

@neiljp neiljp removed the PR needs review PR requires feedback to proceed label Jul 9, 2020
@preetmishra preetmishra changed the title bugfix: Left strip search text to avoid empty search results [WIP] bugfix: Left strip search text to avoid empty search results Jul 9, 2020
@preetmishra preetmishra changed the title [WIP] bugfix: Left strip search text to avoid empty search results bugfix: Left strip search text to avoid empty search results Jul 10, 2020
@preetmishra
Copy link
Member Author

preetmishra commented Jul 10, 2020 •
edited

@neiljp Thanks for the review and the exposition. In the latest update, I have resolved the user search issue, the cancel search issue and reworked the approach to fix the scrolling issue.

However, the scrolling issue fix, the last commit, has a regression where the stream/topic list is shifted when esc is pressed post a search (note that the focus_index_before_search logic still works). I can't seem to figure out what is causing this particular behaviour. I would greatly appreciate any pointers.

Besides, the first three commits work as expected and should be good to go.

@preetmishra preetmishra added the PR needs review PR requires feedback to proceed label Jul 11, 2020
@preetmishra preetmishra mentioned this pull request Aug 17, 2020
Open
@neiljp neiljp mentioned this pull request Aug 21, 2020
Merged
@preetmishra
boxes/views: Treat spaces as an empty search in PanelSearchBox.
ecd883b 
This amends PanelSearchBox and related update functions to treat spaces
as an empty search by using left strip, i.e., the search is
canceled and the list is not updated.
@preetmishra
boxes/views: Keep the log focus constant while the search is active.
addb62c 
This amends update_streams and update_topics to set the log focus to the
start while the search is active.

NOTE: Sets a regression in the focus_index_before_search logic where now
the list shifts itself after 'esc' is pressed.
@zulipbot zulipbot added size: S and removed size: M labels Aug 21, 2020
@neiljp
Copy link
Collaborator

neiljp commented Aug 29, 2020

@preetmishra Thanks for working on this and the associated earlier commits - which were merged already. As per agreement elsewhere, we'll likely move forward with an approach like in #784, though we can always return to this if we change our mind 馃憤

@neiljp neiljp closed this Aug 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
PR needs review PR requires feedback to proceed size: S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@preetmishra @neiljp @zulipbot