| ngraham | |
| elvisangelaccio |
| Dolphin | |
| VDG |
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
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
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:
I can give it a try on the first one, but I cannot promise anything :)
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.
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:
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. :)
Rebased to Accepted D23075, to avoid the collision.
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
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.
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.
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 | Please use Title Capitalization: https://hig.kde.org/style/writing/capitalization.html#title-capitalization | |