D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

2020-01-29 Thread Dan Leinir Turthra Jensen 
leinir added a comment.


  In D21721#602167 , @aacid wrote:
  
  > FWIW this broke artikulate since you changed the expected entry for engine: 
in KNS.ItemsModel
  
  
  Ah, would've been wy 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.

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #vdg, #frameworks, ahiemstra
Cc: aacid, cfeck, davidedmundson, broulik, ahiemstra, anthonyfieroni, pino, 
ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

2020-01-28 Thread Albert Astals Cid 
aacid added a comment.


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

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #vdg, #frameworks, ahiemstra
Cc: aacid, cfeck, davidedmundson, broulik, ahiemstra, anthonyfieroni, pino, 
ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

2019-09-27 Thread Dan Leinir Turthra Jensen 
This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R304:140c0d0b7be8: Bring KNewStuffQuick to feature parity with 
KNewStuff(Widgets) (authored by leinir).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D21721?vs=66876=66944#toc

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21721?vs=66876=66944

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

AFFECTED FILES
  src/attica/atticaprovider.cpp
  src/attica/atticaprovider_p.h
  src/core/CMakeLists.txt
  src/core/author.cpp
  src/core/author.h
  src/core/commentsmodel.cpp
  src/core/commentsmodel.h
  src/core/engine.cpp
  src/core/engine.h
  src/core/entryinternal.cpp
  src/core/installation.cpp
  src/core/itemsmodel.cpp
  src/core/itemsmodel.h
  src/core/provider.h
  src/core/question.cpp
  src/core/question.h
  src/qtquick/CMakeLists.txt
  src/qtquick/author.cpp
  src/qtquick/author.h
  src/qtquick/categoriesmodel.cpp
  src/qtquick/categoriesmodel.h
  src/qtquick/commentsmodel.cpp
  src/qtquick/commentsmodel.h
  src/qtquick/qml/Button.qml
  src/qtquick/qml/Dialog.qml
  src/qtquick/qml/DialogContent.qml
  src/qtquick/qml/DownloadItemsSheet.qml
  src/qtquick/qml/EntryDetails.qml
  src/qtquick/qml/NewStuffItem.qml
  src/qtquick/qml/NewStuffList.qml
  src/qtquick/qml/Page.qml
  src/qtquick/qml/QuestionAsker.qml
  src/qtquick/qml/private/ConditionalLoader.qml
  src/qtquick/qml/private/EntryCommentDelegate.qml
  src/qtquick/qml/private/EntryCommentsPage.qml
  src/qtquick/qml/private/EntryScreenshots.qml
  src/qtquick/qml/private/GridTileDelegate.qml
  src/qtquick/qml/private/Rating.qml
  src/qtquick/qml/private/Shadow.qml
  src/qtquick/qml/private/entrygriddelegates/BigPreviewDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/ThumbDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/TileDelegate.qml
  src/qtquick/qmldir
  src/qtquick/qmlplugin.cpp
  src/qtquick/quickengine.cpp
  src/qtquick/quickengine.h
  src/qtquick/quickitemsmodel.cpp
  src/qtquick/quickitemsmodel.h
  src/qtquick/quickquestionlistener.cpp
  src/qtquick/quickquestionlistener.h
  tests/CMakeLists.txt
  tests/khotnewstuff-dialog-ui/main.qml
  tests/khotnewstuff-dialog.cpp

To: leinir, #knewstuff, #vdg, #frameworks, ahiemstra
Cc: cfeck, davidedmundson, broulik, ahiemstra, anthonyfieroni, pino, ngraham, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

2019-09-27 Thread Dan Leinir Turthra Jensen 
leinir added a comment.


  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.

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #vdg, #frameworks, ahiemstra
Cc: cfeck, davidedmundson, broulik, ahiemstra, anthonyfieroni, pino, ngraham, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

2019-09-26 Thread Dan Leinir Turthra Jensen 
leinir added a comment.


  In D21721#538404 , @cfeck wrote:
  
  > 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)

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #vdg, #frameworks, ahiemstra
Cc: cfeck, davidedmundson, broulik, ahiemstra, anthonyfieroni, pino, ngraham, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

2019-09-26 Thread Christoph Feck 
cfeck added a comment.


  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.

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #vdg, #frameworks, ahiemstra
Cc: cfeck, davidedmundson, broulik, ahiemstra, anthonyfieroni, pino, ngraham, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

2019-09-26 Thread Dan Leinir Turthra Jensen 
leinir added a comment.


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

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #vdg, #frameworks, ahiemstra
Cc: davidedmundson, broulik, ahiemstra, anthonyfieroni, pino, ngraham, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

2019-09-26 Thread Dan Leinir Turthra Jensen 
leinir updated this revision to Diff 66876.
leinir added a comment.


  Ping @Frameworks - new cycle, chunky thing, be good to get it in early :)
  
  - Since 5.63 now that 5.62 is out the door

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21721?vs=66529=66876

BRANCH
  knsquick-feature-parity-with-kns (branched from master)

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

AFFECTED FILES
  CMakeLists.txt
  src/attica/atticaprovider.cpp
  src/attica/atticaprovider_p.h
  src/core/CMakeLists.txt
  src/core/author.cpp
  src/core/author.h
  src/core/commentsmodel.cpp
  src/core/commentsmodel.h
  src/core/engine.cpp
  src/core/engine.h
  src/core/entryinternal.cpp
  src/core/installation.cpp
  src/core/itemsmodel.cpp
  src/core/itemsmodel.h
  src/core/provider.h
  src/core/question.cpp
  src/core/question.h
  src/qtquick/CMakeLists.txt
  src/qtquick/author.cpp
  src/qtquick/author.h
  src/qtquick/categoriesmodel.cpp
  src/qtquick/categoriesmodel.h
  src/qtquick/commentsmodel.cpp
  src/qtquick/commentsmodel.h
  src/qtquick/qml/Button.qml
  src/qtquick/qml/Dialog.qml
  src/qtquick/qml/DialogContent.qml
  src/qtquick/qml/DownloadItemsSheet.qml
  src/qtquick/qml/EntryDetails.qml
  src/qtquick/qml/NewStuffItem.qml
  src/qtquick/qml/NewStuffList.qml
  src/qtquick/qml/Page.qml
  src/qtquick/qml/QuestionAsker.qml
  src/qtquick/qml/private/ConditionalLoader.qml
  src/qtquick/qml/private/EntryCommentDelegate.qml
  src/qtquick/qml/private/EntryCommentsPage.qml
  src/qtquick/qml/private/EntryScreenshots.qml
  src/qtquick/qml/private/GridTileDelegate.qml
  src/qtquick/qml/private/Rating.qml
  src/qtquick/qml/private/Shadow.qml
  src/qtquick/qml/private/entrygriddelegates/BigPreviewDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/ThumbDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/TileDelegate.qml
  src/qtquick/qmldir
  src/qtquick/qmlplugin.cpp
  src/qtquick/quickengine.cpp
  src/qtquick/quickengine.h
  src/qtquick/quickitemsmodel.cpp
  src/qtquick/quickitemsmodel.h
  src/qtquick/quickquestionlistener.cpp
  src/qtquick/quickquestionlistener.h
  tests/CMakeLists.txt
  tests/khotnewstuff-dialog-ui/main.qml
  tests/khotnewstuff-dialog.cpp

To: leinir, #knewstuff, #vdg, #frameworks, ahiemstra
Cc: davidedmundson, broulik, ahiemstra, anthonyfieroni, pino, ngraham, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

2019-09-20 Thread Arjen Hiemstra 
ahiemstra added inline comments.

INLINE COMMENTS

> leinir wrote in Button.qml:112
> 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 ;)

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.

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #vdg, #frameworks, ahiemstra
Cc: davidedmundson, broulik, ahiemstra, anthonyfieroni, pino, ngraham, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

2019-09-20 Thread Dan Leinir Turthra Jensen 
leinir requested review of this revision.
leinir added a comment.


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

INLINE COMMENTS

> ahiemstra wrote in author.cpp:37
> Suggestion: It might make sense to do this caching at the provider level so 
> users don't need to reimplement this.

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

> ahiemstra wrote in categoriesmodel.cpp:49
> Suggestion: I tend to make roles static since it doesn't change between 
> instances anyway.
> Like so:
> 
>   static QHash roles{
>   { NameRole, "name" },
>   ...
>   };

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)

> ahiemstra wrote in categoriesmodel.h:61
> Suggestion: I use `const std::unique_ptr d;` these days, it removes 
> the need to explicitly delete the d pointer.

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

> ahiemstra wrote in commentsmodel.h:38
> Like Author, it might be a good idea to use QQmlParserStatus here.

Less critical codepath, but yeah, why not :)

> ahiemstra wrote in Button.qml:112
> 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.

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

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #vdg, #frameworks, ahiemstra
Cc: davidedmundson, broulik, ahiemstra, anthonyfieroni, pino, ngraham, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

2019-09-20 Thread Dan Leinir Turthra Jensen 
leinir updated this revision to Diff 66529.
leinir added a comment.


  - Fix some whitespace issues

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21721?vs=66368=66529

BRANCH
  knsquick-feature-parity-with-kns (branched from master)

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

AFFECTED FILES
  CMakeLists.txt
  src/attica/atticaprovider.cpp
  src/attica/atticaprovider_p.h
  src/core/CMakeLists.txt
  src/core/author.cpp
  src/core/author.h
  src/core/commentsmodel.cpp
  src/core/commentsmodel.h
  src/core/engine.cpp
  src/core/engine.h
  src/core/entryinternal.cpp
  src/core/installation.cpp
  src/core/itemsmodel.cpp
  src/core/itemsmodel.h
  src/core/provider.h
  src/core/question.cpp
  src/core/question.h
  src/qtquick/CMakeLists.txt
  src/qtquick/author.cpp
  src/qtquick/author.h
  src/qtquick/categoriesmodel.cpp
  src/qtquick/categoriesmodel.h
  src/qtquick/commentsmodel.cpp
  src/qtquick/commentsmodel.h
  src/qtquick/qml/Button.qml
  src/qtquick/qml/Dialog.qml
  src/qtquick/qml/DialogContent.qml
  src/qtquick/qml/DownloadItemsSheet.qml
  src/qtquick/qml/EntryDetails.qml
  src/qtquick/qml/NewStuffItem.qml
  src/qtquick/qml/NewStuffList.qml
  src/qtquick/qml/Page.qml
  src/qtquick/qml/QuestionAsker.qml
  src/qtquick/qml/private/ConditionalLoader.qml
  src/qtquick/qml/private/EntryCommentDelegate.qml
  src/qtquick/qml/private/EntryCommentsPage.qml
  src/qtquick/qml/private/EntryScreenshots.qml
  src/qtquick/qml/private/GridTileDelegate.qml
  src/qtquick/qml/private/Rating.qml
  src/qtquick/qml/private/Shadow.qml
  src/qtquick/qml/private/entrygriddelegates/BigPreviewDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/ThumbDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/TileDelegate.qml
  src/qtquick/qmldir
  src/qtquick/qmlplugin.cpp
  src/qtquick/quickengine.cpp
  src/qtquick/quickengine.h
  src/qtquick/quickitemsmodel.cpp
  src/qtquick/quickitemsmodel.h
  src/qtquick/quickquestionlistener.cpp
  src/qtquick/quickquestionlistener.h
  tests/CMakeLists.txt
  tests/khotnewstuff-dialog-ui/main.qml
  tests/khotnewstuff-dialog.cpp

To: leinir, #knewstuff, #vdg, #frameworks, ahiemstra
Cc: davidedmundson, broulik, ahiemstra, anthonyfieroni, pino, ngraham, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

2019-09-18 Thread Dan Leinir Turthra Jensen 
leinir updated this revision to Diff 66368.
leinir marked 7 inline comments as done.
leinir added a comment.


  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

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21721?vs=65696=66368

BRANCH
  knsquick-feature-parity-with-kns (branched from master)

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

AFFECTED FILES
  CMakeLists.txt
  src/attica/atticaprovider.cpp
  src/attica/atticaprovider_p.h
  src/core/CMakeLists.txt
  src/core/author.cpp
  src/core/author.h
  src/core/commentsmodel.cpp
  src/core/commentsmodel.h
  src/core/engine.cpp
  src/core/engine.h
  src/core/entryinternal.cpp
  src/core/installation.cpp
  src/core/itemsmodel.cpp
  src/core/itemsmodel.h
  src/core/provider.h
  src/core/question.h
  src/qtquick/CMakeLists.txt
  src/qtquick/author.cpp
  src/qtquick/author.h
  src/qtquick/categoriesmodel.cpp
  src/qtquick/categoriesmodel.h
  src/qtquick/commentsmodel.cpp
  src/qtquick/commentsmodel.h
  src/qtquick/qml/Button.qml
  src/qtquick/qml/Dialog.qml
  src/qtquick/qml/DialogContent.qml
  src/qtquick/qml/DownloadItemsSheet.qml
  src/qtquick/qml/EntryDetails.qml
  src/qtquick/qml/NewStuffItem.qml
  src/qtquick/qml/NewStuffList.qml
  src/qtquick/qml/Page.qml
  src/qtquick/qml/QuestionAsker.qml
  src/qtquick/qml/private/ConditionalLoader.qml
  src/qtquick/qml/private/EntryCommentDelegate.qml
  src/qtquick/qml/private/EntryCommentsPage.qml
  src/qtquick/qml/private/EntryScreenshots.qml
  src/qtquick/qml/private/GridTileDelegate.qml
  src/qtquick/qml/private/Rating.qml
  src/qtquick/qml/private/Shadow.qml
  src/qtquick/qml/private/entrygriddelegates/BigPreviewDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/ThumbDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/TileDelegate.qml
  src/qtquick/qmldir
  src/qtquick/qmlplugin.cpp
  src/qtquick/quickengine.cpp
  src/qtquick/quickengine.h
  src/qtquick/quickitemsmodel.cpp
  src/qtquick/quickitemsmodel.h
  src/qtquick/quickquestionlistener.cpp
  src/qtquick/quickquestionlistener.h
  tests/CMakeLists.txt
  tests/khotnewstuff-dialog-ui/main.qml
  tests/khotnewstuff-dialog.cpp

To: leinir, #knewstuff, #vdg, #frameworks, ahiemstra
Cc: davidedmundson, broulik, ahiemstra, anthonyfieroni, pino, ngraham, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

2019-09-18 Thread Arjen Hiemstra 
ahiemstra accepted this revision.
ahiemstra added a comment.
This revision is now accepted and ready to land.


  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.

INLINE COMMENTS

> commentsmodel.cpp:99
> +int pageToLoad = comments.count() / commentsPerPage;
> +qDebug() << "Loading comments, page" << pageToLoad << "with 
> current comment count" << comments.count() << "out of a total of" << 
> entry.numberOfComments();
> +provider->loadComments(entry, commentsPerPage, pageToLoad);

Categorise or drop :)

> engine.h:155
> + * The sort mode set on the current request
> + * @see setFilter(Provider::SortMode)
> + * @since 5.62

I assume you mean setSortMode here?

