D6513: Add support for Attica tags support

2018-10-01 Thread Dan Leinir Turthra Jensen
This revision was automatically updated to reflect the committed changes.
Closed by commit R304:158ba4e2420b: Add support for Attica tags support 
(authored by leinir).

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6513?vs=42360=42657

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

AFFECTED FILES
  autotests/knewstuffentrytest.cpp
  src/attica/atticaprovider.cpp
  src/core/CMakeLists.txt
  src/core/engine.cpp
  src/core/engine.h
  src/core/entryinternal.cpp
  src/core/entryinternal.h
  src/core/provider.cpp
  src/core/provider.h
  src/core/tagsfilterchecker.cpp
  src/core/tagsfilterchecker.h
  src/staticxml/staticxmlprovider.cpp
  tests/CMakeLists.txt
  tests/khotnewstuff_test-ui/main.qml
  tests/khotnewstuff_test-ui/main.qmlc
  tests/khotnewstuff_test.knsrc.in
  tests/knewstuff2_test.cpp
  tests/knewstuff2_test.h
  tests/knewstuff2_test.knsrc
  tests/testdata/entry.xml
  tests/testdata/provider.xml

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfaure, 
cfeck
Cc: rikmills, dfaure, cfeck, mlaurent, ngraham, ahiemstra, 
kde-frameworks-devel, #knewstuff, michaelh, ZrenBot, bruns


D6513: Add support for Attica tags support

2018-09-30 Thread Christoph Feck
cfeck accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfaure, 
cfeck
Cc: rikmills, dfaure, cfeck, mlaurent, ngraham, ahiemstra, 
kde-frameworks-devel, #knewstuff, michaelh, ZrenBot, bruns


D6513: Add support for Attica tags support

2018-09-30 Thread Dan Leinir Turthra Jensen
leinir added a comment.


  Sorry for pinging again, but i'd really quite like to not have to wait (yet) 
another cycle for this to get out...

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfaure, 
cfeck
Cc: rikmills, dfaure, cfeck, mlaurent, ngraham, ahiemstra, 
kde-frameworks-devel, #knewstuff, michaelh, ZrenBot, bruns


D6513: Add support for Attica tags support

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


  Address @cfeck's comments

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6513?vs=41886=42360

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

AFFECTED FILES
  autotests/knewstuffentrytest.cpp
  src/attica/atticaprovider.cpp
  src/core/CMakeLists.txt
  src/core/engine.cpp
  src/core/engine.h
  src/core/entryinternal.cpp
  src/core/entryinternal.h
  src/core/provider.cpp
  src/core/provider.h
  src/core/tagsfilterchecker.cpp
  src/core/tagsfilterchecker.h
  src/staticxml/staticxmlprovider.cpp
  tests/CMakeLists.txt
  tests/khotnewstuff_test-ui/main.qml
  tests/khotnewstuff_test-ui/main.qmlc
  tests/khotnewstuff_test.knsrc.in
  tests/knewstuff2_test.cpp
  tests/knewstuff2_test.h
  tests/knewstuff2_test.knsrc
  tests/testdata/entry.xml
  tests/testdata/provider.xml

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfaure, 
cfeck
Cc: rikmills, dfaure, cfeck, mlaurent, ngraham, ahiemstra, 
kde-frameworks-devel, #knewstuff, michaelh, ZrenBot, bruns


D6513: Add support for Attica tags support

2018-09-26 Thread Dan Leinir Turthra Jensen
leinir marked 3 inline comments as done.
leinir added a comment.


  In D6513#332106 , @cfeck wrote:
  
  > Using a hash to map public to private class is a good idea. The only thing 
I'm not sure about is if there is a memory leak. I would think that having only 
pointers in the hash will never free the referenced values.
  
  
  It's the standard "oh dear, no pimpl" solution suggested on the BCI page on 
Community - 
https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B#Adding_new_data_members_to_classes_without_d-pointer
 :) The potential memory leak should be taken care of through the 
delete_d(this) call in Provider's destructor. Thanks for the style bits, 
thought i'd caught them all...

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfaure, 
cfeck
Cc: rikmills, dfaure, cfeck, mlaurent, ngraham, ahiemstra, 
kde-frameworks-devel, #knewstuff, michaelh, ZrenBot, bruns


D6513: Add support for Attica tags support

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


  Using a hash to map public to private class is a good idea. The only thing 
I'm not sure about is if there is a memory leak. I would think that having only 
pointers in the hash will never free the referenced values.

INLINE COMMENTS

