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

2020-03-08 Thread Méven Car 
This revision was automatically updated to reflect the committed changes.
Closed by commit R237:ec207330d5bd: KconfigXT: Add a value attribute to Enum 
field choices (authored by meven).

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27463?vs=77225=77226

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

AFFECTED FILES
  autotests/kconfig_compiler/test4.cpp.ref
  autotests/kconfig_compiler/test4.kcfg
  autotests/kconfig_compiler/test5.cpp.ref
  autotests/kconfig_compiler/test5.kcfg
  src/core/kcoreconfigskeleton.cpp
  src/core/kcoreconfigskeleton.h
  src/core/kcoreconfigskeleton_p.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigXmlParser.cpp
  src/kconfig_compiler/README.dox
  src/kconfig_compiler/kcfg.xsd

To: meven, ervin, bport, crossi, #frameworks
Cc: ngraham, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
bruns

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

2020-03-08 Thread Méven Car 
meven updated this revision to Diff 77225.
meven added a comment.


  Rebase on master

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27463?vs=77077=77225

BRANCH
  arcpatch-D27463_2

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

AFFECTED FILES
  autotests/kconfig_compiler/test4.cpp.ref
  autotests/kconfig_compiler/test4.kcfg
  autotests/kconfig_compiler/test5.cpp.ref
  autotests/kconfig_compiler/test5.kcfg
  src/core/kcoreconfigskeleton.cpp
  src/core/kcoreconfigskeleton.h
  src/core/kcoreconfigskeleton_p.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigXmlParser.cpp
  src/kconfig_compiler/README.dox
  src/kconfig_compiler/kcfg.xsd

To: meven, ervin, bport, crossi, #frameworks
Cc: ngraham, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
bruns

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

2020-03-06 Thread Kevin Ottens 
ervin accepted this revision.

REPOSITORY
  R237 KConfig

BRANCH
  arcpatch-D27463_2

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

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

2020-03-06 Thread Méven Car 
meven updated this revision to Diff 77077.
meven added a comment.


  Fix typo

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27463?vs=77064=77077

BRANCH
  arcpatch-D27463_2

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

AFFECTED FILES
  autotests/kconfig_compiler/test4.cpp.ref
  autotests/kconfig_compiler/test4.kcfg
  autotests/kconfig_compiler/test5.cpp.ref
  autotests/kconfig_compiler/test5.kcfg
  src/core/kcoreconfigskeleton.cpp
  src/core/kcoreconfigskeleton.h
  src/core/kcoreconfigskeleton_p.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigXmlParser.cpp
  src/kconfig_compiler/README.dox
  src/kconfig_compiler/kcfg.xsd

To: meven, ervin, bport, crossi, #frameworks
Cc: ngraham, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
bruns

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

2020-03-06 Thread Kevin Ottens 
ervin accepted this revision.
ervin added a comment.
This revision is now accepted and ready to land.


  LGTM, please fix the typo in the docs before pushing though

INLINE COMMENTS

> README.dox:372
>  
> +Since 5.68, if present the value attribute will be used used as the 
> choice value written to the backend 
> +instead of the name, allowing to write text incompatible with enum 
> naming.

typo: it says "used used"

REPOSITORY
  R237 KConfig

BRANCH
  arcpatch-D27463_2

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

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

2020-03-06 Thread Méven Car 
meven updated this revision to Diff 77064.
meven marked 2 inline comments as done.
meven added a comment.


  Fix comment and formatting

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27463?vs=77006=77064

BRANCH
  arcpatch-D27463_2

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

AFFECTED FILES
  autotests/kconfig_compiler/test4.cpp.ref
  autotests/kconfig_compiler/test4.kcfg
  autotests/kconfig_compiler/test5.cpp.ref
  autotests/kconfig_compiler/test5.kcfg
  src/core/kcoreconfigskeleton.cpp
  src/core/kcoreconfigskeleton.h
  src/core/kcoreconfigskeleton_p.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigXmlParser.cpp
  src/kconfig_compiler/README.dox
  src/kconfig_compiler/kcfg.xsd

To: meven, ervin, bport, crossi, #frameworks
Cc: ngraham, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
bruns

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

