[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-panel-devel
Subject:    Re: Review Request: config
From:       Michael Jansen <info () michael-jansen ! biz>
Date:       2009-04-30 21:46:03
Message-ID: 200904302346.04035.info () michael-jansen ! biz
[Download RAW message or body]

Hi

I can't say anything on the plasma side of the patch. But some word regarding 
Actions/ActionCollections.

    KAction *configAction = new KAction(i18n("Widget Settings"), actions);
    configAction->setIcon(KIcon("configure"));
    configAction->setShortcut(KShortcut("alt+d, s"));
    actions->addAction("configure", configAction);

The code is correct and will work that way. But we should try to get everyone 
used to do:

    KAction *configAction = actions->addAction("configure");
    configAction->setIcon(KIcon("configure"));
    configAction->setText("Widget Settings"))
    configAction->setShortcut(KShortcut("alt+d, s"));

This is more future proof. If someone later decides to add an global shortcut 
or just allow one things tend do go wrong with the first version. 

The Developer just adds the relevant line. The following version won't work. 
To set a global shortcut the action must have a id.

    KAction *configAction = new KAction(i18n("Widget Settings"), actions);
    configAction->setIcon(KIcon("configure"));
    configAction->setGlobalShortcut(KShortcut("alt+d, s"));
    actions->addAction("configure", configAction);

Swapping the last to lines would make it work again but i more often see:

    KAction *configAction = new KAction(i18n("Widget Settings"), actions);
    configAction->setObjectName("configure");
    configAction->setIcon(KIcon("configure"));
    configAction->setGlobalShortcut(KShortcut("alt+d, s"));
    actions->addAction("configure", configAction);

This version works but you now have to manually make sure both id's are 
identical. And they tend to not be identical or diverge later. Which will make 
your global shortcuts stop working and your application printing out some 
warnings.

Mike


On Thursday 30 April 2009 08:58:24 Chani wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/639/
> -----------------------------------------------------------
>
> (Updated 2009-04-29 23:58:24.808392)
>
>
> Review request for Plasma.
>
>
> Changes
> -------
>
> code is cleaner now :) and more stuff works.
>
>
> Summary (updated)
> -------
>
> -it's still modal
> -needs testing without my local patch that fixes a qt shortcut bug
>
>
> Diffs (updated)
> -----
>
>   /trunk/KDE/kdelibs/plasma/applet.h 960161
>   /trunk/KDE/kdelibs/plasma/applet.cpp 960161
>   /trunk/KDE/kdelibs/plasma/containment.h 960161
>   /trunk/KDE/kdelibs/plasma/containment.cpp 960161
>   /trunk/KDE/kdelibs/plasma/corona.h 960161
>   /trunk/KDE/kdelibs/plasma/corona.cpp 960161
>   /trunk/KDE/kdelibs/plasma/private/applet_p.h 960161
>   /trunk/KDE/kdelibs/plasma/private/containment_p.h 960161
>
> Diff: http://reviewboard.kde.org/r/639/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chani
>
> _______________________________________________
> Plasma-devel mailing list
> Plasma-devel@kde.org
> https://mail.kde.org/mailman/listinfo/plasma-devel

_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
[prev in list] [next in list] [prev in thread] [next in thread]