D23523: [SlaveBase] Use QMap instead of KConfig to store ioslave config

2020-05-13 Thread Méven Car 
meven added inline comments.

INLINE COMMENTS

> kossebau wrote in slavebase.h:355
> Given there are still a few usages of config() left which seem not easily 
> replaceable, it would be better to remove the deprecation tag for the 
> compiler, to not have false warnings on those places (see e.g. http.cpp for 
> certain usages still).
> A deprecation tag should be only set if there is a full migration path 
> available ideally, otherwise people will run into warnings they cannot do 
> something about.

Agreed, perhaps update the documentation slightly

REPOSITORY
  R241 KIO

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

To: meven, davidedmundson, dfaure, #frameworks
Cc: kossebau, broulik, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ngraham, bruns

D23523: [SlaveBase] Use QMap instead of KConfig to store ioslave config

2020-05-13 Thread Friedrich W. H. Kossebau 
kossebau added inline comments.

INLINE COMMENTS

> slavebase.h:355
>   */
> -KConfigGroup *config();
> +KIOCORE_DEPRECATED KConfigGroup *config();
>  

Given there are still a few usages of config() left which seem not easily 
replaceable, it would be better to remove the deprecation tag for the compiler, 
to not have false warnings on those places (see e.g. http.cpp for certain 
usages still).
A deprecation tag should be only set if there is a full migration path 
available ideally, otherwise people will run into warnings they cannot do 
something about.

REPOSITORY
  R241 KIO

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

To: meven, davidedmundson, dfaure, #frameworks
Cc: kossebau, broulik, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ngraham, bruns

D23523: [SlaveBase] Use QMap instead of KConfig to store ioslave config

2019-10-15 Thread Méven Car 
This revision was automatically updated to reflect the committed changes.
Closed by commit R241:a497d47c45a5: [SlaveBase] Use QMap instead of KConfig to 
store ioslave config (authored by meven).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D23523?vs=67763=67940#toc

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23523?vs=67763=67940

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

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

To: meven, davidedmundson, dfaure, #frameworks
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23523: [SlaveBase] Use QMap instead of KConfig to store ioslave config

2019-10-15 Thread Méven Car 
meven added a comment.


  In D23523#547265 , @dfaure wrote:
  
  > Looks fine, but it seems that the uploaded diff is only the delta compared 
to the previous diff, not the overall change. Anyhow, feel free to push both.
  
  
  It was on purpose to have two clean commits : add a new API, use it in the 
second commit.

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D23523

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

To: meven, davidedmundson, dfaure, #frameworks
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23523: [SlaveBase] Use QMap instead of KConfig to store ioslave config

2019-10-15 Thread David Faure 
dfaure accepted this revision.
dfaure added a comment.


  Looks fine, but it seems that the uploaded diff is only the delta compared to 
the previous diff, not the overall change. Anyhow, feel free to push both.

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D23523

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

To: meven, davidedmundson, dfaure, #frameworks
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23523: [SlaveBase] Use QMap instead of KConfig to store ioslave config

2019-10-12 Thread Méven Car 
meven added a dependent revision: D24582: Replace usage of SlaveBase::config() 
by SlaveBase::mapConfig().

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D23523

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

To: meven, davidedmundson, dfaure, #frameworks
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23523: [SlaveBase] Use QMap instead of KConfig to store ioslave config

2019-10-12 Thread Méven Car 
meven updated this revision to Diff 67763.
meven added a comment.


  Fix slavebase.cpp following QVariant changes

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23523?vs=67762=67763

BRANCH
  arcpatch-D23523

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

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

To: meven, davidedmundson, dfaure, #frameworks
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23523: [SlaveBase] Use QMap instead of KConfig to store ioslave config

2019-10-12 Thread Méven Car 
meven updated this revision to Diff 67762.
meven added a comment.


  Update since, use QVariant as mapConfig value

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23523?vs=65991=67762

BRANCH
  arcpatch-D23523

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

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

To: meven, davidedmundson, dfaure, #frameworks
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23523: [SlaveBase] Use QMap instead of KConfig to store ioslave config

2019-10-02 Thread Méven Car 
meven edited the summary of this revision.

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D23523

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

To: meven, davidedmundson, dfaure, #frameworks
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23523: [SlaveBase] Use QMap instead of KConfig to store ioslave config

2019-09-14 Thread David Faure 
dfaure accepted this revision.
dfaure added a comment.
This revision is now accepted and ready to land.


  Thanks!

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D23523

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

