Review Request 115097: KParts: Move each class into its own header/source file pair

Sun Jan 19 21:54:12 UTC 2014

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115097/
-----------------------------------------------------------

(Updated Jan. 19, 2014, 9:54 p.m.)

Status
------

This change has been marked as submitted.

Review request for KDE Frameworks and David Faure.

Repository: kparts

Description
-------

As discussed in http://lists.kde.org/?l=kde-frameworks-devel&m=138994749530457&w=2 this patch ensures that all (exported) classes are declared in header files which match the class name. And while includes had to be touched the patch also does some fixup/cleanup of includes, so the headers only include what is needed and have <kparts/part.h> in the installed headers and use "part.h" in the sources.

These headers have been split out as new ones:

>From browserextension.h:
* browserarguments.h
* windowargs.h
* openurlevent.h
* browserhostextension.h
* liveconnectextension.h

>From event.h:
* guiactivateevent.h
* partactivateevent.h
* partselectevent.h

>From part.h:
* openurlarguments.h
* partbase.h
* readonlypart.h
* readwritepart.h

>From htmlextension.h:
* selectorinterface.h
* htmlsettingsinterface.h

listingextension.h has been split up even, as there is no class ListingExtension, into:
* listingfilterextension.h
* listingnotificationextension.h

Matching adaption for KDE4Support: https://git.reviewboard.kde.org/r/115098/

There is one issue:
KDE4 code relies on getting all the now moved-out classes when including browserextension.h, event.h, part.h, htmlextension.h. While for CamelCase-forwarding headers, like <KParts/Part> or <KParts/Event> in KDE4Support this can be catched by just including all the now needed headers, I found no way yet to also have proper backward support for normal includes like <kparts/part.h>. There is lots of code which includes that to use KParts::ReadOnlyPart. Because <kparts/part.h> is now also the correct path for the now-only KParts::Part declaring header from the KParts modul, I could not simply install a kde4-compat part.h from KDE4Support into KDE4Support/kparts/ and then forward include the current headers, because it will fail at least with #include <kparts/part.h>

Would it be acceptable to simply break SC for old code including the lowercase headers (like <kparts/part.h>) and using classes declared in there not matching the filename (like KParts::ReadOnlyPart)?

Or is there a smart workaround for the problem?

One workaround I proposed in the email: The real headers would just have the fallback support directly.
E.g the new part.h would have
// KDE4 compat
#if KDE4COMPATIBLE
#include <kparts/guiactivateevent.h>
#include <kparts/partactivateevent.h>
#include <kparts/partselectevent.h>
#endif
to include the other headers at the places where their classes had been defined before.
This would mean having still KDE4 mentioned in KF5 code :) But KDE4COMPATIBLE (or a better name) should be only defined when KDE4Support is linked as well.

No strong opinion myself. I would opt for the small SC-break :)

Looking at all the stuff build with kdesrc-build/kf5-qt5-build-include there are small adaptions needed for
* khtml
* kross
* kmediaplayer
* kdewebkit
* ktexteditor
* kprintutils
* okteta

Mainly stuff like
-#include <kparts/part.h>
+#include <kparts/readonlypart.h>
Locally all fixed, would commit the adaptions directly to those modules.

Diffs
-----

  src/windowargs.cpp PRE-CREATION 
  tests/normalktm.h f3bc291 
  tests/notepad.h 73a6fa2 
  tests/parts.h 1207c2c 
  tests/parts.cpp 872bdb8 
  tests/partviewer.h bfe3158 
  tests/terminal_test.cpp eda318a 
  tests/testmainwindow.h 7ba44e0 
  src/browserinterface.cpp a008e84 
  src/browseropenorsavequestion.h cb8d3ed 
  src/browseropenorsavequestion.cpp dd52608 
  src/browserrun.h 5a0b996 
  src/browserrun.cpp bcf50df 
  src/browserrun_p.h fbfbea6 
  src/event.h 056d735 
  src/event.cpp 1d88c82 
  src/fileinfoextension.h e1efbc1 
  src/fileinfoextension.cpp 8ecb234 
  src/guiactivateevent.h PRE-CREATION 
  src/guiactivateevent.cpp PRE-CREATION 
  src/historyprovider.h f167f30 
  src/historyprovider.cpp 1e4733a 
  src/htmlextension.h 66966e4 
  src/htmlextension.cpp 3a6df16 
  src/htmlsettingsinterface.h PRE-CREATION 
  src/htmlsettingsinterface.cpp PRE-CREATION 
  src/kde_terminal_interface.h d029e02 
  src/listingextension.h 0b2e1dd 
  src/listingextension.cpp d380584 
  src/listingfilterextension.h PRE-CREATION 
  src/listingfilterextension.cpp PRE-CREATION 
  src/listingnotificationextension.h PRE-CREATION 
  src/listingnotificationextension.cpp PRE-CREATION 
  src/liveconnectextension.h PRE-CREATION 
  src/liveconnectextension.cpp PRE-CREATION 
  src/mainwindow.h 11d0067 
  src/mainwindow.cpp 5c3917e 
  src/openurlarguments.h PRE-CREATION 
  src/openurlarguments.cpp PRE-CREATION 
  src/openurlevent.h PRE-CREATION 
  src/openurlevent.cpp PRE-CREATION 
  src/part.h 05194d3 
  src/part.cpp 5a63777 
  src/part_p.h PRE-CREATION 
  src/partactivateevent.h PRE-CREATION 
  src/partactivateevent.cpp PRE-CREATION 
  src/partbase.h PRE-CREATION 
  src/partbase.cpp PRE-CREATION 
  src/partbase_p.h PRE-CREATION 
  src/partmanager.h 624a97c 
  src/partmanager.cpp 7c1f3b0 
  src/partselectevent.h PRE-CREATION 
  src/partselectevent.cpp PRE-CREATION 
  src/plugin.h b7d130f 
  src/plugin.cpp 344f75b 
  src/readonlypart.h PRE-CREATION 
  src/readonlypart.cpp PRE-CREATION 
  src/readonlypart_p.h PRE-CREATION 
  src/readwritepart.h PRE-CREATION 
  src/readwritepart.cpp PRE-CREATION 
  src/readwritepart_p.h PRE-CREATION 
  src/scriptableextension.h 9cec71f 
  src/scriptableextension.cpp d31a558 
  src/scriptableextension_p.h 82808f7 
  src/selectorinterface.h PRE-CREATION 
  src/selectorinterface.cpp PRE-CREATION 
  src/statusbarextension.h c46fd55 
  src/statusbarextension.cpp 2d8de8a 
  src/textextension.h f8c77e4 
  src/textextension.cpp 3dc5a99 
  src/windowargs.h PRE-CREATION 
  autotests/parttest.cpp 7505948 
  src/CMakeLists.txt d987f46 
  src/browserarguments.h PRE-CREATION 
  src/browserarguments.cpp PRE-CREATION 
  src/browserextension.h 998f7ac 
  src/browserextension.cpp a367de9 
  src/browserhostextension.h PRE-CREATION 
  src/browserhostextension.cpp PRE-CREATION 
  src/browserinterface.h cf10499 

Diff: https://git.reviewboard.kde.org/r/115097/diff/

Testing
-------

kdesrc-build/kf5-qt5-build-include builds fine for me with all the patches.

Thanks,

Friedrich W. H. Kossebau

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20140119/e89ed5fe/attachment-0001.html>