[Differential] [Accepted] D4226: [KNS] Take into account the distribution type

[Dan Leinir Turthra Jensen]

leinir accepted this revision.
leinir added a comment.
This revision is now accepted and ready to land.


  Yup, this is fairly clearly what this is really supposed to be doing...

REPOSITORY
  R304 KNewStuff

BRANCH
  master

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: apol, #frameworks, whiting, leinir

[Differential] [Accepted] D4118: if is not an archive, always copy

[Dan Leinir Turthra Jensen]

leinir accepted this revision.
leinir added a comment.
This revision is now accepted and ready to land.


  Right. I think we can sensibly pick this. I would like a comment added, 
though, that future "don't simply install things" logic should go into the 
section above this one, rather than into the if statement there... but, other 
than that, yes, the logic itself is good, let's go for it :)

REPOSITORY
  R304 KNewStuff

BRANCH
  phab/isarchive

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: mart, #plasma, leinir
Cc: plasma-devel, #frameworks, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas

[Differential] [Accepted] D4156: sort alphabetically category list

[Dan Leinir Turthra Jensen]

leinir accepted this revision.
leinir added a comment.
This revision is now accepted and ready to land.


  LGTM :)

REPOSITORY
  R304 KNewStuff

BRANCH
  phab/sort

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: mart, #plasma, leinir
Cc: broulik, plasma-devel, #frameworks, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas

[Differential] [Accepted] D4065: Don't ask if we're getting the file in /tmp

[Dan Leinir Turthra Jensen]

leinir accepted this revision.
leinir added a comment.
This revision is now accepted and ready to land.


  Go for it

REPOSITORY
  R304 KNewStuff

BRANCH
  master

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: apol, #frameworks, leinir

Re: Review Request 129298: KPackage dependencies support

[Dan Leinir Turthra Jensen]

> On Nov. 17, 2016, 5:06 p.m., Marco Martin wrote:
> > like it, still have some issues with the 
> > my concern is that an id by itself is not really identifying information 
> > enough (would in this case for instance colorschemes.knsrc in the url 
> > identify something on the server which can be used to check the item of id 
> > 1159726 that has been downloaded is actually the needed color scheme or is 
> > just description?)
> 
> Aleix Pol Gonzalez wrote:
> This is a review for the other patch... ;)
> 
> To see what information we have ATM, you can look at 
> `.local/share/knewstuff3/colorschemes.knsregistry` although it won't be 
> trivial to make OCS searches on most of them. IMHO, the way to improve it 
> would be to provide unique appstreamid's from OCS/kde-store.
> 
> Aleix Pol Gonzalez wrote:
> Another weird complication, the provider id is weird as f*k: 
> `https://api.kde-look.org/ocs/v1/`
> 
> I'd suggest to get this patch in with the appstream/pk variant on 
> frameworksintegration and then approach kns specifically.

Hm... i would have to agree with that idea... No reason to block a genuinely 
useful thing because one of the optional sub-parts is being problematic :)


- Dan Leinir Turthra


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129298/#review100913
---


On Nov. 17, 2016, 4 p.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129298/
> -------
> 
> (Updated Nov. 17, 2016, 4 p.m.)
> 
> 
> Review request for KDE Frameworks, Plasma, Dan Leinir Turthra Jensen, and 
> Marco Martin.
> 
> 
> Repository: kpackage
> 
> 
> Description
> ---
> 
> Makes it possible to specify components to be installed together with a 
> KPackage. They will be specified by a url, we'll have handlers for any kind, 
> making reasonably extensible and doesn't tie us to a technology.
> 
> See RR in frameworksintegration for kns and appstream: 
> https://git.reviewboard.kde.org/r/129419/
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt 395b16e 
>   autotests/data/testpackagesdep/contents/ui/main.qml PRE-CREATION 
>   autotests/data/testpackagesdep/metadata.json PRE-CREATION 
>   autotests/data/testpackagesdep/testpackagesdep.testappdataxml PRE-CREATION 
>   src/kpackage/config-package.h.cmake 61f2f0c 
>   src/kpackage/private/packagejobthread.cpp 0a0cc01 
>   src/kpackage/private/packagejobthread_p.h 1babf0b 
> 
> Diff: https://git.reviewboard.kde.org/r/129298/diff/
> 
> 
> Testing
> ---
> 
> None. just builds. It's a proof of concept, not even the test I added was 
> tested, it was just to display what it could look like.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

Re: Review Request 129298: RFC: supporting dependencies on KPackage

[Dan Leinir Turthra Jensen]

> On Oct. 31, 2016, 5:19 p.m., Marco Martin wrote:
> > autotests/data/testpackagesdep/metadata.json, line 14
> > <https://git.reviewboard.kde.org/r/129298/diff/1/?file=483564#file483564line14>
> >
> > if kns ends up using ids, maybe the server should be specified as well, 
> > as the id would be server-dependent?
> 
> Aleix Pol Gonzalez wrote:
> I'm not sure, either way we need changes in the KNS API, as the current 
> search in place won't work. I'd prefer if we could use rdn-like notation on 
> kns.
> 
> I don't think it should be server-dependent. If anything, if the user 
> changes the contents server, it might not find the component.

Hmm... a knsrc points to a providers file, which in turn can hold more than one 
provider. The providers in the provider file have an ID, so perhaps we can use 
that? So we'd end up with e.g. kns://colors.knsrc/api.kde-look.org/1159726 
instead. While the api bit looks like a server address, it's just the ID as 
found in the providers file (and might be any string, technically), and so that 
would be enough (just) to uniquely identify the item as found on a provider 
which (like with the kde store) might have multiple "servers".


- Dan Leinir Turthra


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129298/#review100442
---


On Oct. 31, 2016, 5:09 p.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129298/
> ---
> 
> (Updated Oct. 31, 2016, 5:09 p.m.)
> 
> 
> Review request for KDE Frameworks, Plasma, Dan Leinir Turthra Jensen, and 
> Marco Martin.
> 
> 
> Repository: kpackage
> 
> 
> Description
> ---
> 
> Makes it possible to specify components to be installed together with a 
> KPackage. They will be specified by a url, we'll have handlers for any kind, 
> making reasonably extensible and doesn't tie us to a technology.
> 
> In this repository I created two Proof of Concept of such handlers, one for 
> the appstream urls and one for KNS: kde:scratch/apol/kpackage-install-helpers
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt 395b16e 
>   autotests/data/testpackagesdep/contents/ui/main.qml PRE-CREATION 
>   autotests/data/testpackagesdep/metadata.json PRE-CREATION 
>   autotests/data/testpackagesdep/testpackagesdep.testappdataxml PRE-CREATION 
>   src/kpackage/config-package.h.cmake 61f2f0c 
>   src/kpackage/private/packagejobthread.cpp 0a0cc01 
>   src/kpackage/private/packagejobthread_p.h 1babf0b 
> 
> Diff: https://git.reviewboard.kde.org/r/129298/diff/
> 
> 
> Testing
> ---
> 
> None. just builds. It's a proof of concept, not even the test I added was 
> tested, it was just to display what it could look like.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

Re: Review Request 129298: RFC: supporting dependencies on KPackage

[Dan Leinir Turthra Jensen]

> On Oct. 31, 2016, 5:19 p.m., Marco Martin wrote:
> > autotests/data/testpackagesdep/metadata.json, line 14
> > <https://git.reviewboard.kde.org/r/129298/diff/1/?file=483564#file483564line14>
> >
> > if kns ends up using ids, maybe the server should be specified as well, 
> > as the id would be server-dependent?
> 
> Aleix Pol Gonzalez wrote:
> I'm not sure, either way we need changes in the KNS API, as the current 
> search in place won't work. I'd prefer if we could use rdn-like notation on 
> kns.
> 
> I don't think it should be server-dependent. If anything, if the user 
> changes the contents server, it might not find the component.
> 
> Dan Leinir Turthra Jensen wrote:
> Hmm... a knsrc points to a providers file, which in turn can hold more 
> than one provider. The providers in the provider file have an ID, so perhaps 
> we can use that? So we'd end up with e.g. 
> kns://colors.knsrc/api.kde-look.org/1159726 instead. While the api bit 
> looks like a server address, it's just the ID as found in the providers file 
> (and might be any string, technically), and so that would be enough (just) to 
> uniquely identify the item as found on a provider which (like with the kde 
> store) might have multiple "servers".
> 
> Marco Martin wrote:
> could a content be identified like org.joe.beautifulicontheme ?
> then the server having some function to resolve 
> org.joe.beautifulicontheme to its id like 137345