To: meven, davidedmundson, dfaure, #frameworks
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23523: [SlaveBase] Use QMap instead of KConfig to store ioslave config

2019-09-13 Thread Méven Car 
meven updated this revision to Diff 65991.
meven added a comment.


  Update @deprecated message to be more clear

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23523?vs=65931=65991

BRANCH
  arcpatch-D23523

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

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

To: meven, davidedmundson, dfaure, #frameworks
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23523: [SlaveBase] Use QMap instead of KConfig to store ioslave config

2019-09-13 Thread Méven Car 
meven marked 3 inline comments as done.

REPOSITORY
  R241 KIO

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

To: meven, davidedmundson, dfaure, #frameworks
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23523: [SlaveBase] Use QMap instead of KConfig to store ioslave config

2019-09-13 Thread David Faure 
dfaure added a comment.


  Yes, I see the "see" :-)
  But that's just a generic way to say "also look at these other methods", it 
doesn't constitute a clear statement like "use B instead of A".

REPOSITORY
  R241 KIO

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

To: meven, davidedmundson, dfaure, #frameworks
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23523: [SlaveBase] Use QMap instead of KConfig to store ioslave config

2019-09-13 Thread Méven Car 
meven added inline comments.

INLINE COMMENTS

> dfaure wrote in slavebase.h:353
> Can you update the docu as suggested above, to point more clearly to the 
> replacement? This makes things easier for people doing porting. Thanks.

I have a `@see mapConfig` on the line below, did you see ? Or do you mean I 
should have all this on the same line.

REPOSITORY
  R241 KIO

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

To: meven, davidedmundson, dfaure, #frameworks
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23523: [SlaveBase] Use QMap instead of KConfig to store ioslave config

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

INLINE COMMENTS

> dfaure wrote in slavebase.h:353
>   @deprecated since 5.63, use mapConfig() instead.

Can you update the docu as suggested above, to point more clearly to the 
replacement? This makes things easier for people doing porting. Thanks.

REPOSITORY
  R241 KIO

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

To: meven, davidedmundson, dfaure, #frameworks
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23523: [SlaveBase] Use QMap instead of KConfig to store ioslave config

2019-09-12 Thread Méven Car 
meven updated this revision to Diff 65931.
meven marked 2 inline comments as done.
meven added a comment.


  Clean up and update @since

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23523?vs=65374=65931

BRANCH
  arcpatch-D23523

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

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

To: meven, davidedmundson, dfaure, #frameworks
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23523: [SlaveBase] Use QMap instead of KConfig to store ioslave config

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

INLINE COMMENTS

> slavebase.cpp:38
>  
> +#include 
>  #include 

what is this used for?

> meven wrote in slavebase.cpp:281
> That's on purpose, the first use of mapConfig is in rebuildConfig and is a 
> clear() call.

So? This line is not needed, an empty map is an empty map, no need to assign an 
empty map to it.

> dfaure wrote in slavebase.h:342
> missing @since

Time passed, this is for 5.63 now, sorry about that.

> dfaure wrote in slavebase.h:353
> @deprecated since 5.xx

@deprecated since 5.63, use mapConfig() instead.

REPOSITORY
  R241 KIO

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

To: meven, davidedmundson, dfaure, #frameworks
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23523: [SlaveBase] Use QMap instead of KConfig to store ioslave config

2019-09-04 Thread Méven Car 
meven updated this revision to Diff 65374.
meven marked 5 inline comments as done.
meven added a comment.


  Review feedback, add @since, review mapConfig signature, reset config and 
configGroup to nullptr after deletion

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23523?vs=65200=65374

BRANCH
  arcpatch-D23523

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

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

To: meven, davidedmundson, dfaure, #frameworks
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23523: [SlaveBase] Use QMap instead of KConfig to store ioslave config

2019-09-02 Thread David Faure 
dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  I admit that I never understood why this was using KConfig. I guess Waldo had 
a hammer and everything looked like nails ;-)

INLINE COMMENTS

> slavebase.cpp:191
> +delete configGroup;
> +delete config;
>  }

missing `config = nullptr` so the `delete config` in the destructor doesn't 
crash.

Same with configGroup.

