D27544: Fix update scenarios with no explicit downloadlink selected

[Arjen Hiemstra]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20200317/aaf77e47/attachment-0001.html>