ahiemstra added inline comments.
INLINE COMMENTS
> engine.cpp:614
> + QString identifiedLink;
> + const QString& payloadToIdentify = d->payloadToIdentify[entry];
> + const QStringList& payloads = d->payloads[entry];
Code style: & attaches to the name, not the type. (Yes I hate it too).
There's a few instances of this around.
> engine.cpp:621
> + // 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) {
This makes a reference to a temporary which I'm not sure is a good idea. Since
you are already using splitRef, just make a copy.
> engine.cpp:631
> + // Least simple option, no match - ask the user to pick
> (and if we still haven't got one... that's us done, no installation)
> + Question* question = new
> Question(Question::SelectFromListQuestion, this);
> + question->setTitle(i18n("Pick Update Item"));
This object will linger until the engine is destroyed, which seems like a
suboptimal thing. Maybe better to do:
auto question = std::make_unique<Question>(Question::SelectFromListQuestion);
then it will be automatically cleaned up once you exit the scope.
> engine.cpp:644
> + m_installation->install(theEntry);
> + }
> + // As the serverside data may change before next time this is
> called, even in the same session,
You may want to log something in the else here, at least then it will be
possible to figure out why an update is not happening.
> EntryDetails.qml:101
> if (component.downloadCount == 1) {
> - newStuffModel.installItem(component.index);
> + newStuffModel.installItem(component.index, 1);
> } else {
That "1" here is a bit mysterious, what does it actually mean?
> quickitemsmodel.h:154
> * @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
Rather than using -1, you could add an enum that defines these values a bit
more, like:
enum LinkId {
AutoDetectLink = -1,
FirstItem = 1
}
You can then also expose that to QML to avoid the magical 1 up there.
REPOSITORY
R304 KNewStuff
BRANCH
fix-update (branched from master)
REVISION DETAIL
https://phabricator.kde.org/D27544
To: leinir, #knewstuff, #frameworks, #plasma, ngraham, apol,
#discover_software_store
Cc: ahiemstra, alexde, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh,
ngraham, bruns
