Review Request: Fix KShortcut to really allow the usage of multiple shortcuts

Tue May 1 19:05:01 BST 2012

> 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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20120501/d378e88f/attachment.htm>