> provider.cpp:44
> +if (!ret)
> +{
> +ret = new ProviderPrivate;

Frameworks coding style:

  if (...) {
  ...
  }

> provider.cpp:51
> +
> +static void delete_d(const Provider* provider)
> +{

`* ` -> ` *`

> provider.cpp:54
> +if (auto d = d_func())
> +{
> +delete d->take(provider);

again

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfaure, 
cfeck
Cc: rikmills, dfaure, cfeck, mlaurent, ngraham, ahiemstra, 
kde-frameworks-devel, #knewstuff, michaelh, ZrenBot, bruns


D6513: Add support for Attica tags support

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


  Sorry for pinging again, but i'd quite like this to get in, bic fixed and all 
that, before the next release...

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfaure, 
cfeck
Cc: rikmills, dfaure, cfeck, mlaurent, ngraham, ahiemstra, 
kde-frameworks-devel, #knewstuff, michaelh, ZrenBot, bruns


D6513: Add support for Attica tags support

2018-09-21 Thread Dan Leinir Turthra Jensen
leinir added a comment.


  For ease of re-reviewing, the relevant bits of the patch (the bits that 
didn't already get the tickmark) can be seen on their own like so: 
https://phabricator.kde.org/D6513?vs=41133=41886=ignore-most#toc
  
  (thank you very much differential for being a useful tool ;) )

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfaure, 
cfeck
Cc: rikmills, dfaure, cfeck, mlaurent, ngraham, ahiemstra, 
kde-frameworks-devel, #knewstuff, michaelh, ZrenBot, bruns


D6513: Add support for Attica tags support

2018-09-18 Thread Dan Leinir Turthra Jensen
leinir updated this revision to Diff 41886.
leinir added a comment.


  Forgot `@since 5.51` in the new api in Provider.h

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6513?vs=41815=41886

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

AFFECTED FILES
  autotests/knewstuffentrytest.cpp
  src/attica/atticaprovider.cpp
  src/core/CMakeLists.txt
  src/core/engine.cpp
  src/core/engine.h
  src/core/entryinternal.cpp
  src/core/entryinternal.h
  src/core/provider.cpp
  src/core/provider.h
  src/core/tagsfilterchecker.cpp
  src/core/tagsfilterchecker.h
  src/staticxml/staticxmlprovider.cpp
  tests/CMakeLists.txt
  tests/khotnewstuff_test-ui/main.qml
  tests/khotnewstuff_test-ui/main.qmlc
  tests/khotnewstuff_test.knsrc.in
  tests/knewstuff2_test.cpp
  tests/knewstuff2_test.h
  tests/knewstuff2_test.knsrc
  tests/testdata/entry.xml
  tests/testdata/provider.xml

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfaure, 
cfeck
Cc: rikmills, dfaure, cfeck, mlaurent, ngraham, ahiemstra, 
kde-frameworks-devel, #knewstuff, michaelh, ZrenBot, bruns


D6513: Add support for Attica tags support

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


  Previous patch had BIC issues - new patch attempts to address this, but as a 
result (of course) requires another round of review.

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfaure, 
cfeck
Cc: rikmills, dfaure, cfeck, mlaurent, ngraham, ahiemstra, 
kde-frameworks-devel, #knewstuff, michaelh, ZrenBot, bruns


D6513: Add support for Attica tags support

2018-09-17 Thread Dan Leinir Turthra Jensen
leinir updated this revision to Diff 41815.
leinir added a comment.


  After discovering that the previous version of this patch had introduced a 
binary incompatibility, and panicking momentarily that this had been done in a 
release, Jonathan reverted it (as i was away for a bit and unable to do so 
myself). The patch here attempts to fix the BIC issue, while also readjusting 
the logic for how the tag filters reach the provider. It is now done directly 
by the provider itself, rather than through each search request. While this 
does introduce that nasty d-pointer hack, it also makes more logical sense that 
the Provider itself holds the information (as that's where those filters are 
directly relevant). So, nasty surprise to find out i'd caused things to break, 
but the end result is, i think, kind of better anyway (even though it 
introduces that todo for kf-next).

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6513?vs=41133=41815

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

AFFECTED FILES
  autotests/knewstuffentrytest.cpp
  src/attica/atticaprovider.cpp
  src/core/CMakeLists.txt
  src/core/engine.cpp
  src/core/engine.h
  src/core/entryinternal.cpp
  src/core/entryinternal.h
  src/core/provider.cpp
  src/core/provider.h
  src/core/tagsfilterchecker.cpp
  src/core/tagsfilterchecker.h
  src/staticxml/staticxmlprovider.cpp
  tests/CMakeLists.txt
  tests/khotnewstuff_test-ui/main.qml
  tests/khotnewstuff_test-ui/main.qmlc
  tests/khotnewstuff_test.knsrc.in
  tests/knewstuff2_test.cpp
  tests/knewstuff2_test.h
  tests/knewstuff2_test.knsrc
  tests/testdata/entry.xml
  tests/testdata/provider.xml

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfaure, 
cfeck
Cc: rikmills, dfaure, cfeck, mlaurent, ngraham, ahiemstra, 
kde-frameworks-devel, #knewstuff, michaelh, ZrenBot, bruns


D6513: Add support for Attica tags support

2018-09-15 Thread Dan Leinir Turthra Jensen
leinir reopened this revision.
leinir added a comment.
This revision is now accepted and ready to land.


  In D6513#326519 , @rikmills wrote:
  
  > In D6513#326071 , @ngraham wrote:
  >
  > > FYI, this included an ABI change to `SearchRequest` that broke Discover: 
https://bugs.kde.org/show_bug.cgi?id=398412
  >
  >
  > I see this revision has been reverted in master in 
293ae2448f54fd1b1f7cacc86cd40b30a3fb087d 

  
  
  It has. I am currently (bar the odd break for a cup of tea) knee deep in 
house renovations, but i will be reopening this when i work out what to do to 
make this not ABI incompatible. In the meantime, suggestions for achieving the 
least hacky approach would be appreciated.

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfaure, 
cfeck
Cc: rikmills, dfaure, cfeck, mlaurent, ngraham, ahiemstra, 
kde-frameworks-devel, #knewstuff, michaelh, ZrenBot, bruns


D6513: Add support for Attica tags support

2018-09-15 Thread Rik Mills
rikmills added a comment.


  In D6513#326071 , @ngraham wrote:
  
  > FYI, this included an ABI change to `SearchRequest` that broke Discover: 
https://bugs.kde.org/show_bug.cgi?id=398412
  
  
  I see this revision has been reverted in master in 
293ae2448f54fd1b1f7cacc86cd40b30a3fb087d 


REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfaure, 
cfeck
Cc: rikmills, dfaure, cfeck, mlaurent, ngraham, ahiemstra, 
kde-frameworks-devel, #knewstuff, michaelh, ZrenBot, bruns


D6513: Add support for Attica tags support

2018-09-14 Thread Nathaniel Graham
ngraham added a comment.


  FYI, this included an ABI change to `SearchRequest` that broke Discover: 
https://bugs.kde.org/show_bug.cgi?id=398412

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfaure, 
cfeck
Cc: dfaure, cfeck, mlaurent, ngraham, ahiemstra, kde-frameworks-devel, 
#knewstuff, michaelh, ZrenBot, bruns


D6513: Add support for Attica tags support

2018-09-13 Thread Dan Leinir Turthra Jensen
leinir added a comment.


  In D6513#324915 , @ngraham wrote:
  
  > In D6513#324458 , @leinir wrote:
  >
  > > In D6513#324419 , @ngraham 
wrote:
  > >
  > > > So I see `ghns_exclude` over at store.kde.org, but it doesn't feel 
quite right to check that box next to everything KDE4. Is that what I should be 
doing?
  > >
  > >
  > > That is precisely what you should be doing, yes :) You are right, it 
doesn't quite feel right to just outright exclude everything KDE4 from GHNS, 
but the reason it works is that the filtering happens clientside, and it will 
still show up for anybody who doesn't have this patch (or, in other words, 
anybody who has a less than version 5.51 Frameworks).
  >
  >
  > OK, got it! Will commence that work and then document it with some 
instructions in https://community.kde.org/Get_Involved. :)
  
  
  Great, thank you! :)
  
  >> We should, however, also be talking about which other tags we want to have 
- it seems like we might well want to have some more capable filtering in our 
various dialogues. That, however, is not really something that'd make sense to 
discuss in a comment thread under some differential revision ;) Perhaps we 
should take it up at a Plasma meeting, sort of air the idea that it's now a 
thing that can be done, and then collect ideas in a Task?
  > 
  > Sounds good to me. Can I get an invite/mention/whatever when that happens?
  
  They're every Monday on the Plasma IRC channel, so i'd say sooner is better, 
