> On Aug. 29, 2014, noon, David Edmundson wrote: > > src/plasma/packagestructure.h, line 99 > > <https://git.reviewboard.kde.org/r/119988/diff/1/?file=308188#file308188line99> > > > > would removing a #define count as a SIC? > > Aaron J. Seigo wrote: > Yes, but I don't think it maters for two reasons: > > 1) DataEngine also had both forms but only one actually currently works; > so it is on the face of it *broken* .. removing that is the same as this > change at the end of the day > 2) There are no Plasma 5 PackageStructure plugins out there except the > share dataengine in plasma-workspace (which itself was done in a very odd > manner) > > Neither reason is particularly *good* from the perspective of strict > policy adherence, but they demonstrate that it is a harmless violation. May > as well get it right imho. > > I'd add a third consideration as well: It is quite evident that > plasma-framework was release before it was ready. The plugin loading is > entirely inconsistent with DataEngines using JSON and the Qt5 plugin loading > an the rest of the plugins not. Some of the plugins are obviously not being > used at all due to changes brought about with QML2 and that probably > contributed to the lack of attention. However, PluginLoader quite obviously > went through a partial refactoring that was never completed. It's a little > brash to commit to source and binary compatibility when things are in that > shape. > > Hrvoje Senjan wrote: > >Yes, but I don't think it maters for two reasons > > Can users of Plasma 5.0.x can expect a broken desktop once KF5 5.2 is out > (if this patch goes in)? > > David Edmundson wrote: > Whilst I agree that the frameworks situation sucks, I'm not sure we have > a choice. > > Frameworks 5.2 releases independently of plasma-workspace. At which point > there is a point in time where one can't compile plasma-workspace. > We could make it go via a no-op that does nothing. > > Martin Gräßlin wrote: > if one doesn't recompile it should still work, shouldn't it? Otherwise > I'd suggest to wrap the define in a ifndef NO_DEPRECATED block to keep SIC > (even if it means that we carry deprecated stuff for the time being, but > after all that's the point of deprecation). > > Aaron J. Seigo wrote: > "Can users of Plasma 5.0.x can expect a broken desktop once KF5 5.2 is > out (if this patch goes in)?" > > Not due to the issue you are responding to, no. > > The real issue for breakeage is the use of plasmaquick in Workspaces > which violates the contract between Frameworks and Workspaces (due to being > differnet products) by using a library in workspaces from framweorks that > does not have a binary compatibility guarantee and which has no installed > headers. I am at a loss as to how this could have shipped in this fashion. > > One possibility is to ship libplasmaquick as-is (with the > PackageStructures duplicated and ShellPluginLoader) and simply remove its > usage from Plasma Workspace (as already done in 119989), and then at some > later point remove it from libplasmaquick and simply require a certain > version # of Frameworks for Plasma Desktop. Requiring a minium v# of > libraries would be a pretty normal state of affairs. > > If the intention is to allow Frameworks 5.2 to be used with Plasma > Desktop 5.0, then that is probably the only realistic option. In which case > the Plasma team ought to just install the headers for libplasmaquick and make > it official. There is no point to requiring binary compatibility and trying > to pretend a library is private. > > "At which point there is a point in time where one can't compile > plasma-workspace." > > An alternative is to state compatible versions of Frameworks with > plasma-workspace. Either way, it doesn't resolve the DataEngine situation > where it may be SC but certainly is not functional. > > Currently relatively few people are using Plasma 5; I would urge the > Plasma team to come up with a long-term solution before that changes. > Carrying the impact of these decisions for 5+ years after Plasma 5 actually > has users is .. well .. it's up to you. > > If it is decided not to merge this patchset, that's fine by me. I can > probably re-work the related plasma-workspace patch set to use the older > KService based plugin loading. It would just be nice to know *which* path > (JSON or .desktop files) is being taken. > > Aaron J. Seigo wrote: > "Otherwise I'd suggest to wrap the define in a ifndef NO_DEPRECATED block > to keep SIC" > > This will ensure things still compile, but the result will be plugins > that do not get loaded. If the old defines are desired to be kept, then > PluginLoader should probably be extended to try to load with KPluginTrader > first and then if that fails try KServiceTypeTrader. Or vice versa > (KServiceTypeTrader first and then KPluginTrader). Which is tried first > should reflect the long-term plan for plugin loading as that will be the > common path (and so should be as fast as reasonably possible). > > Note that currently DataEngine is broken for plugins compiled with the > old define, so this is an issue that needs addressing with or without this > patch set. > > Marco Martin wrote: > "In which case the Plasma team ought to just install the headers for > libplasmaquick and make it official. There is no point to requiring binary > compatibility and trying to pretend a library is private." > > I am not sure about the current state, maybe ConfigView should be a bit > different, but I'm fine with officially release libpolasmaquick with at lease > View and Dialog. > Maybe configview and its models, but not i'm not completely sure about > it since i don't like them too much to be there, but they are used/needed, > may be moved to a similar thing in workspace, or made more generic by making > them not views but to create windows from QML > > Aaron J. Seigo wrote: > That's kind of the problem, though: you may not be comfortable with the > library, but you've (the entire team) already committed to binary and source > compatibility by putting the library in a different product (Frameworks). > > However it goes, my current work project's requirements dictate the need > to pick libplasmaquick out of plasma-workspace and I will continue to work on > that so that it eventually becomes a moot point. > > Since this is an issues which the Plasma team apparently has no clear > path in mind, let's just assume for the same of argument that libplasmaquick > remains exactly as it is in plasma-framework. What I actually need your input > on is what plugin loading mechanism is to be used: > > a) KServiceTypeTrader > b) KPluginTrader > c) KPlugintTrader with a KServiceTypeTrade fallback > d) KServiceTypeTrader with a KPluginTrader fallback > > In fact, I'll open a new review request that reflects that more clearly. > > David Edmundson wrote: > >Either way, it doesn't resolve the DataEngine situation where it may be > SC but certainly is not functional. > > There's some crossed communication I think. I'm not asking for anyone to > resolve it, I'm wanting it to not break things. > > I'm happy with your changes. If what we have doesn't work now, it can > proceed to not work, and we can add the additional new thing that fixes it. > That's all great. > > What concerns me is the idea that it would stop the rest of > plasma-workspace stable compiling for a whole month. That would have most > packagers screaming at us. > > We just need to keep the old #define till frameworks 5.4 > > >The real issue for breakeage is the use of plasmaquick > > That's a completely different topic and we should try not to get > distracted by it.
"We just need to keep the old #define till frameworks 5.4" do you think it may be removed after 5.4 or better leaving it for the time being? dataengine already has both, so i would put as a policy c) KPlugintTrader with a KServiceTypeTrade fallback K_EXPORT_PLASMA_DATAENGINE K_EXPORT_PLASMA_PACKAGE etc, may be wrapped in an #idef no_deprecated - Marco ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119988/#review65481 ----------------------------------------------------------- On Aug. 29, 2014, 2:20 p.m., Aaron J. Seigo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/119988/ > ----------------------------------------------------------- > > (Updated Aug. 29, 2014, 2:20 p.m.) > > > Review request for KDE Frameworks and Plasma. > > > Repository: plasma-framework > > > Description > ------- > > This patch set does the following: > > * tidy up the data engine plugin loading code > * make PackageStructure plugins use the json method as with DataEngines > * remove ShellPackage; it moves to live with plasmashell (review #119989) > > The goal here is to get rid of the plasmaquick library as much as possible. > It was unnecessary in the first place since PackageStructure supports > plugins. The only potentially controversial change here is to move > PackageStructure to use the json-based plugin loading. That seems to be the > more modern approach, but plugin loading in libplasma is currently a mix of > the old and the new. As PackageStructure changed API in plasma-framework, > meaning any existing plugins from 4.x would need updating anyways, this seems > a safe enough change to make as it should impact exactly zero plugins out > there currently. > > > Diffs > ----- > > src/plasma/packagestructure.h fb32c22a6e7df1528b3d7a5b30b94c60a85a93e3 > src/plasma/pluginloader.cpp d2ba5ca2d3a96fe6f1ce26be41df3b0954b924df > src/plasma/private/packages.cpp 5eb6f0021392257634dfd958c940b2945989e48b > src/plasma/private/packages_p.h 0833a4ed1b5704efffccade5e52589878e8b4957 > src/plasmaquick/CMakeLists.txt 1ed7c67efcba0e6dbef1ff929b176090786503de > src/plasmaquick/private/packages.h 7498832d0537611903c13e544db6486bab163dd3 > src/plasmaquick/private/packages.cpp > 52758482230d271712e4bb3b6d33f8fdeaa848a8 > src/plasmaquick/shellpluginloader.h > 6c56e5f7b269c3af7587a58cbe104468a2c679c4 > src/plasmaquick/shellpluginloader.cpp > 2824760e6f64a694bd14b46d2f80151304e3e4d3 > src/plasma/dataengine.h d87a6f8361c892a249374a5c9da7e84836195156 > src/plasma/package.cpp 6ad332167bb83c2f794f9f5d059e9f369ad33841 > > Diff: https://git.reviewboard.kde.org/r/119988/diff/ > > > Testing > ------- > > Ran a full Plasma Desktop session. > > > Thanks, > > Aaron J. Seigo > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
