Review Request: Make KXMLGUIClient::set[Local]XMLFile() public

Thomas Friedrichsmeier thomas.friedrichsmeier at
Thu Aug 6 17:35:11 BST 2009

This is an automatically generated e-mail. To reply, visit:

(Updated 2009-08-06 16:35:10.487227)

Review request for kdelibs.


Ok, version 3 of the diff.

This time I've added a new non-virtual public function replaceXMLFile(), which asks for both the xmlFile and the localXMLFile at once. Note that this is not typing-laziness. Rather when overriding the xmlFile (which an absolute file name), it should be encouraged to also set the localXMLFile, so shortcut / toolbar changes can be saved. Another nice side-effect is that this provides a good way to sort the api-documentation.

I check for and warn on non-absolute parameters for the xmlfile. I think in almost all cases, an absolute file name should be specified, here, as else the lookup would happen in the context of the component of the client. But perhaps I'm missing a use case, where the component in the KXMLGUIClient itself and in the code calling replaceXMLFile() would be identical?

The patch starts out with another half, however. I discovered, that settings are written to localXMLFile() alright (only since SVN rev 994640, however, not in KDE 4.3.0!). But the localXMLFile() is never merged back when loading the xmlFile(). The first half of the patch fixes that.


--- 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. 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 (and follow-ups) and for an approach using setXMLGUIBuildDocument() (which is much hackier for obvious reasons).

Diffs (updated)

  trunk/KDE/kdelibs/kdeui/xmlgui/kxmlguiclient.h 1006783 
  trunk/KDE/kdelibs/kdeui/xmlgui/kxmlguiclient.cpp 1006783 





More information about the kde-core-devel mailing list