Check all changed shortcuts on all platforms.
Details
Details
- Reviewers
rizzitello elvisangelaccio - Group Reviewers
Dolphin - Commits
- R318:ec12391a1bb8: Convert hard coded shortcuts to standard keys
Diff Detail
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.
| src/dolphinmainwindow.cpp | ||
|---|---|---|
| 1077 | QKeySequence::AddTab == Ctrl + T . | |
| 1108 | This is the default for QKeySequence::Deselect. If this is used in other Kapplications this could create an inconsistency in a common action. Maybe we can come up with a different shortcut for this? Crtl+I is common or Ctrl + Shift + I . Personally I am not fond of using another letter maybe ctrl+alt+ A ? | |
| src/dolphinmainwindow.cpp | ||
|---|---|---|
| 1077 | As written in https://doc.qt.io/qt-5/qkeysequence.html#details QKeySequence::AddTab = Ctrl+T for all DE, except KDE (Ctrl+⇧+N; Ctrl+T). But in practice, only the first sequence (Ctrl+⇧+N) works. In my opinion Ctrl+T should work for KDE too, hence the code. | |
| src/dolphinmainwindow.cpp | ||
|---|---|---|
| 1077 | I agree that Ctrl+T should be our main shortcut. This looks like a work around for a Qt bug now so I get why we are sending both. Maybe we should talk with Qt about resolving this bug? | |
| src/dolphinmainwindow.cpp | ||
|---|---|---|
| 1077 | I'm not sure that it's Qt bug. It can be the bug of kxmlgui (actionCollection()->setDefaultShortcut()). I wanted to verify this, but couldn't build it on my openSUSE, because it has extra-cmake-modules version 5.43, while kxmlgui requires 5.44. If you have a chance, please, try it. | |
| src/dolphinmainwindow.cpp | ||
|---|---|---|
| 1077 | I was not able to build with kxmlgui 5.44 . I did test a non kxmlgui application using QKeySequence::AddTab and it seams for KDE crtl+t does not trigger the action. | |
| src/dolphinmainwindow.cpp | ||
|---|---|---|
| 1077 | I have created a Bug Report for the QKeysequence problem | |
| src/dolphinmainwindow.cpp | ||
|---|---|---|
| 1072–1073 | It stopped to work after I made this change. | |
| src/dolphinpart.cpp | ||
|---|---|---|
| 188 | should a shortcut be set for this action? | |
| src/dolphinpart.cpp | ||
|---|---|---|
| 188 | I don't know. I just unhardcoded (softcoded?) shortcuts | |
| src/dolphinpart.cpp | ||
|---|---|---|
| 188 | Whether or not this action should get a shortcut (I think it should!) Is unrelated to this patch and should be handled separately. But in fact I don't even see it exposed in the UI at all... | |
| src/dolphinmainwindow.cpp | ||
|---|---|---|
| 1072–1073 | How did you do it? Did you use KStandardAction::openNew()? Have a look at dolphinviewactionhandler.cpp where we create some KStandardActions. | |
| src/dolphinmainwindow.cpp | ||
|---|---|---|
| 1072–1073 | OK. Done. Please review. | |
Comment Actions
What I said in D11012 applies also here: commit message should be more descriptive.
| src/dolphincontextmenu.cpp | ||
|---|---|---|
| 220 ↗ | (On Diff #28992) | This is a different action, please revert. |
| 253 ↗ | (On Diff #28992) | Same. This is a different action, please revert the change. |
| src/dolphinmainwindow.cpp | ||
| 861 | We also need to replace select_all with edit_select_all (which is the name from kstandardaction) in dolphinui.rc, otherwise the action won't show up in the Edit menu. | |
| 994 | Same: please replace close_tab with file_close in dolphinui.rc. | |
| 1071 | Please replace new_window with file_new in dolphinui.rc. | |
| 1126–1127 | This should be ported to KStandardAction::redisplay(). | |
| src/dolphinpart.cpp | ||
| 183 | Please port to KStandardAction::selectAll() also here. | |
| src/dolphinui.rc | ||
|---|---|---|
| 2 | You should increase version to 18 | |