This patch adds the shortcut ⇧+F for zoom option Fill.
CCBUG: 396360
Details
Details
- Reviewers
rkflx - Group Reviewers
Gwenview - Commits
- R260:9b898af627ab: Add shortcut Shift+F for zoom option "Fill"
- Open Gwenview in View Mode
- Use ⇧+F to switch to Fill zoom factor
Diff Detail
Diff Detail
- Repository
- R260 Gwenview
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
| lib/documentview/documentviewcontroller.cpp | ||
|---|---|---|
| 109–111 | The standard action may have any system shortcuts which we should not replace. | |
| 110 | This seems to be a copy/paste error from 3e10699ac37c. | |
Comment Actions
Thanks for the patch. I should triage more bugs if this seems to result in quick fixes ;)
| lib/documentview/documentviewcontroller.cpp | ||
|---|---|---|
| 109–111 | When testing I realized I mentioned the wrong shortcut for 100%: It's actually Ctrl+0, which makes even more sense if you consider how this would work on an English keyboard layout. On a German layout this probably means pressing Ctrl instead of ⇧, with 0 and = being on the same key. As this is a KStandardAction::ActualSize, could you check whether we could add the shortcut for everyone in R265 KConfigWidgets, like it's done already for KStandardAction::ZoomOut? In particular check https://lxr.kde.org for conflicts in existing apps. Thanks ;) | |
| 110 | Thanks for noticing, makes sense. In fact, https://doc.qt.io/qt-5/qaction.html#checked-prop reads
…so you could even remove both calls to setChecked(false). | |
Comment Actions
But only that bugs which are quickly fixed. ;)
| lib/documentview/documentviewcontroller.cpp | ||
|---|---|---|
| 109–111 | I found no conflicts. Some code uses Ctrl+0 but in this case the shortcut is replaced and will not raise any error. | |
| lib/documentview/documentviewcontroller.cpp | ||
|---|---|---|
| 109–111 |
| |
Comment Actions
With monthly KF5 releases users should be able to wait until new features become available in Gwenview, so we don't have to add unnecessary fallback code. Actually KF 5.49 will be released 5 days earlier than KDE Applications 18.08 :D (But it is optional, as it's not worth or even allowed to bump the required KF5 version that late.)
Interesting, so the action and the shortcut live in two different places.
Anyway, I also checked for conflicts now (Qt::Key_0 and KStandardAction::ActualSize). Most things work out great (e.g. KWebKitPart, KStars, Falkon etc.), but I found Cirkuit (frameworks branch), which defines both the standard action and a different action with Ctrl+0, leading to a conflict. Would it be too much to ask to solve this, e.g. by removing the conflicting standard shortcut?
Apart from that we should be fine (nevertheless wait for the actual KF5 reviewer).
Comment Actions
Not sure what you mean. Should we submit a diff for Cirkuit or ask someone of its team to solve this?
Comment Actions
LGTM, cannot get any simpler really…
Well, in general silently breaking other apps is not nice. In this case there does not seem to be lots of active development, so perhaps just posting a Diff and tagging the contributors of the last few patches (Kai and Andreas) is better than opening a bug or asking others to solve the issue for us. (Let me know if I should help, IOW if I was asking too much from you ;)
Comment Actions
Just found this wish for adding Ctrl+0 to KStandardAction::ActualSize.
https://bugs.kde.org/show_bug.cgi?id=305702
Comment Actions
It's true that there will be a conflict if users have manually set Ctrl+0 to something else than ActualSize. Nevertheless I'd assume that most people who set it will actually use it for ActualSize. I'd say in this case adding the shortcut for everyone is more important than the (likely) very few outliers.
Also, similar browser-inspired shortcuts have been changed in the past, see D6553 and others. I'd say you should give it a try, and add dfaure, Kai and Nate as reviewers.