Requesting freeze exception for JtG
faure at kde.org
Thu Dec 8 15:56:16 GMT 2011
On Wednesday 23 November 2011 00:28:07 Pau Garcia i Quiles wrote:
> > 1) Changes to ui_standards.rc
> > This is a high-risk change. It affects every application that uses
> > XMLGUI and KParts plugins. I did quite a lot of work in this area in
> > KDE 3.x and I know that changes to this file could have a lot of
> > knock-on effects regarding gui merging at the time. I don't know if
> > this is still the case, but it's a big concern IMHO.
> Now that we have established that there might be a risk (I took care
> of not breaking anything, but you can never double-check enough),
> could you please check the patch does not break anything? I'd like to
> move forward as soon as possible so that the strings can be
In general, I agree that ui_standards.rc is a touchy place. On the other hand,
the help menu is typically "all or nothing" in the apps, and adding an action
should be fine (less potential changes than adding a group, or a merge point,
or moving stuff around).
But why is the action added to KStandardAction? The only user (in terms of
code) is KHelpMenu, isn't it? Why not just define and create the action there?
Adding API for this seems overkill. Imagine we want to cancel (or rename) that
program at some point, surely we don't want source compatibility to prevent us
to do that.
I see that you did that "to mimic what was already there", but it's not
useful. Just have a KAction* createJoinTheGameAction() in khelpmenu and call
that from both the "add to kactioncollection" code path and the menu() method.
khelpmenu seems to have been ported from qt3 without much thinking, the
actions in menu() could just use kstandardaction too, instead of duplicating
the action details... but that's for frameworks 5 :)
The addition to KStandardShortcut is even less useful, without a default
shortcut. Please remove that altogether.
khelpmenu.cpp: weird indentation, with statements starting at column 0.
All this is while reviewing this commit:
David Faure, faure at kde.org, http://www.davidfaure.fr
Sponsored by Nokia to work on KDE, incl. KDE Frameworks 5
More information about the kde-core-devel