kick it off this Monday and see where we go from there :)

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfaure, 
cfeck
Cc: dfaure, cfeck, mlaurent, ngraham, ahiemstra, kde-frameworks-devel, 
#knewstuff, michaelh, ZrenBot, bruns


D6513: Add support for Attica tags support

2018-09-12 Thread Nathaniel Graham
ngraham added a comment.


  In D6513#324458 , @leinir wrote:
  
  > In D6513#324419 , @ngraham wrote:
  >
  > > So I see `ghns_exclude` over at store.kde.org, but it doesn't feel quite 
right to check that box next to everything KDE4. Is that what I should be doing?
  >
  >
  > That is precisely what you should be doing, yes :) You are right, it 
doesn't quite feel right to just outright exclude everything KDE4 from GHNS, 
but the reason it works is that the filtering happens clientside, and it will 
still show up for anybody who doesn't have this patch (or, in other words, 
anybody who has a less than version 5.51 Frameworks).
  
  
  OK, got it! Will commence that work and then document it with some 
instructions in https://community.kde.org/Get_Involved. :)
  
  > We should, however, also be talking about which other tags we want to have 
- it seems like we might well want to have some more capable filtering in our 
various dialogues. That, however, is not really something that'd make sense to 
discuss in a comment thread under some differential revision ;) Perhaps we 
should take it up at a Plasma meeting, sort of air the idea that it's now a 
thing that can be done, and then collect ideas in a Task?
  
  Sounds good to me. Can I get an invite/mention/whatever when that happens?

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfaure, 
cfeck
Cc: dfaure, cfeck, mlaurent, ngraham, ahiemstra, kde-frameworks-devel, 
#knewstuff, michaelh, ZrenBot, bruns


D6513: Add support for Attica tags support

2018-09-12 Thread Dan Leinir Turthra Jensen
leinir added a comment.


  In D6513#324419 , @ngraham wrote:
  
  > In D6513#322167 , @leinir wrote:
  >
  > > In D6513#322078 , @ngraham 
wrote:
  > >
  > > > So is this enough for people to start tagging KDE4 content as such? Or 
is anything else still required before that capability lands?
  > >
  > >
  > > In short, this is basically enough :) When people run KNewStuff with this 
patch, any content which has been marked as ghns_exclude (that tick box you and 
the other moderators have on the store) will be hidden from the user :) For 
doing more "proper" filtering, changes will want to be done to either just the 
knsrc files, or to the clients themselves (which would be for things like 
"don't show wallpapers with incorrect resolutions" or "only show things with 
x86 binaries" or that sort of stuff).
  >
  >
  > So I see `ghns_exclude` over at store.kde.org, but it doesn't feel quite 
right to check that box next to everything KDE4. Is that what I should be doing?
  
  
  That is precisely what you should be doing, yes :) You are right, it doesn't 
quite feel right to just outright exclude everything KDE4 from GHNS, but the 
reason it works is that the filtering happens clientside, and it will still 
show up for anybody who doesn't have this patch (or, in other words, anybody 
who has a less than version 5.51 Frameworks).
  
  We should, however, also be talking about which other tags we want to have - 
it seems like we might well want to have some more capable filtering in our 
various dialogues. That, however, is not really something that'd make sense to 
discuss in a comment thread under some differential revision ;) Perhaps we 
should take it up at a Plasma meeting, sort of air the idea that it's now a 
thing that can be done, and then collect ideas in a Task?

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfaure, 
cfeck
Cc: dfaure, cfeck, mlaurent, ngraham, ahiemstra, kde-frameworks-devel, 
#knewstuff, michaelh, ZrenBot, bruns


D6513: Add support for Attica tags support

2018-09-11 Thread Nathaniel Graham
ngraham added a comment.


  In D6513#322167 , @leinir wrote:
  
  > In D6513#322078 , @ngraham wrote:
  >
  > > So is this enough for people to start tagging KDE4 content as such? Or is 
anything else still required before that capability lands?
  >
  >
  > In short, this is basically enough :) When people run KNewStuff with this 
patch, any content which has been marked as ghns_exclude (that tick box you and 
the other moderators have on the store) will be hidden from the user :) For 
doing more "proper" filtering, changes will want to be done to either just the 
knsrc files, or to the clients themselves (which would be for things like 
"don't show wallpapers with incorrect resolutions" or "only show things with 
x86 binaries" or that sort of stuff).
  
  
  So I see `ghns_exclude` over at store.kde.org, but it doesn't feel quite 
right to check that box next to everything KDE4. Is that what I should be doing?

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfaure, 
cfeck
Cc: dfaure, cfeck, mlaurent, ngraham, ahiemstra, kde-frameworks-devel, 
#knewstuff, michaelh, ZrenBot, bruns


D6513: Add support for Attica tags support

2018-09-08 Thread Dan Leinir Turthra Jensen
leinir added a comment.


  In D6513#322078 , @ngraham wrote:
  
  > So is this enough for people to start tagging KDE4 content as such? Or is 
anything else still required before that capability lands?
  
  
  In short, this is basically enough :) When people run KNewStuff with this 
patch, any content which has been marked as ghns_exclude (that tick box you and 
the other moderators have on the store) will be hidden from the user :) For 
doing more "proper" filtering, changes will want to be done to either just the 
knsrc files, or to the clients themselves (which would be for things like 
"don't show wallpapers with incorrect resolutions" or "only show things with 
x86 binaries" or that sort of stuff).

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfaure, 
cfeck
Cc: dfaure, cfeck, mlaurent, ngraham, ahiemstra, kde-frameworks-devel, 
#knewstuff, michaelh, ZrenBot, bruns


D6513: Add support for Attica tags support

2018-09-07 Thread Nathaniel Graham
ngraham added a comment.


  So is this enough for people to start tagging KDE4 content as such? Or is 
anything else still required before that capability lands?

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfaure, 
cfeck
Cc: dfaure, cfeck, mlaurent, ngraham, ahiemstra, kde-frameworks-devel, 
#knewstuff, michaelh, ZrenBot, bruns


D6513: Add support for Attica tags support

2018-09-07 Thread Dan Leinir Turthra Jensen
leinir added inline comments.

INLINE COMMENTS

> dfaure wrote in tagsfilterchecker.cpp:103
> Yes.

Great, thank you :) Yeah, going to have to nab a copy of that for a readthrough 
:)

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfaure, 
cfeck
Cc: dfaure, cfeck, mlaurent, ngraham, ahiemstra, kde-frameworks-devel, 
#knewstuff, michaelh, ZrenBot, bruns


D6513: Add support for Attica tags support

