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

view: Fix focus index before and after Panel Search. #978

Merged
merged 1 commit into from
Jun 23, 2021
Merged

view: Fix focus index before and after Panel Search. #978

merged 1 commit into from
Jun 23, 2021

Conversation

Rohitth007
Copy link
Collaborator

@Rohitth007 Rohitth007 commented Apr 2, 2021 •
edited
Loading

This PR corrects what the code for focus_index_before_search was intended to do. See #975
The focus index was still being updated even in the search results.

This is fixed by moving get_focus into the SEARCH_STREAM/TOPIC
conditional.

Fixes #975

Before Commit: After Commit:
wrong-focus right-focus

@zulipbot zulipbot added size: M [Automatic label added by zulipbot] further discussion required Discuss this on #zulip-terminal on chat.zulip.org labels Apr 2, 2021
@zulipbot
Copy link
Member

zulipbot commented Apr 2, 2021

ERROR: Label "further discussion needed" does not exist and was thus not removed from this pull request.

@Rohitth007
Copy link
Collaborator Author

@zulipbot remove "further discussion required"

@zulipbot zulipbot removed the further discussion required Discuss this on #zulip-terminal on chat.zulip.org label Apr 2, 2021
preetmishra
preetmishra reviewed Jun 2, 2021
View reviewed changes
Copy link
Member

@preetmishra preetmishra left a comment

Choose a reason for hiding this comment

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

@Rohitth007 Thanks for putting the work through! This seems to work well. 👍

However, I think we could simplify the approach (see my in-line comment). I would encourage you to add tests so that we can ship it with more confidence.

Also, it is not apparent through the commit title where the search is being done. You might want to explicate it a bit more.

zulipterminal/ui_tools/views.py Outdated Show resolved Hide resolved
zulipterminal/ui_tools/views.py Outdated Show resolved Hide resolved
zulipterminal/ui_tools/views.py Outdated Show resolved Hide resolved
zulipterminal/ui_tools/views.py Outdated Show resolved Hide resolved
zulipterminal/ui_tools/views.py Outdated Show resolved Hide resolved
zulipterminal/ui_tools/views.py Outdated Show resolved Hide resolved
zulipterminal/ui_tools/views.py Outdated Show resolved Hide resolved
@preetmishra preetmishra added the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Jun 2, 2021
@Rohitth007 Rohitth007 force-pushed the focus-before-after-search-976 branch from 0868e44 to 64da010 Compare June 5, 2021 15:25
@zulipbot zulipbot added size: XL [Automatic label added by zulipbot] and removed size: M [Automatic label added by zulipbot] labels Jun 5, 2021
@Rohitth007 Rohitth007 force-pushed the focus-before-after-search-976 branch from 64da010 to b8fdbdd Compare June 5, 2021 16:51
@zulipbot zulipbot added size: M [Automatic label added by zulipbot] and removed size: XL [Automatic label added by zulipbot] labels Jun 5, 2021
@Rohitth007 Rohitth007 force-pushed the focus-before-after-search-976 branch from b8fdbdd to 1a42792 Compare June 6, 2021 12:47
@Rohitth007 Rohitth007 changed the title view: Fix focus before and after search. view: Fix focus index before and after Panel Search. Jun 6, 2021
@Rohitth007 Rohitth007 force-pushed the focus-before-after-search-976 branch from 1a42792 to b433eb8 Compare June 6, 2021 12:53
@preetmishra preetmishra 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 Jun 6, 2021
preetmishra
preetmishra reviewed Jun 6, 2021
View reviewed changes
Copy link
Member

@preetmishra preetmishra left a comment

Choose a reason for hiding this comment

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

@Rohitth007 Thanks for the update. This seems to work well! 👍

It would be good to have it in the User search as well while we're at it. It should be straightforward to implement.

The second commit looks good to me. However, I would strongly encourage you to accompany the changes with tests.

Re the first commit, let's see what Neil has to say about the refactor.

@preetmishra 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 Jun 6, 2021
@Rohitth007
Copy link
Collaborator Author

We could also have it for the user list but it wasn't there to begin with so I thought it wasn't needed. Also we have to make sure that if a user goes to the bottom of the list and come back to the message box, the list doesn't get stuck theme, as same section of code is used. I haven't checked if this would happen.

@Rohitth007
Copy link
Collaborator Author

A more useful feature would be to focus back to where it was before search if it was not in the list. Eg: If we are in the message view and press q, escaping should bring us back to that message. That doesn't happen currently but is also out of the scope of this PR

@Rohitth007
Copy link
Collaborator Author

Rohitth007 commented Jun 6, 2021 •
edited
Loading

Re tests, I'll update them by tomorrow 😅

@preetmishra
Copy link
Member

@Rohitth007 I am not sure if I understand your first premise completely. Let's talk more about it on CZO. Re the latter, I had something similar while reviewing. Could you file an issue for the same as it is something that we can take up separately?