2020-03-05 Thread Kevin Ottens 
ervin requested changes to this revision.
ervin added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kcoreconfigskeleton.cpp:581
> +// HACK for BC concerns
> +// TODO KF6: remove KCoreConfigSkeletonPrivate::mValues and add a value 
> field to KCoreConfigSkeleton::ItemEnum::Choice
> +const auto inHash = d_ptr->mValues.value(name);

You mean KConfigSkeletonItemPrivate aren't you? (instead of 
KCoreConfigSkeletonPrivate)

> meven wrote in kcoreconfigskeleton.h:788
> I expect those to be found through grep, and I had comments in the past to 
> put it somewhere where it could not get in documentation, in cpp guarantees 
> this.

Well, regular comments don't go in the docs ;-)
(you need the triple slash or the double start at start of comment for it to be 
picked up by doxygen)

But cpp, why not.

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

New line before opening curly brace please

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

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

2020-03-05 Thread Méven Car 
meven marked 4 inline comments as done.
meven added inline comments.

INLINE COMMENTS

> ervin wrote in kcoreconfigskeleton.cpp:580
> No I meant the comment needs to be adjusted due to the changes (fields 
> changing place and such) sorry if I was unclear.

I updated the comment since your first comment, I think it is ok now.

> ervin wrote in kcoreconfigskeleton.h:788
> Yeah, noticed only later... I wonder if it's more obvious in the header or 
> the cpp...

I expect those to be found through grep, and I had comments in the past to put 
it somewhere where it could not get in documentation, in cpp guarantees this.

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

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

2020-03-05 Thread Méven Car 
meven updated this revision to Diff 77006.
meven added a comment.


  Fix typo

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27463?vs=76999=77006

BRANCH
  arcpatch-D27463_2

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

AFFECTED FILES
  autotests/kconfig_compiler/test4.cpp.ref
  autotests/kconfig_compiler/test4.kcfg
  autotests/kconfig_compiler/test5.cpp.ref
  autotests/kconfig_compiler/test5.kcfg
  src/core/kcoreconfigskeleton.cpp
  src/core/kcoreconfigskeleton.h
  src/core/kcoreconfigskeleton_p.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigXmlParser.cpp
  src/kconfig_compiler/README.dox
  src/kconfig_compiler/kcfg.xsd

To: meven, ervin, bport, crossi, #frameworks
Cc: ngraham, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
bruns

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

2020-03-05 Thread Kevin Ottens 
ervin added inline comments.

INLINE COMMENTS

> meven wrote in kcoreconfigskeleton.cpp:580
> You mean I should prepare this and put it in KF6 waiting for merge queue ?

No I meant the comment needs to be adjusted due to the changes (fields changing 
place and such) sorry if I was unclear.

> meven wrote in kcoreconfigskeleton.h:788
> I have one in `QString KCoreConfigSkeleton::ItemEnum::valueForChoice(QString 
> name) const`

Yeah, noticed only later... I wonder if it's more obvious in the header or the 
cpp...

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

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

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

INLINE COMMENTS

> ervin wrote in kcoreconfigskeleton.cpp:580
> Will need an update

You mean I should prepare this and put it in KF6 waiting for merge queue ?

> ervin wrote in kcoreconfigskeleton.h:788
> const QString 
> 
> 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.

I have one in `QString KCoreConfigSkeleton::ItemEnum::valueForChoice(QString 
name) const`

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

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

2020-03-05 Thread Méven Car 
meven updated this revision to Diff 76999.
meven marked 9 inline comments as done.
meven added a comment.


  Remove enumValues, const ref args, move mValues to 
KCoreConfigSkeletonItemPrivate, improve comments

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27463?vs=76866=76999

BRANCH
  arcpatch-D27463_2

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

AFFECTED FILES
  autotests/kconfig_compiler/test4.cpp.ref
  autotests/kconfig_compiler/test4.kcfg
  autotests/kconfig_compiler/test5.cpp.ref
  autotests/kconfig_compiler/test5.kcfg
  src/core/kcoreconfigskeleton.cpp
  src/core/kcoreconfigskeleton.h
  src/core/kcoreconfigskeleton_p.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigXmlParser.cpp
  src/kconfig_compiler/README.dox
  src/kconfig_compiler/kcfg.xsd

