Add wallpaperplugin.knsrc + QML function to open GHNS dialog
ClosedPublic

Authored by Zren on Apr 8 2018, 6:31 AM.

Details

Summary

Patch 1/2 (other patch is D12041 for plasma-desktop)

CCBUG: 386621

Screenshot of the dialog:

This screenshot is more releavant to Patch 2 for plasma-desktop:

TODO:

  • Move C++ plugin to components/shellprivate/wallpaperplugin ? I probably need to move the new GHNS code to components/shellprivate/wallpaperplugin since "desktoppackage" has the widgetexplorer C++ plugins there. I figured I'd put up a working patch first.
  • Update wallpaperComboBox and configDialog.wallpaperConfigModel so we don't have to close and reopen the "Configure Desktop" window for the plugin to show up in the dropdown.
    • The wallpaperComboBox model will automatically update thanks to D15178 (which depends on D15177).
Test Plan
  1. Install both patches
  2. Configure Desktop > Get Wallpaper Plugins > Install something (eg: Ken Burns Effect)
  3. Confirm it showed up in ~/.local/share/plasma/wallpapers/
  4. Close GHNS dialog and Configure Desktop dialog
  5. Reopen Configure Desktop
  6. Confirm "Ken Burns Effect" shows up

Diff Detail

Repository
R131 Plasma Wallpapers
Lint
Lint Skipped
Unit
Unit Tests Skipped
Zren created this revision.Apr 8 2018, 6:31 AM
Restricted Application added a project: Plasma. · View Herald TranscriptApr 8 2018, 6:31 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
Zren requested review of this revision.Apr 8 2018, 6:31 AM
Zren updated this revision to Diff 32247.Apr 16 2018, 4:39 AM
Zren edited the summary of this revision. (Show Details)

Move open GHNS window function from Wallpaper.Image to ShellPrivate.WallpaperPlugin

Zren retitled this revision from [WIP] Add wallpaperplugin.knsrc + QML function to open GHNS dialog to Add wallpaperplugin.knsrc + QML function to open GHNS dialog.Apr 16 2018, 4:41 AM

cool, seems mostly fine. One minor style comment

components/shellprivate/wallpaperplugin/wallpaperplugin.cpp
26

You don't need to do this.

QPointer has a custom -> operator

So m_newStuffDialog->setTitle works just fine.

Same for in newStuffFinished

Zren added inline comments.Apr 16 2018, 12:24 PM
components/shellprivate/wallpaperplugin/wallpaperplugin.cpp
26

I copy pasted from image.cpp ... which apparently hasn't been touched in 4 years since the import from kde-workspace.

Will that function need updating too?

mart added a subscriber: mart.Apr 24 2018, 1:43 PM
mart added inline comments.
components/shellprivate/wallpaperplugin/wallpaperplugin.cpp
26

yeah, possibly, that may ome from early Qt4 days which didn't have ->

Zren updated this revision to Diff 33024.Apr 24 2018, 11:56 PM
Zren changed the repository for this revision from R131 Plasma Wallpapers to R120 Plasma Workspace.

Remove .data() since it seems to work without it.

cfeck added a subscriber: cfeck.Apr 25 2018, 2:12 AM
In D12040#253335, @Zren wrote:

Remove .data() since it seems to work without it.

Even with gcc 4.8 (openSUSE Leap)?

Zren added a comment.Apr 25 2018, 2:51 AM

I use gcc --version 5.4.0, so I've no idea. I assume that is a QPointer feature though? We can always check the other DownloadDialog initializations:

mart added a comment.May 2 2018, 1:16 PM
In D12040#253335, @Zren wrote:

Remove .data() since it seems to work without it.

Even with gcc 4.8 (openSUSE Leap)?

so, if the qpointer is used in a connect, it needs to have .data() to work on gcc 4.8.
if accessed normally, it doesn't need it

Zren added a comment.May 2 2018, 1:26 PM
In D12040#257170, @mart wrote:

