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

[David Faure]

On Friday 25 September 2009, Thomas Friedrichsmeier wrote:
> Taking this to the mailing list as suggested.

Thanks. KMail is much more convenient than reviewboard for replying to 
individual paragraphs ;-)

> 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.

OK.
Well, good opportunity to get me to think about the synchronization issue ;-)
(I saw the review request but had no time for it until now)

> 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...

And that's a difference with the kaddressbookpart case: the same xmlguiclient
(well, rather, the same rc file shared by two guiclients) is used by the part 
and the standalone application.
The solution I committed today is to mark two actions as 
"remove_in_kontact=true" in the xml, and have some code inside kontact which 
removes those actions from the domdocument, then calls setDomDocument.
This solution does NOT prevent using the same xmlguiclient elsewhere.......
until someone edits toolbars. Then the modified file gets written out and the 
standalone kaddressbookpart loses two actions. It's kind of ok in this case 
because most people use -either- kontact or kaddressbook, but this is 
obviously not a good generic solution.
So I'm interested in this discussion, because we might as well find a better 
solution that works in all cases indeed.
Something like replaceXmlFile indeed, but I'm not sure how to make this
work in all cases (no local xml file yet, modified local xml file but the app is 
upgraded and a newer version of the app xml appears, etc.).

> >  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.

You mean... during the app's lifetime, right?
But if it's a tempfile, then kedittoolbars would write to it, and you would 
lose the changes made by the user when you restart the app, no?

Seems to me that what is needed in a simple setLocalXmlFile() [if we ignore 
the fact that it's protected, that's easy to circumvent].

> 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).

Oh... I see. This doesn't trigger a GUI rebuild, but indeed it means re-
creating the domdocument from the on-disk file. Argh :-)
This makes the whole setDomDocument() idea moot.

It's starting to really look like we have to do everything with on-disk files
indeed.

> 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.

http://lxr.kde.org :-)

> >  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?

Well, there are solutions. Like, making synchronization optional, so that apps 
which enable synchronization also have to connect to some new signal
and re-do the dynamic stuff there (plugActionList, possibly domDocument hacks
even though I'm starting to think those are a bad idea).

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

Well, we can move code indeed, but we can't trigger the stuff the apps
have to do (like plugActionList etc.) from there...

> 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.

Right.

> 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.

One of us is very confused about this.
AFAIK reloadXML only reloads the local XML file.
There is no merging between xmlFile and localXmlFile.
There are enough mergings as it is in kxmlgui:
 - the ui_standards.rc + appui.rc merging
 - the merging when adding clients to the factory (known as "the kparts case" 
even though the mechanism is available to much more than kparts nowadays).

reloadXML calls setXMLFile(file (with merge defaulting to false)), which looks 
for the most-local-xml file and loads it (setXML(xml, merge=false)). I see no 
merging there.

However I do see what you mean by "losing application customizations",
which are those changes hardcoded in the app, those which are triggering
this whole discussion. But now I think the best way to handle those would be
to simply have code like this:

given a part with the xml file "part/part.rc", and we want to customize it
inside the app "kfoo", kfoo would copy part.rc to 
kdehome/share/apps/kfoo/kfoo-part.rc (any time these two differ)
and do the customizations, then call
  replaceXmlFile("kdehome/share/apps/kfoo/kfoo-part.rc", 
"kdehome/share/apps/part/local-kfoo-part.rc");
(the local- isn't necessary but will make reading this mail easier)

Basically apps/kfoo/kfoo-part.rc plays the role of a "global" file,
but it has to be local because it's generated at the time the user runs the 
application.

This has the following benefit:

  * when the user edits toolbars, the changes get saved to local-kfoo-part.rc
and all the use cases for reloading work fine; the app-specific customization
code does NOT have to re-run.

  * when the developer changes part.rc, a new customized kfoo/kfoo-part.rc is 
made, including the app-specific customizations and (since it has a newer 
version number) it will override local-kfoo-part.rc, that's the standard 
behavior for handling changes after an upgrade. Shortcuts are kept, toolbar 
customizations are not, this is expected.

And it's easy to implement... it's the current code for modifying that xml 
document + saving to a file + replaceXmlFile(). I'll give this a try right 
away.

AFAICS this is the only way to implement what we need, for our 3 use cases:
a) kontact hides some kaddressbook actions; the above will make a rc file on 
disk with the actions removed and this way they won't appear in kedittoolbar;
and the changes made in kontact will NOT affect kaddressbook-standalone,
since that still uses the unmodified part/part.rc.
Also, the user _can_ add again those actions that the app removed, which 
wasn't possible before today either (when the app blindly removed these 
actions by name at createGUI time...).
b) your case of setting the xml for a specific part from the outside (which is
the same except that it doesn't have the problem of not affecting the other 
users of the part)
c) the synchronizer, which has to make the app reload the XML; with this
solution there is no need to re-run the app-specific customizations; it will
reload the file from disk (kfoo-part.rc or local-kfoo-part.rc), which includes
these customizations already.

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

In the case of the synchronizer, running after someone edit toolbars,
this is the only thing that can be done indeed. reloadXML isn't enough
in this case, of course, one also needs a removeClient+addClient plus
redoing the dynamic stuff like actionLists. New signal needed.

-- 
David Faure, faure at kde.org, sponsored by Nokia to work on KDE,
Konqueror (http://www.konqueror.org), and KOffice (http://www.koffice.org).