Move from the searchbox to the results with the down arrow key
ClosedPublic

Authored by shlomif on Jan 2 2020, 1:02 PM.

Details

Summary

Move from the searchbox to the search results listbox/view using the
down arrow key in addition to the existing methods using the tab key,
return key, or the mouse.

Test Plan

use ctrl+f to search in a directory tree, press down arrow key
to go to the results

Diff Detail

Repository
R318 Dolphin
Branch
shlomif--down-arrow-to-search-results--accessibility
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 20581
Build 20599: arc lint + arc unit
shlomif created this revision.Jan 2 2020, 1:02 PM
Restricted Application added a project: Dolphin. · View Herald TranscriptJan 2 2020, 1:02 PM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
shlomif requested review of this revision.Jan 2 2020, 1:02 PM

As far as I can tell, currently the focus will change from the searchbar to the view when you press <Enter>.
Would that be an acceptable workflow?

As far as I can tell, currently the focus will change from the searchbar to the view when you press <Enter>.
Would that be an acceptable workflow?

‎Thanks! That indeed appears to be working even without the patch. But I couldn't figure it out by myself. Using Enter is an acceptable workflow as far as I'm concerned, but my patch may be indicative of a UI issue.

ngraham requested changes to this revision.Jan 3 2020, 12:42 AM
ngraham added a subscriber: ngraham.

Given that the down arrow key currently does nothing here, I think that this makes sense conceptually. I'm not a huge fan of this setView() function though. Instead, pressing the down arrow key should emit focusViewRequest(); This is how the filter bar implements this functionality.

src/search/dolphinsearchbox.cpp
223

don't assert; instead check for m_view being valid, and if it isn't, just do nothing.

This revision now requires changes to proceed.Jan 3 2020, 12:42 AM
shlomif updated this revision to Diff 72689.Jan 3 2020, 2:49 PM

Convert to emit signal per @ngraham 's comment.

Thanks, this is much better technically.

One thing I notice is that moving the focus to the view using focusViewRequest() doesn't actually make an item become pre-selected if it wasn't before. Given that the whole point of this feature is to quickly select an item in the view after filtering or searching, maybe it would also make sense to create a new function that will do that which we can call from the filter bar and search field when they request to move focus to the view.

Then again personally I kind of think that focusing the main view should *always* select an item, since otherwise that's just a manual step the user has to do for themselves. In this case, we would have DolphinViewContainer::requestFocus() always automatically select the item with implicit focus, if it's not already selected.

What do you think, @elvisangelaccio?

One thing I notice is that moving the focus to the view using focusViewRequest() doesn't actually make an item become pre-selected if it wasn't before. Given that the whole point of this feature is to quickly select an item in the view after filtering or searching, maybe it would also make sense to create a new function that will do that which we can call from the filter bar and search field when they request to move focus to the view.

Then again personally I kind of think that focusing the main view should *always* select an item, since otherwise that's just a manual step the user has to do for themselves. In this case, we would have DolphinViewContainer::requestFocus() always automatically select the item with implicit focus, if it's not already selected.

I agree. And this behavior should be also implemented for the Return case which is lacking this at the moment.
A requestFocusAndSelectFirstItem for instance.

@shlomif are you interested in finishing up this patch?

@ngraham : hi! I am interested in finishing it up, but will be fine with someone else doing that. I'll try to get to it soon.

@ngraham @meven : here is a new patch: https://www.shlomifish.org/Files/files/code/dolphin--shlomif-patch-D26362--modified-v0.2.0.patch but note that it makes no difference and om fedora 32 x64 it seems that pressing enter twice invokes the top item in the results which was already selected. It also can be done as "down arrow; enter" using either of my patches.

Thanks, but you need to update the diff using arc rather than providing a link to a new patch file.

https://community.kde.org/Infrastructure/Phabricator#Step_2:_Update_your_diff_in_response_to_review_comments

Thanks, but you need to update the diff using arc rather than providing a link to a new patch file.

https://community.kde.org/Infrastructure/Phabricator#Step_2:_Update_your_diff_in_response_to_review_comments

@ngraham : well, I didn't want to override the existing patch because the new one does not provide extra functionality - it is just more complex code-wise

So what's the status of this patch and the relationship between it and the other one you've mentioned? I'm confused.

So what's the status of this patch and the relationship between it and the other one you've mentioned? I'm confused.

Well, the new patch was an attempt at revising the patch tracked by this phabricator ticket for implementing your commentary that it should make an item in the search results become pre-selected . However, this seems to also be the case while using the phabricator tracked patch and the new path makes no difference, so it is safe to ignore the https-hyperlinked-linker and I just shared it for completeness sake.

ngraham accepted this revision.Apr 14 2020, 2:54 PM

LGTM in its current form.

@elvisangelaccio & @meven ?

This revision is now accepted and ready to land.Apr 14 2020, 2:54 PM
meven added a comment.Apr 15 2020, 1:13 PM

If we could just clean up the commit message, seems good to me.

ngraham retitled this revision from [Tentative] Move from the searchbox to the results to Move from the searchbox to the results with the down arrow key.Apr 15 2020, 1:30 PM
ngraham edited the summary of this revision. (Show Details)
ngraham requested changes to this revision.Apr 15 2020, 1:39 PM

Actually I see a change that's needed: when the return key is pressed, it triggers DolphinSearchBox::returnPressed(), but when the down arrow key is pressed, it triggers DolphinSearchBox::focusViewRequest() And I notice that returnPressed() emits the search result too. So now with this patch we have two inconsistent codepaths to get to the same thing. We should probably unify these so that both trigger your new focusViewRequest() signal, and make sure that it emits the search result too.

This revision now requires changes to proceed.Apr 15 2020, 1:39 PM
shlomif updated this revision to Diff 80453.Apr 18 2020, 9:33 AM
  • Unify signals - returnPressed->focusViewRequest.

    Per the input of @ngraham .

@ngraham : I applied the change now, but was unable to build and test the change due to git master requiring newer KDE deps. I hope it works.

ngraham accepted this revision.Apr 18 2020, 4:36 PM

Great job!

This revision is now accepted and ready to land.Apr 18 2020, 4:36 PM
This revision was automatically updated to reflect the committed changes.