2018-09-07 Thread Dan Leinir Turthra Jensen
This revision was automatically updated to reflect the committed changes.
Closed by commit R304:2ad3e66d81b6: Add support for Attica tags support 
(authored by leinir).

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6513?vs=41131=41133

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

AFFECTED FILES
  autotests/knewstuffentrytest.cpp
  src/attica/atticaprovider.cpp
  src/core/CMakeLists.txt
  src/core/engine.cpp
  src/core/engine.h
  src/core/entryinternal.cpp
  src/core/entryinternal.h
  src/core/provider.h
  src/core/tagsfilterchecker.cpp
  src/core/tagsfilterchecker.h
  src/staticxml/staticxmlprovider.cpp
  tests/CMakeLists.txt
  tests/khotnewstuff_test.cpp
  tests/khotnewstuff_test.knsrc.in
  tests/knewstuff2_test.cpp
  tests/knewstuff2_test.h
  tests/knewstuff2_test.knsrc
  tests/testdata/entry.xml
  tests/testdata/provider.xml

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfaure, 
cfeck
Cc: dfaure, cfeck, mlaurent, ngraham, ahiemstra, kde-frameworks-devel, 
#knewstuff, michaelh, ZrenBot, bruns


D6513: Add support for Attica tags support

2018-09-07 Thread David Faure
dfaure accepted this revision.
dfaure added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> leinir wrote in tagsfilterchecker.cpp:103
> Scott Meyers' book? (might need to grab a copy of that, then :) )

Yes.

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfaure, 
cfeck
Cc: dfaure, cfeck, mlaurent, ngraham, ahiemstra, kde-frameworks-devel, 
#knewstuff, michaelh, ZrenBot, bruns


D6513: Add support for Attica tags support

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


  Address @dfaure's comments

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6513?vs=41034=41131

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

AFFECTED FILES
  autotests/knewstuffentrytest.cpp
  src/attica/atticaprovider.cpp
  src/core/CMakeLists.txt
  src/core/engine.cpp
  src/core/engine.h
  src/core/entryinternal.cpp
  src/core/entryinternal.h
  src/core/provider.h
  src/core/tagsfilterchecker.cpp
  src/core/tagsfilterchecker.h
  src/staticxml/staticxmlprovider.cpp
  tests/CMakeLists.txt
  tests/khotnewstuff_test.cpp
  tests/khotnewstuff_test.knsrc.in
  tests/knewstuff2_test.cpp
  tests/knewstuff2_test.h
  tests/knewstuff2_test.knsrc
  tests/testdata/entry.xml
  tests/testdata/provider.xml

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfaure, 
cfeck
Cc: dfaure, cfeck, mlaurent, ngraham, ahiemstra, kde-frameworks-devel, 
#knewstuff, michaelh, ZrenBot, bruns


D6513: Add support for Attica tags support

2018-09-07 Thread Dan Leinir Turthra Jensen
leinir marked 6 inline comments as done.
leinir added inline comments.

INLINE COMMENTS

> dfaure wrote in tagsfilterchecker.cpp:103
> .insert(tag, val) is raster, see Effective STL

Scott Meyers' book? (might need to grab a copy of that, then :) )

> dfaure wrote in CMakeLists.txt:4
> Qick ? :)

Oups, yes, quite ;)

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfaure, 
cfeck
Cc: dfaure, cfeck, mlaurent, ngraham, ahiemstra, kde-frameworks-devel, 
#knewstuff, michaelh, ZrenBot, bruns


D6513: Add support for Attica tags support

2018-09-06 Thread David Faure
dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> tagsfilterchecker.cpp:33
> +{
> +qDeleteAll(validators.values());
> +}

Unnecessarily slow. Remove the .values() call.

> tagsfilterchecker.cpp:46
> +m_tag = tag;
> +if (value != QString::null) {
> +m_acceptedValues << value;

!value.isEmpty()

Or isNull()?

> tagsfilterchecker.cpp:102
> +if (!val) {
> +val = new EqualityValidator(tag, QString::null);
> +validators[tag] = val;

Consider QString::null deprecated. Use QString().

> tagsfilterchecker.cpp:103
> +val = new EqualityValidator(tag, QString::null);
> +validators[tag] = val;
> +}

.insert(tag, val) is raster, see Effective STL

> tagsfilterchecker.cpp:113
> +val = new InequalityValidator(tag, QString::null);
> +validators[tag] = val;
> +}

Insert

> CMakeLists.txt:4
>  
> -find_package(Qt5 ${REQUIRED_QT_VERSION} CONFIG REQUIRED Test Widgets) # 
> Widgets for KMoreTools
> +find_package(Qt5 ${REQUIRED_QT_VERSION} CONFIG REQUIRED Test Widgets Gui 
> Quick) # Widgets for KMoreTools and Qick for the interactive KNS test
> +

Qick ? :)

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfaure, 
cfeck
Cc: dfaure, cfeck, mlaurent, ngraham, ahiemstra, kde-frameworks-devel, 
#knewstuff, michaelh, ZrenBot, bruns


D6513: Add support for Attica tags support

2018-09-05 Thread Laurent Montel
mlaurent accepted this revision.

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfaure, 
cfeck
Cc: dfaure, cfeck, mlaurent, ngraham, ahiemstra, kde-frameworks-devel, 
#knewstuff, michaelh, ZrenBot, bruns


D6513: Add support for Attica tags support

2018-09-05 Thread Christoph Feck
cfeck added a comment.


  @dfaure, @mlaurent okey?

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfaure, 
cfeck
Cc: dfaure, cfeck, mlaurent, ngraham, ahiemstra, kde-frameworks-devel, 
#knewstuff, michaelh, ZrenBot, bruns


D6513: Add support for Attica tags support

2018-09-05 Thread Christoph Feck
cfeck accepted this revision.

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfaure, 
cfeck
Cc: dfaure, cfeck, mlaurent, ngraham, ahiemstra, kde-frameworks-devel, 
#knewstuff, michaelh, ZrenBot, bruns


D6513: Add support for Attica tags support

2018-09-05 Thread Dan Leinir Turthra Jensen
leinir updated this revision to Diff 41034.
leinir marked an inline comment as done.
leinir added a comment.


  More codestyle fixes. I also notice some which were there before this work, 
but fixing that seems distinctly out of scope for this patch...

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6513?vs=40981=41034

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

