Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)
Needs ReviewPublic

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

Details

Reviewers
ahiemstra
Group Reviewers
KNewStuff
VDG
Frameworks
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 16824
Build 16842: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
leinir added inline comments.Wed, Aug 28, 3:01 PM
src/attica/atticaprovider.cpp
355

That'd be good, except the rest of the KNewStuff API is all QList based. It'll want doing for KF6, but since QList is being deprecated for that anyway, i'm thinking we'll end up with a general QList->QVector porting effort for that time anyway, and right now it'd just be introducing a different API style for seemingly no reason... Otherwise yes :)

359

Handy :) One of those places where i'm not opposed to auto, as it's easily read from the right hand side ;)

370

Actually leftovers... but also yes :)

src/qtquick/qml/Dialog.qml
35

Don't actually need 5.12 ones... so i'll scale back to 5.11, but also a bunch of cleanup elsewhere, and also actually /require/ 5.11 because of that other bit of handy functionality with checkIndex :)

77

Because a changedEntries property would logically be for the lifetime of the component instance, where dialogFinished is for this specific time the dialog was opened... The property would reasonably also be useful, but it would be semantically different (unless it's documented as being cleared when the dialog is shown, and then filled with whatever's changed once the dialog has been closed... which we could do, but kind of feels uglier than this signal)

src/qtquick/qml/DownloadItemsSheet.qml
50

This definitely is something that'd be great to hear from the VDG about (who i seem to not be able to @ as a group, i guess because it's not really a user or whatnot... certainly would be handy ;) )

src/qtquick/qml/Page.qml
211

It would, yes... Feared it was a bit clugey, but it was super straightforward, so yay for that ;)

src/qtquick/qmlplugin.cpp
56

Hmm... Quite. It doesn't really matter functionally, but the code in the QuickQuestionListener suggests that it's cpp owned, so yeah, let's just do that :)

leinir updated this revision to Diff 64855.Wed, Aug 28, 3:02 PM
leinir marked 11 inline comments as done.

Address comments by @ahiemstra

  • Add a comment about why Button's configfile isn't aliased
  • Set the object ownership policy for QuickQuestionListener to cpp
  • std::make_shared for clarity, and a bit of debug categorisation
  • Simplify some whiley recursion, and use checkIndex()
  • Remember to check before setting properties to the same value
  • Actually require 5.11 (and sanitise some of the QtQuick imports)
  • Pull out the three delegate components
  • Add a wrapper component for CommentsModel
  • Use the new CommentsModel component
leinir updated this revision to Diff 64934.Thu, Aug 29, 12:23 PM
  • 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)Thu, Aug 29, 12:25 PM
ahiemstra added inline comments.Thu, Aug 29, 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.Fri, Aug 30, 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)Fri, Aug 30, 9:40 AM
leinir marked an inline comment as done.Fri, Aug 30, 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.Fri, Aug 30, 9:54 AM
  • Switch to using a FileDialog-like property for changedEntries
leinir updated this revision to Diff 64986.Fri, Aug 30, 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)Fri, Aug 30, 12:16 PM
leinir marked 2 inline comments as done.
leinir updated this revision to Diff 65219.Mon, Sep 2, 10:04 AM
  • Whole lot of @since-ing
  • Adopt Qt-style KF5-version-aligned import versioning
leinir updated this revision to Diff 65236.Mon, Sep 2, 1:19 PM
  • Make sure to import the right version of newstuff
leinir updated this revision to Diff 65506.Fri, Sep 6, 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.Sat, Sep 7, 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.Sun, Sep 8, 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
804

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.Sun, Sep 8, 2:24 PM
leinir updated this revision to Diff 65689.Mon, Sep 9, 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.Mon, Sep 9, 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
804

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

leinir updated this revision to Diff 65696.Mon, Sep 9, 6:24 PM
leinir marked 2 inline comments as done.
  • Add a simple cache for CommentsModel instances
leinir marked 6 inline comments as done.Mon, Sep 9, 6:27 PM
ahiemstra accepted this revision.Wed, Sep 18, 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.Wed, Sep 18, 9:27 AM
leinir updated this revision to Diff 66368.Wed, Sep 18, 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.Fri, Sep 20, 9:46 AM
  • Fix some whitespace issues
leinir requested review of this revision.Fri, Sep 20, 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.Fri, Sep 20, 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.