D27463: KconfigXT: Add a value attribute to Enum field choices
Kevin Ottens
noreply at phabricator.kde.org
Thu Mar 5 08:54:44 GMT 2020
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20200305/67b0b72e/attachment-0001.html>
More information about the Kde-frameworks-devel
mailing list