D27463: KconfigXT: Add a value attribute to Enum field choices

[Kevin Ottens]

ervin requested changes to this revision.
ervin added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kcoreconfigskeleton.h:766
> +
> +        public:
> +            QString value() const {

It's a struct you can drop the public: here.

> kcoreconfigskeleton.h:767
> +        public:
> +            QString value() const {
> +                return val.isEmpty() ? name : val;

There should be a new line before the opening curly brace.

I also wonder if it wouldn't be better with the implementation being moved to the cpp file, otherwise it risks being inlined which might not be the most convenient for longer term management of the change (if for some reason the implementation needs to be changed).

> KConfigCommonStructs.h:61
> +    public:
> +        QString value() const{
> +            return val.isEmpty() ? name : val;

New line before opening curly brace please.

> KConfigXmlParser.cpp:203
>          }
> +        else if (choice.name.contains(QLatin1Char(' ')) || choice.name.contains(QLatin1Char('/')) || choice.name.contains(QLatin1Char('.')) || choice.name.contains(QLatin1Char(':'))) {
> +            std::cerr << "Tag <choice> attribute 'name' must be compatible with Enum naming. name was '" << qPrintable(choice.name) << "'. You can use attribute 'value' to pass any string as the choice value." << std::endl;

else if should be just after the closing curly brace

As for checking the valid characters for an enum name this is "easy" it can only be alphabetical, numerical and underscore characters (technically shouldn't start with a digit). Any other character will be rejected by the compiler, the current filter is thus not discriminating enough at all and I think its logic should be reversed (the blacklist being potentially infinite it should be white list based).

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/20200224/7b86bfa6/attachment.html>