...what server? That is just another string ID (technically the number that you 
get in OCS is just a string, and at least one implementation (MidGard) uses 
that fact). i really don't see how we can get away with having anything less 
than two string IDs (server ID as defined in a providers.xml, and content ID as 
unique to that provider) to uniquely identify a content item... i also don't 
really see why it'd necessarily be better, in any real way other than 
aesthetics of the link, to have anything more concise than 
kns://knsrcfile/providerID/contentID (and possibly kns://providerID/contentID 
in cases of using the default provider as defined by kns internally, which 
resolves to using KDE's providers.xml).


- Dan Leinir Turthra


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129298/#review100442
---


On Oct. 31, 2016, 5:09 p.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129298/
> ---------------
> 
> (Updated Oct. 31, 2016, 5:09 p.m.)
> 
> 
> Review request for KDE Frameworks, Plasma, Dan Leinir Turthra Jensen, and 
> Marco Martin.
> 
> 
> Repository: kpackage
> 
> 
> Description
> ---
> 
> Makes it possible to specify components to be installed together with a 
> KPackage. They will be specified by a url, we'll have handlers for any kind, 
> making reasonably extensible and doesn't tie us to a technology.
> 
> In this repository I created two Proof of Concept of such handlers, one for 
> the appstream urls and one for KNS: kde:scratch/apol/kpackage-install-helpers
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt 395b16e 
>   autotests/data/testpackagesdep/contents/ui/main.qml PRE-CREATION 
>   autotests/data/testpackagesdep/metadata.json PRE-CREATION 
>   autotests/data/testpackagesdep/testpackagesdep.testappdataxml PRE-CREATION 
>   src/kpackage/config-package.h.cmake 61f2f0c 
>   src/kpackage/private/packagejobthread.cpp 0a0cc01 
>   src/kpackage/private/packagejobthread_p.h 1babf0b 
> 
> Diff: https://git.reviewboard.kde.org/r/129298/diff/
> 
> 
> Testing
> ---
> 
> None. just builds. It's a proof of concept, not even the test I added was 
> tested, it was just to display what it could look like.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

Re: Review Request 129642: Introduce org.kde.kconfig QML import with KAuthorized

[Dan Leinir Turthra Jensen]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129642/#review101438
---


Ship it!




Ship It!

- Dan Leinir Turthra Jensen


On Dec. 12, 2016, 3:11 p.m., Kai Uwe Broulik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129642/
> ---
> 
> (Updated Dec. 12, 2016, 3:11 p.m.)
> 
> 
> Review request for KDE Frameworks, Plasma and Dan Leinir Turthra Jensen.
> 
> 
> Repository: kdeclarative
> 
> 
> Description
> ---
> 
> This allows to query KAuthorized restrictions declaratively.
> 
> 
> Diffs
> -
> 
>   src/qmlcontrols/CMakeLists.txt 8a506df 
>   src/qmlcontrols/kconfig/CMakeLists.txt PRE-CREATION 
>   src/qmlcontrols/kconfig/kauthorizedproxy.h PRE-CREATION 
>   src/qmlcontrols/kconfig/kauthorizedproxy.cpp PRE-CREATION 
>   src/qmlcontrols/kconfig/kconfigplugin.h PRE-CREATION 
>   src/qmlcontrols/kconfig/kconfigplugin.cpp PRE-CREATION 
>   src/qmlcontrols/kconfig/qmldir PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/129642/diff/
> 
> 
> Testing
> ---
> 
> import org.kde.kconfig 1.0
> 
> Button {
> visible: KAuthorized.authorize("ghns")
> }
> 
> 
> Thanks,
> 
> Kai Uwe Broulik
> 
>

Re: Review Request 129654: Introduce the resource name in the knsrc file

[Dan Leinir Turthra Jensen]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129654/#review101473
---


Ship it!




Ship It!

- Dan Leinir Turthra Jensen


On Dec. 15, 2016, 4:05 p.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129654/
> ---
> 
> (Updated Dec. 15, 2016, 4:05 p.m.)
> 
> 
> Review request for KDE Frameworks and Jeremy Whiting.
> 
> 
> Repository: knewstuff
> 
> 
> Description
> ---
> 
> Discover has been changed to display all installed `knsrc` files in the 
> system instead of having a static list in the system. For that to happen I'd 
> like to include in the `knsrc` file a title that describes the contained 
> resources.
> 
> This should be accompanied with the following change to scripty to enable 
> translations of the field:
> ```
> Index: findfiles
> ===
> --- findfiles   (revision 1477134)
> +++ findfiles   (working copy)
> @@ -28,7 +28,7 @@
>  continue
>fi
>echo "$dir"
> -  find "$dir" ( -path "*/test/*" -o -path "*/tests/*" -o -path 
> "*/autotest/*" -o -path "*/autotests/*" ) -prune -o ( ( -name *.directory -o 
> -name *.desktop -o -name *.desktop.cmake -o -name *.kimap -o -name *.themerc 
> -o -name *.kcsrc -o -name *.setdlg -o -name index.theme -o -name *.notifyrc 
> -o -name *.protocol -o -name *.profile -o -name *.actions ) -a ( -type f -o 
> -type l ) -print ) >> $filelist
> +  find "$dir" ( -path "*/test/*" -o -path "*/tests/*" -o -path 
> "*/autotest/*" -o -path "*/autotests/*" ) -prune -o ( ( -name *.directory -o 
> -name *.desktop -o -name *.knsrc -o -name *.desktop.cmake -o -name *.kimap -o 
> -name *.themerc -o -name *.kcsrc -o -name *.setdlg -o -name index.theme -o 
> -name *.notifyrc -o -name *.protocol -o -name *.profile -o -name *.actions ) 
> -a ( -type f -o -type l ) -print ) >> $filelist
>extradesktopscripts=`find $dir -name ExtraDesktop.sh`
>initialdir=`pwd`
>for extradesktopscript in $extradesktopscripts; do
> ```
> 
> 
> Diffs
> -
> 
>   src/downloaddialog.cpp 5831211 
> 
> Diff: https://git.reviewboard.kde.org/r/129654/diff/
> 
> 
> Testing
> ---
> 
> The `Name` field is used when available, also adopted it in Discover.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

[Differential] [Accepted] D4121: add support for display_name in categories

[Dan Leinir Turthra Jensen]

leinir accepted this revision.
leinir added a comment.
This revision is now accepted and ready to land.


  This being an extension of ocs, we will need to prepare a proposal of this at 
some point in the not too far distant future. However, as other things (tags 
etc) are also going to be required, perhaps that could all be done at the same 
time. OCS being implementation-driven, and attica/ocs-server being the 
reference implementations, i did think that we might be venturing into 
embrace/extend territory here, however it is more akin to how OCS has always 
been developed (just more in the public eye now, rather than behind closed 
doors). So, yup, let's do this.

REPOSITORY
  R235 Attica

BRANCH
  phab/displayname

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: mart, #plasma, leinir
Cc: plasma-devel, #frameworks, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas

[Differential] [Accepted] D4120: add support for display_name in categories

[Dan Leinir Turthra Jensen]

leinir accepted this revision.
leinir added a comment.
This revision is now accepted and ready to land.


  We will need to consider the implications of adding functionality into ocs, 
but technically this would be fine.

REPOSITORY
  R304 KNewStuff

BRANCH
  phab/displayname

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: mart, #plasma, leinir
Cc: plasma-devel, #frameworks, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas

[Differential] [Accepted] D4048: Support some of the KNSCore questions using notifications

[Dan Leinir Turthra Jensen]

leinir accepted this revision.
leinir added a comment.
This revision is now accepted and ready to land.


  That works, yup :)

REPOSITORY
  R252 Framework Integration

