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

David Faure faure at kde.org
Sat Jan 18 08:56:46 UTC 2014 

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

Ship it!


Good work, thanks.
I agree with the small SC break for the sake of consistency.

- David Faure


On Jan. 18, 2014, 3:47 a.m., Friedrich W. H. Kossebau wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115097/
> -----------------------------------------------------------
> 
> (Updated Jan. 18, 2014, 3:47 a.m.)
> 
> 
> 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/20140118/3678cd78/attachment-0001.html>

More information about the Kde-frameworks-devel mailing list