Review Request: Use in-memory dom document when editing toolbars, to allow for dynamic changes

[Thomas Friedrichsmeier]

Hi!

Taking this to the mailing list as suggested.

First to clarify: My comment does not concern your patch that directly. Your 
patch does not change really change anything in that respect. But your patch 
addresses the case of making changes using setDomDocument(), i.e. changes 
which are not (necessarily) written to disk, and that's the thing I am 
concerned about. Sorry for causing this confusion.

On Friday 25 September 2009, David Faure wrote:
> I think the confusion is about which "kxmlguiclient changes" we're talking
>  about. The use case I see for setDomDocument() (which means, dynamic
>  changes not on disk), is the case where applications hardcode stuff like
>  "remove those actions that should not appear in this context" (e.g.
>  kaddressbookpart-inside-kontact).

In fact, as of now, I'm doing the same in RKWard. (See also this (submitted) 
request of mine: http://reviewboard.kde.org/r/1229/). This is all perfectly 
fine as long as we're talking about the "top" level of embedding, i.e. there is 
no expectation that the modified xmlguiclient will be embedded anywhere else, 
directly or indirectly. I guess this is another thing I should have made clear 
from the beginning...

>  This is not a user-configured change, it
>  should not be written to disk upfront (if it was, it would mess up
>  kaddressbook-standalone, while at least now it only does that when the
>  user configure toolbars; corner case).

Writing to disk would not necessarily mean writing to the original location, 
though. Annoyed by the need to re-apply my local xml modifications time and 
again, when changes happened (well, aesthetically annoyed - it's not really 
that difficult, just ugly), I was considering to write out the modified xml to 
disk in a local/temporary path, then call replaceXMLFile() to use this modified 
file on future reloads.

Note that there appear to be a number of occasions when the client's xml is 
reloaded besides editing toolbars. E.g. 
KXMLGUIFactory::refreshActionProperties() (called when switching shortcut 
schemes, IIRC). I don't currently have a source tree attached to grep through, 
but I guess all instances where KXMLGUIClient::reloadXML() is used may need 
similar patches as well.

> In fact
>  this isn't the only thing that needs to be done after rebuilding the GUI
>  (from edittoolbars), the other thing is plugging back any ActionLists. For
>  that reason, the "syncer" probably breaks actionlists right now...

Eww. I'll need to check this.

>  With
>  the current infrastructure, it sounds like what we need is a
>  kxmlguifactory signal that apps would connect to that same
>  slotNewToolbarConfig slot, which is where the code for "stuff that has to
>  be done after createGUI" is located. It would be emitted by the method you
>  mention it should have, for removing+readding all clients, moving that
>  code out of kedittoolbarwidget.

Indeed, that would make things easier. But this sounds like apps would need to 
adjust to this, so not possible before KDE 5?

Even without this, though, couldn't/shouldn't all this code from 
kedittoolbarwidget be moved into a proper function 
KXMLGuiFactory::rebuildTheWholeGuiWithAllBellsAndWhistles()?

> PS: I don't understand your "Alternative" suggestion.

Well, overall it seems we have three problems, here. That "alternative" 
suggestion concerns number 2, only:

1) Detecting when there are changes in other instances of the same 
XMLGUIClient that should be propagated. Solvable by simply monitoring the 
localXMLFile() for changes.

2) Merging only the changes (configured shortcuts / toolbars), without throwing 
away customizations. I admit, I'm not quite sure the solution is this simple, 
but my idea was roughly: "Well, so instead of reloading the original xmlFile() 
*and* then merging it with the localXMLFile(), why don't we just skip the first 
step and only merge the new version of the localXMLFile()". So basically in 
most places where reloadXML() is called right now, reloadLocalXML() or 
something like that would be called. Wouldn't it be possible to preserve 
applicaiton customizations while still merging user changes this way?

3) Properly rebuilding the GUI after the changes have been merged. The problem 
with ActionLists you pointed out.

Regards
Thomas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20090925/ed7756e3/attachment.sig>