> author.cpp:37
> +typedef QHash> AllAuthorsHash;
> +Q_GLOBAL_STATIC(AllAuthorsHash, allAuthors)
> +

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

> categoriesmodel.cpp:49
> +{
> +QHash roles;
> +roles[NameRole] = "name";

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

  static QHash roles{
  { NameRole, "name" },
  ...
  };

> categoriesmodel.h:61
> +class Private;
> +Private* d;
> +};

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

> commentsmodel.h:38
> + */
> +class CommentsModel : public QIdentityProxyModel
> +{

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

> Button.qml:112
> +visible: enabled || visibleWhenDisabled
> +enabled: ghnsDialog.engine.allowedByKiosk
> +

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.

REPOSITORY
  R304 KNewStuff

BRANCH
  knsquick-feature-parity-with-kns (branched from master)

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

To: leinir, #knewstuff, #vdg, #frameworks, ahiemstra
Cc: davidedmundson, broulik, ahiemstra, anthonyfieroni, pino, ngraham, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

2019-09-09 Thread Dan Leinir Turthra Jensen 
leinir marked 6 inline comments as done.

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #vdg, #frameworks, ahiemstra
Cc: davidedmundson, broulik, ahiemstra, anthonyfieroni, pino, ngraham, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

2019-09-09 Thread Dan Leinir Turthra Jensen 
leinir updated this revision to Diff 65696.
leinir marked 2 inline comments as done.
leinir added a comment.


  - Add a simple cache for CommentsModel instances

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21721?vs=65689=65696

BRANCH
  knsquick-feature-parity-with-kns (branched from master)

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

AFFECTED FILES
  CMakeLists.txt
  src/attica/atticaprovider.cpp
  src/attica/atticaprovider_p.h
  src/core/CMakeLists.txt
  src/core/author.cpp
  src/core/author.h
  src/core/commentsmodel.cpp
  src/core/commentsmodel.h
  src/core/engine.cpp
  src/core/engine.h
  src/core/entryinternal.cpp
  src/core/installation.cpp
  src/core/itemsmodel.cpp
  src/core/itemsmodel.h
  src/core/provider.h
  src/core/question.h
  src/qtquick/CMakeLists.txt
  src/qtquick/author.cpp
  src/qtquick/author.h
  src/qtquick/categoriesmodel.cpp
  src/qtquick/categoriesmodel.h
  src/qtquick/commentsmodel.cpp
  src/qtquick/commentsmodel.h
  src/qtquick/qml/Button.qml
  src/qtquick/qml/Dialog.qml
  src/qtquick/qml/DialogContent.qml
  src/qtquick/qml/DownloadItemsSheet.qml
  src/qtquick/qml/EntryDetails.qml
  src/qtquick/qml/NewStuffItem.qml
  src/qtquick/qml/NewStuffList.qml
  src/qtquick/qml/Page.qml
  src/qtquick/qml/QuestionAsker.qml
  src/qtquick/qml/private/ConditionalLoader.qml
  src/qtquick/qml/private/EntryCommentDelegate.qml
  src/qtquick/qml/private/EntryCommentsPage.qml
  src/qtquick/qml/private/EntryScreenshots.qml
  src/qtquick/qml/private/GridTileDelegate.qml
  src/qtquick/qml/private/Rating.qml
  src/qtquick/qml/private/Shadow.qml
  src/qtquick/qml/private/entrygriddelegates/BigPreviewDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/ThumbDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/TileDelegate.qml
  src/qtquick/qmldir
  src/qtquick/qmlplugin.cpp
  src/qtquick/quickengine.cpp
  src/qtquick/quickengine.h
  src/qtquick/quickitemsmodel.cpp
  src/qtquick/quickitemsmodel.h
  src/qtquick/quickquestionlistener.cpp
  src/qtquick/quickquestionlistener.h
  tests/CMakeLists.txt
  tests/khotnewstuff-dialog-ui/main.qml
  tests/khotnewstuff-dialog.cpp

To: leinir, #knewstuff, #vdg, #frameworks, ahiemstra
Cc: davidedmundson, broulik, ahiemstra, anthonyfieroni, pino, ngraham, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

2019-09-09 Thread Dan Leinir Turthra Jensen 
leinir marked 3 inline comments as done.
leinir added inline comments.

INLINE COMMENTS

> broulik wrote in author.h:111
> `ProfilePage`?

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

> broulik wrote in commentsmodel.cpp:184
> Any particular reason not do just return a null `QVariant()`?

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.

> broulik wrote in commentsmodel.h:70
> Why is this an explicit `Engine` pointer rather than generic `QObject`?

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

> broulik wrote in commentsmodel.h:75
> You might want to set that to `Qt::DisplayRole`

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.

> broulik wrote in commentsmodel.h:88
> `QModelIndex` is usable from QML these days, this overload isn't neccessary, 
> you can do from QML:
> 
>   model.data(model.index(row, column), role);

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

> broulik wrote in engine.cpp:803
> ah, so this is why it's not `const`

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

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #vdg, #frameworks, ahiemstra
Cc: davidedmundson, broulik, ahiemstra, anthonyfieroni, pino, ngraham, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

2019-09-09 Thread Dan Leinir Turthra Jensen 
leinir updated this revision to Diff 65689.
leinir marked 3 inline comments as done.
leinir added a comment.


  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)

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21721?vs=65579=65689

BRANCH
  knsquick-feature-parity-with-kns (branched from master)

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

AFFECTED FILES
  CMakeLists.txt
  src/attica/atticaprovider.cpp
  src/attica/atticaprovider_p.h
  src/core/CMakeLists.txt
  src/core/author.cpp
  src/core/author.h
  src/core/commentsmodel.cpp
  src/core/commentsmodel.h
  src/core/engine.cpp
  src/core/engine.h
  src/core/entryinternal.cpp
  src/core/installation.cpp
  src/core/itemsmodel.cpp
  src/core/itemsmodel.h
  src/core/provider.h
  src/core/question.h
  src/qtquick/CMakeLists.txt
  src/qtquick/author.cpp
  src/qtquick/author.h
  src/qtquick/categoriesmodel.cpp
  src/qtquick/categoriesmodel.h
  src/qtquick/commentsmodel.cpp
  src/qtquick/commentsmodel.h
  src/qtquick/qml/Button.qml
  src/qtquick/qml/Dialog.qml
  src/qtquick/qml/DialogContent.qml
  src/qtquick/qml/DownloadItemsSheet.qml
  src/qtquick/qml/EntryDetails.qml
  src/qtquick/qml/NewStuffItem.qml
  src/qtquick/qml/NewStuffList.qml
  src/qtquick/qml/Page.qml
  src/qtquick/qml/QuestionAsker.qml
  src/qtquick/qml/private/ConditionalLoader.qml
  src/qtquick/qml/private/EntryCommentDelegate.qml
  src/qtquick/qml/private/EntryCommentsPage.qml
  src/qtquick/qml/private/EntryScreenshots.qml
  src/qtquick/qml/private/GridTileDelegate.qml
  src/qtquick/qml/private/Rating.qml
  src/qtquick/qml/private/Shadow.qml
  src/qtquick/qml/private/entrygriddelegates/BigPreviewDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/ThumbDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/TileDelegate.qml
  src/qtquick/qmldir
  src/qtquick/qmlplugin.cpp
  src/qtquick/quickengine.cpp
  src/qtquick/quickengine.h
  src/qtquick/quickitemsmodel.cpp
  src/qtquick/quickitemsmodel.h
  src/qtquick/quickquestionlistener.cpp
  src/qtquick/quickquestionlistener.h
  tests/CMakeLists.txt
  tests/khotnewstuff-dialog-ui/main.qml
  tests/khotnewstuff-dialog.cpp