BRANCH
  master

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: apol, #frameworks, mart, leinir

Re: Review Request 129628: knewstuff - install missing headers

[Dan Leinir Turthra Jensen]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129628/#review101508
---



i'm afraid this patch fixes the wrong problem - but thank you very much for 
pointing out it existed! i've finally got around to sorting it, and things 
should now be working much more as they are supposed to if you do a pull from 
master.

- Dan Leinir Turthra Jensen


On Dec. 9, 2016, 1:28 a.m., José Manuel  Santamaría Lema wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129628/
> ---
> 
> (Updated Dec. 9, 2016, 1:28 a.m.)
> 
> 
> Review request for KDE Frameworks and Dan Leinir Turthra Jensen.
> 
> 
> Repository: knewstuff
> 
> 
> Description
> ---
> 
> This patch installs a couple of missing headers:
> 
> * knewstuffcore_export.h - which is #include'd from many files in 
> /include/KF5/KNewStuff3/knscore/
> * entry.h - this is #include'd from 
> /usr/include/KF5/KNewStuff3/knscore/entryinternal.h as "../entry.h"
> 
> If the "entry.h" part of the patch is fine, maybe this commit should be 
> reverted:
> https://cgit.kde.org/knewstuff.git/commit/?id=7a644d7147edb816b69fa973630cea6aa72bc3ef
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 79ef39b 
>   src/core/CMakeLists.txt c2a1a24 
> 
> Diff: https://git.reviewboard.kde.org/r/129628/diff/
> 
> 
> Testing
> ---
> 
> I just ran a test from the kubuntu package meant to check that all the needed 
> headers are available.
> 
> 
> Thanks,
> 
> José Manuel  Santamaría Lema
> 
>

Re: Review Request 129660: Fix set up of the KNSCore::Engine when created with an absolute config file path

[Dan Leinir Turthra Jensen]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129660/#review101475
---


Ship it!




Ship It!

- Dan Leinir Turthra Jensen


On Dec. 16, 2016, 4:56 p.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129660/
> ---
> 
> (Updated Dec. 16, 2016, 4:56 p.m.)
> 
> 
> Review request for KDE Frameworks and Jeremy Whiting.
> 
> 
> Repository: knewstuff
> 
> 
> Description
> ---
> 
> `QStrandardPaths::locate` returns `{}` when an absolute path is passed so we 
> can't use it to extract the file's baseName.
> Also simplifies a bit the logic that was relying on a class attribute that 
> was only used in that function and we turned into a local variable. It was 
> adding a `':'` that doesn't make sense anymore and I dropped as well.
> 
> 
> Diffs
> -
> 
>   src/core/engine.h e69dee3 
>   src/core/engine.cpp a5758f0 
> 
> Diff: https://git.reviewboard.kde.org/r/129660/diff/
> 
> 
> Testing
> ---
> 
> Tests still pass, doesn't choke anymore on a full path (tested from Discover).
> Kate, KDevelop still work (listing of installed resources etc)
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

D5402: always close the downloaded file after downloading

[Dan Leinir Turthra Jensen]

leinir accepted this revision.
leinir added a comment.
This revision is now accepted and ready to land.


  Yes, good catch. I am unsure of why that happened, but i quite agree, that 
certainly needs to happen whether or not a redirection has been involved.

REPOSITORY
  R304 KNewStuff

BRANCH
  phab/closeFile

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

To: mart, #plasma, leinir, bshah
Cc: plasma-devel, #frameworks, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D5003: Update docs to Frameworks API

[Dan Leinir Turthra Jensen]

leinir requested changes to this revision.
leinir added a comment.
This revision now requires changes to proceed.


  Thinking this is... well under way, with a couple of details :)

INLINE COMMENTS

> kauthactionreply.h:338
> -*
> -* In case of errors coming from the library, the type() is
> -* ActionReply::KAuthError. In this case, errorCode() will always be one of 
> the

Thinking it would be kind of good to translate this into something more useful 
for the Action::AuthStatus enum documentation... Just removing it entirely 
seems to me to get rid of the verbose "don't use this for your own errors, this 
is for authentication only, you should look over here" thing which is inherent 
in the current text... Perhaps simply adding @see ExecuteJob::newData to that 
enum's documentation might do the trick?

> kauthexecutejob.h:82
>  /**
> - * @returns the data sent by the helper
> + * @returns the data returned by the helper
>   */

Returned isn't semantically right, here, though the term "helper" is a bit 
ambiguous... What is meant by "sent by the helper" is more to the point "send 
by the helper using HelperSupport::progressStep(QVariant)"... which is somewhat 
clunky, but explains more properly what is going on here. Return would suggest 
that it is only done at the end, whereas this can, specifically, be done 
multiple times (basically, every time progressStep(QVariant) is called). So... 
perhaps changing this to be something like

  /**
   * Use this to get the data set by HelperSupport::progressStep(QVariant)
   * This function is particularly useful once the
   * job has completed. During execution, simply
   * read the data in the newData signal.
   * @see ExecuteJob::newData
   * returns the data sent by the helper
   */

REPOSITORY
  R283 KAuth

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

To: jriddell, leinir
Cc: #frameworks

D5003: Update docs to Frameworks API

[Dan Leinir Turthra Jensen]

leinir accepted this revision.
leinir added a comment.
This revision is now accepted and ready to land.


  That looks about right to me! :)

REPOSITORY
  R283 KAuth

BRANCH
  docs

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

To: jriddell, leinir
Cc: #frameworks

D5001: Add support for killing a KAuth::ExecuteJob

[Dan Leinir Turthra Jensen]

leinir closed this revision.
leinir added a comment.


  Closed by commit 
https://commits.kde.org/kauth/39621f485f434fb4453a1fd6af2796cde23eec53

REPOSITORY
  R283 KAuth

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

To: leinir, #frameworks, jriddell
Cc: jriddell

D5001: Add support for killing a KAuth::ExecuteJob

[Dan Leinir Turthra Jensen]

leinir created this revision.
Restricted Application added a project: Frameworks.

REVISION SUMMARY
  This implements the standard method for stopping a KJob in KAuth::ExecuteJob. 
It is done using a minimal-intrusion approach, and is supposed to feel familiar 
to people already used to working with KJobs.

TEST PLAN
  Tested using an existing kcm which assumed the existence of this slot in 
ExecuteJob, and the job stops as expected, and returns the appropriate signals.

REPOSITORY
  R283 KAuth

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

AFFECTED FILES
  src/kauthexecutejob.cpp
  src/kauthexecutejob.h
  src/kauthhelpersupport.h

To: leinir, #frameworks

D6492: Also use m_currentRequest when checking for installed and updates

[Dan Leinir Turthra Jensen]

leinir accepted this revision.
leinir added a comment.
This revision is now accepted and ready to land.


  That is indeed a very good point. I think we might possibly have an issue 
here, though, in that since these two searches are no longer stand-alone, if 
the filter is not explicitly reset before attempting to perform a search we 
will only search installed and updateable items. I don't think it is 
necessarily an enormous or insurmountable issue here, as far as i can tell the 
only code which currently uses these two functions is in KNS' own 
DownloadManagers (at least, lxr suggests as much), and if we ensure those two 
are fixed to reset the filter before launching a search, i think we're probably 
ok.
  
  PS: These two functions also lack documentation, which shall need fixing 
(though not necessarily as a part of this D)

REPOSITORY
  R304 KNewStuff

BRANCH
  master

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

To: apol, #frameworks, leinir

D6492: Also use m_currentRequest when checking for installed and updates

[Dan Leinir Turthra Jensen]

leinir accepted this revision.
leinir added a comment.
This revision is now accepted and ready to land.


  Different approach, but yeah, telling people what'll actually happen works :)

REPOSITORY
  R304 KNewStuff

BRANCH
  master

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

To: apol, #frameworks, leinir

D6492: Also use m_currentRequest when checking for installed and updates

[Dan Leinir Turthra Jensen]

leinir requested changes to this revision.
leinir added a comment.
This revision now requires changes to proceed.


  derp, not accepted, my bad...

REPOSITORY
  R304 KNewStuff

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

To: apol, #frameworks, leinir

