Commit 25797117 authored by Dan Leinir Turthra Jensen's avatar Dan Leinir Turthra Jensen 🌈
Browse files

Less risk of infinite spinner on uninstalling KPackage based things

Prior to this patch, the uninstallation function would attempt to work out if the installed item was a directory by checking for a backslash at the end of the installed files. Since we have QFileInfo, which we're already using further down in this function anyway, we might as well use the ability of that to tell us whether or not the item is a directory.

The end result is that we no longer have an infinite spinner after (successfully, as it happens) uninstalling KPackage based entries, such as Plasmoids. The symptom was most easily visible in the Plasmoid installer, which retains its engine across the entirety of a Plasma session (since the dialogue lived inside the Plasma process, and consequently never was destructed, meaning the weird state was retained).

This patch further rewords the text shown when uninstalling entries, which since it is also the text shown when installing them must be more generic. They now say some variant of "Working", to show that the entry is being worked on, which is sufficiently generic to not involve awkward formulations such as long winded "Installing or Uninstalling", or even worse "(Un)installing". At a later point in time, introducing a more full feedback system for the installation progress would be advantageous, but for a rapid fix to the immediate issue, this works.

BUG:434371
parent dac90839
......@@ -33,6 +33,7 @@ private Q_SLOTS:
void testInstallCommandArchive();
void testInstallCommandTopLevelFilesInArchive();
void testUninstallCommand();
void testUninstallCommandDirectory();
};
void InstallationTest::initTestCase()
......@@ -99,6 +100,29 @@ void InstallationTest::testUninstallCommand()
QVERIFY(QFileInfo::exists("uninstalled.txt"));
}
void InstallationTest::testUninstallCommandDirectory()
{
static const QLatin1String testDir{"testDirectory"};
// This will be left over from the previous test, so clean it up first
QFile::remove("uninstalled.txt");
EntryInternal entry;
entry.setUniqueId("0");
QDir().mkdir(testDir);
entry.setStatus(KNS3::Entry::Installed);
entry.setInstalledFiles(QStringList(testDir));
QVERIFY(QFileInfo(testDir).exists());
QVERIFY(!QFileInfo::exists("uninstalled.txt"));
installation->uninstall(entry);
QSignalSpy spy(installation, &Installation::signalEntryChanged);
QVERIFY(spy.wait());
QCOMPARE(entry.status(), KNS3::Entry::Deleted);
QVERIFY(!QFileInfo(testDir).exists());
QVERIFY(QFileInfo::exists("uninstalled.txt"));
}
void InstallationTest::testInstallCommandArchive()
{
EntryInternal entry;
......
......@@ -729,10 +729,12 @@ void Installation::uninstall(EntryInternal entry)
{
// TODO Put this in pimpl or job
const auto deleteFilesAndMarkAsUninstalled = [entry, this]() {
KNS3::Entry::Status newStatus{KNS3::Entry::Deleted};
const auto lst = entry.installedFiles();
for (const QString &file : lst) {
// This was used to delete the download location if there are no more entries
if (file.endsWith(QLatin1Char('/'))) {
// This is used to delete the download location if there are no more entries
QFileInfo info(file);
if (info.isDir()) {
QDir().rmdir(file);
} else if (file.endsWith(QLatin1String("/*"))) {
QDir dir(file.left(file.size() - 2));
......@@ -742,12 +744,15 @@ void Installation::uninstall(EntryInternal entry)
continue;
}
} else {
QFileInfo info(file);
if (info.exists() || info.isSymLink()) {
bool worked = QFile::remove(file);
if (!worked) {
qWarning() << "unable to delete file " << file;
return;
Q_EMIT signalInstallationFailed(
i18n("The removal of %1 failed, as the installed file %2 could not be automatically removed. You can attempt to manually delete this file, if you believe this is in error.", entry.name(), file));
// Assume that the uninstallation has failed, and reset the entry to an installed state
newStatus = KNS3::Entry::Installed;
break;
}
} else {
qWarning() << "unable to delete file " << file << ". file does not exist.";
......@@ -755,9 +760,11 @@ void Installation::uninstall(EntryInternal entry)
}
}
EntryInternal newEntry = entry;
newEntry.setUnInstalledFiles(entry.installedFiles());
newEntry.setInstalledFiles(QStringList());
newEntry.setStatus(KNS3::Entry::Deleted);
if (newStatus == KNS3::Entry::Deleted) {
newEntry.setUnInstalledFiles(entry.installedFiles());
newEntry.setInstalledFiles(QStringList());
}
newEntry.setStatus(newStatus);
Q_EMIT signalEntryChanged(newEntry);
};
......
......@@ -61,7 +61,7 @@ KCM.SimpleKCM {
|| status == NewStuff.ItemsModel.DeletedStatus) {
statusCard.message = "";
} else if (status == NewStuff.ItemsModel.InstallingStatus) {
statusCard.message = i18ndc("knewstuff5", "Status message to be shown when the entry is in the process of being installed", "Currently installing the item %1 by %2. Please wait...", component.name, entryAuthor.name);
statusCard.message = i18ndc("knewstuff5", "Status message to be shown when the entry is in the process of being installed OR uninstalled", "Currently working on the item %1 by %2. Please wait...", component.name, entryAuthor.name);
} else if (status == NewStuff.ItemsModel.UpdatingStatus) {
statusCard.message = i18ndc("knewstuff5", "Status message to be shown when the entry is in the process of being updated", "Currently updating the item %1 by %2. Please wait...", component.name, entryAuthor.name);
} else {
......
......@@ -101,7 +101,8 @@ Kirigami.SwipeListItem {
bottom: parent.bottom;
margins: Kirigami.Units.smallSpacing;
}
text: (model.status == NewStuff.ItemsModel.InstallingStatus) ? "Installing" : ((model.status == NewStuff.ItemsModel.UpdatingStatus) ? "Updating" : "");
// There is no distinction between installing and uninstalling as a status, so we have to word things accordingly
text: (model.status == NewStuff.ItemsModel.InstallingStatus) ? "Working" : ((model.status == NewStuff.ItemsModel.UpdatingStatus) ? "Updating" : "");
width: paintedWidth;
}
}
......
......@@ -41,7 +41,7 @@ Item {
|| status == NewStuff.ItemsModel.DeletedStatus) {
statusLabel.text = "";
} else if (status == NewStuff.ItemsModel.InstallingStatus) {
statusLabel.text = i18ndc("knewstuff5", "Label for the busy indicator showing an item is being installed", "Installing...");
statusLabel.text = i18ndc("knewstuff5", "Label for the busy indicator showing an item is being installed OR uninstalled", "Working...");
} else if (status == NewStuff.ItemsModel.UpdatingStatus) {
statusLabel.text = i18ndc("knewstuff5", "Label for the busy indicator showing an item is in the process of being updated", "Updating...");
} else {
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment