Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)
ClosedPublic

Authored by leinir on Jun 10 2019, 1:11 PM.

Details

Summary

Implement a new set of Qt Quick components, aimed at bringing the KNewStuffQuick submodule up to feature parity with the original QWidget based versions. This also introduces the ability for KNewStuff to handle comments. The principal components are:

  • Button A button which, when clicked, opens up a dialog with a Page as its base. Equivalent to the old Button widget.
  • Dialog A dialog (QtQuick Dialogs based) with a Page as its base. Equivalent to the old DownloadDialog (though the intention in especially Kirigami based applications would be to use a Page directly, rather than opening a dialog).
  • DialogContents The contents of the above dialog. Equivalent to the old DownloadWidget, though the same caveats apply as above.
  • DownloadItemsSheet A sheet (Kirigami.OverlaySheet based) which shows the available installable items in a KNewStuff Entry, for use when there is more than one option.
  • EntryDetails A Page (org.kde.kcm SimpleKCM based) which shows the details for a single Entry. Equivalent to the details view in the old DownloadDialog.
  • Page A Page (org.kde.kcm GridViewKCM based) which allows the management of KNewStuff entries. Functionally equivalent to the old DownloadDialog (and intended to be used directly as a page, rather than through the Dialog, though this also exists for ease of porting and integration).
  • QuestionAsker A component which forwards questions from the engine to the UI. This was supposed to be there from the start, but ended up missing in the initial version of KNewStuffQuick
  • Author A component without visual representation which provides access to named authors on an OCS server. This is intended for ease of using OCS Person information in applications (such as about dialogues and the like).

KNewStuffCore also received a couple of new bits, most importantly the addition of support for comments, and support for additional fields for authors and fetching a specific author by id.

Further, the NewStuffList and NewStuffItem components have received considerable attention and are now much more stable and capable (including the missing QuestionAsker functionality, which resulted in things like "this download file already exists" type situations not being handled, which could yield a hung application. However, as these components were only ever really used by Peruse, it seems like this was less of an issue)


The test tool


The Dialog


Detail view for one entry


The comments display for the entry shown above (using a Discover inspired delegate, with a visualisation of nesting added in, after chatting with members of the VDG about it some)


Another view mode for the entries list


A view mode (designed for highly visual content such as wallpapers) with large screenshots and more of the description


The download items sheet

Note on patch scope and intentions: This does not replace the old QWidget versions, and does not attempt to replace those with the new versions here (as discussed with @ngraham). The intention is to bring the components up to (and past) what the original QWidget versions are able to do, and at a later point switch the implementations of those widgets to using the new components, in a way which causes the least amount of impact.

Test Plan

Use the new dialog test tool to interact with the KNewStuffQuick components

Once built and installed, this tool can be invoked (for example by pointing it at the look and feel config) by doing so:

QT_LOGGING_RULES="org.kde.knewstuff*=true" ./bin/khotnewstuff-dialog /usr/share/knsrcfiles/lookandfeel.knsrc

You can also simply launch it and then pick a knsrc file using the test tool's file picker.

Diff Detail

Repository
R304 KNewStuff
Branch
knsquick-feature-parity-with-kns (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 15996
Build 16014: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
  • Debug--
  • Ensure data is actually loaded properly, also from cache
  • Include the right header
  • Adapt the Discover comments delegate (and add a nesting indicator)
leinir edited the summary of this revision. (Show Details)Aug 29 2019, 12:25 PM
ahiemstra added inline comments.Aug 29 2019, 1:47 PM
src/attica/atticaprovider.cpp
355

Fair enough. I do try to make sure to use vectors as much as possible in new API, but consistency is also a good argument. :)

src/qtquick/qml/Dialog.qml
77