To: meven, ervin, bport, crossi, #frameworks
Cc: ngraham, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
bruns

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

2020-03-05 Thread Kevin Ottens 
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 

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 mChoices;
> +QHash 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 enumValues;
>  QList signalList;

Well, on that side you should have kept the more natural value() method IMO.

> KConfigXmlParser.cpp:193
>  QList 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

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

2020-03-03 Thread Méven Car 
meven added a comment.


  I fixed an issue when testing this with D27477 
 and it is fixed and the issue it revealed 
is tested.

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

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

2020-03-03 Thread Méven Car 
meven updated this revision to Diff 76866.
meven added a comment.


  Improve naming, fix case of normal entries, adding a test for this case

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27463?vs=76856=76866

BRANCH
  arcpatch-D27463_1

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

AFFECTED FILES
  autotests/kconfig_compiler/test4.cpp.ref
  autotests/kconfig_compiler/test4.kcfg
  autotests/kconfig_compiler/test5.cpp.ref
  autotests/kconfig_compiler/test5.kcfg
  src/core/kcoreconfigskeleton.cpp
  src/core/kcoreconfigskeleton.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigXmlParser.cpp
  src/kconfig_compiler/README.dox
  src/kconfig_compiler/kcfg.xsd

To: meven, ervin, bport, crossi, #frameworks
Cc: ngraham, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
bruns

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

2020-03-03 Thread Méven Car 
meven updated this revision to Diff 76856.
meven added a comment.


  Fix no value set case

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27463?vs=76849=76856

BRANCH
  arcpatch-D27463_1

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

AFFECTED FILES
  autotests/kconfig_compiler/test4.cpp.ref
  autotests/kconfig_compiler/test4.kcfg
  src/core/kcoreconfigskeleton.cpp
  src/core/kcoreconfigskeleton.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigXmlParser.cpp
  src/kconfig_compiler/README.dox
  src/kconfig_compiler/kcfg.xsd

To: meven, ervin, bport, crossi, #frameworks
Cc: ngraham, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
bruns

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

2020-03-03 Thread Méven Car 
meven updated this revision to Diff 76849.
meven added a comment.


  Move values attribute for ItemEnum::Choice to 
KCoreConfigSkeleton::ItemEnum::ItemEnum::mValues to preserve Binary compatiblity

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27463?vs=76446=76849

BRANCH
  arcpatch-D27463_1

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

AFFECTED FILES
  autotests/kconfig_compiler/test4.cpp.ref
  autotests/kconfig_compiler/test4.kcfg
  src/core/kcoreconfigskeleton.cpp
  src/core/kcoreconfigskeleton.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigXmlParser.cpp
  src/kconfig_compiler/README.dox
  src/kconfig_compiler/kcfg.xsd

To: meven, ervin, bport, crossi, #frameworks
Cc: ngraham, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
bruns

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

2020-02-26 Thread Méven Car 
meven marked an inline comment as done.

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

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

2020-02-26 Thread Méven Car 
meven updated this revision to Diff 76446.
meven added a comment.


  Move construct of QRegularExpression out of loop

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27463?vs=76405=76446

BRANCH
  arcpatch-D27463

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

AFFECTED FILES
  autotests/kconfig_compiler/test4.cpp.ref
  autotests/kconfig_compiler/test4.kcfg
  src/core/kcoreconfigskeleton.cpp
  src/core/kcoreconfigskeleton.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigXmlParser.cpp
  src/kconfig_compiler/README.dox
  src/kconfig_compiler/kcfg.xsd

To: meven, ervin, bport, crossi, #frameworks
Cc: ngraham, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
bruns

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

2020-02-26 Thread Kevin Ottens 
ervin requested changes to this revision.
ervin added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> ervin wrote in kcoreconfigskeleton.h:764
> Oh right, stupid me, this obviously breaks binary compatibility, we need to 
> find another way to store this.