> slavebase.cpp:411
>  
> +QMap ::mapConfig() const
> +{

That is a weird abuse of const. If the member wasn't in a d pointer, this 
wouldn't compile.

If the slave is supposed to only read from the map, then it should be a value 
or const ref.
If the slave can write into the map, then this method shouldn't conceptually be 
`const`.

> slavebase.h:342
> + * relevant for the current protocol and host.
> + */
> +QMap () const;

missing @since

> slavebase.h:353
> + * TODO KF6: remove
> + * @deprecated use mapConfig(StatSide side)
>   */

@deprecated since 5.xx

REPOSITORY
  R241 KIO

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

To: meven, davidedmundson, dfaure, #frameworks
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23523: [SlaveBase] Use QMap instead of KConfig to store ioslave config

2019-09-02 Thread Méven Car 
meven marked 3 inline comments as done.
meven added inline comments.

INLINE COMMENTS

> broulik wrote in slavebase.cpp:281
> This is initialized automatically like this

That's on purpose, the first use of mapConfig is in rebuildConfig and is a 
clear() call.

REPOSITORY
  R241 KIO

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

To: meven, davidedmundson, dfaure, #frameworks
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23523: [SlaveBase] Use QMap instead of KConfig to store ioslave config

2019-09-02 Thread Méven Car 
meven updated this revision to Diff 65200.
meven marked an inline comment as done.
meven added a comment.


  Review feedback

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23523?vs=65006=65200

BRANCH
  arcpatch-D23523_1

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

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

To: meven, davidedmundson, dfaure, #frameworks
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23523: [SlaveBase] Use QMap instead of KConfig to store ioslave config

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

INLINE COMMENTS

> slavebase.cpp:189
> +}
> +if (config != nullptr) {
> +delete config;

`delete nullptr` is fine, no need to check first

> slavebase.cpp:281
>  d->needSendCanResume = false;
> -d->config = new KConfig(QString(), KConfig::SimpleConfig);
> -// The KConfigGroup needs the KConfig to exist during its whole lifetime.
> -d->configGroup = new KConfigGroup(d->config, QString());
> +d->mapConfig = QMap();
>  d->onHold = false;

This is initialized automatically like this

> slavebase.cpp:290
>  {
> -delete d->configGroup;
> -delete d->config;

Don't you still want this?

> slavebase.cpp:417
>  {
> +if (d->config == nullptr) {
> +d->config = new KConfig(QString(), KConfig::SimpleConfig);

Just check `if (!d->config)`

REPOSITORY
  R241 KIO

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

To: meven, davidedmundson, dfaure, #frameworks
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23523: [SlaveBase] Use QMap instead of KConfig to store ioslave config

2019-08-30 Thread Méven Car 
meven updated this revision to Diff 65006.
meven added a comment.


  Lazily construct the KConfig but keep the instance as long as no config 
changes has occured

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23523?vs=64816=65006

BRANCH
  arcpatch-D23523_1

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

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

To: meven, davidedmundson, dfaure, #frameworks
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23523: [SlaveBase] Use QMap instead of KConfig to store ioslave config

2019-08-28 Thread Méven Car 
meven updated this revision to Diff 64816.
meven added a comment.


  Make mapConfig return a const ref

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23523?vs=64815=64816

BRANCH
  arcpatch-D23523

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

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

To: meven, davidedmundson, dfaure, #frameworks
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23523: [SlaveBase] Use QMap instead of KConfig to store ioslave config

2019-08-28 Thread Méven Car 
meven marked an inline comment as done.

REPOSITORY
  R241 KIO

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

To: meven, davidedmundson, dfaure, #frameworks
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23523: [SlaveBase] Use QMap instead of KConfig to store ioslave config

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

INLINE COMMENTS

> slavebase.h:343
> + */
> +QMap *mapConfig();
> +

Why return a pointer, and why is this method not `const`?
Or is this supposed to be writable? Then I would prefer returning a reference 
to communicate ownership properly.

REPOSITORY
  R241 KIO

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

To: meven, davidedmundson, dfaure, #frameworks
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23523: [SlaveBase] Use QMap instead of KConfig to store ioslave config

2019-08-28 Thread Méven Car 
meven created this revision.
meven added reviewers: davidedmundson, dfaure, Frameworks.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
meven requested review of this revision.

REVISION SUMMARY
  Inspired by 
https://github.com/blue-systems/plasma-5.16/issues/139#issuecomment-441704579
  After some review, it seems to me using KConfig is not necessary here.
  Removing its use should improve IO throughput since rebuildConfig is called 
in finished.
  So mark the old config() function as deprecated.
  
  TODO
  
  - Discussion: does it make sense ?
  - benchmark (if anyone has some, it would be very appreciated)
  - port ioslaves in kio and kio-extras to use mapConfig() instead of config()
  - Clean up this description

TEST PLAN
  ctest

REPOSITORY
  R241 KIO

BRANCH
  master

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

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

To: meven, davidedmundson, dfaure, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns