[dolphin] Add an action to toggle the searchbar
ClosedPublic

Authored by iasensio on Aug 17 2019, 10:18 PM.

Details

Summary

Make search action toggle the searchbar instead of just launching it.

The search action in dolphin did only bring up the search bar, but to close it again you had to go to the closing button on the same searchbar.
This behavior in inconsistent with other dolphin actions which toggle panels or tools.

BEFORE:

AFTER:

BUG: 344617
FIXED-IN: 19.12.0
Closes T8473

Depends on D23075

Test Plan
  • Enable the search mode: the searchbar appears and the toolbar button gets checked
  • Press the search button again, and it goes back to normal mode.
  • The search button state is coherent with the searchbox
  • Coherence is kept when changing to a split view or different tab
  • Shorcut <Ctrl-F> does not close the searchbar, but moves the focus to it.

Diff Detail

Repository
R318 Dolphin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
iasensio created this revision.Aug 17 2019, 10:18 PM
Restricted Application added a project: Dolphin. · View Herald TranscriptAug 17 2019, 10:18 PM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
iasensio requested review of this revision.Aug 17 2019, 10:18 PM
iasensio edited the summary of this revision. (Show Details)Aug 17 2019, 10:20 PM
iasensio added reviewers: Dolphin, ngraham.

Thanks for the patch! In fact, this comes up a lot. I'm aware of at least two other patches that have attempted to fix it: D10246 & D12094. Both of their submitters have effectively abandoned them, so I think we can move forward with this.

However both prior patches have run into the same objection: that for people who prefer to keep the search bar always open, the keyboard shortcut should focus its search field rather than closing the search panel. However the fact that we now have no fewer than three independently-submitted patches to do the same thing is IMO pretty powerful evidence that this needs to be fixed somehow. @elvisangelaccio

If you can figure out a way to make ctrl+F focus the search field when the search panel is already open rather than closing it, I think this patch could be a winner.

Wow, I did a little search through the bug tracker but I didn't find that it had been already addresed.
Those other two patches seem more detailed, as I haven't dealt with the filter bar, only the search bar. I have already seen some inconsistencies in mine when using tabs or split view.

I see three possibilities:

  • try to catch if the shortcut is being pressed when the action dispatches
  • two different actions 'open-search' and 'toggle-search': doesn't seem a valid approach according to D10246
  • an option in dolphin settings (maybe in behavior)

I can give it a try on the first one, but I cannot promise anything :)

iasensio updated this revision to Diff 63957.Aug 17 2019, 11:14 PM
  • Update search action on view changes
iasensio updated this revision to Diff 63960.EditedAug 18 2019, 12:30 AM
  • Add an independent toggle_search action

After a while, I came to the conclussion that providing two different actions is the only sane way to do it.
QActions are meant to achive the same behavior independent of the source (keyboard, button, menu, ...) so it encapsulates the source very deeply. In this case we want two different behaviors, which leads us to two different actions.

In the screenshow I keep both toolbuttons for comparison, and to "simulate" visually the effect of Ctrl-F, but ideally every user would keep only one of them. This way, the request on bug 344617 (and my own :)) gets satisfied, and the Ctrl-F behavior works as before.

I've also tested the behavior against split view and tabs (which was the main concern on T8473), and it seems consistent, as the ActiveViewChanged signal updates the button status.

iasensio retitled this revision from [dolphin] Make search action toggle the searchbar to [dolphin] Add an action to toggle the searchbar.Aug 18 2019, 12:37 AM
iasensio edited the summary of this revision. (Show Details)
iasensio edited the test plan for this revision. (Show Details)
ngraham added a reviewer: VDG.Aug 18 2019, 1:52 AM

Giving choice to the user isn't really the best solution here IMO. The fact that these toolbars are customizable at all is not widely known, and allowing people to choose between multiple actions that both do the same thing except for one slightly different close/focus behavior is likely to be super confusing to everyone who's not a Dolphin developer. :) Adding VDG for more opinions and comments.

If it's really impossible to gain the desired behavior, my personal preference would be to make it a pure toggle action and live with the fact that hitting the shortcut will always toggle it. While this may be slightly irritating for people who want to keep it open all the time, part of the reason why they might want to do that is because it's not as easy to close as it could be. So maybe they'll be able to easily adjust to the new behavior and even gain some space by not having to have it open all the time.

