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

Fix update scenarios with no explicit downloadlink selected

Summary:
The old downloadwidget's update system would explicitly require a user
to pick which download item to use for updating if there was more than
one available. This work is an attempt at implementing this at the
engine level, while also allowing for there to be no requirement to
make an active choice, unless there is no way to deduce which of
the various potential download items is the one we are trying to
update.

The current heuristics are:
- If there is one downloaditem, that's what we're updating
- If there are more, first try and see if one has the precise url
  as the previously installed payload
- If that fails, check for filename matches (without the rest of
  the url)
- Only if that fails, present the user with a choice

Add an explicit update function in the QtQuick items model

Fix erroneous uses of installItem, and use updateItem for updates
in the QtQuick components

BUG:417510

Reviewers: #knewstuff, #frameworks, #plasma, ngraham, apol, #discover_software_store, ahiemstra

Reviewed By: ngraham, ahiemstra

Subscribers: ahiemstra, alexde, kde-frameworks-devel

Tags: #frameworks

Differential Revision: https://phabricator.kde.org/D27544
parent 707ef317
......@@ -4,6 +4,11 @@ set(KF5_VERSION "5.69.0") # handled by release scripts
set(KF5_DEP_VERSION "5.68.0") # handled by release scripts
project(KNewStuff VERSION ${KF5_VERSION})
# Require at least C++14
set(CMAKE_CXX_STANDARD 14)
set(CMAKE_CXX_STANDARD_REQUIRED ON)
set(CMAKE_CXX_EXTENSIONS OFF)
include(FeatureSummary)
find_package(ECM 5.68.0 NO_MODULE)
set_package_properties(ECM PROPERTIES TYPE REQUIRED DESCRIPTION "Extra CMake Modules." URL "https://commits.kde.org/extra-cmake-modules")
......
......@@ -24,9 +24,11 @@
#include "../entry.h"
#include "commentsmodel.h"
#include "installation.h"
#include "question.h"
#include "xmlloader.h"
#include "imageloader_p.h"
#include <memory>
#include <kconfig.h>
#include <kconfiggroup.h>
#include <knewstuffcore_debug.h>
......@@ -67,6 +69,11 @@ public:
bool configLocationFallback = true;
QString name;
QMap<EntryInternal, CommentsModel*> commentsModels;
// Used for updating purposes - we ought to be saving this information, but we also have to deal with old stuff, and so... this will have to do for now, and so
// TODO KF6: Installed state needs to move onto a per-downloadlink basis rather than per-entry
QMap<EntryInternal, QStringList> payloads;
QMap<EntryInternal, QString> payloadToIdentify;
};
Engine::Engine(QObject *parent)
......@@ -544,6 +551,24 @@ void Engine::install(KNSCore::EntryInternal entry, int linkId)
<< " from: " << entry.providerId();
QSharedPointer<Provider> p = m_providers.value(entry.providerId());
if (p) {
// If linkId is -1, assume that it's an update and that we don't know what to update
if (entry.status() == KNS3::Entry::Updating && linkId == -1) {
if (entry.downloadCount() == 1) {
// If there is only one downloadable item, then we can fairly safely assume that's what we're wanting
// to update, meaning we can bypass some of the more expensive operations in downloadLinkLoaded
d->payloadToIdentify[entry] = QString{};
linkId = 1;
} else {
// While this seems silly, the payload gets reset when fetching the new download link information
d->payloadToIdentify[entry] = entry.payload();
// Drop a fresh list in place so we've got something to work with when we get the links
d->payloads[entry] = QStringList{};
}
} else {
// If there is no payload to identify, we will assume the payload is already known and just use that
d->payloadToIdentify[entry] = QString{};
}
p->loadPayloadLink(entry, linkId);
++m_numInstallJobs;
......@@ -570,7 +595,64 @@ void Engine::slotEntryDetailsLoaded(const KNSCore::EntryInternal &entry)
void Engine::downloadLinkLoaded(const KNSCore::EntryInternal &entry)
{
m_installation->install(entry);
if (entry.status() == KNS3::Entry::Updating) {
if (d->payloadToIdentify.isEmpty()) {
// If there's nothing to identify, and we've arrived here, then we know what the payload is
m_installation->install(entry);
} else if (d->payloads[entry].count() < entry.downloadCount()) {
// We've got more to get before we can attempt to identify anything, so fetch the next one...
QStringList payloads = d->payloads[entry];
payloads << entry.payload();
d->payloads[entry] = payloads;
QSharedPointer<Provider> p = m_providers.value(entry.providerId());
if (p) {
// ok, so this should definitely always work, but... safety first, kids!
p->loadPayloadLink(entry, payloads.count());
}
} else {
// We now have all the links, so let's try and identify the correct one...
QString identifiedLink;
const QString payloadToIdentify = d->payloadToIdentify[entry];
const QStringList &payloads = d->payloads[entry];
if (payloads.contains(payloadToIdentify)) {
// Simplest option, the link hasn't changed at all
identifiedLink = payloadToIdentify;
} else {
// Next simplest option, filename is the same but in a different folder
const QStringRef fileName = payloadToIdentify.splitRef(QChar::fromLatin1('/')).last();
for (const QString &payload : payloads) {
if (payload.endsWith(fileName)) {
identifiedLink = payload;
break;
}
}
if (identifiedLink.isEmpty()) {
// Least simple option, no match - ask the user to pick (and if we still haven't got one... that's us done, no installation)
auto question = std::make_unique<Question>(Question::SelectFromListQuestion);
question->setTitle(i18n("Pick Update Item"));
question->setQuestion(i18n("Please pick the item from the list below which should be used to apply this update. We were unable to identify which item to select, based on the original item, which was named %1").arg(fileName));
question->setList(payloads);
if(question->ask() == Question::OKResponse) {
identifiedLink = question->response();
}
}
}
if (!identifiedLink.isEmpty()) {
KNSCore::EntryInternal theEntry(entry);
theEntry.setPayload(identifiedLink);
m_installation->install(theEntry);
} else {
qCWarning(KNEWSTUFFCORE) << "We failed to identify a good link for updating" << entry.name() << "and are unable to perform the update";
}
// As the serverside data may change before next time this is called, even in the same session,
// let's not make assumptions, and just get rid of this
d->payloads.remove(entry);
d->payloadToIdentify.remove(entry);
}
} else {
m_installation->install(entry);
}
}
void Engine::uninstall(KNSCore::EntryInternal entry)
......
......@@ -98,7 +98,7 @@ KCM.SimpleKCM {
icon.name: "install"
onTriggered: {
if (component.downloadCount == 1) {
newStuffModel.installItem(component.index);
newStuffModel.installItem(component.index, NewStuff.ItemsModel.FirstLinkId);
} else {
downloadItemsSheet.downloadLinks = component.downloadLinks;
downloadItemsSheet.entryId = component.index;
......@@ -111,7 +111,7 @@ KCM.SimpleKCM {
Kirigami.Action {
text: i18ndc("knewstuff5", "Request updating of this item", "Update");
icon.name: "update"
onTriggered: { newStuffModel.installItem(component.index); }
onTriggered: { newStuffModel.updateItem(component.index); }
enabled: component.status == NewStuff.ItemsModel.UpdateableStatus;
visible: enabled;
},
......
......@@ -36,14 +36,14 @@ Kirigami.SwipeListItem {
Kirigami.Action {
text: i18ndc("knewstuff5", "Request installation of this item", "Install");
iconName: "list-add"
onTriggered: { listModel.installItem(model.index, 1); }
onTriggered: { listModel.installItem(model.index, NewStuff.ItemsModel.FirstLinkId); }
enabled: model.status == NewStuff.ItemsModel.DownloadableStatus || model.status == NewStuff.ItemsModel.DeletedStatus;
visible: enabled;
},
Kirigami.Action {
text: i18ndc("knewstuff5", "Request updating of this item", "Update");
iconName: "refresh"
onTriggered: { listModel.installItem(model.index, 1); }
onTriggered: { listModel.updateItem(model.index); }
enabled: model.status == NewStuff.ItemsModel.UpdateableStatus;
visible: enabled;
},
......
......@@ -65,7 +65,7 @@ Private.GridTileDelegate {
iconName: "install"
onTriggered: {
if (model.downloadLinks.length === 1) {
newStuffModel.installItem(model.index, 1);
newStuffModel.installItem(model.index, NewStuff.ItemsModel.FirstLinkId);
} else {
downloadItemsSheet.downloadLinks = model.downloadLinks;
downloadItemsSheet.entryId = model.index;
......@@ -78,7 +78,7 @@ Private.GridTileDelegate {
Kirigami.Action {
text: i18ndc("knewstuff5", "Request updating of this item", "Update");
iconName: "update"
onTriggered: { newStuffModel.installItem(model.index); }
onTriggered: { newStuffModel.updateItem(model.index); }
enabled: model.status == NewStuff.ItemsModel.UpdateableStatus;
visible: enabled;
},
......
......@@ -58,7 +58,7 @@ KCM.GridDelegate {
iconName: "install"
onTriggered: {
if (model.downloadLinks.length === 1) {
newStuffModel.installItem(model.index);
newStuffModel.installItem(model.index, NewStuff.ItemsModel.FirstLinkId);
} else {
downloadItemsSheet.downloadLinks = model.downloadLinks;
downloadItemsSheet.entryId = model.index;
......@@ -71,7 +71,7 @@ KCM.GridDelegate {
Kirigami.Action {
text: i18ndc("knewstuff5", "Request updating of this item", "Update");
iconName: "update"
onTriggered: { newStuffModel.installItem(model.index); }
onTriggered: { newStuffModel.updateItem(model.index); }
enabled: model.status == NewStuff.ItemsModel.UpdateableStatus;
visible: enabled;
},
......
......@@ -66,7 +66,7 @@ Private.GridTileDelegate {
iconName: "install"
onTriggered: {
if (model.downloadLinks.length === 1) {
newStuffModel.installItem(model.index, 1);
newStuffModel.installItem(model.index, NewStuff.ItemsModel.FirstLinkId);
} else {
downloadItemsSheet.downloadLinks = model.downloadLinks;
downloadItemsSheet.entryId = model.index;
......@@ -79,7 +79,7 @@ Private.GridTileDelegate {
Kirigami.Action {
text: i18ndc("knewstuff5", "Request updating of this item", "Update");
iconName: "update"
onTriggered: { newStuffModel.installItem(model.index); }
onTriggered: { newStuffModel.updateItem(model.index); }
enabled: model.status == NewStuff.ItemsModel.UpdateableStatus;
visible: enabled;
},
......
......@@ -406,6 +406,11 @@ void ItemsModel::installItem(int index, int linkId)
}
}
void ItemsModel::updateItem(int index)
{
installItem(index, AutoDetectLinkId);
}
void ItemsModel::uninstallItem(int index)
{
if (d->coreEngine) {
......
......@@ -121,6 +121,14 @@ public:
UpdatingStatus
};
Q_ENUM(ItemStatus)
// The lists in OCS are one-indexed, and that isn't how one usually does things in C++.
// Consequently, this enum removes what would seem like magic numbers from the code, and
// makes their meaning more explicit.
enum LinkId {
AutoDetectLinkId = -1,
FirstLinkId = 1
};
Q_ENUM(LinkId)
QHash< int, QByteArray > roleNames() const override;
QVariant data(const QModelIndex& index, int role = Qt::DisplayRole) const override;
......@@ -151,8 +159,23 @@ public:
* the function will simply return without performing any actions)
*
* @param index The index of the item to install or update
* @param linkId The download item to install. If this is -1, it is assumed to be an update with an unknown payload, and a number of heuristics will be applied by the engine
* @see Engine::downloadLinkLoaded implementation for details
* @see LinkId
*/
Q_INVOKABLE void installItem(int index, int linkId);
/**
* @brief This will request an update of the given item
*
* There are no side effects of this function if it is called on an item which is not
* in an updateable state (that is, nothing will happen if this is called on an item
* which is not already installed, or on an installed item which does not have updates
* available).
*
* @param index The index of the item you wish to update
* @since 5.69
*/
Q_INVOKABLE void updateItem(int index);
/**
* @brief Uninstall an already installed item
*
......
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