ervin requested changes to this revision.
ervin added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> kcoreconfigskeleton.cpp:580
> +{
> + // TODO KF6 move value to ItemEnum::Choice and remove
> KCoreConfigSkeleton::ItemEnum::mValues
> + const auto inHash = mValues.value(name);
Will need an update
> kcoreconfigskeleton.cpp:586
> +/**
> + * Stores a choice value for name
> + */
This comment should go away
> kcoreconfigskeleton.h:788
> + */
> + QString valueForChoice(QString name) const;
> +
const QString &name
Probably worth adding a KF6 comment somewhere as well, because your first
attempt felt more natural, we're going for this weird construct only for BC
concerns.
> kcoreconfigskeleton.h:793
> + */
> + void setValueForChoice(QString name, QString valueForChoice);
> +
const ref for the parameters
> kcoreconfigskeleton.h:797
> QList<Choice> mChoices;
> + QHash<QString, QString> mValues;
> };
Nope, can't be here, this still breaks binary compatibility. As I mentioned in
my last comment it should be buried all the way into the d-pointer inherited
from KConfigSkeletonItem... This is inefficient in term of memory load but it's
the only option to keep BC.
> KConfigCommonStructs.h:108
> Choices choices;
> + QHash<QString, QString> enumValues;
> QList<Signal> signalList;
Well, on that side you should have kept the more natural value() method IMO.
> KConfigXmlParser.cpp:193
> QList<CfgEntry::Choice> chlist;
> + const QRegularExpression choiceNameRegex =
> QRegularExpression(QStringLiteral("\\w+"));
> +
nitpick: I'd const auto that one, but it's your call
REPOSITORY
R237 KConfig
REVISION DETAIL
https://phabricator.kde.org/D27463
To: meven, ervin, bport, crossi, #frameworks
Cc: ngraham, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh,
bruns
