Changeset View
Standalone View
src/core/kcoreconfigskeleton.h
| Show First 20 Lines • Show All 779 Lines • ▼ Show 20 Line(s) | 775 | #endif | |||
|---|---|---|---|---|---|
| 780 | class KCONFIGCORE_EXPORT ItemEnum: public ItemInt | 780 | class KCONFIGCORE_EXPORT ItemEnum: public ItemInt | ||
| 781 | { | 781 | { | ||
| 782 | public: | 782 | public: | ||
| 783 | struct Choice { | 783 | struct Choice { | ||
| 784 | QString name; | 784 | QString name; | ||
| 785 | QString label; | 785 | QString label; | ||
| 786 | QString toolTip; | 786 | QString toolTip; | ||
| 787 | QString whatsThis; | 787 | QString whatsThis; | ||
| 788 | }; | 788 | }; | ||
meven: Something I have noticed while testing this.
Since it changes the memory of a very common data… | |||||
Oh right, stupid me, this obviously breaks binary compatibility, we need to find another way to store this. ervin: Oh right, stupid me, this obviously breaks binary compatibility, we need to find another way to… | |||||
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. ervin: Most likely place to hide it would be inside KConfigSkeletonItem's d-pointer (likely a hash… | |||||
| 789 | 789 | | |||
| 790 | /** @copydoc KConfigSkeletonGenericItem::KConfigSkeletonGenericItem | 790 | /** @copydoc KConfigSkeletonGenericItem::KConfigSkeletonGenericItem | ||
ervin: It's a struct you can drop the public: here. | |||||
| 791 | @param choices The list of enums that can be stored in this item | 791 | @param choices The list of enums that can be stored in this item | ||
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). ervin: There should be a new line before the opening curly brace.
I also wonder if it wouldn't be… | |||||
| 792 | */ | 792 | */ | ||
| 793 | ItemEnum(const QString &_group, const QString &_key, qint32 &reference, | 793 | ItemEnum(const QString &_group, const QString &_key, qint32 &reference, | ||
| 794 | const QList<Choice> &choices, qint32 defaultValue = 0); | 794 | const QList<Choice> &choices, qint32 defaultValue = 0); | ||
| 795 | 795 | | |||
| 796 | QList<Choice> choices() const; | 796 | QList<Choice> choices() const; | ||
| 797 | 797 | | |||
| 798 | /** @copydoc KConfigSkeletonItem::readConfig(KConfig*) */ | 798 | /** @copydoc KConfigSkeletonItem::readConfig(KConfig*) */ | ||
| 799 | void readConfig(KConfig *config) override; | 799 | void readConfig(KConfig *config) override; | ||
| 800 | 800 | | |||
| 801 | /** @copydoc KConfigSkeletonItem::writeConfig(KConfig*) */ | 801 | /** @copydoc KConfigSkeletonItem::writeConfig(KConfig*) */ | ||
| 802 | void writeConfig(KConfig *config) override; | 802 | void writeConfig(KConfig *config) override; | ||
| 803 | 803 | | |||
| 804 | // Source compatibility with 4.x | 804 | // Source compatibility with 4.x | ||
| 805 | // TODO KF6 remove | ||||
| 805 | typedef Choice Choice2; | 806 | typedef Choice Choice2; | ||
| 806 | QList<Choice> choices2() const; | 807 | QList<Choice> choices2() const; | ||
| 807 | 808 | | |||
| 809 | /** | ||||
| 810 | * Returns the value for for the choice with the given name | ||||
| 811 | */ | ||||
| 812 | QString valueForChoice(const 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. ervin: const QString &name
Probably worth adding a KF6 comment somewhere as well, because your first… | |||||
I have one in QString KCoreConfigSkeleton::ItemEnum::valueForChoice(QString name) const meven: 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... ervin: 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. meven: I expect those to be found through grep, and I had comments in the past to put it somewhere… | |||||
Well, regular comments don't go in the docs ;-) But cpp, why not. ervin: Well, regular comments don't go in the docs ;-)
(you need the triple slash or the double start… | |||||
| 813 | | ||||
| 814 | /** | ||||
| 815 | * Stores a choice value for name | ||||
| 816 | */ | ||||
| 817 | void setValueForChoice(const QString &name, const QString &valueForChoice); | ||||
ervin: const ref for the parameters | |||||
| 818 | | ||||
| 808 | private: | 819 | private: | ||
| 809 | QList<Choice> mChoices; | 820 | QList<Choice> mChoices; | ||
| 810 | }; | 821 | }; | ||
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. ervin: Nope, can't be here, this still breaks binary compatibility. As I mentioned in my last comment… | |||||
| 811 | 822 | | |||
| 812 | /** | 823 | /** | ||
| 813 | * Class for handling an unsigned 32-bit integer preferences item. | 824 | * Class for handling an unsigned 32-bit integer preferences item. | ||
| 814 | */ | 825 | */ | ||
| 815 | class KCONFIGCORE_EXPORT ItemUInt: public KConfigSkeletonGenericItem < quint32 > | 826 | class KCONFIGCORE_EXPORT ItemUInt: public KConfigSkeletonGenericItem < quint32 > | ||
| 816 | { | 827 | { | ||
| 817 | public: | 828 | public: | ||
| 818 | /** @copydoc KConfigSkeletonGenericItem::KConfigSkeletonGenericItem */ | 829 | /** @copydoc KConfigSkeletonGenericItem::KConfigSkeletonGenericItem */ | ||
| ▲ Show 20 Lines • Show All 820 Lines • Show Last 20 Lines | |||||
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.