> On April 30, 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 30, 2012, 10:19 p.m., David Faure wrote: > > kdeui/actions/kaction.cpp, line 185 > > <http://git.reviewboard.kde.org/r/104801/diff/1/?file=59740#file59740line185> > > > > 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 30, 2012, 10:19 p.m., David Faure wrote: > > kdeui/actions/kaction.h, line 326 > > <http://git.reviewboard.kde.org/r/104801/diff/1/?file=59739#file59739line326> > > > > 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 30, 2012, 10:19 p.m., David Faure wrote: > > kdeui/shortcuts/kshortcut.h, line 91 > > <http://git.reviewboard.kde.org/r/104801/diff/1/?file=59741#file59741line91> > > > > I see no reason to deprecate this constructor. > > It's just convenience for the two-shortcuts case. > > Reverting this change. > On April 30, 2012, 10:19 p.m., David Faure wrote: > > kdeui/shortcuts/kshortcut.h, line 101 > > <http://git.reviewboard.kde.org/r/104801/diff/1/?file=59741#file59741line101> > > > > Ditto. Reverting this change. > On April 30, 2012, 10:19 p.m., David Faure wrote: > > kdeui/shortcuts/kshortcut.h, line 139 > > <http://git.reviewboard.kde.org/r/104801/diff/1/?file=59741#file59741line139> > > > > Ditto. Reverting this change. > On April 30, 2012, 10:19 p.m., David Faure wrote: > > kdeui/shortcuts/kshortcut.h, line 145 > > <http://git.reviewboard.kde.org/r/104801/diff/1/?file=59741#file59741line145> > > > > Even this one could stay. Reverting this change. > On April 30, 2012, 10:19 p.m., David Faure wrote: > > kdeui/shortcuts/kshortcut.h, line 205 > > <http://git.reviewboard.kde.org/r/104801/diff/1/?file=59741#file59741line205> > > > > 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 ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104801/#review13178 ----------------------------------------------------------- On April 30, 2012, 8:59 p.m., Mark Gaiser wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/104801/ > ----------------------------------------------------------- > > (Updated April 30, 2012, 8:59 p.m.) > > > Review request for kdelibs. > > > 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. > > > Diffs > ----- > > kdeui/actions/kaction.h d877554 > kdeui/actions/kaction.cpp 309cf82 > kdeui/shortcuts/kshortcut.h c720830 > kdeui/shortcuts/kshortcut.cpp e307ab0 > > Diff: http://git.reviewboard.kde.org/r/104801/diff/ > > > 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. > > > Thanks, > > Mark Gaiser > >