so, if the qpointer is used in a connect, it needs to have .data() to work on gcc 4.8.
if accessed normally, it doesn't need it

So I need to change

connect(m_newStuffDialog, &QDialog::accepted, this, &WallpaperPlugin::newStuffFinished);

to

connect(m_newStuffDialog.data(), &QDialog::accepted, this, &WallpaperPlugin::newStuffFinished);

to support gcc 4.8?

mart added a comment.May 2 2018, 1:27 PM
In D12040#257172, @Zren wrote:
In D12040#257170, @mart wrote:

so, if the qpointer is used in a connect, it needs to have .data() to work on gcc 4.8.
if accessed normally, it doesn't need it

So I need to change

connect(m_newStuffDialog, &QDialog::accepted, this, &WallpaperPlugin::newStuffFinished);

to

connect(m_newStuffDialog.data(), &QDialog::accepted, this, &WallpaperPlugin::newStuffFinished);

to support gcc 4.8?

yup

Zren updated this revision to Diff 33951.May 10 2018, 3:56 PM

Use connect(m_newStuffDialog.data(), ...) to support gcc 4.8.
Add license headers.

What's the status of this? Any progress toward resolving the outstanding TODO? Would be nice if we can get it in for 5.14 alongside the effort to clean up GHNS a bit.

Other than the one comment it's good to go.

Though my stuff requires a frameworks feature and I haven't got my stuff is done in time for 5.14, sorry !
It can be the first 5.15 feature.

components/shellprivate/wallpaperplugin/wallpaperplugin.h
43–44

We can get rid of this and the slot that emits this.

I made the wallpaper plugin model update magically.

Zren added a comment.Aug 31 2018, 12:06 AM

Awesome!

The KPackage patch will end up in... Frameworks 5.51?
And the Plasma 5.14.0 release is set to depend on Frameworks... 5.45 (according to plasma-workspace's CMakeList.txt).
So we'll need to wait for the 5.14 tag to appear (Oct 4th), but to be safe, wait for the 5.14 release on Oct 9th.
Frameworks 5.51 is released Oct 13th, so it'll be easier to test after that (though I'll try to build kpackage with the patch and test it in a sec).
Alright, I'll set a reminder on Oct 17 (just after 5.14.1) to merge my 2 patches.

Plasma 5.14.0 will depend on Frameworks 5.50 (see https://community.kde.org/Schedules/Plasma_5), which is why this has to go into Plasma 5.15.0.

Looks like the dependencies have been committed and we've branched for 5.14. Plasma folks/@davidedmundson, does this look good to land now?

In principle, sure.

My previous review comments remain to be done.

Zren added a comment.Oct 5 2018, 9:02 PM

Finally got around to updating this patch and formally testing with kdesrc-build (sort of) in Neon Unstable which has Frameworks 5.51.

Updated diff incoming as soon as I figure out how to diff ~/kde/src/.../plasma-workspace since they aren't git repos.

Zren updated this revision to Diff 42950.Oct 5 2018, 9:50 PM
  • Got rid of the signal+slot that emits KNewStuff changes since wallpaperComboBox is automagically updated.
  • Reordered the command arguments so the variable is last in wallpaperplugin.knsrc.
  • Put the plasmoids.knsrc and wallpaperplugin.knsrc filepaths on separate lines in the CMakeList.txt

That looks really sweet.

davidedmundson accepted this revision.Oct 6 2018, 12:11 PM

Thanks, sorry for the delay on my side with the updates.

This revision is now accepted and ready to land.Oct 6 2018, 12:11 PM

So can this land now?

This revision was automatically updated to reflect the committed changes.
Zren added a comment.Oct 10 2018, 12:23 AM

Sorry, just merged both to master. Thanks davidedmundson for the patches that updated the ComboBox.

No problem at all, I just wanted it to get in so I could blog about it, since I think it will prove popular. :)