@Rohitth007 Rohitth007 force-pushed the focus-before-after-search-976 branch from b433eb8 to 0e82dbb Compare June 7, 2021 13:51
@zulipbot zulipbot added size: XL [Automatic label added by zulipbot] and removed size: M [Automatic label added by zulipbot] labels Jun 7, 2021
@Rohitth007
Copy link
Collaborator Author

@preetmishra I have added some tests for this and removed bad tests which gave false positives

@Rohitth007 Rohitth007 force-pushed the focus-before-after-search-976 branch from 0e82dbb to 62bb0a0 Compare June 7, 2021 15:10
@Rohitth007
Copy link
Collaborator Author

I've implemented user list refocus in the 3rd commit. It needs some checking.

@Rohitth007 Rohitth007 force-pushed the focus-before-after-search-976 branch 2 times, most recently from eeecca5 to 982a848 Compare June 8, 2021 15:11
@Rohitth007 Rohitth007 requested a review from preetmishra June 8, 2021 15:12
@Rohitth007
Copy link
Collaborator Author

I have updated the tests for user list refocusing as well

preetmishra
preetmishra reviewed Jun 8, 2021
View reviewed changes
Copy link
Member

@preetmishra preetmishra left a comment

Choose a reason for hiding this comment

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

@Rohitth007 This continues to work well functionally. Thanks for the update! 👍

We could potentially secure the tests for RightColumnView. I have left a few in-line comments related to tests.

tests/ui/test_ui_tools.py Show resolved Hide resolved
tests/ui/test_ui_tools.py Show resolved Hide resolved
tests/ui/test_ui_tools.py Show resolved Hide resolved
tests/ui/test_ui_tools.py Outdated Show resolved Hide resolved
neiljp
neiljp reviewed Jun 9, 2021
View reviewed changes
zulipterminal/ui_tools/views.py Outdated Show resolved Hide resolved
tests/ui/test_ui_tools.py Show resolved Hide resolved
zulipterminal/ui_tools/views.py Outdated Show resolved Hide resolved
@neiljp
Copy link
Collaborator

neiljp commented Jun 9, 2021

@Rohitth007 Let's focus on the bugfix commit which functionally works well and tune that test 👍

We could incorporate the initial refactor into a more general clean-up like I mentioned.
Regarding the last commit, I think we were discussing that in the stream and there are challenges with the automatic refresh which make knowing where to return to unclear, so let's put that into another set of work targeted at understanding how to resolve that separately?

@Rohitth007 Rohitth007 force-pushed the focus-before-after-search-976 branch 2 times, most recently from 2957fd2 to b7e3d55 Compare June 9, 2021 06:38
@Rohitth007
Copy link
Collaborator Author

I have made few changes as suggested and moved the bugfix commit to the top. I'll move the other commits into new PR's to be worked on separately.

@preetmishra preetmishra 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 Jun 9, 2021
@Rohitth007 Rohitth007 force-pushed the focus-before-after-search-976 branch from b7e3d55 to b6e8e57 Compare June 14, 2021 14:34
@Rohitth007
Copy link
Collaborator Author

I have moved the other commits into separate branches. This is ready to be merged

@Rohitth007 Rohitth007 added PR ready to be merged PR has been reviewed & is ready to be merged bug Something isn't working labels Jun 14, 2021
@Rohitth007 @neiljp
bugfix: views: Fix focus index before and after Panel Search.
8423efc 
This commit corrects what the code for focus_index_before_search was
intended to do. The focus index was still being updated even in the
search results.

This is fixed by moving `get_focus` into the SEARCH_STREAM/TOPIC
conditional.

Tests amended. Bad tests (`test_return_to_focus_after_search`)
removed as they pass even though they don't do the right thing.
Current tests fail before this commit.

Fixes zulip#975.
@neiljp neiljp force-pushed the focus-before-after-search-976 branch from b6e8e57 to 8423efc Compare June 23, 2021 23:28
@neiljp
Copy link
Collaborator

neiljp commented Jun 23, 2021

@Rohitth007 Thanks for the test reworking 👍 There's still a lot of manual implementation-dependent setup going on in the tests which I'll be glad to see gone in future! Identifying the bugfix was a good find and this is small and clean to merge now 🎉

@neiljp neiljp merged commit d791682 into zulip:main Jun 23, 2021
@neiljp neiljp removed the PR needs review PR requires feedback to proceed label Jun 23, 2021
@neiljp neiljp added this to the Next Release milestone Jun 23, 2021
@Rohitth007 Rohitth007 deleted the focus-before-after-search-976 branch June 24, 2021 15:21
@neiljp neiljp added the area: UI General user interface update label Feb 28, 2022
@neiljp neiljp mentioned this pull request Apr 22, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@preetmishra preetmishra preetmishra left review comments

@neiljp neiljp neiljp left review comments

Assignees
No one assigned
Labels
area: UI General user interface update bug Something isn't working PR ready to be merged PR has been reviewed & is ready to be merged size: XL [Automatic label added by zulipbot]
Projects
None yet
Milestone
0.7.0
Development

Successfully merging this pull request may close these issues.

Focus_Index_Before_Search doesn't work as intended
4 participants
@Rohitth007 @zulipbot @preetmishra @neiljp