Most likely place to hide it would be inside KConfigSkeletonItem's d-pointer 
(likely a hash associating vals with names)... it'd be unused by most item 
types of course which sucks but at least it'd be safe BC wise.

> KConfigXmlParser.cpp:202
>  std::cerr << "Tag  requires attribute 'name'." << 
> std::endl;
> +} else if 
> (!(QRegularExpression(QStringLiteral("\\w+"))).match(choice.name).hasMatch()) 
> {
> +std::cerr << "Tag  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;

We should move the QRegularExpression out of the loop, otherwise it'll get 
compiled over and over for each choice (premature pessimisation imo). We could 
go one step further and have it as a member of the parser to have it compiled 
only once of course, but maybe we'd get in premature optimization territory.

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

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

2020-02-26 Thread Kevin Ottens 
ervin added inline comments.

INLINE COMMENTS

> meven wrote in kcoreconfigskeleton.h:764
> Something I have noticed while testing this.
> Since it changes the memory of a very common data struct in a very common 
> lib, it creates a lot of crashes if apps are not compiled with the installed 
> version.
> So all local builds should be rebuild or reinstalled, once this has landed, 
> or you end up with a lot of crashes.

Oh right, stupid me, this obviously breaks binary compatibility, we need to 
find another way to store this.

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

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

2020-02-25 Thread Méven Car 
meven added inline comments.

INLINE COMMENTS

> kcoreconfigskeleton.h:764
>  QString whatsThis;
> +QString val;
> +

Something I have noticed while testing this.
Since it changes the memory of a very common data struct in a very common lib, 
it creates a lot of crashes if apps are not compiled with the installed version.
So all local builds should be rebuild or reinstalled, once this has landed, or 
you end up with a lot of crashes.

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

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

2020-02-25 Thread Méven Car 
meven updated this revision to Diff 76405.
meven marked 6 inline comments as done.
meven added a comment.


  Move KCoreConfigSkeleton::ItemEnum::Choice::value to cpp file, use a regex to 
filter valid choice name

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27463?vs=76040=76405

BRANCH
  arcpatch-D27463_1

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

AFFECTED FILES
  autotests/kconfig_compiler/test4.cpp.ref
  autotests/kconfig_compiler/test4.kcfg
  src/core/kcoreconfigskeleton.cpp
  src/core/kcoreconfigskeleton.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigXmlParser.cpp
  src/kconfig_compiler/README.dox
  src/kconfig_compiler/kcfg.xsd

To: meven, ervin, bport, crossi, #frameworks
Cc: ngraham, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
bruns

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

2020-02-24 Thread 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  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

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

2020-02-20 Thread Cyril Rossi 
crossi added inline comments.

INLINE COMMENTS

> meven wrote in KConfigXmlParser.cpp:203
> If you don't mind I am putting aside the second part as it is not directly 
> related to this PR, i.e name attribute validation. (FYI we had 
> KConfigXmlParser::validateNameAndKey)

I guess there are more problematic characters. IMO this is not the right place, 
you pointed `validateNameAndKey()` which seems to be a better choice to 
implement such checks.

BTW, this is not related to this PR.

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

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

2020-02-20 Thread Méven Car 
meven edited the summary of this revision.

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

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

2020-02-20 Thread Méven Car 
meven updated this revision to Diff 76040.
meven marked an inline comment as done.
meven added a comment.


  Warn user about / . : being forbidden in choice's attribute name

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27463?vs=75969=76040

BRANCH
  arcpatch-D27463

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

AFFECTED FILES
  autotests/kconfig_compiler/test4.cpp.ref
  autotests/kconfig_compiler/test4.kcfg
  src/core/kcoreconfigskeleton.cpp
  src/core/kcoreconfigskeleton.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigXmlParser.cpp
  src/kconfig_compiler/README.dox
  src/kconfig_compiler/kcfg.xsd

To: meven, ervin, bport, crossi, #frameworks
Cc: ngraham, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
bruns

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

2020-02-20 Thread Méven Car 
meven added inline comments.

INLINE COMMENTS

> bport wrote in KConfigXmlParser.cpp:203
> Can we do a test on more than '  ' value, proably solve other case.
> And we probably want to limit what is a correct value, any string seems a bit 
> too large, some character can be problematic when we write back to config 
> file.

If you don't mind I am putting aside the second part as it is not directly 
related to this PR, i.e name attribute validation. (FYI we had 
KConfigXmlParser::validateNameAndKey)

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

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

2020-02-20 Thread Benjamin Port 
bport added inline comments.

INLINE COMMENTS

> KConfigXmlParser.cpp:203
>  }
> +else if (choice.name.contains(QLatin1Char(' '))) {
> +std::cerr << "Tag  attribute 'name' cannot contain a 
> space. You can use attribute 'value' to pass any string as the choice value." 
> << std::endl;

Can we do a test on more than '  ' value, proably solve other case.
And we probably want to limit what is a correct value, any string seems a bit 
too large, some character can be problematic when we write back to config file.

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

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

2020-02-19 Thread Méven Car 
meven updated this revision to Diff 75969.
meven added a comment.


  Add a since to dox

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27463?vs=75968=75969

BRANCH
  arcpatch-D27463

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

AFFECTED FILES
  autotests/kconfig_compiler/test4.cpp.ref
  autotests/kconfig_compiler/test4.kcfg
  src/core/kcoreconfigskeleton.cpp
  src/core/kcoreconfigskeleton.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigXmlParser.cpp
  src/kconfig_compiler/README.dox
  src/kconfig_compiler/kcfg.xsd

To: meven, ervin, bport, crossi, #frameworks
Cc: ngraham, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
bruns

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

2020-02-19 Thread Méven Car 
meven updated this revision to Diff 75968.
meven added a comment.


  Add documentation about value in dox

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27463?vs=75967=75968

BRANCH
  arcpatch-D27463

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

AFFECTED FILES
  autotests/kconfig_compiler/test4.cpp.ref
  autotests/kconfig_compiler/test4.kcfg
  src/core/kcoreconfigskeleton.cpp
  src/core/kcoreconfigskeleton.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigXmlParser.cpp
  src/kconfig_compiler/README.dox
  src/kconfig_compiler/kcfg.xsd

To: meven, ervin, bport, crossi, #frameworks
Cc: ngraham, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
bruns

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

2020-02-19 Thread Méven Car 
meven added a comment.


  In D27463#613961 , @davidre wrote:
  
  > Could you also add documentation to whatever  
https://api.kde.org/frameworks/kconfig/html/kconfig_compiler.html is generated 
from?
  
  
  Thanks for pointing it out.
  
  I meant to edit 
https://techbase.kde.org/Development/Tutorials/Using_KConfig_XT as well after 
merge.

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

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

2020-02-19 Thread David Redondo 
davidre added a comment.


  Could you also add documentation to whatever  
https://api.kde.org/frameworks/kconfig/html/kconfig_compiler.html is generated 
from?

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

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

2020-02-19 Thread Méven Car 
meven updated this revision to Diff 75967.
meven added a comment.


  Ignore empty value attribute for choice

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27463?vs=75966=75967

BRANCH
  arcpatch-D27463

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

AFFECTED FILES
  autotests/kconfig_compiler/test4.cpp.ref
  autotests/kconfig_compiler/test4.kcfg
  src/core/kcoreconfigskeleton.cpp
  src/core/kcoreconfigskeleton.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigXmlParser.cpp
  src/kconfig_compiler/kcfg.xsd

To: meven, ervin, bport, crossi, #frameworks
Cc: ngraham, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
bruns

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

2020-02-19 Thread Méven Car 
meven updated this revision to Diff 75966.
meven added a comment.


  Default value must refere to the choice name

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27463?vs=75964=75966

BRANCH
  arcpatch-D27463

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

AFFECTED FILES
  autotests/kconfig_compiler/test4.cpp.ref
  autotests/kconfig_compiler/test4.kcfg
  src/core/kcoreconfigskeleton.cpp
  src/core/kcoreconfigskeleton.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigXmlParser.cpp
  src/kconfig_compiler/kcfg.xsd

To: meven, ervin, bport, crossi, #frameworks
Cc: ngraham, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
bruns

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

2020-02-19 Thread Méven Car 
meven updated this revision to Diff 75964.
meven marked 2 inline comments as done.
meven added a comment.


  Clean garbage files

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27463?vs=75917=75964

BRANCH
  arcpatch-D27463

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

AFFECTED FILES
  autotests/kconfig_compiler/test4.cpp.ref
  autotests/kconfig_compiler/test4.kcfg
  src/core/kcoreconfigskeleton.cpp
  src/core/kcoreconfigskeleton.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigXmlParser.cpp
  src/kconfig_compiler/kcfg.xsd

To: meven, ervin, bport, crossi, #frameworks
Cc: ngraham, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
bruns

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

2020-02-18 Thread Nathaniel Graham 
ngraham added inline comments.

INLINE COMMENTS

> CTestCostData.txt:1
> +---

Seems like this file was added in error

> LastTest.log:3
> +--
> +End testing: Jan 29 14:08 CET

Seems like this file was added in error

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

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

2020-02-18 Thread Méven Car 
meven updated this revision to Diff 75917.
meven added a comment.


  Default value must refere to the choice name

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27463?vs=75852=75917

BRANCH
  arcpatch-D27463

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

AFFECTED FILES
  Testing/Temporary/CTestCostData.txt
  Testing/Temporary/LastTest.log
  autotests/kconfig_compiler/test4.cpp.ref
  autotests/kconfig_compiler/test4.kcfg
  src/core/kcoreconfigskeleton.cpp
  src/core/kcoreconfigskeleton.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigXmlParser.cpp
  src/kconfig_compiler/kcfg.xsd

To: meven, ervin, bport, crossi, #frameworks
Cc: davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

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

2020-02-18 Thread Méven Car 
meven added a dependent revision: D27477: KCM/Kwinoptions: Port title bar and 
window actions tabs UI and conf to KConfigXT.

REPOSITORY
  R237 KConfig

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

To: meven, ervin, bport, crossi, #frameworks
Cc: davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

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

2020-02-18 Thread Méven Car 
meven added a comment.


  In D27463#613114 , @davidre wrote:
  
  > If you touch the xsd do you neeed to increase the version number?
  
  
  That would be a good practice but how to publish a 1.1 version ?
  And what is the point ? I mean this xsd and associated dtd is AFAIK not 
really used (kcfg files are not even validated against it, in kconfig_compiler).
  This xsd is mostly living documentation.
  
  It was not done previously the 
  https://phabricator.kde.org/D7415

REPOSITORY
  R237 KConfig

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

To: meven, ervin, bport, crossi, #frameworks
Cc: davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

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

2020-02-17 Thread David Redondo 
davidre added a comment.


  If you touch the xsd do you neeed to increase the version number?

REPOSITORY
  R237 KConfig

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

To: meven, ervin, bport, crossi, #frameworks
Cc: davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

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

2020-02-17 Thread Méven Car 
meven updated this revision to Diff 75852.
meven added a comment.


  update xsd

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27463?vs=75837=75852

BRANCH
  master

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

AFFECTED FILES
  Testing/Temporary/CTestCostData.txt
  Testing/Temporary/LastTest.log
  autotests/kconfig_compiler/test4.cpp.ref
  autotests/kconfig_compiler/test4.kcfg
  src/core/kcoreconfigskeleton.cpp
  src/core/kcoreconfigskeleton.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigXmlParser.cpp
  src/kconfig_compiler/kcfg.xsd

To: meven, ervin, bport, crossi, #frameworks
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

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

2020-02-17 Thread Méven Car 
meven created this revision.
meven added reviewers: ervin, bport, crossi, Frameworks.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
meven requested review of this revision.

REVISION SUMMARY
  Allow to write choices such as :
  
  
  


  
  

TEST PLAN
  ctest

REPOSITORY
  R237 KConfig

BRANCH
  master

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

AFFECTED FILES
  autotests/kconfig_compiler/test4.cpp.ref
  autotests/kconfig_compiler/test4.kcfg
  src/core/kcoreconfigskeleton.cpp
  src/core/kcoreconfigskeleton.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigXmlParser.cpp

To: meven, ervin, bport, crossi, #frameworks
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns