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

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.

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

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

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

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

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

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

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

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

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

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,

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

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

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

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

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

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

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,

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

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

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

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

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,

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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(),

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

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

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

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

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

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

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

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

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