We probably need @elvisangelaccio's input on this (he's the Dolphin maintainer). IIRC he's on vacation right now and will be back in a week or so.

I think it is fine having two actions that both open the Search/Findbar. The new 'toggle-search' would replace 'open-search' in the Toolbar and 'open-search' would still be associated with Ctrl+F.
This is similar to having two actions for typing a new location:


Making the names of the two actions more distinct might be enough to keep the users from getting super confused.

I think the expected behaviour when pressing Ctrl+F should probably stay as it is because that is also somewhat consistent with the behaviour of browsers when pressing Ctrl+F. (I personally don't have a preference here.)

Hmm, now that I give it more thought, you might be right. All right, let's give it a try. @iasensio we'll need for dolphinrc.ui to have the new toggle_search action on the default toolbar, rather than the old edit_find one. And don't forget to bump the version number at the top of that file. :)

iasensio updated this revision to Diff 64008.Aug 19 2019, 12:30 AM
  • Update dolphinui.rc to use toggle_search instead of edit_find

Rebased to Accepted D23075, to avoid the collision.

ngraham edited the summary of this revision. (Show Details)Aug 19 2019, 2:01 AM

Looks like you incorporated changes in D23075 rather than rebasing on top of it.

Looks like you incorporated changes in D23075 rather than rebasing on top of it.

You are right. I'm still learning to use arcanist together with git and I got messed applying the patch.
Tomorrow i'll try to fix the diff

iasensio updated this revision to Diff 64026.Aug 19 2019, 11:15 AM
  • Set toggle_search as default toolbar action (on top of D23075)
iasensio updated this revision to Diff 64030.Aug 19 2019, 11:34 AM
  • Set toggle_search as default toolbar action

Hopefully this time, on top of D23075

ndavis added a subscriber: ndavis.Aug 19 2019, 1:38 PM

I also think that using toggle for search makes more sense. It would be nice to have a general pattern of using toggle buttons in the toolbar for things that can be opened and closed, especially when they're activated via the toolbar in the first place.

I also think that using toggle for search makes more sense. It would be nice to have a general pattern of using toggle buttons in the toolbar for things that can be opened and closed, especially when they're activated via the toolbar in the first place.

+1. In fact, we already do have this pattern; there are just a few that don't conform.

ngraham accepted this revision as: VDG, ngraham.Aug 19 2019, 5:06 PM

Nice. you even differentiated the names that show up in the toolbar configuratin window! Looks great from my perspective. Let's wait for @elvisangelaccio now.

This revision is now accepted and ready to land.Aug 19 2019, 5:06 PM
elvisangelaccio requested changes to this revision.Aug 25 2019, 5:32 PM

Patch looks good, but please note that bug #344617 also asks to make the filter button a toggle.
So either we rename the bug report to only refer to the search button, or this commit should have CCBUG: rather than BUG:.

(I think we should rename the bug report and leave the filter button as-is).

src/dolphinmainwindow.cpp
620–626

Please make DolphinViewContainer::setSearchModeEnabled() a public slot instead, so that we can just connect it to the triggered signal of the searchAction.

1242
This revision now requires changes to proceed.Aug 25 2019, 5:32 PM
iasensio updated this revision to Diff 64624.Aug 25 2019, 11:04 PM
iasensio marked 2 inline comments as done.
  • Address inline comments
  • Use setSearchModeEnabled as a public slot
  • Capitalize action text
elvisangelaccio added inline comments.Sep 1 2019, 6:16 PM
src/dolphinmainwindow.cpp
1102

This comment doesn't add much, I'd remove it.

1102–1104

Please move this disconnect to the beginning of this if block. (to not get in between the two existing comments).

iasensio updated this revision to Diff 65169.Sep 1 2019, 7:10 PM
iasensio marked 2 inline comments as done.
  • reorder disconnect
elvisangelaccio accepted this revision.Sep 1 2019, 8:07 PM

Patch looks good, but please note that bug #344617 also asks to make the filter button a toggle.
So either we rename the bug report to only refer to the search button, or this commit should have CCBUG: rather than BUG:.

(I think we should rename the bug report and leave the filter button as-is).

Going to push the patch and rename the bug report.

Thanks @iasensio :)

This revision is now accepted and ready to land.Sep 1 2019, 8:07 PM
This revision was automatically updated to reflect the committed changes.