Review Request 119988: Package structure cleanups
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119988/ --- 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 ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119988: Package structure cleanups
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119988/#review65481 --- src/plasma/packagestructure.h https://git.reviewboard.kde.org/r/119988/#comment45738 would removing a #define count as a SIC? - David Edmundson On Aug. 29, 2014, 11:51 a.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, 11:51 a.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 ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119988: Package structure cleanups
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119988/#review65482 --- Ship it! +1 for removing shellpackage. good for plugin-ification - Marco Martin On Aug. 29, 2014, 11:51 a.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, 11:51 a.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 ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119988: Package structure cleanups
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? 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. - Aaron J. --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119988/#review65481 --- On Aug. 29, 2014, 11:51 a.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, 11:51 a.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 ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119988: Package structure cleanups
On Aug. 29, 2014, 2 p.m., 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. 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)? - Hrvoje --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119988/#review65481 --- On Aug. 29, 2014, 1:51 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, 1:51 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 ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119988: Package structure cleanups
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)? 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. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119988/#review65481 --- On Aug. 29, 2014, 11:51 a.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, 11:51 a.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 ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119988: Package structure cleanups
On Aug. 29, 2014, 2 p.m., 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. 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). - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119988/#review65481 --- On Aug. 29, 2014, 1:51 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, 1:51 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 ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119988: Package structure cleanups
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). 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. --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119988/#review65481 --- On Aug. 29, 2014, 11:51 a.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, 11:51 a.m.) Review request for KDE Frameworks and Plasma. Repository: plasma-framework
Re: Review Request 119988: Package structure cleanups
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. 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
Re: Review Request 119988: Package structure cleanups
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
Re: Review Request 119988: Package structure cleanups
--- 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.) Status -- This change has been discarded. 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 ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119988: Package structure cleanups
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
Re: Review Request 119988: Package structure cleanups
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
