On April 30th, 2012, 10:19 p.m., David Faure wrote:
I am in favour of the idea, since I was hit by this limitation in the past, too.
However I think the API changes should be more conservative, there's no need to deprecate so many of the existing methods, just because it is *possible* to define more than 2 shortcuts.
Also, Qt's QAction already has void QAction::setShortcuts(const QList<QKeySequence> &shortcuts)... remind me why we are not using that? Because it doesn't know "default shortcut" vs "current shortcut"?
I would love to remind you, but i simply don't know. I took it for granted since the current documentation talks about it. The doc above http://quickgit.kde.org/index.php?p=kdelibs.git&a=blob&h=d87755435e9131767e63311a8b3edc34adbf87ce&hb=dc217720bd685a30ef7914f09ab8c1e321b92c88&f=kdeui%2Factions%2Fkaction.h (line 316)
On April 30th, 2012, 10:19 p.m., David Faure wrote:
How does this allow multiple shortcuts per action?
The docu is a bit confusing, for a method that returns only one shortcut.
I think you mean this is the convenience method for returning the primary shortcut, and for other shortcuts, one should use shortcuts().
In that case, just remove the default value from the deprecated method, to fix the ambiguity.
Can i remove the default value? Isn't that going to break the API?
As for the name. Yeah, shortcut() returns shortcutS .. it's confusing indeed but i don't think i can change that without breaking the API. It's a function that seems to be used on quite a few places within KDE.
On April 30th, 2012, 10:19 p.m., David Faure wrote:
kdeui/actions/kaction.cpp
(Diff revision 1)
| void KAction::setShortcutConfigurable( bool b ) |
185
| Q_ASSERT(type);
|
185
Q_UNUSED(type);
|
This can't possibly be right.
You can deprecate the method, but don't break it.
It should still return the shortcut number 0 or 1, depending on the type argument.
I don't know about that.
This is how the function looks now (without the patch):
KShortcut KAction::shortcut(ShortcutTypes type) const
{
Q_ASSERT(type);
if (type == DefaultShortcut) {
QKeySequence primary = property("defaultPrimaryShortcut").value<QKeySequence>();
QKeySequence secondary = property("defaultAlternateShortcut").value<QKeySequence>();
return KShortcut(primary, secondary);
}
QKeySequence primary = shortcuts().value(0);
QKeySequence secondary = shortcuts().value(1);
return KShortcut(primary, secondary);
}
What it would do if i leave the if in there is:
KShortcut KAction::shortcut(ShortcutTypes type) const
{
Q_ASSERT(type);
if (type == DefaultShortcut) {
return KShortcut(shortcuts());
}
return KShortcut(shortcuts());
}
which is the same as simply returning return KShortcut(shortcuts()); thus "ShortcutTypes type" becomes useless.. Or am i missing something here?
On April 30th, 2012, 10:19 p.m., David Faure wrote:
kdeui/shortcuts/kshortcut.h
(Diff revision 1)
| public: |
91
| KShortcut(const QKeySequence &primary, const QKeySequence &alternate);
|
91
KDE_DEPRECATED KShortcut(const QKeySequence &primary, const QKeySequence &alternate);
|
I see no reason to deprecate this constructor.
It's just convenience for the two-shortcuts case.
Reverting this change.
On April 30th, 2012, 10:19 p.m., David Faure wrote:
kdeui/shortcuts/kshortcut.h
(Diff revision 1)
| public: |
101
| explicit KShortcut(int keyQtPri, int keyQtAlt = 0);
|
101
KDE_DEPRECATED explicit KShortcut(int keyQtPri, int keyQtAlt = 0);
|
Ditto.
Reverting this change.
On April 30th, 2012, 10:19 p.m., David Faure wrote:
kdeui/shortcuts/kshortcut.h
(Diff revision 1)
| public: |
139
| QKeySequence primary() const;
|
139
KDE_DEPRECATED QKeySequence primary() const;
|
Ditto.
Reverting this change.
On April 30th, 2012, 10:19 p.m., David Faure wrote:
kdeui/shortcuts/kshortcut.h
(Diff revision 1)
| public: |
145
| QKeySequence alternate() const;
|
145
KDE_DEPRECATED QKeySequence alternate() const;
|
Even this one could stay.
Reverting this change.
On April 30th, 2012, 10:19 p.m., David Faure wrote:
kdeui/shortcuts/kshortcut.h
(Diff revision 1)
| public: |
205
| QList<QKeySequence> toList(enum EmptyHandling handleEmpty = RemoveEmpty) const;
|
205
KDE_DEPRECATED QList<QKeySequence> toList(enum EmptyHandling handleEmpty = RemoveEmpty) const;
|
I don't understand why you want to deprecate this one. The argument is about handling of empty sequences, this is unrelated to "two, or more shortcuts", isn't it?
Yeah, that was just being over eager. Removed KDE_DEPRECATED and fixed it to work with the new internal structure.
- Mark
On April 30th, 2012, 8:59 p.m., Mark Gaiser wrote:
|
Review request for kdelibs.
By Mark Gaiser.
Updated April 30, 2012, 8:59 p.m.
Description
|
So i was trying to fix this bug: https://bugs.kde.org/show_bug.cgi?id=181531 That only asked for one more shortcut. That issue seems to be a little more complicated than it looks. Till this point KActions could only have a "Primary" and a "Alternate" shortcut. 2 in total which is - in some situations - not enough.
I fixed this by roughly restructuring all of the KShortcut cpp file. The API has remained the same but i would like to deprecate all things that go back to primary/alternate for the KDE 4.x cycle. Remove them for KDE Frameworks.
I have 2 issues remaining:
1. QList<QKeySequence> toList() const;
2. KShortcut shortcut() const;
If i enable those functions the compiler suddenly doesn't know which function to use.. The old ones or the new ones. Some suggestions on how to fix it would be welcome.
|
Testing
|
I tested this by adding the missing bindings for Dolphin's back/forward and it seems to be working just fine. I can use all available shortcuts.
|
Diffs
- kdeui/actions/kaction.h (d877554)
- kdeui/actions/kaction.cpp (309cf82)
- kdeui/shortcuts/kshortcut.h (c720830)
- kdeui/shortcuts/kshortcut.cpp (e307ab0)
View Diff
|
RSS Feed