----------------------------------------------------------- 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 > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
