Add default shortcut "/" for opening filter panel
ClosedPublic

Authored by rominf on Mar 2 2018, 7:53 AM.

Details

Summary

Add default shortcut "/" for opening filter panel.

FEATURE: 156381

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.
rominf created this revision.Mar 2 2018, 7:53 AM
Restricted Application added a subscriber: Dolphin. · View Herald TranscriptMar 2 2018, 7:53 AM
rominf requested review of this revision.Mar 2 2018, 7:53 AM
rominf added a project: Dolphin.
ngraham requested changes to this revision.EditedMar 2 2018, 8:28 PM
ngraham added a subscriber: ngraham.

Works as advertised, and doesn't conflict with any default shortcuts. The whole "type / to quick search/filter" is pretty well established in web browsers nowadays. Makes sense to have it in Dolphin, too.

Konsole still eats it when its panel is open, but it also eats the other ctrl+I shortcut even without this patch, so that seems like another bug (and down the rabbit hole we go...)

It might be nice to take the opportunity to modernize the syntax for the existing shortcut here, replacing the | with a + for readability, since you're touching that code anyway.

But now, to completely confuse you and undermine my credibility on the matter( ;-) ), let me recommend against including unrelated #include changes in this patch. There's a fine line to walk here, and personally, I think it's okay to modernize a piece of code that you happen to be touching, but it's best not to include unrelated code modernizations elsewhere in the file. Cleaning up those redundant #includes is a good idea, but should be separately, and preferably all at once, so it's easy to see if it causes any unexpected regressions.

This revision now requires changes to proceed.Mar 2 2018, 8:28 PM
rominf updated this revision to Diff 28450.Mar 3 2018, 5:02 AM
  • Modernize the syntax of shortcut
rominf added a comment.Mar 3 2018, 5:04 AM

Works as advertised, and doesn't conflict with any default shortcuts. The whole "type / to quick search/filter" is pretty well established in web browsers nowadays. Makes sense to have it in Dolphin, too.

Konsole still eats it when its panel is open, but it also eats the other ctrl+I shortcut even without this patch, so that seems like another bug (and down the rabbit hole we go...)

It might be nice to take the opportunity to modernize the syntax for the existing shortcut here, replacing the | with a + for readability, since you're touching that code anyway.

Done

But now, to completely confuse you and undermine my credibility on the matter( ;-) ), let me recommend against including unrelated #include changes in this patch. There's a fine line to walk here, and personally, I think it's okay to modernize a piece of code that you happen to be touching, but it's best not to include unrelated code modernizations elsewhere in the file. Cleaning up those redundant #includes is a good idea, but should be separately, and preferably all at once, so it's easy to see if it causes any unexpected regressions.

OK. Reverted those changes. Do you mind if I do massive commit (for the whole project) that cleans up the code?

ngraham accepted this revision.Mar 3 2018, 5:18 AM

But now, to completely confuse you and undermine my credibility on the matter( ;-) ), let me recommend against including unrelated #include changes in this patch. There's a fine line to walk here, and personally, I think it's okay to modernize a piece of code that you happen to be touching, but it's best not to include unrelated code modernizations elsewhere in the file. Cleaning up those redundant #includes is a good idea, but should be separately, and preferably all at once, so it's easy to see if it causes any unexpected regressions.

OK. Reverted those changes. Do you mind if I do massive commit (for the whole project) that cleans up the code?

As long as it's in the form of a reviewable Phabricator patch, I think that would be very welcome! Various other KDE projects (Baloo, KDE Connect) are also undergoing code clean-up and refactoring, in fact.

Since you now have a shiny new new contributor account, would you like to land the patch on Dolphin's master branch?

https://community.kde.org/Infrastructure/Phabricator#Step_3:_Land_your_diff

This revision is now accepted and ready to land.Mar 3 2018, 5:18 AM
This revision was automatically updated to reflect the committed changes.

Why should / trigger the filter bar and not the find bar?