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

[David Faure]

> On 2009-09-25 10:09:18, Thomas Friedrichsmeier wrote:
> > I see, I'm a bit late to reply. But anyway:
> > 
> > I have not really tested this, as I do not generally have the time to follow SVN closely. I do not think this will affect me / my application in any way, but:
> > 
> > In light of this: http://reviewboard.kde.org/r/1238/, I think it would generally be cleaner if XMLGUI-changes are always written to disc somewhere. At least if any third application is expected to embed this client. (The details of the linked patch don't matter too much, here, but to solve the synchronization problems outlined in that review request, hosting applications will probably always rely on reloading the XML from disc one way or another. Alternative: Provide public API to make an XMLGUIClient reload the settings from the localXMLFile, only).

Thanks for the feedback.

Well, I don't see how the two things are contradictory. If the user edits toolbars, we still
save the result to a file, and other instances of the application can reload that, i.e. update
the QDomDocument inside their kxmlguiclient.
The only thing I'm changing here is that kedittoolbar starts from the domdocument of the guiclient
from memory, rather than from disk. But for the GUI to be correct already, surely your "syncer"
has to update the domdocument of the guiclient, so no problem?

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

You are right about the fact that the "syncer" means reloading from disk, which in the
kaddressbookpart-inside-kontact case [if kontact wasn't a uniqueapplication ;)] would mean
that those dynamic changes to the XML would have to be redone. Just like they are currently
redone when editing toolbars, btw. (cf MainWindow::slotNewToolbarConfig() in kontact).
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... 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.
Well there's probably a better design for all this, but reviewboard isn't the best place
for thinking about and discussing design :-)
(hmm, and now I'm commenting on r/1238 in the comments for r/1694 :-)

Anyway, going back to this patch, I think that overall the future syncer doesn't prevent
kedittoolbar starting from the in-memory domdocument in order to allow dynamic changes,
but feel free to prove me wrong :)

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


- David


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


On 2009-09-22 18:38:22, David Faure wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1694/
> -----------------------------------------------------------
> 
> (Updated 2009-09-22 18:38:22)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> -------
> 
> Users of KXmlGuiClient::setDOMDocument probably noticed that the dynamic changes
> made to the guiclient's XML, where not taken into account in KEditToolBar. 
> 
> This is because KEditToolBar loads the XML file again from disk, rather than using
> the in-memory XML tree. (And this in turn is probably because in KDE3, we had
> KXMLGUIClient::conserveMemory which got rid of the in-memory xml tree, but we removed
> it for KDE4 because it broke any kind of dynamic changes like action lists, plugins etc.)
> 
> So the fix is simple: using the in-memory domDocument() instead. The question is what
> it breaks. I noticed that the ui_standards.rc merging left the name and version attributes
> of ui_standards.rc into the toplevel tag -- fixed in r1026838.
> 
> If your app does unusual stuff around xmlguiclients and/or toolbar editing, please
> test this patch before I commit it.
> 
> The reason for all this is bug 207296, but surely I'm not the first user of setDOMDocument.
> I see that knotes, kopete, and kross use it already, at least.
> 
> 
> This addresses bug 207296.
>     https://bugs.kde.org/show_bug.cgi?id=207296
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdelibs/kdeui/dialogs/kedittoolbar.cpp 1025321 
> 
> Diff: http://reviewboard.kde.org/r/1694/diff
> 
> 
> Testing
> -------
> 
> Editing toolbars in kontact, konqueror, kate.
> 
> [found an unrelated bug while testing in konq, the Tools menu moves to first position, even w/o the patch...]
> 
> 
> Thanks,
> 
> David
> 
>