Image of the Day wallpaper plugin
AbandonedPublic

Authored by bgupta on Jun 22 2016, 6:56 PM.

Details

Reviewers
sebas
Group Reviewers
Plasma
Summary

This patch adds a wallpaper plugin to Plasma, which fetches an "Image of the Day", or a random image, from an Internet source. Current sources include:

  • Bing
  • NASA Astronomy Picture of the Day
  • Random featured image from Unsplash.com

Sources can be enabled and disabled, and a fallback default image can be specified, which is displayed if an image cannot be fetched from the Internet. The image change interval can be configured to be anything between 15 minutes to 23 hours and 59 minutes.

A logo for the source and credit text is superimposed on the bottom right corner of the wallpaper.

Test Plan

This was tested manually while being written. I use this plugin as my wallpaper.

Diff Detail

Repository
R114 Plasma Addons
Lint
Lint Skipped
Unit
Unit Tests Skipped
bgupta updated this revision to Diff 4678.Jun 22 2016, 6:56 PM
bgupta retitled this revision from to Image of the Day wallpaper plugin.
bgupta updated this object.
bgupta edited the test plan for this revision. (Show Details)
bgupta set the repository for this revision to R114 Plasma Addons.
bgupta added a subscriber: Plasma.
Restricted Application added a project: Plasma. · View Herald TranscriptJun 22 2016, 6:56 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript

Seems generally good.

wallpapers/iotd/contents/ui/config.qml
28