AFFECTED FILES
  autotests/knewstuffentrytest.cpp
  src/attica/atticaprovider.cpp
  src/core/CMakeLists.txt
  src/core/engine.cpp
  src/core/engine.h
  src/core/entryinternal.cpp
  src/core/entryinternal.h
  src/core/provider.h
  src/core/tagsfilterchecker.cpp
  src/core/tagsfilterchecker.h
  src/staticxml/staticxmlprovider.cpp
  tests/CMakeLists.txt
  tests/khotnewstuff_test.cpp
  tests/khotnewstuff_test.knsrc.in
  tests/knewstuff2_test.cpp
  tests/knewstuff2_test.h
  tests/knewstuff2_test.knsrc
  tests/testdata/entry.xml
  tests/testdata/provider.xml

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfaure, 
cfeck
Cc: dfaure, cfeck, mlaurent, ngraham, ahiemstra, kde-frameworks-devel, 
#knewstuff, michaelh, ZrenBot, bruns


D6513: Add support for Attica tags support

2018-09-05 Thread Dan Leinir Turthra Jensen
leinir marked 16 inline comments as done.
leinir added a comment.


  i guess uncrustify isn't a magic bullet either, eh? ;) Thanks for the 
findings!

INLINE COMMENTS

> cfeck wrote in khotnewstuff_test.cpp:66
> Any rationale for using `fromLocal8Bit()` for fixed strings? If, for whatever 
> reason, you do not want to use QStringLiteral or QLatin1String, please use 
> fromUtf8(). This is what we ship for source files.

None apart from this being a modified version of an old test which used that 
function rather than the proper one. Fixed :)

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfaure, 
cfeck
Cc: dfaure, cfeck, mlaurent, ngraham, ahiemstra, kde-frameworks-devel, 
#knewstuff, michaelh, ZrenBot, bruns


D6513: Add support for Attica tags support

2018-09-04 Thread Christoph Feck
cfeck added inline comments.

INLINE COMMENTS

