D27544: Fix update scenarios with no explicit downloadlink selected

[Dan Leinir Turthra Jensen]

This revision was automatically updated to reflect the committed changes.
Closed by commit R304:f1dcbddf12ea: Fix update scenarios with no explicit 
downloadlink selected (authored by leinir).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D27544?vs=77818=77989#toc

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27544?vs=77818=77989

REVISION DETAIL
  https://phabricator.kde.org/D27544

AFFECTED FILES
  CMakeLists.txt
  src/core/engine.cpp
  src/qtquick/qml/EntryDetails.qml
  src/qtquick/qml/NewStuffItem.qml
  src/qtquick/qml/private/entrygriddelegates/BigPreviewDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/ThumbDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/TileDelegate.qml
  src/qtquick/quickitemsmodel.cpp
  src/qtquick/quickitemsmodel.h

To: leinir, #knewstuff, #frameworks, #plasma, ngraham, apol, 
#discover_software_store, ahiemstra
Cc: ahiemstra, alexde, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D27544: Fix update scenarios with no explicit downloadlink selected

[Arjen Hiemstra]

ahiemstra accepted this revision.

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, ahiemstra
Cc: ahiemstra, alexde, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D27544: Fix update scenarios with no explicit downloadlink selected

[Dan Leinir Turthra Jensen]

leinir updated this revision to Diff 77818.
leinir marked 4 inline comments as done.
leinir added a comment.


  Address @ahiemstra's comments - thanks!
  
  - Turn on C++14 support
  - Fix some whitespace issues, a leak, and add a warning
  - Less magic numbers, with the power of enums

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27544?vs=77494=77818

BRANCH
  fix-update (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D27544

AFFECTED FILES
  CMakeLists.txt
  src/core/engine.cpp
  src/core/jobs/httpworker.cpp
  src/core/security.cpp
  src/qtquick/qml/EntryDetails.qml
  src/qtquick/qml/NewStuffItem.qml
  src/qtquick/qml/private/entrygriddelegates/BigPreviewDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/ThumbDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/TileDelegate.qml
  src/qtquick/quickitemsmodel.cpp
  src/qtquick/quickitemsmodel.h

To: leinir, #knewstuff, #frameworks, #plasma, ngraham, apol, 
#discover_software_store
Cc: ahiemstra, alexde, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D27544: Fix update scenarios with no explicit downloadlink selected

[Dan Leinir Turthra Jensen]

leinir marked 5 inline comments as done.
leinir added a comment.


  Thanks for those, @ahiemstra, good stuff there :)

INLINE COMMENTS

> ahiemstra wrote in engine.cpp:614
> Code style: & attaches to the name, not the type. (Yes I hate it too).
> 
> There's a few instances of this around.

so nasty... right, thanks for pointing those out, it feels so wrong to type :P

> ahiemstra wrote in engine.cpp:631
> 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::SelectFromListQuestion);
> 
> then it will be automatically cleaned up once you exit the scope.

Hmm... That's something that'll want doing in a fair few places i think 
(including the documentation for Question) - but yup, might as well start 
somewhere :) It does mean turning on C++14 for KNewStuff, which... should be 
fine?

> ahiemstra wrote in engine.cpp:644
> 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.

That's probably a good idea, yes... It'll commonly be due to the user 
cancelling the dialog, but if there's no good questionasker registered, it may 
happen anyway - plus i guess it's just good style anyway :)

> ahiemstra wrote in EntryDetails.qml:101
> That "1" here is a bit mysterious, what does it actually mean?

It's that whole one-indexed list thing that OCS has... But yup, your idea below 
sounds pretty good.

> ahiemstra wrote in quickitemsmodel.h:154
> 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.

A good call, less magic numbers are always good. Did feel a little... 
unpleasant to add those.

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

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::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

D27544: Fix update scenarios with no explicit downloadlink selected

[Nathaniel Graham]

ngraham accepted this revision as: ngraham.
ngraham added a comment.
This revision is now accepted and ready to land.


  Probably wait for at least one more review from someone smarter and more 
familiar with this code than I am. :)

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: alexde, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27544: Fix update scenarios with no explicit downloadlink selected

[Dan Leinir Turthra Jensen]

leinir updated this revision to Diff 77494.
leinir added a comment.


  - Merge branch 'master' into fix-update
  - Update @since

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27544?vs=76096=77494

BRANCH
  fix-update (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D27544

AFFECTED FILES
  src/core/engine.cpp
  src/core/jobs/httpworker.cpp
  src/core/security.cpp
  src/qtquick/qml/EntryDetails.qml
  src/qtquick/qml/NewStuffItem.qml
  src/qtquick/qml/private/entrygriddelegates/BigPreviewDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/ThumbDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/TileDelegate.qml
  src/qtquick/quickitemsmodel.cpp
  src/qtquick/quickitemsmodel.h

To: leinir, #knewstuff, #frameworks, #plasma, ngraham, apol, 
#discover_software_store
Cc: alexde, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27544: Fix update scenarios with no explicit downloadlink selected

[Dan Leinir Turthra Jensen]

leinir added a comment.


  In D27544#622575 , @alexde wrote:
  
  > In D27544#622272 , @ngraham 
wrote:
  >
  > > (...) there has to be a way to (...) label everything properly etc.
  >
  >
  >
  >
  > In D27544#622501 , @leinir wrote:
  >
  > > We are at least two people not too happy with that situation... The label 
we're using for that now is technically a free-text field. (...) However, each 
downloaditem does have a tags section, and while that would need some thought 
put into it, i think we could do something clever with that. A bit outside the 
scope of this patch, of course, but something to spent some braining time on :)
  >
  >
  > Sounds similar to bug #415483.
  
  
  Vaguely, but they're more orthogonal than anything - the problem here is a 
lack of well named downloads, which is a generic issue, where the issue 
described in that bug is content-type-specific.
  
  It is also not exactly what this diff is about and i'd like to avoid this 
going off on a tangent, especially given the issue this patch addresses is 
causing updates to just not work...
  
  With that in mind - ping? Been a few weeks now, and i'll have to update the 
patch because we've had a frameworks release in the meantime...

REPOSITORY
  R304 KNewStuff

REVISION DETAIL
  https://phabricator.kde.org/D27544

To: leinir, #knewstuff, #frameworks, #plasma, ngraham, apol, 
#discover_software_store
Cc: alexde, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27544: Fix update scenarios with no explicit downloadlink selected

[Alex Debus]

alexde added a comment.


  In D27544#622272 , @ngraham wrote:
  
  > (...) there has to be a way to (...) label everything properly etc.
  
  
  
  
  In D27544#622501 , @leinir wrote:
  
  > We are at least two people not too happy with that situation... The label 
we're using for that now is technically a free-text field. (...) However, each 
downloaditem does have a tags section, and while that would need some thought 
put into it, i think we could do something clever with that. A bit outside the 
scope of this patch, of course, but something to spent some braining time on :)
  
  
  Sounds similar to bug #415483.

REPOSITORY
  R304 KNewStuff

REVISION DETAIL
  https://phabricator.kde.org/D27544

To: leinir, #knewstuff, #frameworks, #plasma, ngraham, apol, 
#discover_software_store
Cc: alexde, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27544: Fix update scenarios with no explicit downloadlink selected

[Dan Leinir Turthra Jensen]

leinir added a comment.


  In D27544#622272 , @ngraham wrote:
  
  > As an aside, I'm somewhat dissatisfied with the current UX when there are 
multiple files. :( I converted a friend of mine to Plasma the other day and he 
was very confused by the multiple items available when he was downloading new 
stuff using the new GHNS window. I know that some of this is the fault of 
content uploaders, but if so, there has to be a way to guide them to upload 
only one file, or to mark a specific file as the real/primary one, and to label 
everything properly etc.
  
  
  We are at least two people not too happy with that situation... The label 
we're using for that now is technically a free-text field, so it certainly 
should be possible to fix in a backwards-compatible sort of way (as it's a 
question of what the server provides us with)... but as for "the real one", 
that's not a thing that makes sense except in very specific cases (for example, 
it makes no sense conceptually when you have e.g. icon sets or mouse cursor 
sets with multiple variants...). However, each downloaditem does have a tags 
section, and while that would need some thought put into it, i think we could 
do something clever with that. A bit outside the scope of this patch, of 
course, but something to spent some braining time on :)
  
  In the meantime, the situation needs some work anyway, as what currently 
happens when there's crazy long downloaditem names is that it gets elided on 
the right... which would be sort of fine, if you could get to that information 
some other way, which you can't, so... yup, wants fixing ;) (also outside the 
scope of this patch, mind, but still)

REPOSITORY
  R304 KNewStuff

REVISION DETAIL
  https://phabricator.kde.org/D27544

To: leinir, #knewstuff, #frameworks, #plasma, ngraham, apol, 
#discover_software_store
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27544: Fix update scenarios with no explicit downloadlink selected

[Nathaniel Graham]

ngraham added a comment.


  As an aside, I'm somewhat dissatisfied with the current UX when there are 
multiple files. :( I converted a friend of mine to Plasma the other day and he 
was very confused by the multiple items available when he was downloading new 
stuff using the new GHNS window. I know that some of this is the fault of 
content uploaders, but if so, there has to be a way to guide them to upload 
only one file, or to mark a specific file as the real/primary one, and to label 
everything properly etc.

REPOSITORY
  R304 KNewStuff

REVISION DETAIL
  https://phabricator.kde.org/D27544

To: leinir, #knewstuff, #frameworks, #plasma, ngraham, apol, 
#discover_software_store
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27544: Fix update scenarios with no explicit downloadlink selected

[Dan Leinir Turthra Jensen]

leinir added reviewers: ngraham, apol, Discover Software Store.
leinir added a comment.


  Tagging in some Discover peeps, because software management is a thing that 
is done there and whatnot ;) (also general ping, this really kind of wants to 
go in soon...)

REPOSITORY
  R304 KNewStuff

REVISION DETAIL
  https://phabricator.kde.org/D27544

To: leinir, #knewstuff, #frameworks, #plasma, ngraham, apol, 
#discover_software_store
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27544: Fix update scenarios with no explicit downloadlink selected

[Dan Leinir Turthra Jensen]

leinir added reviewers: KNewStuff, Frameworks, Plasma.

REPOSITORY
  R304 KNewStuff

REVISION DETAIL
  https://phabricator.kde.org/D27544

To: leinir, #knewstuff, #frameworks, #plasma
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27544: Fix update scenarios with no explicit downloadlink selected

[Dan Leinir Turthra Jensen]

leinir created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
leinir requested review of this revision.

REVISION 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

REPOSITORY
  R304 KNewStuff

BRANCH
  fix-update (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D27544

AFFECTED FILES
  src/core/engine.cpp
  src/qtquick/qml/EntryDetails.qml
  src/qtquick/qml/NewStuffItem.qml
  src/qtquick/qml/private/entrygriddelegates/BigPreviewDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/ThumbDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/TileDelegate.qml
  src/qtquick/quickitemsmodel.cpp
  src/qtquick/quickitemsmodel.h

To: leinir
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns