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

List:       kde-core-devel
Subject:    Re: Review Request: Make KXMLGUIClient::set[Local]XMLFile() public
From:       "David Faure" <faure () kde ! org>
Date:       2009-08-19 16:18:43
Message-ID: 20090819161843.11746.29890 () localhost
[Download RAW message or body]


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/1229/#review2087
-----------------------------------------------------------

Ship it!


Missing @since 4.4.

OK for the replaceXMLFile addition. An addition to kdeui/tests/kxmlgui_unittest.cpp \
would be welcome too.

Just one note, about the "previous experiments" you did: there's another solution, \
reimplementing createContainer from KXMLGUIBuilder, like Konqueror does in \
KonqMainWindow::createContainer. This way you can at least prevent individual \
containers (menus or toolbars) from being created.

- David


On 2009-08-06 16:35:10, Thomas Friedrichsmeier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1229/
> -----------------------------------------------------------
> 
> (Updated 2009-08-06 16:35:10)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> -------
> 
> --- What this patch does:
> This patch makes KXMLGUIClient::setXMLFile() and KXMLGUIClient::setLocalXMLFile() \
> public, instead of protected. 
> --- What this is good for:
> The problem I'm trying to address is to allow flexibility in merging the GUI of \
> KXMLGUIClients, and esp. of KParts. Currently an embedding application has the \
> following options for merging a KParts GUI: 
> 1) Do not insert the part into the KXMLGUIFactory at all.
> 1b) Individual actions can be inserted, manually, if their names are known.
> 2) Merge the full GUI of the part.
> 2b) Individual actions can be removed from the actionCollection() manually, if \
> their names are known, and if the part does not rely on having the actions in the \
> collection. 
> Yet in some cases more control over GUI merging would be nice. This applies in \
> particular to applications that a) already come with a complex menu structure of \
> their own, and/or b) embed more than one part, and/or
> c) embed one of the more heavy kparts like khtmlpart of katepart, while possibly \
> needing only a limited subset of the functionality of those parts. 
> The solution that I'm trying to enable is to simply override the entire ui.rc of \
> the embedded client/part (before adding it to the KXMLGUIFactory, obviously). This \
> does require knowledge of the internal names of the KActions in question, but so do \
> solutions 1b and 2b, above. Apart from that it allows complete control in a \
> straight-forward manner. 
> --- Does this break things?
> Definitely not as long as the embedding application does not make use of the new \
> public functions. But let's look at what may break, when it does: 
> ------ In the embedded client:
> Obviously, the embedded client is only affected in the context of the embedding \
> application. It does still have an intact actionCollection(). Also, a kpart cannot \
> rely on being added to a KXMLGUIFactory at all. I don't see any potential for \
> breakage, here. 
> ------ In the embedding application:
> Things may break, here, if the KAction names change in the embedded client. This, \
> however is a problem that is inherent in the wish to achieve this sort of control \
> (also in solutions 1b and 2b, above). 
> Things would also break, if the embedded client were to call set[Local]XMLFile (or \
> one of the other setters) *after* the embedding client has tried to override the \
> ui-definition. Yet, this usage is highly atypical. In general setXMLFile() and \
> friends are called only during the construction phase, and in fact they do not \
> usually have any effect at all if called after the client was added to the factory \
> (unless the factory is told to rebuild, which the embedding application could try \
> to detect, if needed). 
> ------ What about that "virtual" keyword? Won't things break, when setXMLFile() is \
> re-implemented, in a client? I have no idea, why these functions are declared \
> virtual at all, or what would be the point of re-implementing them. lxr.kde.org \
> does not show any re-implementations. Using wcgrep I did not find relevant matches \
> to "void setXMLFile", either. I suggest, that should be scheduled for removal in \
> the next BIC release, but that's beyond the scope of this review request. 
> --- Overall:
> I admit, this solution is not terribly elegant in some respects, esp. in requiring \
> knowledge of the KAction names. Yet, I think the problem is real, and this solution \
> provides a decent way to cope with it. 
> --- Related reading:
> I've been fighting this problem for a while, now. See \
> http://lists.kde.org/?l=kde-devel&m=112911871111633&w=2 (and follow-ups) and \
> http://techbase.kde.org/Development/Architecture/KDE4/XMLGUI_Technology#stripping_down_a_KPart.27s_GUI \
> for an approach using setXMLGUIBuildDocument() (which is much hackier for obvious \
> reasons). 
> 
> Diffs
> -----
> 
> trunk/KDE/kdelibs/kdeui/xmlgui/kxmlguiclient.h 1006783 
> trunk/KDE/kdelibs/kdeui/xmlgui/kxmlguiclient.cpp 1006783 
> 
> Diff: http://reviewboard.kde.org/r/1229/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Thomas
> 
> 


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