It would match with the FileDialog API (https://doc.qt.io/qt-5/qml-qtquick-dialogs-filedialog.html#fileUrls-prop) however, which also has this behaviour. My main problem with signal parameters is that you cannot bind to them, so using the result gets trickier.

leinir updated this revision to Diff 64978.Aug 30 2019, 9:39 AM
  • Add a kf6 todo item (our api's QListey and should become QVectory)
  • Adopt a more pleasant comments layout
leinir edited the summary of this revision. (Show Details)Aug 30 2019, 9:40 AM
leinir marked an inline comment as done.Aug 30 2019, 9:54 AM
leinir added inline comments.
src/attica/atticaprovider.cpp
355

Think i might just pop in a TODO KF6 comment... just sort of make sure that even if it ends up not being a wholesale policy, we do it here :)

src/qtquick/qml/Dialog.qml
77

Hmm, so it would. I guess documentation helps anyway, i'll switch to doing it like that :)

leinir updated this revision to Diff 64979.Aug 30 2019, 9:54 AM
  • Switch to using a FileDialog-like property for changedEntries
leinir updated this revision to Diff 64986.Aug 30 2019, 12:15 PM
leinir marked 18 inline comments as done.
  • Fiddle with the downloaditems sheet layout a touch
leinir edited the summary of this revision. (Show Details)Aug 30 2019, 12:16 PM
leinir marked 2 inline comments as done.
leinir updated this revision to Diff 65219.Sep 2 2019, 10:04 AM
  • Whole lot of @since-ing
  • Adopt Qt-style KF5-version-aligned import versioning
leinir updated this revision to Diff 65236.Sep 2 2019, 1:19 PM
  • Make sure to import the right version of newstuff
leinir updated this revision to Diff 65506.Sep 6 2019, 1:26 PM
  • Also catch the error messages from the non-deprecated error function
  • Equalize border colour with Breeze
leinir updated this revision to Diff 65579.Sep 7 2019, 1:13 PM
  • Add a getter for the first provider (usually the default)
  • Get the default provider, if one is not yet set
  • Handle the multi-provider scenario
broulik added a subscriber: broulik.Sep 8 2019, 2:17 PM
broulik added inline comments.
src/core/author.h
111

ProfilePage?

src/core/commentsmodel.cpp
185

Any particular reason not do just return a null QVariant()?

src/core/commentsmodel.h
71

Why is this an explicit Engine pointer rather than generic QObject?

72

~CommentsModel() override;?

76

You might want to set that to Qt::DisplayRole

89

QModelIndex is usable from QML these days, this overload isn't neccessary, you can do from QML:

model.data(model.index(row, column), role);
src/core/engine.cpp
803

ah, so this is why it's not const

davidedmundson added inline comments.
src/qtquick/author.h
38

Use of QQmlParserStatus would help here.

It means we can defer initial resetConnections until all 3 initial properties are set.

ognarb awarded a token.Sep 8 2019, 2:24 PM
leinir updated this revision to Diff 65689.Sep 9 2019, 4:17 PM
leinir marked 3 inline comments as done.

Address comments by Kai Uwe and David

  • Use QQmlParserStatus to ensure we don't do unnecessary work
  • Get rid of all the extraneous data entries (also a couple of other bits)
leinir marked 3 inline comments as done.Sep 9 2019, 4:19 PM
leinir added inline comments.
src/core/author.h
111

In principle yes, except we can't change homepage to homePage, until KF6... and then we'd end up with mixed spelling styles, and that just makes me sad ;)

src/core/commentsmodel.cpp
185

Hmm... This actually is supposed to be "Unknown model role"... It's kind of corner-casey, but i've noticed in the past that something other than just an invalid QVariant is occasionally useful for those times where you've, say, missed the h in "depth" ;) Usually this wouldn't be hit, of course, so it's not actually expensive in any real way. But yes, compared to this, QVariant() would be the better option.

src/core/commentsmodel.h
71

Entirely to make it awkward for people to use it in a counter-intended fashion :)

76

Hmm... i guess, except that it's not reeeally the obvious choice (because i don't really see one of those)... but yeah, having one is probably not terrible anyway.

89

Aah! i did not know this, very handy indeed. Couple of places where that'll make life simpler :)

src/core/engine.cpp
803

Indeed :) (and yes, that cache still needs to be made...)

leinir updated this revision to Diff 65696.Sep 9 2019, 6:24 PM
leinir marked 2 inline comments as done.
  • Add a simple cache for CommentsModel instances
leinir marked 6 inline comments as done.Sep 9 2019, 6:27 PM
ahiemstra accepted this revision.Sep 18 2019, 9:27 AM

I went over it again and found a few more small things and also added some suggestions. Feel free to apply or ignore the suggestions. Once the other items have been taken care of, I think this is good to go.

src/core/commentsmodel.cpp
100

Categorise or drop :)

src/core/engine.h
155

I assume you mean setSortMode here?

src/qtquick/author.cpp
38

Suggestion: It might make sense to do this caching at the provider level so users don't need to reimplement this.

src/qtquick/categoriesmodel.cpp
50

Suggestion: I tend to make roles static since it doesn't change between instances anyway.
Like so:

static QHash<int, QByteArray> roles{
    { NameRole, "name" },
    ...
};
src/qtquick/categoriesmodel.h
62

Suggestion: I use const std::unique_ptr<Private> d; these days, it removes the need to explicitly delete the d pointer.

