proposal for kaction api change

[Hamish Rodda]

On Friday 05 May 2006 16:22, Jason Harris wrote:
> Hi,
>
> Jens Herden wrote:
> > inspired by a discussion on kde-devel I was looking into kaction and I
> > would like to propose to clean up the interface a little. There are many
> > methods related to shortcuts and I think we should reduce them.
>
> As the one who started that thread, I'm obviously interested in such a
> cleanup :)

Great to see people discussing this, I agree it isn't quite optimal (I rewrote 
it a bit, but the global stuff was added afterwards).

> > enum ShortcutType {
> >       /// normal shortcut
> >       Shortcut = 0x1,
> >       /// default shortcut
> >       DefaultShortcut = 0x2
> >       /// global shortcut
> >       GlobalShortcut = 0x4
> >       /// default global shortcut
> >       DefaultGlobalShortcut = 0x8
> >     };
>
> This is better than the current scheme.  However, I still think that the
> word "Default" strongly implies that it will be used in the absence of
> other defined shortcuts, which (IIUC) is not the case, even under your
> proposed scheme.  I guess I still don't understand the point of the Default
> shortcut, as currently implemented.  By definition, the "Default" shortcut
> must be whatever the program sets when you start it for the first time.

Yes, it is really the "OriginalShortcut" or "ProgrammerDefinedShortcut", but I 
like your next suggestion...

> I think we ought to regard the Default shortcut as the "normal" one.  The
> Custom shortcut should only rarely be used, when the user decides to
> override the shortcuts defined by the program.  So my enum would look like:
>
> 	Shortcut = 0x1,  //normal (Default) shortcut
> 	CustomShortcut = 0x2,  //user-defined custom shortcut (overrides default)
> 	GlobalShortcut = 0x4,
> 	GlobalCustomShortcut = 0x8

I like this better than what I had :)

> However, I'd actually prefer separate accessors for the two types of
> shortcuts, rather than passing an enum argument.

I somewhat agree here, in that they have quite different implementations.  
However, a more uniform api might be more convenient for users... I'd like to 
hear other opinions on this.

> Here's how I think it should work:
>
> ...
> 	//in the init code, setting up actions:
> 	KAction *ka = new KAction( KIcon( "window_new" ), i18n("&New Window"),
> actionCollection(), "new_window" );
> 	ka->setShortcut( KShortcut( "Ctrl+N" ) );
> ...
>
> At this point, the shortcut for a new window is "Ctrl+N", because no Custom
> shortcut has been defined.  This is implicitly the "Default" shortcut.

This I disagree with, you should have to set the CustomShortcut to the same as 
the default shortcut, otherwise you could not disable the default shortcut.  
I like that the default for setShortcut sets the default and custom 
shortcuts.

> The code has a slot triggered when the user wants to set a custom shortcut:
>
> void slotSetCustomShortcut( const KShortcut &ks )
> {
> 	actionCollection()->action("new_window")->setCustomShortcut( ks );
> }
>
> At this point, the shortcut for a new window has been replaced by whatever
> the user specified.  Internally, both the default Shortcut and
> CustomShortcut have now been defined, and when this is the case, the
> CustomShortcut is used.

Fine.

> Now, the user decides to go back to the default, and presses a button to
> trigger this slot:
>
> void slotRevertToDefaultShortcut()
> {
> 	actionCollection()->action("new_window")->setCustomShortcut( KShortcut()
> ); }
>
> The CustomShortcut is now null, and so the program reverts to the "default"
> shortcut that was defined in the init code ("Ctrl+N").

This I again disagree with for the reason above.  You should have to set the 
CustomShortcut to the same as the default shortcut, otherwise you could not 
disable the default shortcut.

Cheers,
Hamish.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20060506/6d3c250f/attachment.sig>