> atticaprovider.cpp:282
> +for (const Attica::DownloadDescription  : 
> content.downloadUrlDescriptions()) {
> +if(downloadschecker.filterAccepts(dli.tags())) {
> +filterAcceptsDownloads = true;

Missing Space after `if`

> atticaprovider.cpp:296
> +}
> +else {
> +qCDebug(KNEWSTUFFCORE) << "Filter has excluded" << 
> content.name() << "on entry filter" << mCurrentRequest.tagFilter;

Coding style:

  if (...) {
  ...
  } else {
  ...
  }

> engine.h:226
> + * @see setDownloadTagFilter(QStringList)
> + * @since 5.50
> + */

-> `5.51` (everywhere)

> staticxmlprovider.cpp:266
>  }
> +else {
> +qCDebug(KNEWSTUFFCORE) << "Filter has excluded" << 
> entry.name() << "on download filter" << mCurrentRequest.downloadTagFilter;

again

> staticxmlprovider.cpp:270
> +}
> +else {
> +qCDebug(KNEWSTUFFCORE) << "Filter has excluded" << entry.name() 
> << "on entry filter" << mCurrentRequest.tagFilter;

again

> khotnewstuff_test.cpp:42
> +#include  // for exit()
> +#include  // for stdout
> +



> khotnewstuff_test.cpp:49
> +m_configFile = configFile;
> +m_engine = NULL;
> +m_testall = false;

nullptr

> khotnewstuff_test.cpp:66
> +{
> +addMessage(QString::fromLocal8Bit("-- test kns2 entry class"), 
> QStringLiteral("msg_info"));
> +

Any rationale for using `fromLocal8Bit()` for fixed strings? If, for whatever 
reason, you do not want to use QStringLiteral or QLatin1String, please use 
fromUtf8(). This is what we ship for source files.

> khotnewstuff_test.cpp:190
> +
> +void KNewStuff2Test::slotEngineError(const QString& error)
> +{

`& ` -> ` &`

> khotnewstuff_test.cpp:202
> +{
> +QStandardItem* item = new QStandardItem(message);
> +item->setData(iconName, Qt::WhatsThisRole);

`* ` -> ` *`

> khotnewstuff_test.cpp:211
> +{
> +if(test) {
> +test->addMessage(msg, QStringLiteral("msg_info"));

Missing space

> khotnewstuff_test.cpp:222
> +
> +QCommandLineParser* parser = new QCommandLineParser;
> +parser->addHelpOption();

`* ` -> ` *`

> khotnewstuff_test.cpp:231
> +}
> +else {
> +test = new 
> KNewStuff2Test(QString::fromLatin1("%1/khotnewstuff_test.knsrc").arg(QStringLiteral(KNSBUILDDIR)));

again

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfaure, 
cfeck
Cc: dfaure, cfeck, mlaurent, ngraham, ahiemstra, kde-frameworks-devel, 
#knewstuff, michaelh, ZrenBot, bruns


D6513: Add support for Attica tags support

2018-09-04 Thread Dan Leinir Turthra Jensen
leinir updated this revision to Diff 40981.
leinir added a comment.


  Uncrustify-kf5 things as requested, hope this is better!

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6513?vs=39174=40981

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

AFFECTED FILES
  autotests/knewstuffentrytest.cpp
  src/attica/atticaprovider.cpp
  src/core/CMakeLists.txt
  src/core/engine.cpp
  src/core/engine.h
  src/core/entryinternal.cpp
  src/core/entryinternal.h
  src/core/provider.h
  src/staticxml/staticxmlprovider.cpp
  tests/CMakeLists.txt
  tests/khotnewstuff_test.cpp
  tests/khotnewstuff_test.knsrc.in
  tests/knewstuff2_test.cpp
  tests/knewstuff2_test.h
  tests/knewstuff2_test.knsrc
  tests/testdata/entry.xml
  tests/testdata/provider.xml

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfaure, 
cfeck
Cc: dfaure, cfeck, mlaurent, ngraham, ahiemstra, kde-frameworks-devel, 
#knewstuff, michaelh, ZrenBot, bruns


D6513: Add support for Attica tags support

2018-09-04 Thread Christoph Feck
cfeck requested changes to this revision.
cfeck added a comment.
This revision now requires changes to proceed.


  I still see many coding style issues. If you cannot run it through the 
formatting scripts, I will review it later.

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfaure, 
cfeck
Cc: dfaure, cfeck, mlaurent, ngraham, ahiemstra, kde-frameworks-devel, 
#knewstuff, michaelh, ZrenBot, bruns


D6513: Add support for Attica tags support

2018-08-29 Thread Dan Leinir Turthra Jensen
leinir added a comment.


  Welcome back from Akademy - sorry for being a pest, but it'd be really nifty 
if we could get this and it's dependent in D6512 
 through before 5.50... :)

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfaure
Cc: dfaure, cfeck, mlaurent, ngraham, ahiemstra, kde-frameworks-devel, 
#knewstuff, michaelh, ZrenBot, bruns


D6513: Add support for Attica tags support

2018-08-06 Thread Dan Leinir Turthra Jensen
leinir updated this revision to Diff 39174.
leinir marked 3 inline comments as done.
leinir added a comment.


  Address the various comments by dfaure, cfeck and ngraham - with thanks!

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6513?vs=38506=39174

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

AFFECTED FILES
  autotests/knewstuffentrytest.cpp
  src/attica/atticaprovider.cpp
  src/core/CMakeLists.txt
  src/core/engine.cpp
  src/core/engine.h
  src/core/entryinternal.cpp
  src/core/entryinternal.h
  src/core/provider.h
  src/core/tagsfilterchecker.cpp
  src/core/tagsfilterchecker.h
  src/staticxml/staticxmlprovider.cpp
  tests/CMakeLists.txt
  tests/khotnewstuff_test.cpp
  tests/khotnewstuff_test.knsrc.in
  tests/knewstuff2_test.cpp
  tests/knewstuff2_test.h
  tests/knewstuff2_test.knsrc
  tests/testdata/entry.xml
  tests/testdata/provider.xml

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfaure
Cc: dfaure, cfeck, mlaurent, ngraham, ahiemstra, kde-frameworks-devel, 
#knewstuff, michaelh, ZrenBot, bruns


D6513: Add support for Attica tags support

2018-08-06 Thread Dan Leinir Turthra Jensen
leinir marked 17 inline comments as done.
leinir added inline comments.

INLINE COMMENTS

> dfaure wrote in engine.cpp:133
> That's out of bounds, if tagFilter is empty!! It will assert.
> 
> You meant append or push_back, I think.

i did indeed!

> dfaure wrote in engine.cpp:138
> !? What's the point in clearing a list that is already empty?

None at all - that would be a bit of leftovers, thanks for spotting that :)

> cfeck wrote in khotnewstuff_test.knsrc.in:3
> Do we still need the kalzium references?

No, i guess we don't, really... gone :)

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfaure
Cc: dfaure, cfeck, mlaurent, ngraham, ahiemstra, kde-frameworks-devel, 
#knewstuff, michaelh, ZrenBot, bruns


D6513: Add support for Attica tags support

2018-08-04 Thread David Faure
dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  @since 5.50 is missing on any new *public* API (methods or classes).
  
  (This won't get into 5.49 which I just tagged)

INLINE COMMENTS

> engine.cpp:132
> +d->tagFilter = group.readEntry("TagFilter", QStringList());
> +if(d->tagFilter.length() == 0) {
> +d->tagFilter[0] = QStringLiteral("ghns_exclude!=1");

use isEmpty()

> engine.cpp:133
> +if(d->tagFilter.length() == 0) {
> +d->tagFilter[0] = QStringLiteral("ghns_exclude!=1");
> +}

That's out of bounds, if tagFilter is empty!! It will assert.

You meant append or push_back, I think.

> engine.cpp:137
> +d->downloadTagFilter = group.readEntry("DownloadTagFilter", 
> QStringList());
> +if(d->downloadTagFilter.length() == 0) {
> +d->downloadTagFilter.clear();

isEmpty()

> engine.cpp:138
> +if(d->downloadTagFilter.length() == 0) {
> +d->downloadTagFilter.clear();
> +}

!? What's the point in clearing a list that is already empty?

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfaure
Cc: dfaure, cfeck, mlaurent, ngraham, ahiemstra, kde-frameworks-devel, 
#knewstuff, michaelh, ZrenBot, bruns


D6513: Add support for Attica tags support

2018-08-02 Thread Christoph Feck
cfeck added inline comments.

INLINE COMMENTS

> ngraham wrote in atticaprovider.cpp:280
> Also isn't Qt `foreach` deprecated anyway? 
> https://www.kdab.com/goodbye-q_foreach/

Ah, yes. `Q_FOREACH`, or C++11 `for (x : y)` if you see it does not cause 
unneeded detaches.

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent
Cc: cfeck, mlaurent, ngraham, ahiemstra, kde-frameworks-devel, #knewstuff, 
michaelh, ZrenBot, bruns


D6513: Add support for Attica tags support

2018-08-02 Thread Nathaniel Graham
ngraham added inline comments.

INLINE COMMENTS

> cfeck wrote in atticaprovider.cpp:280
> Space after `foreach`. No space after `&`.

Also isn't Qt `foreach` deprecated anyway? 
https://www.kdab.com/goodbye-q_foreach/

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent
Cc: cfeck, mlaurent, ngraham, ahiemstra, kde-frameworks-devel, #knewstuff, 
michaelh, ZrenBot, bruns


D6513: Add support for Attica tags support

2018-08-02 Thread Christoph Feck
cfeck added inline comments.

INLINE COMMENTS

> atticaprovider.cpp:276
>  Q_FOREACH (const Content , contents) {
> -mCachedContent.insert(content.id(), content);
> -entries.append(entryFromAtticaContent(content));
> +if(checker.filterAccepts(content.tags())) {
> +bool filterAcceptsDownloads = true;

Coding style: Space after `if` (multiple occurences).

> atticaprovider.cpp:280
> +filterAcceptsDownloads = false;
> +foreach(const Attica::DownloadDescription & dli, 
> content.downloadUrlDescriptions()) {
> +if(downloadschecker.filterAccepts(dli.tags())) {

Space after `foreach`. No space after `&`.

> atticaprovider.cpp:291
> +}
> +else {
> +qCDebug(KNEWSTUFFCORE) << "Filter has excluded" << 
> content.name() << "on download filter" << mCurrentRequest.downloadTagFilter;

Frameworks coding style:

  if (...) {
  ...
  } else {
  ...
  }

> engine.h:239
> + */
> +void addTagFilter(const QString& filter);
> +/**

const QString 

> tagsfilterchecker.cpp:27
> +
> +class TagsFilterChecker::Private
> +{

This could be marked non-exported.

Please also check this and all other new files for coding style issues. If 
unsure, run it through the `astyle-kdelibs` or `uncrustify-kf5` script from 
`kde-dev-scripts.git`.

> khotnewstuff_test.knsrc.in:3
>  #ProvidersUrl=http://edu.kde.org/kalzium/molecules.xml
> -ProvidersUrl=http://new.kstuff.org/provider-kalzium.xml
> +#ProvidersUrl=http://new.kstuff.org/provider-kalzium.xml
> +ProvidersUrl=file://@CMAKE_CURRENT_SOURCE_DIR@/testdata/provider.xml

Do we still need the kalzium references?

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent
Cc: cfeck, mlaurent, ngraham, ahiemstra, kde-frameworks-devel, #knewstuff, 
michaelh, ZrenBot, bruns


D6513: Add support for Attica tags support

2018-08-01 Thread Dan Leinir Turthra Jensen
leinir added a comment.


  @mlaurent quick ping? :)
  
  Also in general, sorry, would really like to get this merged in asap - i 
realise it's the summer holidays for a lot of people ;)

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent
Cc: mlaurent, ngraham, ahiemstra, kde-frameworks-devel, #knewstuff, michaelh, 
ZrenBot, bruns


D6513: Add support for Attica tags support

2018-07-28 Thread Arjen Hiemstra
ahiemstra accepted this revision.

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent
Cc: mlaurent, ngraham, ahiemstra, kde-frameworks-devel, #knewstuff, michaelh, 
ZrenBot, bruns


D6513: Add support for Attica tags support

2018-07-26 Thread Dan Leinir Turthra Jensen
leinir updated this revision to Diff 38506.
leinir marked an inline comment as done.
leinir added a comment.


  Address mlaurent's comments

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6513?vs=38489=38506

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

AFFECTED FILES
  autotests/knewstuffentrytest.cpp
  src/attica/atticaprovider.cpp
  src/core/CMakeLists.txt
  src/core/engine.cpp
  src/core/engine.h
  src/core/entryinternal.cpp
  src/core/entryinternal.h
  src/core/provider.h
  src/core/tagsfilterchecker.cpp
  src/core/tagsfilterchecker.h
  src/staticxml/staticxmlprovider.cpp
  tests/CMakeLists.txt
  tests/khotnewstuff_test.cpp
  tests/khotnewstuff_test.knsrc.in
  tests/knewstuff2_test.cpp
  tests/knewstuff2_test.h
  tests/knewstuff2_test.knsrc
  tests/testdata/entry.xml
  tests/testdata/provider.xml

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent
Cc: mlaurent, ngraham, ahiemstra, kde-frameworks-devel, #knewstuff, michaelh, 
ZrenBot, bruns


D6513: Add support for Attica tags support

2018-07-26 Thread Dan Leinir Turthra Jensen
leinir marked 3 inline comments as done.
leinir added inline comments.

INLINE COMMENTS

> mlaurent wrote in entryinternal.cpp:495
> qCWarning(KNEWSTUFF_CORE) ?

I was just adding in a bit of verbosity to the output... but yes, makes sense 
to fix that while i'm there anyway :)

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent
Cc: mlaurent, ngraham, ahiemstra, kde-frameworks-devel, #knewstuff, michaelh, 
ZrenBot, bruns


D6513: Add support for Attica tags support

2018-07-26 Thread Laurent Montel
mlaurent requested changes to this revision.
mlaurent added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> entryinternal.cpp:495
>  if (reader.name() != QLatin1String("stuff")) {
> -qWarning() << "Parsing Entry from invalid XML";
> +qWarning() << "Parsing Entry from invalid XML. Reader tag name was 
> expected to be \"stuff\", but was found as:" << reader.name();
>  return false;

qCWarning(KNEWSTUFF_CORE) ?

> tagsfilterchecker.cpp:147
> +for(const QString  : tags) {
> +if(tag.length() == 0) {
> +// This happens when you do a split on an empty string (not an 
> empty list, a list with one empty element... because reasons).

isEmpty() ?

> tagsfilterchecker.cpp:152
> +}
> +QStringList current = tag.split(QStringLiteral("="));
> +if(current.length() > 2) {

use QLatin1Char('=')

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent
Cc: mlaurent, ngraham, ahiemstra, kde-frameworks-devel, #knewstuff, michaelh, 
ZrenBot, bruns


D6513: Add support for Attica tags support

2018-07-26 Thread Dan Leinir Turthra Jensen
leinir updated this revision to Diff 38489.
leinir marked 4 inline comments as done.
leinir added a comment.


  Address ahiemstra's comments

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6513?vs=37047=38489

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

AFFECTED FILES
  autotests/knewstuffentrytest.cpp
  src/attica/atticaprovider.cpp
  src/core/CMakeLists.txt
  src/core/engine.cpp
  src/core/engine.h
  src/core/entryinternal.cpp
  src/core/entryinternal.h
  src/core/provider.h
  src/core/tagsfilterchecker.cpp
  src/core/tagsfilterchecker.h
  src/staticxml/staticxmlprovider.cpp
  tests/CMakeLists.txt
  tests/khotnewstuff_test.cpp
  tests/khotnewstuff_test.knsrc.in
  tests/knewstuff2_test.cpp
  tests/knewstuff2_test.h
  tests/knewstuff2_test.knsrc
  tests/testdata/entry.xml
  tests/testdata/provider.xml

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra
Cc: ngraham, ahiemstra, kde-frameworks-devel, #knewstuff, michaelh, ZrenBot, 
bruns


D6513: Add support for Attica tags support

2018-07-26 Thread Dan Leinir Turthra Jensen
leinir marked 7 inline comments as done.
leinir added inline comments.

INLINE COMMENTS

> ahiemstra wrote in atticaprovider.cpp:283
> Wouldn't it be simpler to first check if the content should be considered at 
> all, and then check if one of the downloads is valid? So something like:
> 
>   if(!checker.filterAccepts(content.tags()) {
> continue
>   }
>   
>   foreach download {
>if(downloadsChecker.filterAccepts(download.tags) {
>  add download
>  break
>}
>   }

Hmm... It's not quite so simple as that (it's just just a straight foreach, 
there's also the preselection logic, which is what that ternary is for), but 
the fact that wasn't clear to you while reading does suggest it was too 
convoluted. I've remodelled it somewhat, and i think the new logic is perhaps 
more easily readable as well, which is not a bad thing. Thanks :)

> ahiemstra wrote in engine.cpp:131
> Considering KConfig has methods to read and write lists, wouldn't it be 
> better to use those?

It would indeed, and also removes the frankly distasteful jiggling about on the 
next line because QString::split returns a list with a single empty item if the 
string you're trying to split is zero length ;)

> ahiemstra wrote in engine.h:190
> Would it make sense to add an `addTagFilter` method or similar for 
> convenience that appends instead of replaces? Or maybe make the setter a bit 
> more intelligent so it is harder to forget adding the `ghns_exclude!=1` 
> filter? Right now, if I need to do anything with the tag filters, I will 
> constantly need to remember to add the ghns_exclude filter.
> 
> Maybe even completely separate the ghns_exclude filter from this setter and 
> make a separate "also show excluded content" switch that removes the filter?

Hmm... Yes, when using this from code, rather than just using it with knsrc 
files, that's not a bad idea at all. Incidentally, something we'll be needing 
when implementing e.g. support for filtering AppImages by architecture (which 
can't really be put into the knsrc files, unless we come up with some kind of 
placeholder for that, which i'm not against, but feel is perhaps a little 
overly magic).

> ahiemstra wrote in tagsfilterchecker.cpp:98
> Wouldn't this be simpler?
> 
>   if(!validators.contains(tag)) {
> validators[tag] = new EqualityValidator(tag, "");
>   }
>   validators.value(tag)->m_acceptedValues << value;

Hmm, not quite, that would create a ghost entry in the list... However, 
allowing the validator to be created without values to work on would simplify 
things, so yeah, just going to do that :)

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra
Cc: ngraham, ahiemstra, kde-frameworks-devel, #knewstuff, michaelh, ZrenBot, 
bruns


D6513: Add support for Attica tags support

2018-07-24 Thread Arjen Hiemstra
ahiemstra requested changes to this revision.
ahiemstra added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> atticaprovider.cpp:283
> +}
> +if(filterAcceptsDownloads && checker.filterAccepts(content.tags())) {
> +mCachedContent.insert(content.id(), content);

Wouldn't it be simpler to first check if the content should be considered at 
all, and then check if one of the downloads is valid? So something like:

  if(!checker.filterAccepts(content.tags()) {
continue
  }
  
  foreach download {
   if(downloadsChecker.filterAccepts(download.tags) {
 add download
 break
   }
  }

> engine.cpp:131
>  
> +d->tagFilter = group.readEntry("TagFilter").split(QStringLiteral(","));
> +if(d->tagFilter.length() == 1 && d->tagFilter.at(0).isEmpty()) {

Considering KConfig has methods to read and write lists, wouldn't it be better 
to use those?

> engine.h:190
> + * out entries marked as ghns_exclude=1. To retain this when setting a 
> custom
> + * filter, add "ghns_exclude!=1" as one of the filters.
> + *

Would it make sense to add an `addTagFilter` method or similar for convenience 
that appends instead of replaces? Or maybe make the setter a bit more 
intelligent so it is harder to forget adding the `ghns_exclude!=1` filter? 
Right now, if I need to do anything with the tag filters, I will constantly 
need to remember to add the ghns_exclude filter.

Maybe even completely separate the ghns_exclude filter from this setter and 
make a separate "also show excluded content" switch that removes the filter?

> engine.h:227
> + */
> +void setTagFilter(QStringList filter);
> +/**

`const QStringList&`

(Also applies to setDownloadTagFilter below)

> entryinternal.cpp:165
> +
> +void KNSCore::EntryInternal::setTags(const QStringList& tags)
> +{

Nitpick, but this doesn't match with the coding style of the declaration 
(`const QStringList& tags`  vs `const QStringList `)

> tagsfilterchecker.cpp:43
> +public:
> +Validator(const QString& tag, const QString value) {
> +m_tag = tag;

I think you mean `const QString& value` ?

> tagsfilterchecker.cpp:98
> +QString value = filter.mid(tag.length() + 2);
> +Validator* val = validators.value(tag, nullptr);
> +if(val)

Wouldn't this be simpler?

  if(!validators.contains(tag)) {
validators[tag] = new EqualityValidator(tag, "");
  }
  validators.value(tag)->m_acceptedValues << value;

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra
Cc: ahiemstra, kde-frameworks-devel, #knewstuff, michaelh, ZrenBot, ngraham, 
bruns


D6513: Add support for Attica tags support

2018-07-12 Thread Dan Leinir Turthra Jensen
leinir added a comment.


  Pingaling, it would be super handy if we could get this merged soon :) It's 
needed to support the moderation happening on the KDE Store, and also needed 
for better handling of specific types of content like e.g. comic books in 
Peruse. Thanks! :D

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, apol, #kde_store, whiting
Cc: kde-frameworks-devel, #knewstuff, michaelh, ZrenBot, ngraham, bruns


D6513: Add support for Attica tags support

2018-07-02 Thread Dan Leinir Turthra Jensen
leinir added a comment.


  Test tool using the default settings, and with 
QT__LOGGING_RULES="org.kde.knewstuff.*=true"
  F5998853: image.png 

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, apol, #kde_store, whiting
Cc: kde-frameworks-devel, #knewstuff, michaelh, ZrenBot, ngraham, bruns


D6513: Add support for Attica tags support

2018-07-02 Thread Dan Leinir Turthra Jensen
leinir updated this revision to Diff 37047.
leinir edited the summary of this revision.
leinir edited the test plan for this revision.
leinir added a comment.
Restricted Application edited subscribers, added: kde-frameworks-devel; 
removed: Frameworks.


  - add test tool
  - add support for filtering by tags on both base entry and download items 
contained within that entry

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6513?vs=16201=37047

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

AFFECTED FILES
  autotests/knewstuffentrytest.cpp
  src/attica/atticaprovider.cpp
  src/core/CMakeLists.txt
  src/core/engine.cpp
  src/core/engine.h
  src/core/entryinternal.cpp
  src/core/entryinternal.h
  src/core/provider.h
  src/core/tagsfilterchecker.cpp
  src/core/tagsfilterchecker.h
  src/staticxml/staticxmlprovider.cpp
  tests/CMakeLists.txt
  tests/khotnewstuff_test-ui/main.qml
  tests/khotnewstuff_test.cpp
  tests/khotnewstuff_test.h
  tests/khotnewstuff_test.knsrc.in
  tests/knewstuff2_test.cpp
  tests/knewstuff2_test.h
  tests/knewstuff2_test.knsrc
  tests/testdata/entry.xml
  tests/testdata/provider.xml

To: leinir, #knewstuff, apol, #kde_store, whiting
Cc: kde-frameworks-devel, #knewstuff, michaelh, ZrenBot, ngraham, bruns, 
#frameworks


D6513: Add support for Attica tags support

2017-07-05 Thread Dan Leinir Turthra Jensen
leinir created this revision.
leinir added a project: KNewStuff.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.

REVISION SUMMARY
  This is to add support for the new tags support in Attica found in 
https://phabricator.kde.org/D6512, which is based on the proposal in 
https://phabricator.kde.org/T6133 to add tags support in the next version of 
OCS.
  
  As it depends on https://phabricator.kde.org/D6512, this patch should 
consequently not be merged before that one is.

REPOSITORY
  R304 KNewStuff

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

AFFECTED FILES
  src/attica/atticaprovider.cpp
  src/core/entryinternal.cpp
  src/core/entryinternal.h

To: leinir, #knewstuff, apol, #kde_store, whiting
Cc: #frameworks, #knewstuff, ZrenBot