Differential D27544

Fix update scenarios with no explicit downloadlink selected
ClosedPublic
Actions

Authored by leinir on Feb 21 2020, 11:52 AM.

Details

Reviewers
ngraham
apol
ahiemstra
Group Reviewers
KNewStuff
Frameworks
Plasma
Discover Software Store
Commits
R304:f1dcbddf12ea: 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

Diff Detail

Repository
R304 KNewStuff
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
leinir created this revision.Feb 21 2020, 11:52 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptFeb 21 2020, 11:52 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
leinir requested review of this revision.Feb 21 2020, 11:52 AM
leinir added reviewers: ngraham, apol, Discover Software Store.Mar 4 2020, 9:12 AM

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...)

ngraham added a comment.Mar 4 2020, 10:47 PM

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.

leinir added a comment.Mar 5 2020, 9:00 AM
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)

alexde added a subscriber: alexde.Mar 5 2020, 11:20 AM
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.

leinir added a comment.Mar 12 2020, 11:53 AM
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...

leinir updated this revision to Diff 77494.Mar 12 2020, 11:55 AM
  • Merge branch 'master' into fix-update
  • Update @since
ngraham accepted this revision as: ngraham.Mar 13 2020, 7:45 PM

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

This revision is now accepted and ready to land.Mar 13 2020, 7:45 PM
ahiemstra added a subscriber: ahiemstra.Mar 17 2020, 10:13 AM
ahiemstra added inline comments.
src/core/engine.cpp
615

Code style: & attaches to the name, not the type. (Yes I hate it too).

There's a few instances of this around.

622

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.

632

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.

645

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.

src/qtquick/qml/EntryDetails.qml
101

That "1" here is a bit mysterious, what does it actually mean?

src/qtquick/quickitemsmodel.h
162

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.

leinir marked 5 inline comments as done.Mar 17 2020, 12:39 PM

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

src/core/engine.cpp
615

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

632

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?

645

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

src/qtquick/qml/EntryDetails.qml
101

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

src/qtquick/quickitemsmodel.h
162

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

leinir updated this revision to Diff 77818.Mar 17 2020, 12:39 PM
leinir marked 4 inline comments as done.

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
ahiemstra accepted this revision.Mar 19 2020, 10:32 AM
Closed by commit R304:f1dcbddf12ea: Fix update scenarios with no explicit downloadlink selected (authored by leinir). · Explain WhyMar 19 2020, 10:34 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 77989

View Options

CMakeLists.txt

Loading...
View Options

src/core/engine.cpp

Loading...
View Options

src/qtquick/qml/EntryDetails.qml

Loading...
View Options

src/qtquick/qml/NewStuffItem.qml

Loading...
View Options

src/qtquick/qml/private/entrygriddelegates/BigPreviewDelegate.qml

Loading...
View Options

src/qtquick/qml/private/entrygriddelegates/ThumbDelegate.qml

Loading...
View Options

src/qtquick/qml/private/entrygriddelegates/TileDelegate.qml

Loading...
View Options

src/qtquick/quickitemsmodel.h

Loading...
View Options

src/qtquick/quickitemsmodel.cpp

Loading...