Switch the Plasma Desktop KCMs to using KNewStuffQuick
ClosedPublic

Authored by leinir on Thu, Jan 9, 1:54 PM.

Details

Summary

This switches the KCMs which previously used a bit of a hack to show the old Widgets based Get Hot New Stuff dialog from a button in the Qt Quick based KCMs so that they instead use the new KNewStuff Qt Quick submodule. The modules affected by this are:

  • Colors
  • Cursor Theme
  • Desktop Theme
  • Icons
  • KSplash

Diff Detail

Repository
R119 Plasma Desktop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
leinir created this revision.Thu, Jan 9, 1:54 PM
Restricted Application added a project: Plasma. · View Herald TranscriptThu, Jan 9, 1:54 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
leinir requested review of this revision.Thu, Jan 9, 1:54 PM
mart accepted this revision.Fri, Jan 10, 11:58 AM
This revision is now accepted and ready to land.Fri, Jan 10, 11:58 AM
leinir planned changes to this revision.Fri, Jan 10, 11:59 AM

i'm working on more of this (main reason for the status change, but thank you!), and there's a patch for KNS that wants to go in first (of course) - it's runtime, so it won't break the CI for this to go in first, but... still :)

leinir updated this revision to Diff 73190.Fri, Jan 10, 12:58 PM
  • Only try and work on newly installed themes
  • Port the cursortheme kcm to KNSQuick
  • Port the plasma theme kcm's ghns support to KNSQuick
  • Port the Icons kcm to KNSQuick
  • Port the Splash kcm to KNSQuick
This revision is now accepted and ready to land.Fri, Jan 10, 12:58 PM
leinir requested review of this revision.Fri, Jan 10, 12:59 PM
leinir retitled this revision from [WIP] Switch the Plasma Desktop KCMs to using KNewStuffQuick to Switch the Plasma Desktop KCMs to using KNewStuffQuick.

Right, that's the lot of the qtquick kcms in plasma-desktop!

Since D26543 is listed as a dependent, but it's in Frameworks, and Plasma 5.18 is going to depend on Frameworks 5.66, but 5.66 has already been tagged, that means that this can't land in 5.18, so we'll ship 5.18 with only the Global Themes KCM using the new dialog, and all the other dialogs using the old one. :/

Is D26543 a hard blocker, or just a nice-to-have bugfix?

Since D26543 is listed as a dependent, but it's in Frameworks, and Plasma 5.18 is going to depend on Frameworks 5.66, but 5.66 has already been tagged, that means that this can't land in 5.18, so we'll ship 5.18 with only the Global Themes KCM using the new dialog, and all the other dialogs using the old one. :/

Gah, i was under the impression the awkwardness with frameworks dependencies on the CI was due to Plasma requiring head...

Is D26543 a hard blocker, or just a nice-to-have bugfix?

Right, the functionality would all still exist (that is, install/uninstall/update/whatnot would function just fine), what would happen without the dependent patch is that the views will be out of sync with the system state (basically the onChangedEntriesChanged will just fail to call the function it's pointed at due to incompatible types, in all but the desktoptheme kcm (which just calls load anyway, which doesn't expect anything weird).

davidedmundson added inline comments.
kcms/icons/main.cpp
175

FWIW, there is an alternative approach that we do in plasma-workspace (shell/containmentconfigview.cpp) for reloading wallpapers types.

kpackage emits a DBus signal when it performs an action, models can listen for this.

We had to do that there because the model was quite separated from the UI.
But it has the advantage that we pick up external changes (i.e through discover or whatever)

196

Does this code still exist? I can't see an equivalent in knewstuff.

It's especially important on wayland

In the case of three Frameworks (namely plasma-framework, kwayland and kwindowsystem) I have on file requests from the Plasma and KWin developers informing me that their software does indeed require latest HEAD (with no delay) of those particular frameworks, hence the convoluted setup on the CI system for those three. They do date back to May 2017 though.

Right, the functionality would all still exist (that is, install/uninstall/update/whatnot would function just fine), what would happen without the dependent patch is that the views will be out of sync with the system state (basically the onChangedEntriesChanged will just fail to call the function it's pointed at due to incompatible types, in all but the desktoptheme kcm (which just calls load anyway, which doesn't expect anything weird).

All right, that's probably not a hard blocker, especially since this was already broken. So we can land this patch for 5.18 IMO.

This patch makes plasma-desktop fail to build without the dependent KNS framework change. That means that the KNS change is in fact a hard dependency and therefore this functionality can't make it into 5.18 with the patch's current state. If you want it for 5.18 (as I assume you do, and I do too!), you'll need to make D26543 not a dependency by only conditionally using the KNSCore::EntryWrapper functionality, or by finding a way to implement the fix in way that doesn't add new classes that have to be used here.

I notice that this also has the effect of changing the button texts to not have ellipses on the end. I guess NewStuff.Button needs to do that itself?

Works great BTW. A big improvement!

leinir planned changes to this revision.Mon, Jan 13, 10:06 AM

This patch makes plasma-desktop fail to build without the dependent KNS framework change. That means that the KNS change is in fact a hard dependency and therefore this functionality can't make it into 5.18 with the patch's current state. If you want it for 5.18 (as I assume you do, and I do too!), you'll need to make D26543 not a dependency by only conditionally using the KNSCore::EntryWrapper functionality, or by finding a way to implement the fix in way that doesn't add new classes that have to be used here.

ah, d'oh, yes, you're quite right... it /is/ a hard dependency, i entirely forgot about that one... not entirely sure how i forgot about it, perhaps repression from having to write it at all, but yes. Right, let's see what we can do about that, then :)

In the case of three Frameworks (namely plasma-framework, kwayland and kwindowsystem) I have on file requests from the Plasma and KWin developers informing me that their software does indeed require latest HEAD (with no delay) of those particular frameworks, hence the convoluted setup on the CI system for those three. They do date back to May 2017 though.

Right, thanks for the clarification! :)

I notice that this also has the effect of changing the button texts to not have ellipses on the end. I guess NewStuff.Button needs to do that itself?

There's a bug report for it (and that is indeed something that wants fixing in NewStuff.Button)

Works great BTW. A big improvement!

Thanks! :D

leinir added inline comments.Mon, Jan 13, 12:49 PM
kcms/icons/main.cpp
175

That does sound like something that would be good to look at at a later point in time (it'd make this whole thing vastly more involved, and i'm trying to keep it a bit more simple for now)

196

The new dialog is entirely QtQuick (clicking the NewStuff.Button essentially just calls open() on a QtQuick.Dialogs Dialog, so if doing show on a QtQuick Dialog does the trick, then yes. Otherwise i am... severely uncertain of how that might be solved (though it would be central at least, since the code would be in KNSQuick).

leinir updated this revision to Diff 73409.Mon, Jan 13, 1:35 PM

As discussed above, we need to be able to build against an older version of
Frameworks than 5.67. As such, wrap up the bits that aren't available before
then in some compile-time checks.

  • Wrap the new requirements up in some handy checks, for older Frameworks
ngraham added inline comments.Mon, Jan 13, 10:34 PM
kcms/colors/colors.cpp
126–129

This wraps a lot of existing functionality into that conditional as well. Was all of that stuff broken and useless without D26543?

Everything seems to work anyway, but I'm curious to know if everything here needs to be wrapped up in this.

kcms/cursortheme/kcmcursortheme.cpp
413–421

ditto

leinir added inline comments.Tue, Jan 14, 9:54 AM
kcms/colors/colors.cpp
126–129

All the code inside this conditional is for setting the most recent newly installed theme as the currently selected - perhaps a slightly long winded way of doing so, but yup, that's really all it does :) (and if it can't use the data being pulled out of the changedEntries, there doesn't seem any good reason to run the rest of the code which arguably would build and run, as it just wouldn't do anything)

kcms/cursortheme/kcmcursortheme.cpp
413–421

Also ditto - all the code here depends on having an entry to work with, and if that can't be pulled out of changedEntries (which doesn't work until 5.67) then there's no particularly good reason to run the other bits of the code :)

ngraham accepted this revision.Tue, Jan 14, 3:34 PM

Thanks for the explanation!

This revision is now accepted and ready to land.Tue, Jan 14, 3:34 PM

Thanks for the explanation!

Not a problem, it's not necessarily easy to spot at a glance - and thanks! :D

leinir updated this revision to Diff 73544.Tue, Jan 14, 4:01 PM
  • Merge remote-tracking branch 'origin/master' into switch-kcms-to-knsquick
  • Fix build (by removing stuff that was removed in master it seems)

Right, everything blew up i guess. Nice.

Phab strikes again...

I would revert to a previous version of this patch using the history tab in the web UI, download the raw diff from the web UI, and then manually apply it on top of HEAD.

leinir updated this revision to Diff 73595.Wed, Jan 15, 8:47 AM

Let's try and unbreak this diff, shall we...

  • Only try and work on newly installed themes
  • Port the cursortheme kcm to KNSQuick
  • Port the plasma theme kcm's ghns support to KNSQuick
  • Port the Icons kcm to KNSQuick
  • Port the Splash kcm to KNSQuick
  • Wrap the new requirements up in some handy checks, for older Frameworks
leinir updated this revision to Diff 73597.Wed, Jan 15, 8:57 AM
  • Unbreak the merge
leinir updated this revision to Diff 73598.Wed, Jan 15, 8:58 AM
  • Unbreak the merge
leinir edited the summary of this revision. (Show Details)Wed, Jan 15, 12:52 PM
mart accepted this revision.Wed, Jan 15, 4:44 PM
This revision was automatically updated to reflect the committed changes.

This change fails to build from source on FreeBSD due to missing headers in Frameworks.

Cancel that, it fails to build on Linux as well - i'm guessing that the KNetstuff changes have not been landed?

Cancel that, it fails to build on Linux as well - i'm guessing that the KNetstuff changes have not been landed?

The KNS changes have indeed not been landed yet

meven added a subscriber: meven.Thu, Jan 16, 9:48 AM

Cancel that, it fails to build on Linux as well - i'm guessing that the KNetstuff changes have not been landed?

The KNS changes have indeed not been landed yet

Please revert this temporally, it breaks all build of master plasma-desktop

Cancel that, it fails to build on Linux as well - i'm guessing that the KNetstuff changes have not been landed?

The KNS changes have indeed not been landed yet

Please revert this temporally, it breaks all build of master plasma-desktop

The problem is that a small part of it depends on functionality to exist master which doesn't currently exist - specifically D26543. I've pushed a thing that works around this, which can be reverted once said code has become available in the CI.

Now there are a lot of errors with localization.
There is a message about them.
https://bugs.kde.org/show_bug.cgi?id=415541
Temporarily fixed this problem with attached patches when building packages.
This error was originally described here https://phabricator.kde.org/D26665#594287