API dox: fix missing note to call setXMLFile with KParts::MainWindow
ClosedPublic

Authored by kossebau on Mar 26 2017, 1:50 PM.

Details

Summary

Calling KXmlGuiWindow::setupGUI(flags, xmlfile) without the "Create"
flag will result in the xmlfile argument being ignored.
So setXMLFile(xmlfile) needs to be explitly called.

Also

  • adds some @c markup for code snippets
  • fixes "Default flag" -> "Default flag set"
  • adds KXmlGuiWindow::Create to links for warning, as it has the used info

Diff Detail

Repository
R306 KParts
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
kossebau created this revision.Mar 26 2017, 1:50 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMar 26 2017, 1:50 PM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
kossebau edited reviewers, added: Frameworks; removed: KDevelop.Mar 26 2017, 1:58 PM
kossebau removed a subscriber: Frameworks.
kossebau edited the summary of this revision. (Show Details)Mar 26 2017, 2:02 PM
dfaure edited edge metadata.EditedMar 26 2017, 2:53 PM

Seems right. The alternative is to make it work.
Not sure which solution is better.

--- kxmlgui/src/kxmlguiwindow.cpp
+++ kxmlgui/src/kxmlguiwindow.cpp
@@ -203,6 +203,8 @@ void KXmlGuiWindow::setupGUI(const QSize &defaultSize, StandardWindowOptions opt
 
     if (options & Create) {
         createGUI(xmlfile);
+    } else if (!xmlfile.isEmpty()) {
+        setXMLFile(xmlfile);
     }
 
     if (d->defaultSize.isValid()) {
In D5181#97662, @dfaure wrote:

Not sure which solution is better.

Considered that alternative as well, but then chose to propose the given patch for these reasons:

  • code supporting KF <5.34 would still need some patching, just having the extra setXMLFile(...) line without #if-version is less complex code
  • lxr.kde.org hints that lots of existing code uses setXMLFile(...), so restoring old API dox hint would match code that exists
  • code not for KParts::MainWindow which does not pass the Create flag would need to call createGUI anyway and one would then pass a non-standard ui.rc filename in that call

(all assuming non-standard ui.rc filename of course :) )

The xmlfile argument with the setupGUI() calls feels to be bound to the Create flag and not related to the other flags. So if the flag is not passed, that argument instintively feels like it should be ignored then.
Adding support in code and related notion in API dox seems like a convenience hack done extra for KParts::MainWindow, where there should be rather special code in KParts::MainWindow itself to handle its specifics of the UI setup. Even if setupGUI() is a convenience method itself, but that seems too much of kitchen-sync. IMHO :) Close call, I agree.

dfaure accepted this revision.Mar 26 2017, 4:56 PM

I agree.
The person who wrote this API doc was aiming for something slightly simpler (one less method to call), but my original design was for everyone to call setXMLFile, and it would make things a bit weird at the kxmlguiwindow level.

(I just don't agree with the #ifdef argument because one just call setXMLFile in all cases, the discussion is more about making things convenient for future new code)

Also note that lxr finds all calls to setXMLFile, including those in parts and plugins, while here we're only talking about kparts mainwindows. But yeah that was the initial design: calling setXMLFile everywhere, in mainwindows, parts and plugins. Then some people tried to make the API more compact in KXMLGuiWindow, but these KParts API remain basically unchanged.

Anyhow, feel free to ship it.

This revision is now accepted and ready to land.Mar 26 2017, 4:56 PM

Also note that lxr finds all calls to setXMLFile, including those in parts and plugins, while here we're only talking about kparts mainwindows.

Eh, I had made the extra effort to first do string search for "KParts::MainWindow", then manually grep in linked sources for "setXMLFile" and "setupGUI" ;) (though just did samples)

Someone should invest into getting something like http://code.woboq.org/ up at least next to lxr.kde.org for the whole of KDE repos (santa claus, do you listen?).

This revision was automatically updated to reflect the committed changes.