If you switch these to ColumnLayout / RowLayout (from QtQuick.Layouts, which you're currently not using) you can get rid of a lot of the anchors and widths in this code.

57

it defeats the point of having spacing as semanticly defined macros, if people then do maths with it to get any arbitrary value

107

this won't acheive anything

you've set a width, but by default there's no eliding, so it'll just overflow past here anyway.

265

why?

wallpapers/iotd/contents/ui/main.qml
94

where is resetTimer defined?

106

we generally try to avoid pixel sizes.

112

why?

you're displaying this at a fixed size

bgupta added inline comments.Jun 23 2016, 6:05 AM
wallpapers/iotd/contents/ui/config.qml
28

I was initially using ColumnLayout, but the problem there was that the contents were expanding to fill the entire height of the dialog, with huge spacing between rows. Using Column just works.

57

I looked at config.qml files from the color and image wallpaper plugins from plasma-workspace. They seem to do things like this, so I did the same.

107

I don't see any overflow on my computer. Is there any?

265

The objectName? Setting an objectName is something that I do by force of habit since trying to add QML bits to Spectacle (to be able to get to objects from the C++ side by name), but of course there's no use for objectNames here. I'll remove them in a later pass.

wallpapers/iotd/contents/ui/main.qml
94

Ah yes, I forgot to remove that line.

106

What would be an appropriate substitute in this case? units.smallSpacing?

112

It's true that there's no regular scaling involved. I clearly misunderstood the point of mipmap scaling over regular smoothing. I'll swap this in favour of smoothing.

You might have seen that I have an open differential request to integrate Plasma Wallpapaer into kscreenlocker.

This makes me ask the question: how will this wallpaper plugin look like if you don't have an internet connection?

bgupta added a comment.EditedJun 23 2016, 7:20 AM

You might have seen that I have an open differential request to integrate Plasma Wallpapaer into kscreenlocker.

This makes me ask the question: how will this wallpaper plugin look like if you don't have an internet connection?

There's a single fallback image that can be selected (a local file that you can browse to). When the plugin is in the "display fallback image" state, it tries to fetch a new image from the Internet every 60 seconds, regardless of the change image interval.

davidedmundson added inline comments.Jun 23 2016, 9:12 AM
wallpapers/iotd/contents/ui/config.qml
28

oh, you can solve that with.

ColumnLayout
{

//your stuff here
Item {
  Layout.fillHeight: true //spacer
}

}

107

Yes.

You can't see it if your locale is set to English because you've set formAlignment (where is that set? ) to be bigger than this text happens to be.

But try changing the text to something much longer and you'll see it just doesn't fit.

http://imgur.com/NLLYkn0
(note, because you have Text.AlignRight the overflow is to the left)

112

See ColorButton in KDeclarative

247

these don't update the apply button

wallpapers/iotd/contents/ui/main.qml
106

yes

sebas requested changes to this revision.Jun 23 2016, 10:30 AM
sebas added a reviewer: sebas.
sebas added a subscriber: sebas.

There are lots of ; (at the end of every line of QML. We generally don't do that. In order to make the code more consistent, please remove them. (They're fine in your javascript functions.)

wallpapers/iotd/contents/ui/config.qml
50

Use PlasmaExtras.Heading instead of sizing on your own, this will lead to more consistency. Look at other config dialogs how it's done.

121

use units.gridUnit here.

179

So it will at most switch every 15 minutes? Seems a bit long as minimal value to me...

206

units.gridUnit instead of theme.mSize

238

PlasmaExtras.Heading

This revision now requires changes to proceed.Jun 23 2016, 10:30 AM
wallpapers/iotd/contents/ui/config.qml
238

Not in the config.

We have this situation where we use Plasma Components in plasmoids, QtQuick.Controls in settings.

Heading inherits Plasma.Label - which potentially means the wrong font and more importantly wrong font colour which could be a white on white situation.

(Kirigami's headers should fix this at least)

bgupta marked 2 inline comments as done.Jun 23 2016, 11:31 AM
bgupta added a subscriber: garg.
bgupta added inline comments.
wallpapers/iotd/contents/ui/config.qml
179

Note that we're hitting APIs, some of which have limits. Unsplash, for example, has a limit of 5000 API requests per hour. Pixabay (which I'm planning to add and have gotten an API key for) has even less and it's per application key, not per IP address.

238

Closing as per @davidedmundson's suggestion

247

They do on mine, but @garg has also reported the same errors. Any ideas?

bgupta marked 2 inline comments as done.Jun 23 2016, 11:32 AM
bgupta marked 13 inline comments as done.Jun 23 2016, 3:21 PM
bgupta added inline comments.
wallpapers/iotd/contents/ui/main.qml
112

From the docs: "Mipmap filtering gives better visual quality when scaling down compared to smooth, but it may come at a performance cost (both when initializing the image and during rendering)"

Since this is a wallpaper, we should be using the absolute highest quality available, shouldn't we?

bgupta updated this revision to Diff 4689.Jun 23 2016, 3:29 PM
bgupta edited edge metadata.
bgupta changed the visibility from "All Users" to "Public (No Login Required)".

I've addressed all of the issues except for layout and alignment in the configuration dialog.

I'm soliciting feedback for all the outstanding issues, and the following:

  • I have no idea where formAlignment is set, but it's coming somewhere from within Plasma, and it makes things line up with the wallpaper type and layout fields above. How would I change things to ensure things still line up properly?
  • As for handling text overflow, should I just change the strings to something smaller?

Do you actually cache the downloaded image somewhere on disk? I wouldn't want every startup of plasma download the image. Also, does it check for mobile internet being used instead of Wifi, looks like we need a NM dataengine.

@graesslin Perhaps we need to re-introduce the X-Required-Permission stuff so a wallpaper can specify it needs internet and can then be filtered out from the lock screen config?

wallpapers/iotd/contents/ui/config.qml
71

Those quotes are unneccessary, label: "foo" is enough

79

Those {} are unneccessary, Component.onCompleted: readSizePosition()

84

model[i].fillMode ===

211

Can we do mime filters instead?

213–214

I suppose those are the default values and don't need to explicitly mentioning?

wallpapers/iotd/contents/ui/main.qml
39

Can you make this more declarative, ie.?

property var enabledSources: {
    // the stuff of this function
}
40

Urgh

sebas requested changes to this revision.Jul 12 2016, 11:11 PM
sebas edited edge metadata.

This review is still pending with a bunch of changes requested. Still, it would be nice to get this into 5.8?

This revision now requires changes to proceed.Jul 12 2016, 11:11 PM

I'll re-work a bunch of stuff, including image caching, and placement of credit text/logo based on where the panel is. Then I'll file a new revision.

bgupta abandoned this revision.Aug 18 2017, 12:26 AM