Attempt to support different collections of Unsplash
ClosedPublic

Authored by guoyunhe on Jul 16 2019, 6:56 PM.

Details

Summary
  1. I used Unsplash Source API (unlimited, public) instead of parsing HTML. It should be faster and more reliable. (Parsing HTML may break anytime they update their website...)
  2. Create UnsplashBaseProvider, which has collectionId in constructor. Extend it in children classes to support different collections. Now support all wallpaper collections.

Diff Detail

Repository
R114 Plasma Addons
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
guoyunhe created this revision.Jul 16 2019, 6:56 PM
Restricted Application added a project: Plasma. · View Herald TranscriptJul 16 2019, 6:56 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
guoyunhe requested review of this revision.Jul 16 2019, 6:56 PM
guoyunhe edited the summary of this revision. (Show Details)Jul 16 2019, 6:59 PM
guoyunhe updated this revision to Diff 61872.Jul 16 2019, 7:36 PM

Solve linking issue

guoyunhe edited the summary of this revision. (Show Details)Jul 16 2019, 7:38 PM
guoyunhe updated this revision to Diff 61873.Jul 16 2019, 7:40 PM

Remove KIO linking

guoyunhe updated this revision to Diff 61876.Jul 16 2019, 7:52 PM

Use constructor parameter to pass collection ID

guoyunhe updated this revision to Diff 61877.Jul 16 2019, 8:04 PM

Better naming

guoyunhe edited the summary of this revision. (Show Details)Jul 16 2019, 8:10 PM
guoyunhe updated this revision to Diff 61899.Jul 17 2019, 10:39 AM

Add all Unsplash Wallpaper collections

guoyunhe edited the summary of this revision. (Show Details)Jul 17 2019, 10:48 AM
guoyunhe updated this revision to Diff 61901.Jul 17 2019, 10:53 AM
guoyunhe edited the summary of this revision. (Show Details)

Change default collectionId to all wallpapers instead of desktop wallpaper

filipf added a subscriber: filipf.Jul 21 2019, 7:54 PM

Could we only have Unsplash as the provider and the an additional combobox that would allow choosing the category?

Could we only have Unsplash as the provider and the an additional combobox that would allow choosing the category?

+1, this should be a single Unsplash plugin whose user-facing UI has a combobox allowing you to specify the collection.

Could we only have Unsplash as the provider and the an additional combobox that would allow choosing the category?

+1, this should be a single Unsplash plugin whose user-facing UI has a combobox allowing you to specify the collection.

I also feel this is better. We can even provide preview thumbnails before users choose a wallpaper collection. The only disadvantage is that I have to write a new wallpaper plugin and its UI.

guoyunhe updated this revision to Diff 62538.Jul 25 2019, 1:27 PM
  • Unsplash category combobox
guoyunhe updated this revision to Diff 62539.Jul 25 2019, 1:28 PM
  • Unsplash category combobox

The new change try to add a new ComboBox "Category" and pass the value to PODT data engine. This also introduced a new configuration option. But now it makes plasmashell crash without useful error output.

guoyunhe updated this revision to Diff 62541.Jul 25 2019, 2:29 PM
Fix crashing
guoyunhe updated this revision to Diff 62546.Jul 25 2019, 3:38 PM
  • Unsplash category combobox
guoyunhe updated this revision to Diff 62547.Jul 25 2019, 3:39 PM
  • Unsplash category combobox
guoyunhe updated this revision to Diff 62549.Jul 25 2019, 3:40 PM
  • Unsplash category combobox
guoyunhe updated this revision to Diff 62550.Jul 25 2019, 3:42 PM
  • Unsplash category combobox
guoyunhe updated this revision to Diff 62551.Jul 25 2019, 3:45 PM
  • Unsplash category combobox

Now it is working again! You will have a Category combobox to choose photo collections from Unsplash

Nice!

How about adding an "All" entry for the combo box which would show you wallpapers from any category? Is that possible?

Nice!

How about adding an "All" entry for the combo box which would show you wallpapers from any category? Is that possible?

I already added "All" option.

Oh sweet! I guess I should actually test it out. :)

Tried it out. My reaction is: "Wow, this is really nice."

One UI improvement I could see is to allow multi-selection of categories, so for example you could see images from all the nature-related categories but not Car or Sports.

The way I'd envision this working is that instead of a combo box, a label would display the currently selected categories. Next to it would be a button labeled "Change categories" that would use the multi-page KCM API to take the user to another page with a checkbox for each category. This page would allow selecting any assortment of categories, which would then be listed on the main page. It would be sort of like the corner selector UI in the new Notifications KCM.

Anyway, that's definitely a nice-to-have, and this seems fine from a functional and UI perspective already.

One UI improvement I could see is to allow multi-selection of categories, so for example you could see images from all the nature-related categories but not Car or Sports.

The way I'd envision this working is that instead of a combo box, a label would display the currently selected categories. Next to it would be a button labeled "Change categories" that would use the multi-page KCM API to take the user to another page with a checkbox for each category. This page would allow selecting any assortment of categories, which would then be listed on the main page. It would be sort of like the corner selector UI in the new Notifications KCM.

Multi-selection might be difficult to implement in back end. The Unsplash Source API I use only supports a single collection ID. Search API might support multiple keywords, but it is like "AND" logic, not "OR" logic. So for now, I cannot achieve this function.

All right, that's just fine!

ngraham added inline comments.Jul 26 2019, 3:13 PM
dataengines/potd/potdprovider.cpp
52

The lack of error handling and else blocks for all the ifs in this new code makes me feel a bit nervous.

wallpapers/potd/contents/ui/config.qml
208

Since this is only used once, you don't need to make it a function; just put all of this stuff in the Component.onCompleted: directly

guoyunhe added inline comments.Sat, Jul 27, 10:36 AM
dataengines/potd/potdprovider.cpp
52

date is optional, so if it not found, just leave it as undefined. I can add some detection to toString() function, in case they passed some non-string args. mach() should be safe.

guoyunhe updated this revision to Diff 62636.Sat, Jul 27, 10:57 AM
  • Unsplash category combobox
guoyunhe updated this revision to Diff 62638.Sat, Jul 27, 11:04 AM
guoyunhe marked an inline comment as done.
  • Unsplash category combobox
guoyunhe added inline comments.Sat, Jul 27, 11:04 AM
dataengines/potd/potdprovider.cpp
52

Read Qt documentation that QVariant::toString() will always return a string even if the type is not supported (empty string). So this code won't produce any error/exception.

wallpapers/potd/contents/ui/config.qml
208

Fixed.

ngraham accepted this revision.Sat, Jul 27, 4:04 PM
This revision is now accepted and ready to land.Sat, Jul 27, 4:04 PM
This revision was automatically updated to reflect the committed changes.