To: leinir, #knewstuff, #vdg, #frameworks, ahiemstra
Cc: davidedmundson, broulik, ahiemstra, anthonyfieroni, pino, ngraham, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

2019-09-08 Thread David Edmundson 
davidedmundson added inline comments.

INLINE COMMENTS

> author.h:37
> + */
> +class Author : public QObject
> +{

Use of QQmlParserStatus would help here.

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

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #vdg, #frameworks, ahiemstra
Cc: davidedmundson, broulik, ahiemstra, anthonyfieroni, pino, ngraham, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

2019-09-08 Thread Kai Uwe Broulik 
broulik added inline comments.

INLINE COMMENTS

> author.h:111
> + */
> +void setProfilepage(const QString );
> +

`ProfilePage`?

> commentsmodel.cpp:184
> +default:
> +value.setValue(i18nc("The value returned for an unknown role 
> when requesting data from the model.", ""));
> +break;

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

> commentsmodel.h:70
> + */
> +explicit CommentsModel(Engine *parent = nullptr);
> +virtual ~CommentsModel();

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

> commentsmodel.h:71
> +explicit CommentsModel(Engine *parent = nullptr);
> +virtual ~CommentsModel();
> +

`~CommentsModel() override;`?

> commentsmodel.h:75
> +IdRole = Qt::UserRole + 1,
> +SubjectRole,
> +TextRole,

You might want to set that to `Qt::DisplayRole`

> commentsmodel.h:88
> +QVariant data(const QModelIndex& index, int role = Qt::DisplayRole) 
> const override;
> +Q_INVOKABLE QVariant data(int index, int role  = Qt::DisplayRole) const;
> +int rowCount(const QModelIndex& parent = QModelIndex()) const override;

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

  model.data(model.index(row, column), role);

> engine.cpp:803
> +model->setEntry(entry);
> +// Cache the models mebby?
> +// Track onDestroyed and remove from cache...

ah, so this is why it's not `const`

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #vdg, #frameworks, ahiemstra
Cc: broulik, ahiemstra, anthonyfieroni, pino, ngraham, kde-frameworks-devel, 
LeGast00n, GB_2, michaelh, bruns

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

2019-09-07 Thread Dan Leinir Turthra Jensen 
leinir updated this revision to Diff 65579.
leinir added a comment.


  - 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

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21721?vs=65506=65579

BRANCH
  knsquick-feature-parity-with-kns (branched from master)

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

AFFECTED FILES
  CMakeLists.txt
  src/attica/atticaprovider.cpp
  src/attica/atticaprovider_p.h
  src/core/CMakeLists.txt
  src/core/author.cpp
  src/core/author.h
  src/core/commentsmodel.cpp
  src/core/commentsmodel.h
  src/core/engine.cpp
  src/core/engine.h
  src/core/entryinternal.cpp
  src/core/installation.cpp
  src/core/itemsmodel.cpp
  src/core/itemsmodel.h
  src/core/provider.h
  src/core/question.h
  src/qtquick/CMakeLists.txt
  src/qtquick/author.cpp
  src/qtquick/author.h
  src/qtquick/categoriesmodel.cpp
  src/qtquick/categoriesmodel.h
  src/qtquick/commentsmodel.cpp
  src/qtquick/commentsmodel.h
  src/qtquick/qml/Button.qml
  src/qtquick/qml/Dialog.qml
  src/qtquick/qml/DialogContent.qml
  src/qtquick/qml/DownloadItemsSheet.qml
  src/qtquick/qml/EntryDetails.qml
  src/qtquick/qml/NewStuffItem.qml
  src/qtquick/qml/NewStuffList.qml
  src/qtquick/qml/Page.qml
  src/qtquick/qml/QuestionAsker.qml
  src/qtquick/qml/private/ConditionalLoader.qml
  src/qtquick/qml/private/EntryCommentDelegate.qml
  src/qtquick/qml/private/EntryCommentsPage.qml
  src/qtquick/qml/private/EntryScreenshots.qml
  src/qtquick/qml/private/GridTileDelegate.qml
  src/qtquick/qml/private/Rating.qml
  src/qtquick/qml/private/Shadow.qml
  src/qtquick/qml/private/entrygriddelegates/BigPreviewDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/ThumbDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/TileDelegate.qml
  src/qtquick/qmldir
  src/qtquick/qmlplugin.cpp
  src/qtquick/quickengine.cpp
  src/qtquick/quickengine.h
  src/qtquick/quickitemsmodel.cpp
  src/qtquick/quickitemsmodel.h
  src/qtquick/quickquestionlistener.cpp
  src/qtquick/quickquestionlistener.h
  tests/CMakeLists.txt
  tests/khotnewstuff-dialog-ui/main.qml
  tests/khotnewstuff-dialog.cpp

To: leinir, #knewstuff, #vdg, #frameworks, ahiemstra
Cc: ahiemstra, anthonyfieroni, pino, ngraham, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, bruns

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

2019-09-06 Thread Dan Leinir Turthra Jensen 
leinir updated this revision to Diff 65506.
leinir added a comment.


  - Also catch the error messages from the non-deprecated error function
  - Equalize border colour with Breeze

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21721?vs=65236=65506

BRANCH
  knsquick-feature-parity-with-kns (branched from master)

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

AFFECTED FILES
  CMakeLists.txt
  src/attica/atticaprovider.cpp
  src/attica/atticaprovider_p.h
  src/core/CMakeLists.txt
  src/core/author.cpp
  src/core/author.h
  src/core/commentsmodel.cpp
  src/core/commentsmodel.h
  src/core/engine.cpp
  src/core/engine.h
  src/core/entryinternal.cpp
  src/core/installation.cpp
  src/core/itemsmodel.cpp
  src/core/itemsmodel.h
  src/core/provider.h
  src/core/question.h
  src/qtquick/CMakeLists.txt
  src/qtquick/author.cpp
  src/qtquick/author.h
  src/qtquick/categoriesmodel.cpp
  src/qtquick/categoriesmodel.h
  src/qtquick/commentsmodel.cpp
  src/qtquick/commentsmodel.h
  src/qtquick/qml/Button.qml
  src/qtquick/qml/Dialog.qml
  src/qtquick/qml/DialogContent.qml
  src/qtquick/qml/DownloadItemsSheet.qml
  src/qtquick/qml/EntryDetails.qml
  src/qtquick/qml/NewStuffItem.qml
  src/qtquick/qml/NewStuffList.qml
  src/qtquick/qml/Page.qml
  src/qtquick/qml/QuestionAsker.qml
  src/qtquick/qml/private/ConditionalLoader.qml
  src/qtquick/qml/private/EntryCommentDelegate.qml
  src/qtquick/qml/private/EntryCommentsPage.qml
  src/qtquick/qml/private/EntryScreenshots.qml
  src/qtquick/qml/private/GridTileDelegate.qml
  src/qtquick/qml/private/Rating.qml
  src/qtquick/qml/private/Shadow.qml
  src/qtquick/qml/private/entrygriddelegates/BigPreviewDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/ThumbDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/TileDelegate.qml
  src/qtquick/qmldir
  src/qtquick/qmlplugin.cpp
  src/qtquick/quickengine.cpp
  src/qtquick/quickengine.h
  src/qtquick/quickitemsmodel.cpp
  src/qtquick/quickitemsmodel.h
  src/qtquick/quickquestionlistener.cpp
  src/qtquick/quickquestionlistener.h
  tests/CMakeLists.txt
  tests/khotnewstuff-dialog-ui/main.qml
  tests/khotnewstuff-dialog.cpp

To: leinir, #knewstuff, #vdg, #frameworks, ahiemstra
Cc: ahiemstra, anthonyfieroni, pino, ngraham, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, bruns

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

2019-09-02 Thread Dan Leinir Turthra Jensen 
leinir updated this revision to Diff 65236.
leinir added a comment.


  - Make sure to import the right version of newstuff

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21721?vs=65219=65236

BRANCH
  knsquick-feature-parity-with-kns (branched from master)

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

AFFECTED FILES
  CMakeLists.txt
  src/attica/atticaprovider.cpp
  src/attica/atticaprovider_p.h
  src/core/CMakeLists.txt
  src/core/author.cpp
  src/core/author.h
  src/core/commentsmodel.cpp
  src/core/commentsmodel.h
  src/core/engine.cpp
  src/core/engine.h
  src/core/entryinternal.cpp
  src/core/installation.cpp
  src/core/itemsmodel.cpp
  src/core/itemsmodel.h
  src/core/provider.h
  src/core/question.h
  src/qtquick/CMakeLists.txt
  src/qtquick/author.cpp
  src/qtquick/author.h
  src/qtquick/categoriesmodel.cpp
  src/qtquick/categoriesmodel.h
  src/qtquick/commentsmodel.cpp
  src/qtquick/commentsmodel.h
  src/qtquick/qml/Button.qml
  src/qtquick/qml/Dialog.qml
  src/qtquick/qml/DialogContent.qml
  src/qtquick/qml/DownloadItemsSheet.qml
  src/qtquick/qml/EntryDetails.qml
  src/qtquick/qml/NewStuffItem.qml
  src/qtquick/qml/NewStuffList.qml
  src/qtquick/qml/Page.qml
  src/qtquick/qml/QuestionAsker.qml
  src/qtquick/qml/private/ConditionalLoader.qml
  src/qtquick/qml/private/EntryCommentDelegate.qml
  src/qtquick/qml/private/EntryCommentsPage.qml
  src/qtquick/qml/private/EntryScreenshots.qml
  src/qtquick/qml/private/GridTileDelegate.qml
  src/qtquick/qml/private/Rating.qml
  src/qtquick/qml/private/Shadow.qml
  src/qtquick/qml/private/entrygriddelegates/BigPreviewDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/ThumbDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/TileDelegate.qml
  src/qtquick/qmldir
  src/qtquick/qmlplugin.cpp
  src/qtquick/quickengine.cpp
  src/qtquick/quickengine.h
  src/qtquick/quickitemsmodel.cpp
  src/qtquick/quickitemsmodel.h
  src/qtquick/quickquestionlistener.cpp
  src/qtquick/quickquestionlistener.h
  tests/CMakeLists.txt
  tests/khotnewstuff-dialog-ui/main.qml
  tests/khotnewstuff-dialog.cpp

To: leinir, #knewstuff, #vdg, #frameworks, ahiemstra
Cc: ahiemstra, anthonyfieroni, pino, ngraham, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, bruns

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

2019-09-02 Thread Dan Leinir Turthra Jensen 
leinir updated this revision to Diff 65219.
leinir added a comment.


  - Whole lot of @since-ing
  - Adopt Qt-style KF5-version-aligned import versioning

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21721?vs=64986=65219

BRANCH
  knsquick-feature-parity-with-kns (branched from master)

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

AFFECTED FILES
  CMakeLists.txt
  src/attica/atticaprovider.cpp
  src/attica/atticaprovider_p.h
  src/core/CMakeLists.txt
  src/core/author.cpp
  src/core/author.h
  src/core/commentsmodel.cpp
  src/core/commentsmodel.h
  src/core/engine.cpp
  src/core/engine.h
  src/core/entryinternal.cpp
  src/core/installation.cpp
  src/core/itemsmodel.cpp
  src/core/itemsmodel.h
  src/core/provider.h
  src/core/question.h
  src/qtquick/CMakeLists.txt
  src/qtquick/author.cpp
  src/qtquick/author.h
  src/qtquick/categoriesmodel.cpp
  src/qtquick/categoriesmodel.h
  src/qtquick/commentsmodel.cpp
  src/qtquick/commentsmodel.h
  src/qtquick/qml/Button.qml
  src/qtquick/qml/Dialog.qml
  src/qtquick/qml/DialogContent.qml
  src/qtquick/qml/DownloadItemsSheet.qml
  src/qtquick/qml/EntryDetails.qml
  src/qtquick/qml/NewStuffItem.qml
  src/qtquick/qml/NewStuffList.qml
  src/qtquick/qml/Page.qml
  src/qtquick/qml/QuestionAsker.qml
  src/qtquick/qml/private/ConditionalLoader.qml
  src/qtquick/qml/private/EntryCommentDelegate.qml
  src/qtquick/qml/private/EntryCommentsPage.qml
  src/qtquick/qml/private/EntryScreenshots.qml
  src/qtquick/qml/private/GridTileDelegate.qml
  src/qtquick/qml/private/Rating.qml
  src/qtquick/qml/private/Shadow.qml
  src/qtquick/qml/private/entrygriddelegates/BigPreviewDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/ThumbDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/TileDelegate.qml
  src/qtquick/qmldir
  src/qtquick/qmlplugin.cpp
  src/qtquick/quickengine.cpp
  src/qtquick/quickengine.h
  src/qtquick/quickitemsmodel.cpp
  src/qtquick/quickitemsmodel.h
  src/qtquick/quickquestionlistener.cpp
  src/qtquick/quickquestionlistener.h
  tests/CMakeLists.txt
  tests/khotnewstuff-dialog-ui/main.qml
  tests/khotnewstuff-dialog.cpp

To: leinir, #knewstuff, #vdg, #frameworks, ahiemstra
Cc: ahiemstra, anthonyfieroni, pino, ngraham, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, bruns

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

2019-08-30 Thread Dan Leinir Turthra Jensen 
leinir marked 2 inline comments as done.

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #vdg, #frameworks, ahiemstra
Cc: ahiemstra, anthonyfieroni, pino, ngraham, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, bruns

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

2019-08-30 Thread Dan Leinir Turthra Jensen 
leinir edited the summary of this revision.

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #vdg, #frameworks, ahiemstra
Cc: ahiemstra, anthonyfieroni, pino, ngraham, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, bruns

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

2019-08-30 Thread Dan Leinir Turthra Jensen 
leinir updated this revision to Diff 64986.
leinir marked 18 inline comments as done.
leinir added a comment.


  - Fiddle with the downloaditems sheet layout a touch

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21721?vs=64979=64986

BRANCH
  knsquick-feature-parity-with-kns (branched from master)

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

AFFECTED FILES
  CMakeLists.txt
  src/attica/atticaprovider.cpp
  src/attica/atticaprovider_p.h
  src/core/CMakeLists.txt
  src/core/author.cpp
  src/core/author.h
  src/core/commentsmodel.cpp
  src/core/commentsmodel.h
  src/core/engine.cpp
  src/core/engine.h
  src/core/entryinternal.cpp
  src/core/installation.cpp
  src/core/itemsmodel.cpp
  src/core/itemsmodel.h
  src/core/provider.h
  src/core/question.h
  src/qtquick/CMakeLists.txt
  src/qtquick/author.cpp
  src/qtquick/author.h
  src/qtquick/categoriesmodel.cpp
  src/qtquick/categoriesmodel.h
  src/qtquick/commentsmodel.cpp
  src/qtquick/commentsmodel.h
  src/qtquick/qml/Button.qml
  src/qtquick/qml/Dialog.qml
  src/qtquick/qml/DialogContent.qml
  src/qtquick/qml/DownloadItemsSheet.qml
  src/qtquick/qml/EntryDetails.qml
  src/qtquick/qml/NewStuffItem.qml
  src/qtquick/qml/NewStuffList.qml
  src/qtquick/qml/Page.qml
  src/qtquick/qml/QuestionAsker.qml
  src/qtquick/qml/private/ConditionalLoader.qml
  src/qtquick/qml/private/EntryCommentDelegate.qml
  src/qtquick/qml/private/EntryCommentsPage.qml
  src/qtquick/qml/private/EntryScreenshots.qml
  src/qtquick/qml/private/GridTileDelegate.qml
  src/qtquick/qml/private/Rating.qml
  src/qtquick/qml/private/Shadow.qml
  src/qtquick/qml/private/entrygriddelegates/BigPreviewDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/ThumbDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/TileDelegate.qml
  src/qtquick/qmldir
  src/qtquick/qmlplugin.cpp
  src/qtquick/quickengine.cpp
  src/qtquick/quickengine.h
  src/qtquick/quickitemsmodel.cpp
  src/qtquick/quickitemsmodel.h
  src/qtquick/quickquestionlistener.cpp
  src/qtquick/quickquestionlistener.h
  tests/CMakeLists.txt
  tests/khotnewstuff-dialog-ui/main.qml
  tests/khotnewstuff-dialog.cpp

To: leinir, #knewstuff, #vdg, #frameworks, ahiemstra
Cc: ahiemstra, anthonyfieroni, pino, ngraham, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, bruns

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

2019-08-30 Thread Dan Leinir Turthra Jensen 
leinir updated this revision to Diff 64979.
leinir added a comment.


  - Switch to using a FileDialog-like property for changedEntries

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21721?vs=64978=64979

BRANCH
  knsquick-feature-parity-with-kns (branched from master)

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

AFFECTED FILES
  CMakeLists.txt
  src/attica/atticaprovider.cpp
  src/attica/atticaprovider_p.h
  src/core/CMakeLists.txt
  src/core/author.cpp
  src/core/author.h
  src/core/commentsmodel.cpp
  src/core/commentsmodel.h
  src/core/engine.cpp
  src/core/engine.h
  src/core/entryinternal.cpp
  src/core/installation.cpp
  src/core/itemsmodel.cpp
  src/core/itemsmodel.h
  src/core/provider.h
  src/core/question.h
  src/qtquick/CMakeLists.txt
  src/qtquick/author.cpp
  src/qtquick/author.h
  src/qtquick/categoriesmodel.cpp
  src/qtquick/categoriesmodel.h
  src/qtquick/commentsmodel.cpp
  src/qtquick/commentsmodel.h
  src/qtquick/qml/Button.qml
  src/qtquick/qml/Dialog.qml
  src/qtquick/qml/DialogContent.qml
  src/qtquick/qml/DownloadItemsSheet.qml
  src/qtquick/qml/EntryDetails.qml
  src/qtquick/qml/NewStuffItem.qml
  src/qtquick/qml/NewStuffList.qml
  src/qtquick/qml/Page.qml
  src/qtquick/qml/QuestionAsker.qml
  src/qtquick/qml/private/ConditionalLoader.qml
  src/qtquick/qml/private/EntryCommentDelegate.qml
  src/qtquick/qml/private/EntryCommentsPage.qml
  src/qtquick/qml/private/EntryScreenshots.qml
  src/qtquick/qml/private/GridTileDelegate.qml
  src/qtquick/qml/private/Rating.qml
  src/qtquick/qml/private/Shadow.qml
  src/qtquick/qml/private/entrygriddelegates/BigPreviewDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/ThumbDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/TileDelegate.qml
  src/qtquick/qmldir
  src/qtquick/qmlplugin.cpp
  src/qtquick/quickengine.cpp
  src/qtquick/quickengine.h
  src/qtquick/quickitemsmodel.cpp
  src/qtquick/quickitemsmodel.h
  src/qtquick/quickquestionlistener.cpp
  src/qtquick/quickquestionlistener.h
  tests/CMakeLists.txt
  tests/khotnewstuff-dialog-ui/main.qml
  tests/khotnewstuff-dialog.cpp

To: leinir, #knewstuff, #vdg, #frameworks, ahiemstra
Cc: ahiemstra, anthonyfieroni, pino, ngraham, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, bruns

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

2019-08-30 Thread Dan Leinir Turthra Jensen 
leinir marked an inline comment as done.
leinir added inline comments.

INLINE COMMENTS

> ahiemstra wrote in 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. :)

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

> ahiemstra wrote in Dialog.qml:76
> 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.

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

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #vdg, #frameworks, ahiemstra
Cc: ahiemstra, anthonyfieroni, pino, ngraham, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, bruns

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

2019-08-30 Thread Dan Leinir Turthra Jensen 
leinir edited the summary of this revision.

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #vdg, #frameworks, ahiemstra
Cc: ahiemstra, anthonyfieroni, pino, ngraham, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, bruns

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

2019-08-30 Thread Dan Leinir Turthra Jensen 
leinir updated this revision to Diff 64978.
leinir added a comment.


  - Add a kf6 todo item (our api's QListey and should become QVectory)
  - Adopt a more pleasant comments layout

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21721?vs=64934=64978

BRANCH
  knsquick-feature-parity-with-kns (branched from master)

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

AFFECTED FILES
  CMakeLists.txt
  src/attica/atticaprovider.cpp
  src/attica/atticaprovider_p.h
  src/core/CMakeLists.txt
  src/core/author.cpp
  src/core/author.h
  src/core/commentsmodel.cpp
  src/core/commentsmodel.h
  src/core/engine.cpp
  src/core/engine.h
  src/core/entryinternal.cpp
  src/core/installation.cpp
  src/core/itemsmodel.cpp
  src/core/itemsmodel.h
  src/core/provider.h
  src/core/question.h
  src/qtquick/CMakeLists.txt
  src/qtquick/author.cpp
  src/qtquick/author.h
  src/qtquick/categoriesmodel.cpp
  src/qtquick/categoriesmodel.h
  src/qtquick/commentsmodel.cpp
  src/qtquick/commentsmodel.h
  src/qtquick/qml/Button.qml
  src/qtquick/qml/Dialog.qml
  src/qtquick/qml/DialogContent.qml
  src/qtquick/qml/DownloadItemsSheet.qml
  src/qtquick/qml/EntryDetails.qml
  src/qtquick/qml/NewStuffItem.qml
  src/qtquick/qml/NewStuffList.qml
  src/qtquick/qml/Page.qml
  src/qtquick/qml/QuestionAsker.qml
  src/qtquick/qml/private/ConditionalLoader.qml
  src/qtquick/qml/private/EntryCommentDelegate.qml
  src/qtquick/qml/private/EntryCommentsPage.qml
  src/qtquick/qml/private/EntryScreenshots.qml
  src/qtquick/qml/private/GridTileDelegate.qml
  src/qtquick/qml/private/Rating.qml
  src/qtquick/qml/private/Shadow.qml
  src/qtquick/qml/private/entrygriddelegates/BigPreviewDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/ThumbDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/TileDelegate.qml
  src/qtquick/qmldir
  src/qtquick/qmlplugin.cpp
  src/qtquick/quickengine.cpp
  src/qtquick/quickengine.h
  src/qtquick/quickitemsmodel.cpp
  src/qtquick/quickitemsmodel.h
  src/qtquick/quickquestionlistener.cpp
  src/qtquick/quickquestionlistener.h
  tests/CMakeLists.txt
  tests/khotnewstuff-dialog-ui/main.qml
  tests/khotnewstuff-dialog.cpp

To: leinir, #knewstuff, #vdg, #frameworks, ahiemstra
Cc: ahiemstra, anthonyfieroni, pino, ngraham, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, bruns

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

2019-08-29 Thread Arjen Hiemstra 
ahiemstra added inline comments.

INLINE COMMENTS

> leinir wrote in 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 :)

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

> leinir wrote in Dialog.qml:76
> 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)

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.

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #vdg, #frameworks, ahiemstra
Cc: ahiemstra, anthonyfieroni, pino, ngraham, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, bruns

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

2019-08-29 Thread Dan Leinir Turthra Jensen 
leinir edited the summary of this revision.

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #vdg, #frameworks, ahiemstra
Cc: ahiemstra, anthonyfieroni, pino, ngraham, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, bruns

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

2019-08-29 Thread Dan Leinir Turthra Jensen 
leinir updated this revision to Diff 64934.
leinir added a comment.


  - Debug--
  - Ensure data is actually loaded properly, also from cache
  - Include the right header
  - Adapt the Discover comments delegate (and add a nesting indicator)

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21721?vs=64855=64934

BRANCH
  knsquick-feature-parity-with-kns (branched from master)

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

AFFECTED FILES
  CMakeLists.txt
  src/attica/atticaprovider.cpp
  src/attica/atticaprovider_p.h
  src/core/CMakeLists.txt
  src/core/author.cpp
  src/core/author.h
  src/core/commentsmodel.cpp
  src/core/commentsmodel.h
  src/core/engine.cpp
  src/core/engine.h
  src/core/entryinternal.cpp
  src/core/installation.cpp
  src/core/itemsmodel.cpp
  src/core/itemsmodel.h
  src/core/provider.h
  src/core/question.h
  src/qtquick/CMakeLists.txt
  src/qtquick/author.cpp
  src/qtquick/author.h
  src/qtquick/categoriesmodel.cpp
  src/qtquick/categoriesmodel.h
  src/qtquick/commentsmodel.cpp
  src/qtquick/commentsmodel.h
  src/qtquick/qml/Button.qml
  src/qtquick/qml/Dialog.qml
  src/qtquick/qml/DialogContent.qml
  src/qtquick/qml/DownloadItemsSheet.qml
  src/qtquick/qml/EntryDetails.qml
  src/qtquick/qml/NewStuffItem.qml
  src/qtquick/qml/NewStuffList.qml
  src/qtquick/qml/Page.qml
  src/qtquick/qml/QuestionAsker.qml
  src/qtquick/qml/private/ConditionalLoader.qml
  src/qtquick/qml/private/EntryCommentDelegate.qml
  src/qtquick/qml/private/EntryCommentsPage.qml
  src/qtquick/qml/private/EntryScreenshots.qml
  src/qtquick/qml/private/GridTileDelegate.qml
  src/qtquick/qml/private/Rating.qml
  src/qtquick/qml/private/Shadow.qml
  src/qtquick/qml/private/entrygriddelegates/BigPreviewDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/ThumbDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/TileDelegate.qml
  src/qtquick/qmldir
  src/qtquick/qmlplugin.cpp
  src/qtquick/quickengine.cpp
  src/qtquick/quickengine.h
  src/qtquick/quickitemsmodel.cpp
  src/qtquick/quickitemsmodel.h
  src/qtquick/quickquestionlistener.cpp
  src/qtquick/quickquestionlistener.h
  tests/CMakeLists.txt
  tests/khotnewstuff-dialog-ui/main.qml
  tests/khotnewstuff-dialog.cpp

To: leinir, #knewstuff, #vdg, #frameworks, ahiemstra
Cc: ahiemstra, anthonyfieroni, pino, ngraham, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, bruns

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

2019-08-28 Thread Dan Leinir Turthra Jensen 
leinir updated this revision to Diff 64855.
leinir marked 11 inline comments as done.
leinir added a comment.


  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

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21721?vs=64465=64855

BRANCH
  knsquick-feature-parity-with-kns (branched from master)

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

AFFECTED FILES
  CMakeLists.txt
  src/attica/atticaprovider.cpp
  src/attica/atticaprovider_p.h
  src/core/CMakeLists.txt
  src/core/author.cpp
  src/core/author.h
  src/core/commentsmodel.cpp
  src/core/commentsmodel.h
  src/core/engine.cpp
  src/core/engine.h
  src/core/entryinternal.cpp
  src/core/installation.cpp
  src/core/itemsmodel.cpp
  src/core/itemsmodel.h
  src/core/provider.h
  src/core/question.h
  src/qtquick/CMakeLists.txt
  src/qtquick/author.cpp
  src/qtquick/author.h
  src/qtquick/categoriesmodel.cpp
  src/qtquick/categoriesmodel.h
  src/qtquick/commentsmodel.cpp
  src/qtquick/commentsmodel.h
  src/qtquick/qml/Button.qml
  src/qtquick/qml/Dialog.qml
  src/qtquick/qml/DialogContent.qml
  src/qtquick/qml/DownloadItemsSheet.qml
  src/qtquick/qml/EntryDetails.qml
  src/qtquick/qml/NewStuffItem.qml
  src/qtquick/qml/NewStuffList.qml
  src/qtquick/qml/Page.qml
  src/qtquick/qml/QuestionAsker.qml
  src/qtquick/qml/private/ConditionalLoader.qml
  src/qtquick/qml/private/EntryCommentDelegate.qml
  src/qtquick/qml/private/EntryCommentsPage.qml
  src/qtquick/qml/private/EntryScreenshots.qml
  src/qtquick/qml/private/GridTileDelegate.qml
  src/qtquick/qml/private/Rating.qml
  src/qtquick/qml/private/Shadow.qml
  src/qtquick/qml/private/entrygriddelegates/BigPreviewDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/ThumbDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/TileDelegate.qml
  src/qtquick/qmldir
  src/qtquick/qmlplugin.cpp
  src/qtquick/quickengine.cpp
  src/qtquick/quickengine.h
  src/qtquick/quickitemsmodel.cpp
  src/qtquick/quickitemsmodel.h
  src/qtquick/quickquestionlistener.cpp
  src/qtquick/quickquestionlistener.h
  tests/CMakeLists.txt
  tests/khotnewstuff-dialog-ui/main.qml
  tests/khotnewstuff-dialog.cpp

To: leinir, #knewstuff, #vdg, #frameworks, ahiemstra
Cc: ahiemstra, anthonyfieroni, pino, ngraham, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, bruns

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

2019-08-28 Thread Dan Leinir Turthra Jensen 
leinir marked 12 inline comments as done.
leinir added inline comments.

INLINE COMMENTS

> ahiemstra wrote in atticaprovider.cpp:355
> Since upstream Qt is starting to discourage usage of QList, maybe use QVector 
> instead.

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

> ahiemstra wrote in atticaprovider.cpp:359
> Generally, it's recommended to use `std::shared_ptr 
> knsComment = std::make_shared()`. As a bonus, that allows 
> you to use `auto knsComment`.

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

> ahiemstra wrote in atticaprovider.cpp:370
> Probably better to use categorised logging for this?

Actually leftovers... but also yes :)

> ahiemstra wrote in commentsmodel.cpp:135
> Might want to add a call to checkIndex here 
> (https://doc.qt.io/qt-5/qabstractitemmodel.html#checkIndex), it would 
> obsolete some additional checking you do later.

After confirming that 5.11 (which introduces that function) is indeed 
acceptable for Frameworks, i'm ll over that ;) Very handy little function, 
that :)

> ahiemstra wrote in commentsmodel.cpp:177
> Since you're already checking for `child` being true (aka not-null), this if 
> becomes rather superfluous.

Hmm... leftovers from earlier versions of that function, there :)

> ahiemstra wrote in commentsmodel.h:50
> Considering this is intended to be used from QML, I would drop this, 
> especially since the current code in engine does nothing special. And 
> changing this would allow for doing `CommentsModel { entry: someEntry }` from 
> QML.

It's not really intended to be used from QML, i really only left out a more 
clever wrapper because... well, it's not really that heavy. But, prompted by 
this, i have in fact produced that abstraction, so there is now a 
NewStuff.CommentsModel, which makes things a bit easier during implementation, 
and more delarativey ;)

> ahiemstra wrote in author.cpp:103
> You should check if the actual property value has changed before changing it. 
> I generally do something along the lines of `if (newEngine == d->engine) 
> return;`. This helps with preventing binding loops in properties exposed to 
> QML.

Used to do that all the time, not honestly sure why i didn't here. Thanks for 
the reminder :)

> ahiemstra wrote in Button.qml:39
> Any particular reason not to alias these directly to the dialog?

Yes, if it's aliased directly, we end up initialising the engine as soon as the 
Button component is instantiated, which we want to avoid (as it's quite an 
expensive operation, and causes internet traffic the user might very well have 
no intention of using... which arguably is also a phone-home situation, and 
that needs to be avoided). I'll make an implementationy comment about that. 
(not really documentation relevant, but certainly relevant when someone 
invariably comes across this in the future and asks the same question)

> ahiemstra wrote in Dialog.qml:34
> If you're using a Qt 5.12 import anyway, might want to unify the other 
> imports to also use .12 versions.

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

> ahiemstra wrote in Dialog.qml:76
> Any particular reason for using a signal parameter here instead of just 
> setting a property on the dialog? Using a property generally allows for more 
> declarative use of the item.

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)

> ahiemstra wrote in DownloadItemsSheet.qml:49
> Design wise, I don't think having a card inside an overlaysheet looks too 
> nice. I'm also not sure if the extra attention is needed, so maybe just use a 
> label?

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

> ahiemstra wrote in Page.qml:210
> For readability, it would be better to move these to their own files.

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

> ahiemstra wrote in qmlplugin.cpp:54
> Note that this makes the qml engine take ownvership of your

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

2019-08-28 Thread Arjen Hiemstra 
ahiemstra requested changes to this revision.
ahiemstra added a comment.
This revision now requires changes to proceed.


  I have probably missed stuff, but here are my initial comments.

INLINE COMMENTS

> atticaprovider.cpp:355
> +
> +QList> getCommentsList(const 
> Attica::Comment::List , std::shared_ptr parent) {
> +QList> knsComments;

Since upstream Qt is starting to discourage usage of QList, maybe use QVector 
instead.

> atticaprovider.cpp:359
> +qDebug() << "Appending comment with id" << comment.id() << ", which 
> has" << comment.childCount() << "children";
> +std::shared_ptr knsComment(new KNSCore::Comment());
> +knsComment->id = comment.id();

Generally, it's recommended to use `std::shared_ptr 
knsComment = std::make_shared()`. As a bonus, that allows you 
to use `auto knsComment`.

> atticaprovider.cpp:370
> +if (comment.childCount() > 0) {
> +qDebug() << "Getting more comments, as this one has children, 
> and we currently have this number of comments:" << knsComments.count();
> +knsComments << getCommentsList(comment.children(), knsComment);

Probably better to use categorised logging for this?

> atticaprovider.cpp:410
> +
> +std::shared_ptr author(new KNSCore::Author);
> +author->setId(job->property("username").toString()); // This is a touch 
> hack-like, but it ensures we actually have the data in case it is not 
> returned by the server

Like above, std::make_shared

> commentsmodel.cpp:135
> +{
> +QVariant value;
> +if (index.isValid() && index.row() < d->comments.count()) {

Might want to add a call to checkIndex here 
(https://doc.qt.io/qt-5/qabstractitemmodel.html#checkIndex), it would obsolete 
some additional checking you do later.

> commentsmodel.cpp:177
> +++depth;
> +if (child->parent) {
> +child = child->parent;

Since you're already checking for `child` being true (aka not-null), this if 
becomes rather superfluous.

> commentsmodel.h:50
> + *
> + * This model should preferably be constructed by asking the Engine to give 
> a model
> + * instance to you for a specific entry using the commentsForEntry function. 
> If you

Considering this is intended to be used from QML, I would drop this, especially 
since the current code in engine does nothing special. And changing this would 
allow for doing `CommentsModel { entry: someEntry }` from QML.

> author.cpp:103
> +{
> +d->engine = qobject_cast(newEngine);
> +d->resetConnections();

You should check if the actual property value has changed before changing it. I 
generally do something along the lines of `if (newEngine == d->engine) 
return;`. This helps with preventing binding loops in properties exposed to QML.

> Button.qml:39
> + */
> +property string configFile: ghnsDialog.configFile
> +

Any particular reason not to alias these directly to the dialog?

> Dialog.qml:34
> +import QtQuick.Dialogs 1.3 as QtDialogs
> +import QtQuick.Layouts 1.12 as QtLayouts
> +import org.kde.newstuff 1.1 as NewStuff

If you're using a Qt 5.12 import anyway, might want to unify the other imports 
to also use .12 versions.

> Dialog.qml:76
> + */
> +signal dialogFinished(var changedEntries);
> +

Any particular reason for using a signal parameter here instead of just setting 
a property on the dialog? Using a property generally allows for more 
declarative use of the item.

> DownloadItemsSheet.qml:49
> +}
> +Kirigami.AbstractCard {
> +QtLayouts.Layout.leftMargin: Kirigami.Units.iconSizes.smallMedium

Design wise, I don't think having a card inside an overlaysheet looks too nice. 
I'm also not sure if the extra attention is needed, so maybe just use a label?

> Page.qml:210
> +
> +Component {
> +id: bigPreviewDelegate

For readability, it would be better to move these to their own files.

> qmlplugin.cpp:54
> +Q_UNUSED(scriptEngine)
> +return KNewStuffQuick::QuickQuestionListener::instance();
> +});

Note that this makes the qml engine take ownvership of your singleton instance 
and it will try to delete it when the engine gets deleted. You probably want to 
do `engine->setOwnership(instance, QQmlEngine::CppOwnership)` before returning 
the instance.

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #vdg, #frameworks, ahiemstra
Cc: ahiemstra, anthonyfieroni, pino, ngraham, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, bruns

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

2019-08-27 Thread Dan Leinir Turthra Jensen 
leinir edited the summary of this revision.

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #vdg, #frameworks
Cc: anthonyfieroni, pino, ngraham, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, bruns

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

2019-08-27 Thread Dan Leinir Turthra Jensen 
leinir edited the summary of this revision.

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #vdg, #frameworks
Cc: anthonyfieroni, pino, ngraham, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, bruns

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

2019-08-27 Thread Dan Leinir Turthra Jensen 
leinir edited the summary of this revision.

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #vdg, #frameworks
Cc: anthonyfieroni, pino, ngraham, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, bruns

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

2019-08-27 Thread Dan Leinir Turthra Jensen 
leinir retitled this revision from "[WIP] Bring KNewStuffQuick to feature 
parity with KNewStuff(Widgets)" to "Bring KNewStuffQuick to feature parity with 
KNewStuff(Widgets)".
leinir edited the summary of this revision.
leinir edited the test plan for this revision.
leinir added reviewers: KNewStuff, VDG, Frameworks.

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #vdg, #frameworks
Cc: anthonyfieroni, pino, ngraham, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, bruns