D5902: Expand KNewStuff documentation

[Dan Leinir Turthra Jensen]

leinir added inline comments.

INLINE COMMENTS

> apol wrote in README.md:35
> Still it shouldn't be looked-for, should it?

It still need to be found before you can set it as runtime (unless i am 
fundamentally misunderstanding something of course) - like how it's done in 
Discover... or am i misunderstanding the question?

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff
Cc: apol, #frameworks

D6512: Add support for proposed tags addition in OCS 1.7

[Dan Leinir Turthra Jensen]

leinir created this revision.
leinir added a project: KDE Store.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.

REVISION SUMMARY
  This patch is to add support for the proposed addition of tags to the OCS 
standard version 1.7, as seen in https://phabricator.kde.org/T6133
  
  It is supposed to:
  
  - handle the addition of tags to content and download items
  - not fail if they are not available (just returning an empty list)

REPOSITORY
  R235 Attica

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

AFFECTED FILES
  src/content.cpp
  src/content.h
  src/contentparser.cpp
  src/downloaddescription.cpp
  src/downloaddescription.h
  src/parser.cpp

To: leinir, #knewstuff, apol, whiting, #kde_store
Cc: #kde_store, #frameworks, ZrenBot, akiraohgaki, alexanderschmidt, 
siyuandong, ronaldv, mikesomov, starbuck, sebas

D6513: Add support for Attica tags support

[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

D6512: Add support for proposed tags addition in OCS 1.7

[Dan Leinir Turthra Jensen]

leinir added a dependent revision: D6513: Add support for Attica tags support.

REPOSITORY
  R235 Attica

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

To: leinir, #knewstuff, apol, whiting, #kde_store
Cc: #kde_store, #frameworks, ZrenBot, akiraohgaki, alexanderschmidt, 
siyuandong, ronaldv, mikesomov, starbuck, sebas

D6512: Add support for proposed tags addition in OCS 1.7

[Dan Leinir Turthra Jensen]

leinir added a comment.


  In https://phabricator.kde.org/D6512#121834, @apol wrote:
  
  > +1 looks sensible to me.
  
  
  Sweet :)
  
  > What's the OCS state in this regard? When will 1.7 be a thing?
  
  OCS 1.7 will be a thing hopefully in the not too distant future... 
Incidentally, i need to regain write access to the fdo wiki, where said 
standard is hosted, as i lost that when they changed all the access methods 
in... some long time ago.

REPOSITORY
  R235 Attica

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

To: leinir, #knewstuff, apol, whiting, #kde_store
Cc: #kde_store, #frameworks, ZrenBot, akiraohgaki, alexanderschmidt, 
siyuandong, ronaldv, mikesomov, starbuck, sebas

D6532: When requesting from the cache, report all entries at bulk

[Dan Leinir Turthra Jensen]

leinir accepted this revision.
leinir added a comment.
This revision is now accepted and ready to land.


  Hmm... i was thinking this //could// cause issues, i don't see anywhere 
assumptions are made about page size on the consumption side (damn we're lucky 
this whole split thing's so new ;) ). Nothing in the API suggests you might get 
more than the requested number when you ask for entries, but conversely nothing 
says you will potentially get less, and that happens on a regular basis, so... 
sure, why not :)

REPOSITORY
  R304 KNewStuff

BRANCH
  master

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

To: apol, #frameworks, whiting, leinir

D5587: Improvements

[Dan Leinir Turthra Jensen]

leinir accepted this revision.
leinir added a comment.
This revision is now accepted and ready to land.


  Looks good to me :)

REPOSITORY
  R304 KNewStuff

BRANCH
  master

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

To: apol, #frameworks, leinir

D5639: Internal cache for provider data on initialisation

[Dan Leinir Turthra Jensen]

leinir created this revision.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.

REVISION SUMMARY
  This is a two layer approach to creating less network traffic when 
initialising a KNSCore::Engine:
  
  - Firstly, cache the xml documents themselves
  - Secondly, share XML fetch jobs when many are created at the same time for 
the same URL
  
  The result of this patch in a single fetch per provider url per application 
launch, and in connection with https://phabricator.kde.org/D5638 we will fetch 
the data from the network only once, until the cache is invalidated, all in all 
resulting in much less traffic and less hammering of the servers.
  
  CCBUG: 379193

TEST PLAN
  Using Discover, this results in much less network traffic during startup and 
engine initialisations.

REPOSITORY
  R304 KNewStuff

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

AFFECTED FILES
  src/core/engine.cpp

To: leinir, whiting, apol
Cc: #frameworks

D5638: Use a single QNAM (and a disk cache) for HTTP jobs

[Dan Leinir Turthra Jensen]

leinir created this revision.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.

REVISION SUMMARY
  Use a single QNetworkAccessManager instance for all our HTTP jobs, and also 
add a simple diskcache to that qnam. Further ensure there is only a single qnam 
for the entire application using kns' http jobs, across all threads (lock when 
accessing the qnam). Without this, we are liable to end up creating and 
destroying a great many qnam instances, which certainly is something to try and 
avoid.
  
  CCBUG: 379193

TEST PLAN
  Using Discover, which creates a great many of these jobs, we have much less 
network traffic this way.

REPOSITORY
  R304 KNewStuff

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

AFFECTED FILES
  src/core/jobs/httpworker.cpp

To: leinir, whiting, apol
Cc: #frameworks

D7194: Detach before setting the d pointer

[Dan Leinir Turthra Jensen]

leinir accepted this revision.
leinir added a comment.
This revision is now accepted and ready to land.


  Ah, yes, good catch :)

REPOSITORY
  R304 KNewStuff

BRANCH
  detach

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

To: apol, leinir
Cc: broulik, #frameworks

D7190: Don't complain the knsregistry file is not present before it's useful

[Dan Leinir Turthra Jensen]

leinir accepted this revision.
leinir added a comment.
This revision is now accepted and ready to land.


  Good call, yes - gets rid of a lump of irrelevant and technically correct but 
subjectively incorrect information.

REPOSITORY
  R304 KNewStuff

BRANCH
  master

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

To: apol, leinir
Cc: #frameworks

D5811: Improve error notification

[Dan Leinir Turthra Jensen]

leinir accepted this revision.
leinir added a comment.
This revision is now accepted and ready to land.


  lgtm :)

REPOSITORY
  R235 Attica

BRANCH
  master

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

To: apol, #frameworks, leinir

D5638: Use a single QNAM (and a disk cache) for HTTP jobs

[Dan Leinir Turthra Jensen]

This revision was automatically updated to reflect the committed changes.
Closed by commit R304:b8d0bc8818ff: Use a single QNAM (and a disk cache) for 
HTTP jobs (authored by leinir).

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5638?vs=14244=14404

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

AFFECTED FILES
  src/core/jobs/httpworker.cpp
  src/core/jobs/httpworker.h

To: leinir, whiting, apol, dfaure
Cc: dfaure, #frameworks

D5639: Internal cache for provider data on initialisation

[Dan Leinir Turthra Jensen]

This revision was automatically updated to reflect the committed changes.
Closed by commit R304:6207a87b71d7: Internal cache for provider data on 
initialisation (authored by leinir).

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5639?vs=14243=14403

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

AFFECTED FILES
  src/core/engine.cpp
  src/core/jobs/httpjob.cpp

To: leinir, whiting, apol, dfaure
Cc: dfaure, #frameworks

D5902: Expand KNewStuff documentation

[Dan Leinir Turthra Jensen]

leinir updated this revision to Diff 14660.
leinir marked 2 inline comments as done.

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5902?vs=14634=14660

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

AFFECTED FILES
  README.md
  src/core/engine.h
  src/qtquick/downloadlinkinfo.h
  src/qtquick/quickengine.h
  src/qtquick/quickitemsmodel.h
  src/upload/atticahelper_p.h

To: leinir, #knewstuff
Cc: apol, #frameworks

D5902: Expand KNewStuff documentation

[Dan Leinir Turthra Jensen]

leinir marked 2 inline comments as done.
leinir added inline comments.

INLINE COMMENTS

> apol wrote in README.md:30
> I'd say that it looks better to have `CONFIG` rather than `NO_MODULE`. Both 
> should work.
> 
> Also `find_package(KF5 COMPONENTS NewStuffCore)` reads better to my eye.

It certainly does - i just expanded what was already there, but yup, needs a 
touch of work there. Also, since KNewStuffQuick is a Qt Quick module, it should 
really be marked as runtime when looking for it, so i'll fix that while i'm at 
it :)

> apol wrote in README.md:35
> Do you ever need to link against `NewStuffQuick`?

No, you are quite right - i don't think it is even technically possible in most 
cases, so this will need some reworking to explain that (it's a Qt Quick 
module, after all)

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff
Cc: apol, #frameworks

D5902: Expand KNewStuff documentation

[Dan Leinir Turthra Jensen]

leinir created this revision.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.

REVISION SUMMARY
  Add mention of KNewStuffCore and KNewStuffQuick to the base documentation, 
and expand the API documentation to mention some of the new functionality, as 
well as add some documentation for things which currently lack it. Prior to 
this, the documentation had no mention of the way the framework is now 
logically split up.
  
  This deficiency was brought to my attention during a chat with a GSoC 
student, who was otherwise in the process of disregarding KNewStuff and headed 
towards reimplementing large parts of KNewStuffCore (as the documentation makes 
no suggestion this is now a thing that can be used on its own).

REPOSITORY
  R304 KNewStuff

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

AFFECTED FILES
  README.md
  src/core/engine.h
  src/qtquick/downloadlinkinfo.h
  src/qtquick/quickengine.h
  src/qtquick/quickitemsmodel.h
  src/upload/atticahelper_p.h

To: leinir
Cc: #frameworks

D5902: Expand KNewStuff documentation

[Dan Leinir Turthra Jensen]

leinir added a reviewer: KNewStuff.

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff
Cc: #frameworks

D6190: Expose and use Engine's page size variable

[Dan Leinir Turthra Jensen]

leinir updated this revision to Diff 15377.
leinir added a comment.


  Don't const & an int, that's just silly.

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6190?vs=15373=15377

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

AFFECTED FILES
  src/core/engine.cpp

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

D6190: Expose and use Engine's page size variable

[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
  Engine has a pageSize variable which has been mostly unused, but which comes 
in very handy when getting large sets of data. This patch exposes that variable 
through a setter, and makes use of it internally whenever a request is made (so 
the page size is actually honoured the way it seems to have been intended)

REPOSITORY
  R304 KNewStuff

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

AFFECTED FILES
  src/core/engine.cpp

To: leinir, #knewstuff
Cc: #frameworks, ZrenBot

D6190: Expose and use Engine's page size variable

[Dan Leinir Turthra Jensen]

leinir added a comment.


  As far as i can gather, it was simply never added because, well, it was never 
used in a lot of places... This really is more a case of equalising some 
features between requestData and other parts of the engine (so they can all be 
paginated by the size they really want to be).

INLINE COMMENTS

> apol wrote in engine.cpp:144
> I wouldn't pass an int as `const&`

No, quite true - force of habit and all that :)

REPOSITORY
  R304 KNewStuff

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

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

D6340: Fix incorrect error detection for missing knsrc files

[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 fixes the error detection for knsrc files, which previously would report 
missing files only if the directory (or file) was unreadable by the user, and 
incorrectly describe the error.

REPOSITORY
  R304 KNewStuff

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

AFFECTED FILES
  src/core/engine.cpp

To: leinir, #knewstuff
Cc: #frameworks, ZrenBot

D6340: Fix incorrect error detection for missing knsrc files

[Dan Leinir Turthra Jensen]

leinir marked an inline comment as done.
leinir added inline comments.

INLINE COMMENTS

> apol wrote in engine.cpp:130
> are you sure the `endl` is necessary?

i am, in fact, reasonably certain it is not required - leftovers are fun, i'll 
get rid of that :)

REPOSITORY
  R304 KNewStuff

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

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

D6340: Fix incorrect error detection for missing knsrc files

[Dan Leinir Turthra Jensen]

This revision was automatically updated to reflect the committed changes.
leinir marked an inline comment as done.
Closed by commit R304:eb2e65a882f0: Fix incorrect error detection for missing 
knsrc files (authored by leinir).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D6340?vs=15742=15781#toc

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6340?vs=15742=15781

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

AFFECTED FILES
  src/core/engine.cpp

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

D6049: Extend unittests to test stable sort.

[Dan Leinir Turthra Jensen]

leinir accepted this revision.
leinir added a comment.
This revision is now accepted and ready to land.


  Sorting correctness (and more thorough testing) is good, yes. LGTM! :)
  
  As to the missing arcconfig... will need someone to produce one of those who 
actually uses arc to fix that ;) Removing the .reviewboardrc, though, sounds 
like a good idea in general.

REPOSITORY
  R304 KNewStuff

BRANCH
  master

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

To: adridg, whiting, #frameworks, leinir
Cc: leinir, #frameworks

D6104: Use the right scope for the installpath variable

[Dan Leinir Turthra Jensen]

leinir accepted this revision.
leinir added a comment.
This revision is now accepted and ready to land.


  Personally less fond of auto than you are... But, that is just me, and this 
is a framework, and it's fine :) A massive reduction in allocations is a very 
good thing, go for it :)

REPOSITORY
  R304 KNewStuff

BRANCH
  installedFiles2

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

To: apol, #frameworks, whiting, leinir

D6067: Make it possible to use QXmlStreamReader to read a KNS registry file

[Dan Leinir Turthra Jensen]

leinir added a comment.


  On a similar note to handling comments, how does it now handle 
unknown/garbage tags? While it won't affect the cache code, it would 
potentially affect other things (ocs is not guaranteed to be perfectly formed, 
and it's one of the ways the framework's retained backwards compatibility). 
From what i can tell, this would assert when an unknown tag is encountered, 
right?

REPOSITORY
  R304 KNewStuff

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

To: apol, #frameworks
Cc: leinir, dfaure

D6190: Expose and use Engine's page size variable

[Dan Leinir Turthra Jensen]

This revision was automatically updated to reflect the committed changes.
Closed by commit R304:2f8580b9e604: Expose and use Engine's page size variable 
(authored by leinir).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D6190?vs=15377=15443#toc

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6190?vs=15377=15443

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

AFFECTED FILES
  src/core/engine.cpp
  src/core/engine.h

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

D5638: Use a single QNAM (and a disk cache) for HTTP jobs

[Dan Leinir Turthra Jensen]

leinir marked 4 inline comments as done.
leinir added inline comments.

INLINE COMMENTS

> dfaure wrote in httpworker.cpp:41
> 0.1% of the partition size is a rather arbitrary value, no? It could go from 
> something very tiny to something really big...
> 
> On my 470GB partition this would lead to a 470MB cache for knewstuff, that's 
> maybe a bit much, given the average size for knewstuff stuff? Maybe a qMin() 
> call with a maximum value would be useful?

Good point yes, it's supposed to be helpful, not take over the system. I'll set 
a max of 50 megs, should be fine for most uses.

REPOSITORY
  R304 KNewStuff

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

To: leinir, whiting, apol, dfaure
Cc: dfaure, #frameworks

D5639: Internal cache for provider data on initialisation

[Dan Leinir Turthra Jensen]

leinir updated this revision to Diff 14243.
leinir added a comment.


  Static var naming change, for consistency and whatnot

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5639?vs=14236=14243

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

AFFECTED FILES
  src/core/engine.cpp
  src/core/jobs/httpjob.cpp

To: leinir, whiting, apol
Cc: dfaure, #frameworks

D5639: Internal cache for provider data on initialisation

[Dan Leinir Turthra Jensen]

leinir added inline comments.

INLINE COMMENTS

> dfaure wrote in engine.cpp:58
> The uppercase first letter on variable name is unusual.
> 
> I personally use a s_ prefix for static vars.

Hmm. Right, there is no established way of naming them in KNS already, so while 
i am not personally so keen on underscores in my variable names, static vars 
are already sort of ugly, and so going with the "ugly things should look a bit 
ugly" that stoustrup suggests for c++ things, i kind of like that :)

REPOSITORY
  R304 KNewStuff

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

To: leinir, whiting, apol
Cc: dfaure, #frameworks

D5638: Use a single QNAM (and a disk cache) for HTTP jobs

[Dan Leinir Turthra Jensen]

leinir updated this revision to Diff 14244.
leinir added a comment.


  Work some numbers a bit

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5638?vs=14242=14244

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

AFFECTED FILES
  src/core/jobs/httpworker.cpp
  src/core/jobs/httpworker.h

To: leinir, whiting, apol, dfaure
Cc: dfaure, #frameworks

D5638: Use a single QNAM (and a disk cache) for HTTP jobs

[Dan Leinir Turthra Jensen]

leinir marked 2 inline comments as done.
leinir added inline comments.

INLINE COMMENTS

> dfaure wrote in httpworker.cpp:41
> 50 bytes? isn't that a bit small? :)

Yes, yes it is ;)

> dfaure wrote in httpworker.cpp:57
> the uppercase first letter is weird, for a variable.

Good point, yes. Not very Qt

REPOSITORY
  R304 KNewStuff

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

To: leinir, whiting, apol, dfaure
Cc: dfaure, #frameworks

D5639: Internal cache for provider data on initialisation

[Dan Leinir Turthra Jensen]

leinir marked 3 inline comments as done.
leinir added inline comments.

INLINE COMMENTS

> dfaure wrote in engine.cpp:206
> This connect (and the following) could be done outside of the if/else, so 
> avoid being repeated, no?
> Or is there a risk that load() will emit those signals immediately?

Normally yes, it would potentially return immediately... However, it makes 
sense for the get job to be postponed just slightly (it's logically an async 
thing anyway), so that is what HTTPJob now does, and we can simplify this code 
muchly.

REPOSITORY
  R304 KNewStuff

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

To: leinir, whiting, apol
Cc: dfaure, #frameworks

D5638: Use a single QNAM (and a disk cache) for HTTP jobs

[Dan Leinir Turthra Jensen]

leinir updated this revision to Diff 14235.
leinir marked an inline comment as done.
leinir added a comment.


  Some style fixes, and set a reasonable maximum size for the cache

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5638?vs=14146=14235

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

AFFECTED FILES
  src/core/jobs/httpworker.cpp
  src/core/jobs/httpworker.h

To: leinir, whiting, apol, dfaure
Cc: dfaure, #frameworks

D5639: Internal cache for provider data on initialisation

[Dan Leinir Turthra Jensen]

leinir updated this revision to Diff 14236.
leinir marked an inline comment as done.
leinir added a comment.


  Simplify the xmlloader cache logic a touch

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5639?vs=14080=14236

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

AFFECTED FILES
  src/core/engine.cpp
  src/core/jobs/httpjob.cpp

To: leinir, whiting, apol
Cc: dfaure, #frameworks

D5739: Improve some error messages

[Dan Leinir Turthra Jensen]

leinir accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R304 KNewStuff

BRANCH
  master

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

To: apol, #frameworks, whiting, leinir

D5638: Use a single QNAM (and a disk cache) for HTTP jobs

[Dan Leinir Turthra Jensen]

leinir added a comment.


  In https://phabricator.kde.org/D5638#107645, @dfaure wrote:
  
  > 5 is 50kB.
  >  You wrote 50 megs which would be 5000 or 50*1024*1024.
  
  
  Yes, i certainly did. I should remember to drink less caffeine sometimes.

REPOSITORY
  R304 KNewStuff

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

To: leinir, whiting, apol, dfaure
Cc: dfaure, #frameworks

D5639: Internal cache for provider data on initialisation

[Dan Leinir Turthra Jensen]

leinir added a comment.


  In https://phabricator.kde.org/D5639#105620, @apol wrote:
  
  > Won't caching already fix the problem of traffic there? This change adds 
quite some complexity (all these QMutex make me cringe, these methods should 
always be called from the same thread anyway...)
  
  
  That was my initial impression as well, but it turns out that because the 
requests go out at the same time, they miss each other (i am not quite sure, 
but i think the redirection chain might have something to do with that)... The 
first step does seem to not be needed, though, as with the rechecking on the 
xml loader which i added after the document cache, the document cache doesn't 
seem to ever be hit.
  
  The mutexes are just to safeguard the QHash access, which isn't thread 
safe... However, i think I could likely swap that for a thread-bound global 
static, since while the document cache would be completely global, the xml 
loader one doesn't really need to be... if i take out the document cache, i can 
get rid of those mutexes which certainly do annoy me as well. I will rework 
that a bit and see what happens.

REPOSITORY
  R304 KNewStuff

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

To: leinir, whiting, apol
Cc: #frameworks

D5638: Use a single QNAM (and a disk cache) for HTTP jobs

[Dan Leinir Turthra Jensen]

leinir updated this revision to Diff 14146.
leinir marked 2 inline comments as done.
leinir added a comment.


  Unpointerify the internals, as agreed

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5638?vs=14079=14146

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

AFFECTED FILES
  src/core/jobs/httpworker.cpp
  src/core/jobs/httpworker.h

To: leinir, whiting, apol
Cc: #frameworks

D5638: Use a single QNAM (and a disk cache) for HTTP jobs

[Dan Leinir Turthra Jensen]

leinir updated this revision to Diff 14079.
leinir marked an inline comment as done.
leinir added a comment.


  Simpler logic, with the global qnam (etc) stored in a locally defined class, 
so we only have the one global static, and also allowing the qnam access to be 
more pleasantly locked.

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5638?vs=13920=14079

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

AFFECTED FILES
  src/core/jobs/httpworker.cpp
  src/core/jobs/httpworker.h

To: leinir, whiting, apol
Cc: #frameworks

D5638: Use a single QNAM (and a disk cache) for HTTP jobs

[Dan Leinir Turthra Jensen]

leinir marked an inline comment as done.
leinir added inline comments.

INLINE COMMENTS

> apol wrote in httpworker.cpp:33
> How about getting a class with these 3 attributes? Looks like we're juggling 
> here...

You know, that sounds like a good plan. Updated diff incoming :)

REPOSITORY
  R304 KNewStuff

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

To: leinir, whiting, apol
Cc: #frameworks

D5639: Internal cache for provider data on initialisation

[Dan Leinir Turthra Jensen]

leinir updated this revision to Diff 14080.
leinir edited the summary of this revision.
leinir added a comment.


  Simplify the logic, and only do the xmlloader caching, not the documents 
themselves which (as apol points out) should be cached already anyway. Not only 
that, but that codepath was never triggered when i rechecked it after 
implementing the xmlloader cache, and so, simpler logic is a good thing. 
Adjusted summary to match.

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5639?vs=13921=14080

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

AFFECTED FILES
  src/core/engine.cpp

To: leinir, whiting, apol
Cc: #frameworks

D6067: Make it possible to use QXmlStreamReader to read a KNS registry file

[Dan Leinir Turthra Jensen]

leinir accepted this revision.
leinir added a comment.
This revision is now accepted and ready to land.


  In https://phabricator.kde.org/D6067#114528, @apol wrote:
  
  > In https://phabricator.kde.org/D6067#114498, @leinir wrote:
  >
  > > On a similar note to handling comments, how does it now handle 
unknown/garbage tags? While it won't affect the cache code, it would 
potentially affect other things (ocs is not guaranteed to be perfectly formed, 
and it's one of the ways the framework's retained backwards compatibility). 
From what i can tell, this would assert when an unknown tag is encountered, 
right?
  >
  >
  > This is for .knsregistry files, not OCS. These files are generated by the 
very same class here.
  
  
  You are quite right, i forgot the attica content's not parsed using knscore's 
functions, just the staticxml provider... because i'm a silly person :) 
Obviously just fine, then, go for it!

REPOSITORY
  R304 KNewStuff

BRANCH
  master

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

To: apol, #frameworks, leinir
Cc: leinir, dfaure

D5902: Expand KNewStuff documentation

[Dan Leinir Turthra Jensen]

leinir updated this revision to Diff 15908.
leinir added a comment.


  Add/fix a whole bunch of more documentation, identified as missing (or 
incorrect) by Aniketh

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5902?vs=14660=15908

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

AFFECTED FILES
  README.md
  src/core/engine.h
  src/core/entryinternal.h
  src/downloaddialog.h
  src/qtquick/downloadlinkinfo.h
  src/qtquick/quickengine.h
  src/qtquick/quickitemsmodel.h
  src/upload/atticahelper_p.h

To: leinir, #knewstuff
Cc: apol, #frameworks

D8311: Require the same internal version as you're building

[Dan Leinir Turthra Jensen]

leinir created this revision.
leinir added a reviewer: KNewStuff.
leinir added a project: KNewStuff.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.

REVISION SUMMARY
  Previously the KNewStuff sub-frameworks would require the dependency 
frameworks version of itself, rather than the version being built, which caused 
fun little issues.

REPOSITORY
  R304 KNewStuff

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

AFFECTED FILES
  KF5NewStuffConfig.cmake.in
  KF5NewStuffQuickConfig.cmake.in

To: leinir, #knewstuff
Cc: #frameworks, ZrenBot

D8111: Require Kirigami 2.1 instead of 1.0 for KNewStuffQuick

[Dan Leinir Turthra Jensen]

leinir created this revision.
leinir added a reviewer: KNewStuff.
leinir added a project: KNewStuff.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.

REVISION SUMMARY
  This reduces the amount of things which require the by now quite out of date 
Kirigami 1.0 further. This also introduces a find clause to the CMakeLists.txt 
(a runtime find, so we should get no issues as it's informational rather than a 
hard require). Preparatory work for extending the KNewStuff Qt Quick components 
in the near future.

REPOSITORY
  R304 KNewStuff

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

AFFECTED FILES
  CMakeLists.txt
  src/qtquick/qml/NewStuffItem.qml

To: leinir, #knewstuff
Cc: #frameworks, ZrenBot

D8111: Require Kirigami 2.1 instead of 1.0 for KNewStuffQuick

[Dan Leinir Turthra Jensen]

This revision was automatically updated to reflect the committed changes.
Closed by commit R304:aad1ca02f183: Require Kirigami 2.1 instead of 1.0 for 
KNewStuffQuick (authored by leinir).

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8111?vs=20248=20251

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

AFFECTED FILES
  CMakeLists.txt
  src/qtquick/qml/NewStuffItem.qml

To: leinir, #knewstuff, mart, apol
Cc: apol, bshah, #frameworks, ZrenBot

D9018: Don't cause circular linking on Windows

[Dan Leinir Turthra Jensen]

leinir created this revision.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.

REVISION SUMMARY
  When building plugins, don't arbitrarily rename the output files (as this 
will occasionally result in circular dependencies).
  
  In this review from three and a half years ago 
, the Sonnet plugins were moved into 
a subdirectory, which was great. They were, however, also renamed at the same 
time, which resulted in some (and specifically the aspell plugin) being named 
the same as the library they should be linked against. End result: Spell 
checking has not worked on Windows for three and a half years.

TEST PLAN
  With these properties set, the plugins fail to load on Windows due to 
circular dependencies
  
  Without them (that is, with this patch), the plugins load

REPOSITORY
  R246 Sonnet

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

AFFECTED FILES
  src/plugins/aspell/CMakeLists.txt
  src/plugins/enchant/CMakeLists.txt
  src/plugins/hspell/CMakeLists.txt
  src/plugins/hunspell/CMakeLists.txt
  src/plugins/nsspellchecker/CMakeLists.txt
  src/plugins/voikko/CMakeLists.txt

To: leinir
Cc: #frameworks

D9018: Don't cause circular linking on Windows

[Dan Leinir Turthra Jensen]

leinir added a reviewer: Frameworks.

REPOSITORY
  R246 Sonnet

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

To: leinir, #frameworks
Cc: #frameworks

D9012: Revert "Detach before setting the d pointer"

[Dan Leinir Turthra Jensen]

leinir accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R304 KNewStuff

BRANCH
  unbreakEntryInternalDataSyncing

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

To: kossebau, whiting, leinir, apol
Cc: #frameworks

D9012: Revert "Detach before setting the d pointer"

[Dan Leinir Turthra Jensen]

leinir added a comment.


  Quicker is better here, i think... Perhaps it is worth adding the 
documentation we discussed as well in this review? Thinking about making it 
easier to track the history and whatnot of what happened and why...

REPOSITORY
  R304 KNewStuff

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

To: kossebau, whiting, leinir, apol
Cc: #frameworks

D9018: Don't cause circular linking on Windows

[Dan Leinir Turthra Jensen]

leinir added a comment.


  In https://phabricator.kde.org/D9018#172789, @alexeymin wrote:
  
  > So it resulted in something like aspell.dll requiring aspell.dll?
  
  
  
  
Exactly that, yes... took me running it through Dependency Walker to work 
out what was going on :P
  
  > What is the simplest/fastest way to test this?
  
  For building a unit test, i'm not really sure (though, i think, running a 
unit test on windows which loads the plugins and check that they load would 
likely do the trick, not sure there is one).
  
  For testing the patch before accepting it, simply attempting to run anything 
on Windows which uses Sonnet should cause the failure. i found out because of 
Calligra Gemini, but anything which does spell checking should cause the issue 
to happen - for example the spell checker in ktexteditor. The symptom is having 
no functioning spell check at all (and debug output will complain that Sonnet 
could not load the plugin because it "could not find the specified procedure").

REPOSITORY
  R246 Sonnet

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

To: leinir, #frameworks
Cc: alexeymin, apol, #frameworks

D8311: Require the same internal version as you're building

[Dan Leinir Turthra Jensen]

This revision was automatically updated to reflect the committed changes.
Closed by commit R304:e33d06f9857a: Require the same internal version as 
youre building (authored by leinir).

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8311?vs=20791=24076

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

AFFECTED FILES
  KF5NewStuffConfig.cmake.in
  KF5NewStuffQuickConfig.cmake.in

To: leinir, #knewstuff, dfaure
Cc: dfaure, #frameworks, ZrenBot

D9398: Fix TagLib detection and build on Windows

[Dan Leinir Turthra Jensen]

leinir added a reviewer: Frameworks.

REPOSITORY
  R320 KIO Extras

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

To: leinir, #frameworks

D9018: Don't cause circular linking on Windows

[Dan Leinir Turthra Jensen]

leinir added a comment.


  Would it be possible to get an accept here, or is there something people 
fundamentally disagree with about it?

REPOSITORY
  R246 Sonnet

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

To: leinir, #frameworks
Cc: vonreth, cgiboudeaux, alexeymin, apol, #frameworks

D9018: Don't cause circular linking on Windows

[Dan Leinir Turthra Jensen]

This revision was automatically updated to reflect the committed changes.
Closed by commit R246:bdc6562faf73: Dont cause circular linking on 
Windows (authored by leinir).

REPOSITORY
  R246 Sonnet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D9018?vs=23028=24150

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

AFFECTED FILES
  src/plugins/aspell/CMakeLists.txt
  src/plugins/enchant/CMakeLists.txt
  src/plugins/hspell/CMakeLists.txt
  src/plugins/hunspell/CMakeLists.txt
  src/plugins/nsspellchecker/CMakeLists.txt
  src/plugins/voikko/CMakeLists.txt

To: leinir, #frameworks, apol
Cc: vonreth, cgiboudeaux, alexeymin, apol, #frameworks

D9421: Remove anchient and broken workaround

[Dan Leinir Turthra Jensen]

leinir accepted this revision.
leinir added a comment.


  Ha, yes, i concur with Aleix there, workaround-be-gone! ;) (i have a 
suspicion the reason i thought this worked before is because i was testing on 
my local machine... which also explains nicely why the packages from BF don't 
have functioning spell checking for me ;) )

REPOSITORY
  R246 Sonnet

BRANCH
  master

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

To: vonreth, #frameworks, #craft, leinir, apol
Cc: apol, #frameworks

D8811: [knewstuff] Do not leak ImageLoader on error

[Dan Leinir Turthra Jensen]

leinir accepted this revision.
leinir added a comment.
This revision is now accepted and ready to land.


  Looks good to me :)

REPOSITORY
  R304 KNewStuff

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

To: anthonyfieroni, leinir, dfaure
Cc: broulik, #frameworks, ZrenBot

D8811: [knewstuff] Do not leak ImageLoader on error

[Dan Leinir Turthra Jensen]

leinir added inline comments.

INLINE COMMENTS

> engine.cpp:536
> +connect(l, ::signalError, this, [=]() {
> +qCDebug(KNEWSTUFFCORE) << "ERROR preview: " << entry.name() << type;
> +--m_numPictureJobs;

A more descriptive error message would be good, but this is already an 
improvement over no error message at all :) As pointed out elsewhere, since 
you're explicitly passing the ImageLoader instance through the signal, might it 
not make sense to output the description of the error (which is available on 
the httpjob, which in turn is available through ImageHandler::job, so you could 
just add l to the capture to access that)?

REPOSITORY
  R304 KNewStuff

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

To: anthonyfieroni, leinir, dfaure
Cc: broulik, #frameworks, ZrenBot

D8811: [knewstuff] Do not leak ImageLoader on error

[Dan Leinir Turthra Jensen]

leinir added inline comments.

INLINE COMMENTS

> anthonyfieroni wrote in engine.cpp:536
> Job can be nullptr, so signal can be a text, it can capture entry by 
> reference?

i guess you could extend the signal further with the error text added in 
instead of capturing l in the lambda, yes

REPOSITORY
  R304 KNewStuff

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

To: anthonyfieroni, leinir, dfaure
Cc: broulik, #frameworks, ZrenBot

D8811: [knewstuff] Do not leak ImageLoader on error

[Dan Leinir Turthra Jensen]

leinir added inline comments.

INLINE COMMENTS

> engine.cpp:536
> +connect(l, ::signalError, this, [this, l](const 
> KNSCore::EntryInternal , EntryInternal::PreviewType type) {
> +qCDebug(KNEWSTUFFCORE) << "ERROR preview: " << entry.name() << type;
> +--m_numPictureJobs;

You did add l to the capture, but unless i'm missing something super obvious... 
you're not using it ;) i meant to do something like add `<< 
l->job()->errorText()` to the debug statement so we can see what the job says 
has gone wrong (which arguably is the more useful information here)...

Maybe also emit a signalError with an appropriate text (because that's how to 
tell the user about errors)... though i'm not entirely certain if we might not 
want to just fail quietly here.

REPOSITORY
  R304 KNewStuff

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

To: anthonyfieroni, leinir, dfaure
Cc: broulik, #frameworks, ZrenBot

D7194: Detach before setting the d pointer

[Dan Leinir Turthra Jensen]

leinir added a subscriber: sitter.
leinir added a comment.


  Damn... Well spotted, @kossebau. Right, so immediate (at least temporary 
solution) to make things not broken would annoyingly enough be to revert the 
patch, yes... I am now thinking that another oddity noticed by @sitter last 
week was caused by this as well (going by the installedFiles data being out of 
sync, it would seem likely it would cause what they were seeing). So... while 
it feels a bit odd, i would have to vote to revert immediately, and create a 
new patch documenting why we can't detach in certain classes... Possibly adding 
in a TODO for Frameworks 6 (there's already a couple of those in kns).

REPOSITORY
  R304 KNewStuff

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

To: apol, leinir
Cc: sitter, kossebau, whiting, mutlaqja, broulik, #frameworks

D13515: Remove KNS::Engine d-pointer hack

[Dan Leinir Turthra Jensen]

leinir added a comment.


  H... this is tricky, but... yes, it seems reasonable to me that replacing 
one pointer with another would cause no BIC issues... and i certainly am happy 
to get rid of the d-pointer hack, so... if we're absolutely super-sure that 
this is BC, i'm all for it... would kind of like someone with better linker-fu 
to give word on this, but in principle, i like :)

REPOSITORY
  R304 KNewStuff

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

To: apol, #frameworks, leinir
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D6513: Add support for Attica tags support

[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

[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

D13714: Fix broken url to API specification

[Dan Leinir Turthra Jensen]

leinir accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R235 Attica

BRANCH
  master

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

To: habacker, mlaurent, leinir
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13733: Add provider auto test

[Dan Leinir Turthra Jensen]

leinir requested changes to this revision.
leinir added a comment.
This revision now requires changes to proceed.


  Autotests are good, but so is documentation - in principle this is good, but 
new public functions without documentation isn't really acceptable :)
  Apart from that, though, looks good! :) (was just about to add a comment 
about your invalid data string being less than descriptive... but hey, it's 
invalid data, why not ;) )

INLINE COMMENTS

> provider.h:275
>  
> +ItemJob *requestConfig();
> +

When adding new functions to anywhere, putting in documentation really needs to 
happen as well :) You've got it in other places already, though, so i'm sure 
that's just a minor bit of forgetfulness, these things happen :)

REPOSITORY
  R235 Attica

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

To: habacker, leinir
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13743: Migrate build system to use find_package in autotests/ki18n_install

[Dan Leinir Turthra Jensen]

leinir added a comment.


  This seems quite sensible to me, a cleanup and simplification of the test 
build system is not bad...
  
  That said, i'm unsure why you think i'm qualified to speak about ki18n and 
the general cmake build system to a level high enough that you highlight me 
specifically to review this. As opposed to, you know, someone who actually 
worked on either of those two... ever ;)

REPOSITORY
  R249 KI18n

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

To: habacker, leinir
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13732: Add debug output for all network requests

[Dan Leinir Turthra Jensen]

leinir requested changes to this revision.
leinir added a comment.
This revision now requires changes to proceed.


  Straight-up qDebug would cause quite a lot of spam... I'd be much happier if 
this was done using qCDebug (which is already used in other parts of the code, 
so just need to include attica_debug.h and swap for qDebug for qCDebug(ATTICA))

REPOSITORY
  R235 Attica

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

To: habacker, leinir
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13733: Add provider auto test

[Dan Leinir Turthra Jensen]

leinir accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R235 Attica

BRANCH
  master

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

To: habacker, leinir
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D7194: Detach before setting the d pointer

[Dan Leinir Turthra Jensen]

leinir added a comment.


  This already went into one release, and it would be quite useful to get it 
sorted before the next one rolls around, and the consensus seems to be 
reverting, as leaving kns with this patch in has some fairly unfortunate side 
effects. Could we get it reverted before the next release is done?
  
  The point about not reporting value changes is very much a good one, though - 
i think it would make plenty of sense to add that. It would be useful in any 
number of places (including e.g. in QtQuick, which obviously is my little baby, 
if we add it as properties... and it was sort of my mental todo anyway as 
something to do as i went ahead, unless anybody has large objections to me 
doing that).

REPOSITORY
  R304 KNewStuff

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

To: apol, leinir
Cc: sitter, kossebau, whiting, mutlaqja, broulik, #frameworks

D8188: Remove PreferCache from network requests

[Dan Leinir Turthra Jensen]

leinir accepted this revision.
leinir added a comment.
This revision is now accepted and ready to land.


  Right, yes, i think that was a bit of left over from working on it all. Go 
for it.

REPOSITORY
  R304 KNewStuff

BRANCH
  master

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

To: aacid, leinir
Cc: #frameworks

D10017: Find Aspell dictionaries on Windows

[Dan Leinir Turthra Jensen]

leinir created this revision.
leinir added a reviewer: vonreth.
leinir added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.
leinir requested review of this revision.

REVISION SUMMARY
  This feeds the sonnet aspell plugin some information to assist it in locating 
dictionaries on Windows. It adds a heuristic which follows the data location 
for applications installed using Craft on Windows.
  
  Partial reintroduction of the hack in https://phabricator.kde.org/D9421 
except this time it's relocateable

TEST PLAN
  Without patch: No squiggly red lines (Sonnet debug-warns that no dictionaries 
are found)
  With patch: Squiggly red lines (Warnings gone)

REPOSITORY
  R246 Sonnet

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

AFFECTED FILES
  src/plugins/aspell/aspellclient.cpp
  src/plugins/aspell/aspelldict.cpp

To: leinir, vonreth
Cc: #frameworks

D6513: Add support for Attica tags support

[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

[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

[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

[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