D25149: Add a new template for KCMs

[Kevin Ottens]

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

INLINE COMMENTS

> %{APPNAMELC}settings.kcfg:8
> +  
> +
> +  The Exam

Now that I think about it, what about using both key and name here? To have a 
nice property name and a "stranger" storage key?
Most people seem to miss that feature somehow, that'd make it more obvious.

> Messages.sh:3
> +$EXTRACTRC `find . -name "*.ui"` >> rc.cpp || exit 11
> +$XGETTEXT `find . -name "*.cpp"` -o $podir/kcm5_%{APPNAMELC}.pot
> +rm -f rc.cpp

I'm talking mostly out of ignorance here, but shouldn't that harvest also the 
qml files somehow?

> kcm.cpp:30
> +
> +%{APPNAME}::%{APPNAME}(QObject* parent, const QVariantList& args)
> +: KQuickAddons::ManagedConfigModule(parent, args)

Ditto

> kcm.cpp:36
> +
> +KAboutData* about = new KAboutData(
> +QStringLiteral("kcm_%{APPNAMELC}"), i18n("%{APPNAME} Configuration 
> Module"),

Ditto

Beside that could use auto

> kcm.h:32
> +public:
> +%{APPNAME}(QObject* parent, const QVariantList& args);
> +virtual ~%{APPNAME}() override;

Nitpick: space should be before * and & not after.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: tcanabrava, #plasma, #frameworks, mart, ervin
Cc: #plasma, GB_2, yurchor, davidedmundson, ognarb, ervin, 
kde-frameworks-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
ragreen, michaelh, ZrenBot, ngraham, bruns, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25149: Add a new template for KCMs

[Tomaz Canabrava]

tcanabrava marked 2 inline comments as done.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: tcanabrava, #plasma, #frameworks, mart, ervin
Cc: #plasma, GB_2, yurchor, davidedmundson, ognarb, ervin, 
kde-frameworks-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
ragreen, michaelh, ZrenBot, ngraham, bruns, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25149: Add a new template for KCMs

[Tomaz Canabrava]

tcanabrava marked 6 inline comments as done.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: tcanabrava, #plasma, #frameworks, mart, ervin
Cc: #plasma, GB_2, yurchor, davidedmundson, ognarb, ervin, 
kde-frameworks-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
ragreen, michaelh, ZrenBot, ngraham, bruns, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25149: Add a new template for KCMs

[Tomaz Canabrava]

tcanabrava updated this revision to Diff 71009.
tcanabrava marked an inline comment as done.
tcanabrava added a comment.


  - Fix indentation

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25149?vs=69942=71009

BRANCH
  arcpatch-D25149

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

AFFECTED FILES
  templates/CMakeLists.txt
  templates/kcm-qml/%{APPNAMELC}settings.kcfg
  templates/kcm-qml/%{APPNAMELC}settings.kcfgc
  templates/kcm-qml/CMakeLists.txt
  templates/kcm-qml/Messages.sh
  templates/kcm-qml/README
  templates/kcm-qml/kcm-qml.kdevtemplate
  templates/kcm-qml/kcm.cpp
  templates/kcm-qml/kcm.h
  templates/kcm-qml/kcm_%{APPNAMELC}.desktop
  templates/kcm-qml/package/contents/ui/main.qml
  templates/kcm-qml/package/metadata.desktop

To: tcanabrava, #plasma, #frameworks, mart, ervin
Cc: #plasma, GB_2, yurchor, davidedmundson, ognarb, ervin, 
kde-frameworks-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
ragreen, michaelh, ZrenBot, ngraham, bruns, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25149: Add a new template for KCMs

[Kevin Ottens]

ervin added inline comments.

INLINE COMMENTS

> kcm.cpp:37
> +KAboutData* about = new KAboutData(
> +QStringLiteral("kcm_%{APPNAMELC}"), i18n("%{APPNAME} Configuration 
> Module"),
> +QStringLiteral("0.1"), QString(), KAboutLicense::GPL,

Nitpick: indentation looks broken here, I think it wasn't earlier.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: tcanabrava, #plasma, #frameworks, mart, ervin
Cc: #plasma, GB_2, yurchor, davidedmundson, ognarb, ervin, 
kde-frameworks-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
ragreen, michaelh, ZrenBot, ngraham, bruns, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25149: Add a new template for KCMs

[Tomaz Canabrava]

tcanabrava updated this revision to Diff 69942.
tcanabrava added a comment.


  - Handle immutability

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25149?vs=69941=69942

BRANCH
  arcpatch-D25149

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

AFFECTED FILES
  templates/CMakeLists.txt
  templates/kcm-qml/%{APPNAMELC}settings.kcfg
  templates/kcm-qml/%{APPNAMELC}settings.kcfgc
  templates/kcm-qml/CMakeLists.txt
  templates/kcm-qml/Messages.sh
  templates/kcm-qml/README
  templates/kcm-qml/kcm-qml.kdevtemplate
  templates/kcm-qml/kcm.cpp
  templates/kcm-qml/kcm.h
  templates/kcm-qml/kcm_%{APPNAMELC}.desktop
  templates/kcm-qml/package/contents/ui/main.qml
  templates/kcm-qml/package/metadata.desktop

To: tcanabrava, #plasma, #frameworks, mart, ervin
Cc: #plasma, GB_2, yurchor, davidedmundson, ognarb, ervin, 
kde-frameworks-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
ragreen, michaelh, ZrenBot, ngraham, bruns, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25149: Add a new template for KCMs

[Tomaz Canabrava]

tcanabrava updated this revision to Diff 69941.
tcanabrava marked an inline comment as done.
tcanabrava added a comment.


  - Adapt to ManagedConfigModule

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25149?vs=69649=69941

BRANCH
  arcpatch-D25149

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

AFFECTED FILES
  templates/CMakeLists.txt
  templates/kcm-qml/%{APPNAMELC}settings.kcfg
  templates/kcm-qml/%{APPNAMELC}settings.kcfgc
  templates/kcm-qml/CMakeLists.txt
  templates/kcm-qml/Messages.sh
  templates/kcm-qml/README
  templates/kcm-qml/kcm-qml.kdevtemplate
  templates/kcm-qml/kcm.cpp
  templates/kcm-qml/kcm.h
  templates/kcm-qml/kcm_%{APPNAMELC}.desktop
  templates/kcm-qml/package/contents/ui/main.qml
  templates/kcm-qml/package/metadata.desktop

To: tcanabrava, #plasma, #frameworks, mart, ervin
Cc: #plasma, GB_2, yurchor, davidedmundson, ognarb, ervin, 
kde-frameworks-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
ragreen, michaelh, ZrenBot, ngraham, bruns, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25149: Add a new template for KCMs

[Kevin Ottens]

ervin added inline comments.

INLINE COMMENTS

> tcanabrava wrote in kcm.cpp:43
> What's the correct form then? No need to connect the settings *at all*?

No need indeed... it's *magic*! ;-)

> tcanabrava wrote in main.qml:39
> The default fake setting will not be immutable, or you mean that you want me 
> to do one mutable and one immutable settings for the example?

In fact you can't predict that, any setting can be made immutable (it's user 
and sysadmin controlled). No need to introduce another one.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: tcanabrava, #plasma, #frameworks, mart, ervin
Cc: #plasma, GB_2, yurchor, davidedmundson, ognarb, ervin, 
kde-frameworks-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
ragreen, michaelh, ZrenBot, ngraham, bruns, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25149: Add a new template for KCMs

[Tomaz Canabrava]

tcanabrava added inline comments.

INLINE COMMENTS

> ervin wrote in kcm.cpp:43
> Shouldn't be needed anymore (and likely wrong in most cases).

What's the correct form then? No need to connect the settings *at all*?

> ervin wrote in main.qml:39
> What about disabling it if the setting is immutable?

The default fake setting will not be immutable, or you mean that you want me to 
do one mutable and one immutable settings for the example?

REPOSITORY
  R242 Plasma Framework (Library)

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

To: tcanabrava, #plasma, #frameworks, mart, ervin
Cc: #plasma, GB_2, yurchor, davidedmundson, ognarb, ervin, 
kde-frameworks-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
ragreen, michaelh, ZrenBot, ngraham, bruns, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25149: Add a new template for KCMs

[Kevin Ottens]

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

INLINE COMMENTS

> %{APPNAMELC}settings.kcfgc:5
> +DefaultValueGetters=true
> +GenerateProperties=true

You also want "ParentInConstructor=true" in here.

> kcm.cpp:32
> +: KQuickAddons::ConfigModule(parent, args)
> +, m_settings(new %{APPNAME}Settings())
> +{

Pass this as parent here (currently you're leaking it)

> kcm.cpp:43
> +
> +connect(m_settings, &%{APPNAME}Settings::configChanged, this, [this] { 
> setNeedsSave(true); });
> +

Shouldn't be needed anymore (and likely wrong in most cases).

> kcm.h:26
> +
> +class %{APPNAME} : public KQuickAddons::ConfigModule
> +{

This should inherit from ManagedConfigModule now.

> kcm.h:36
> +
> +public Q_SLOTS:
> +void load() override;

None of those slots are needed with a ManagedConfigModule (except if you need 
to do something outside the realm of the settings of course, which is not the 
case by default.

> main.qml:39
> +
> +QQC2.TextField {
> +text: kcm.settings.exampleSetting

What about disabling it if the setting is immutable?

REPOSITORY
  R242 Plasma Framework (Library)

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

To: tcanabrava, #plasma, #frameworks, mart, ervin
Cc: #plasma, GB_2, yurchor, davidedmundson, ognarb, ervin, 
kde-frameworks-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
ragreen, michaelh, ZrenBot, ngraham, bruns, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25149: Add a new template for KCMs

[Marco Martin]

mart accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  arcpatch-D25149

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

To: tcanabrava, #plasma, #frameworks, mart
Cc: #plasma, GB_2, yurchor, davidedmundson, ognarb, ervin, 
kde-frameworks-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
ragreen, michaelh, ZrenBot, ngraham, bruns, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25149: Add a new template for KCMs

[Tomaz Canabrava]

tcanabrava updated this revision to Diff 69649.
tcanabrava marked 2 inline comments as done.
tcanabrava added a comment.


  - Add KConfigXT
  - Remove wrong icon

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25149?vs=69400=69649

BRANCH
  arcpatch-D25149

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

AFFECTED FILES
  templates/CMakeLists.txt
  templates/kcm-qml/%{APPNAMELC}settings.kcfg
  templates/kcm-qml/%{APPNAMELC}settings.kcfgc
  templates/kcm-qml/CMakeLists.txt
  templates/kcm-qml/Messages.sh
  templates/kcm-qml/README
  templates/kcm-qml/kcm-qml.kdevtemplate
  templates/kcm-qml/kcm.cpp
  templates/kcm-qml/kcm.h
  templates/kcm-qml/kcm_%{APPNAMELC}.desktop
  templates/kcm-qml/package/contents/ui/main.qml
  templates/kcm-qml/package/metadata.desktop

To: tcanabrava, #plasma, #frameworks
Cc: #plasma, GB_2, yurchor, davidedmundson, ognarb, ervin, 
kde-frameworks-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
ragreen, michaelh, ZrenBot, ngraham, bruns, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25149: Add a new template for KCMs

[Björn Feber]

GB_2 added reviewers: Plasma, Frameworks.
GB_2 added a project: Plasma.
GB_2 added a subscriber: Plasma.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: tcanabrava, #plasma, #frameworks
Cc: #plasma, GB_2, yurchor, davidedmundson, ognarb, ervin, 
kde-frameworks-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
ragreen, michaelh, ZrenBot, ngraham, bruns, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25149: Add a new template for KCMs

[Björn Feber]

GB_2 added a comment.


  In D25149#559684 , @tcanabrava 
wrote:
  
  > We need a better icon. it's the icon for the `Plasmoid` template.
  
  
  Why not use the system settings icon as it's a KCM template?

REPOSITORY
  R242 Plasma Framework (Library)

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

To: tcanabrava
Cc: GB_2, yurchor, davidedmundson, ognarb, ervin, kde-frameworks-devel, 
LeGast00n, michaelh, ngraham, bruns

D25149: Add a new template for KCMs

[Nathaniel Graham]

ngraham retitled this revision from "Add a new Template for KCM's." to "Add a 
new template for KCMs".

REPOSITORY
  R242 Plasma Framework (Library)

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

To: tcanabrava
Cc: yurchor, davidedmundson, ognarb, ervin, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, ngraham, bruns