Wallpaper slideshow: Incorrect checking of file suffix against glob pattern
ClosedPublic

Authored by marten on Sep 9 2016, 9:47 AM.

Details

Summary

When a new image file is detected via Image::pathCreated(), its file extension is checked against BackgroundFinder::suffixes() to ensure that it is a supported format. However, despite its name, suffixes() returns a list of glob patterns (i.e. all prefixed with "*.") so the contains() never succeeds and the new file is not added.

suffixes() is also used within the BackgroundCreator class itself to set a directory name filter. To maintain compatibility it may be better to not change the name or meaning of this function, but instead to take account of the pattern in Image::pathCreated(). This patch does just that.

Test Plan

Built plasma-workspace with this changed, checked correct operation of slideshow wallpaper.

Diff Detail

Repository
R120 Plasma Workspace
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
marten updated this revision to Diff 6578.Sep 9 2016, 9:47 AM
marten retitled this revision from to Wallpaper slideshow: Incorrect checking of file suffix against glob pattern.
marten updated this object.
marten edited the test plan for this revision. (Show Details)
marten added a reviewer: Plasma.
marten set the repository for this revision to R120 Plasma Workspace.
Restricted Application added a project: Plasma. · View Herald TranscriptSep 9 2016, 9:47 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
davidedmundson accepted this revision.Sep 9 2016, 9:50 AM
davidedmundson added a reviewer: davidedmundson.
davidedmundson added a subscriber: davidedmundson.

Comments are more useful in the code than a commit log. Copy and paste the bit about globs in there.

Also do you have commit access?

This revision is now accepted and ready to land.Sep 9 2016, 9:50 AM
broulik added a subscriber: broulik.Sep 9 2016, 9:58 AM

I think it would be cleaner to have a BackgroundFinder::isAcceptableSuffix(const QString &suffix) const method.

broulik's suggestion would make the code clearer, I'll look into it.

marten updated this revision to Diff 6579.Sep 9 2016, 10:15 AM
marten edited edge metadata.
marten removed R120 Plasma Workspace as the repository for this revision.

Implemented BackgroundFinder::isAcceptableSuffix() as suggested.

Yes, I already have Git commit access (assuming that nothing special needs to be set up within Phabricator, this being my first review in that).

marten set the repository for this revision to R120 Plasma Workspace.Sep 9 2016, 10:16 AM

Sorry, I'm not yet totally au fait with the Phabricator workflow. Does "accepted and ready to land" mean to commit it?

This revision was automatically updated to reflect the committed changes.