Fix Skin validation when installed from KNS
ClosedPublic

Authored by chauvin on Jun 4 2018, 3:04 PM.

Details

Summary

When a Skin is downloaded from KNS, Yakuake fails to validate the presence of title.skin and tabs.skin files.
The skin is then uninstalled even if these files exist.

This is due to this optimization of KNS: https://phabricator.kde.org/D6104
Files are not listed anymore by entry.installedFiles().
They are replaced by paths that end with /*

In order to fix this bug I changed the validateSkin method so that it checks the presence of files in the filesystem instead of in a list of entries returned by KNS.

BUG: 395012

Test Plan

Download a Skin from KNS: It should not complain about missing files.
Install a correct skin from local: it should not complain about missing files;
Install a wrong skin from local: it should complain about missing files;
Install a correct skin already installed from local: it should ask for overwrite existing skin.

Diff Detail

Repository
R369 Yakuake
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
chauvin requested review of this revision.Jun 4 2018, 3:04 PM
chauvin created this revision.
chauvin edited the summary of this revision. (Show Details)
apol added a comment.Jan 8 2019, 11:27 PM

Since the skin is already installed, maybe it would make more sense to check over the file system contents, no?

In D13335#389573, @apol wrote:

Since the skin is already installed, maybe it would make more sense to check over the file system contents, no?

I agree but validateSkin() is also used when a skin is installed from local. It checks the presence of files inside a tar archive before extract it.
I don't want to duplicate the validation process.

Installation of themes from local and from KNS is very common in KDE. I am surprised that this does not exist in a common library.

Zren added a subscriber: Zren.Jan 28 2019, 3:26 AM
chauvin updated this revision to Diff 53056.Mar 3 2019, 11:15 AM

As suggested by Aleix Pol Gonzalez, I changed the validateSkin() method in order to check the existence of mandatory files in the filesystem.
This method is called when the skin is installed from kns or from a tar archive.
I changed the validation process in the case of a tar archive: the validation is now done after the tar is extracted.
If the skin is not valid, it is removed from the filesystem. I factored the removal process in the removeSkin() method.
I also fixed a bug in extractKnsSkinIds() line 463: when one skin "my_skin" was installed, it returned QSet("my_skin", "") instead of QSet("my_skin")

chauvin edited the summary of this revision. (Show Details)Mar 3 2019, 11:22 AM
chauvin edited the summary of this revision. (Show Details)

Here a wrong skin with a missing file:


It can help to check the validation process in the case of a tar archive.

ngraham edited the summary of this revision. (Show Details)Mar 3 2019, 2:43 PM
chauvin edited the test plan for this revision. (Show Details)Mar 5 2019, 7:52 PM

Can someone take a look to this patch ?
Thanks.

chauvin edited the summary of this revision. (Show Details)Mar 5 2019, 7:54 PM
hein accepted this revision.Mar 7 2019, 6:03 PM

Looks good now!

Do you need help landing this or do you have dev access?

This revision is now accepted and ready to land.Mar 7 2019, 6:03 PM

I don't think I have dev access.
I don't know how to commit from phabricator.

This revision was automatically updated to reflect the committed changes.
hein added a comment.Mar 10 2019, 7:31 AM

Boom! Landed it for you. Sorry this took so long!