src/qtquick/commentsmodel.h
39

Like Author, it might be a good idea to use QQmlParserStatus here.

src/qtquick/qml/Button.qml
113

Hmm, this is quite tricky, as a user of the Button I can now no longer safely bind enabled as that would override the allowedByKiosk binding. You should probably make this an explicit binding so it doesn't break as easily.

This revision is now accepted and ready to land.Sep 18 2019, 9:27 AM
leinir updated this revision to Diff 66368.Sep 18 2019, 10:50 AM
leinir marked 7 inline comments as done.

Address comments by @ahiemstra

  • roleNames to static const and initialiser list (and qcdebug++)
  • Fix a documentation oops
  • roleNames to static const
  • Quick todo for KF6
  • A todo because more centralised caching would be good
  • Ensure we postpone commentsmodel initialisation until we're ready
  • Warning--
  • Attempt a workaround some binding trouble
leinir updated this revision to Diff 66529.Sep 20 2019, 9:46 AM
  • Fix some whitespace issues
leinir requested review of this revision.Sep 20 2019, 9:47 AM

Ping for some of the frameworks peeps... would be great to get this in sooner rather than later :)

src/qtquick/author.cpp
38

A lot of this will probably want some work when we can break up the BC for KF6... but given the timeframe, yes, that of course means giving the actual bits we want to do some thought now :)

src/qtquick/categoriesmodel.cpp
50

Good call indeed, also perhaps a touch easier to read and such :) (also, this is what the QtQuick ItemsModel already does - i'll leave it to the new models, but probably wants proliferating through the codebase)

src/qtquick/categoriesmodel.h
62

That sounds like something that'll want doing for KF6 as well, yes - i like the consistency, so just leaving it like this for now, but certainly something for the todo list :)

src/qtquick/commentsmodel.h
39

Less critical codepath, but yeah, why not :)

src/qtquick/qml/Button.qml
113

Humhum, yes... so it needs to be possible to change enabled from outside, but it also needs to be locked to disabled if allowedByKiosk is false... i'm thinking this probably won't be the prettiest thing, but... catch enabled changing, check whether allowedByKiosk is false and force it to false in the case it is, and otherwise ignore it... unless there's a magic trick i'm not aware of, which would be neat ;)

ahiemstra added inline comments.Sep 20 2019, 10:35 AM
src/qtquick/qml/Button.qml
113

With a Binding object I can make sure that a button is kept disabled even when the main enabled is bound to a different value. However, I can not prevent javascript "button.enabled = true" from enabling the button, so forcing things in an enabledChanged handler seems the better solution.

leinir updated this revision to Diff 66876.Sep 26 2019, 9:20 AM

Ping @Frameworks - new cycle, chunky thing, be good to get it in early :)

  • Since 5.63 now that 5.62 is out the door

Alrighty, quick chat somewhere else, and i'm going to have to call silent agreement on this one - unless i hear things to the contrary, i'm going to merge this next week (that is, 2nd or 3rd of October 2019) so we can get a bit more wide-spread testing done on it before the next release rolls around. Thanks to those who reviewed already :)

cfeck added a subscriber: cfeck.Sep 26 2019, 7:22 PM

I would suggest to commit it either sooner, or after 5.63 is tagged. If you commit on 3rd, there are only two days to test and decide how to improve or revert before tars are made.

I would suggest to commit it either sooner, or after 5.63 is tagged. If you commit on 3rd, there are only two days to test and decide how to improve or revert before tars are made.

Ow, that's indeed soon, somehow i was under the impression it was later than that, not sure why... right, in that case it'll want to be tomorrow, with an allowance for panic-reverting in case of explosions (which i don't see how would happen, but you never know)

Right, unless i hear otherwise, i'm going to push this 13:00 CEST (that is, in three hours). I realise this is short notice, but the patch has also been sitting here since before Akademy.

This revision was not accepted when it landed; it landed in state Needs Review.Sep 27 2019, 12:40 PM
This revision was automatically updated to reflect the committed changes.
aacid added a subscriber: aacid.Jan 28 2020, 11:40 PM

FWIW this broke artikulate since you changed the expected entry for engine: in KNS.ItemsModel

FWIW this broke artikulate since you changed the expected entry for engine: in KNS.ItemsModel

Ah, would've been waaaay easier to work out what you were talking about if you commented where the change you're referring to actually happened... That is indeed a regression, well spotted (if perhaps a bit late for something merged, after a great many requests for reviews, about half a year ago ;) ). I'll try and get that sorted with the quickness.