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
27 ↗(On Diff #4678)

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.

56 ↗(On Diff #4678)

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

106 ↗(On Diff #4678)

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.

264 ↗(On Diff #4678)

why?

wallpapers/iotd/contents/ui/main.qml
93 ↗(On Diff #4678)

where is resetTimer defined?

105 ↗(On Diff #4678)

we generally try to avoid pixel sizes.

111 ↗(On Diff #4678)

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
27 ↗(On Diff #4678)

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.

56 ↗(On Diff #4678)

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.

106 ↗(On Diff #4678)

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

264 ↗(On Diff #4678)

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
93 ↗(On Diff #4678)

Ah yes, I forgot to remove that line.

105 ↗(On Diff #4678)

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

111 ↗(On Diff #4678)

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
27 ↗(On Diff #4678)

oh, you can solve that with.

ColumnLayout
{

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

}

106 ↗(On Diff #4678)

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)

111 ↗(On Diff #4678)

See ColorButton in KDeclarative

246 ↗(On Diff #4678)

these don't update the apply button

wallpapers/iotd/contents/ui/main.qml
105 ↗(On Diff #4678)

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
49 ↗(On Diff #4678)

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

120 ↗(On Diff #4678)

use units.gridUnit here.

178 ↗(On Diff #4678)

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

205 ↗(On Diff #4678)

units.gridUnit instead of theme.mSize

237 ↗(On Diff #4678)

PlasmaExtras.Heading

This revision now requires changes to proceed.Jun 23 2016, 10:30 AM
wallpapers/iotd/contents/ui/config.qml
237 ↗(On Diff #4678)

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
178 ↗(On Diff #4678)

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.

237 ↗(On Diff #4678)

Closing as per @davidedmundson's suggestion

246 ↗(On Diff #4678)

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
111 ↗(On Diff #4678)

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