Move applet configuration to KCM
ClosedPublic

Authored by jgrulich on Dec 16 2019, 9:51 AM.

Details

Summary

It doesn't make sense to have additional applet configuration, especially when
the configuration is not related only to the applet.

BUG: 407561

Diff Detail

Repository
R116 Plasma Network Management Applet
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
jgrulich created this revision.Dec 16 2019, 9:51 AM
Restricted Application added a project: Plasma. · View Herald TranscriptDec 16 2019, 9:51 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
jgrulich requested review of this revision.Dec 16 2019, 9:51 AM
jgrulich edited the summary of this revision. (Show Details)Dec 16 2019, 9:53 AM
jgrulich added reviewers: ngraham, Plasma.

Lovely. Works great, and much better UI. I have just a few UI and code suggestions:

kcm/qml/ConfigurationDialog.qml
24

as QQC2

39

A bit wider would be better so the window's title (which is quite long) doesn't get elided

49

a top padding of one gridUnit might make this look nicer

56

this shouldn't need to be in Component.onCompleted:; just set checked directly as a binding: checked: configuration.unlockModemOnDetection

63

ditto

72

use units.smallSpacing instead

74

ditto

kcm/qml/main.qml
193

units.smallSpacing

195

ditto

202

Use the attached ToolTip property instead of creating a whole object; it's lighter that way

jgrulich updated this revision to Diff 71707.Dec 17 2019, 10:06 AM

Fix review comments

jgrulich marked 10 inline comments as done.Dec 17 2019, 10:09 AM
jgrulich added inline comments.
kcm/qml/ConfigurationDialog.qml
56

I did it this way, because if I use checked: foo, then it reports warning about non-NOTIFYable property.

kcm/qml/main.qml
202

I tried, doesn't seem to work. I followed the example from https://doc-snapshots.qt.io/qt5-5.12/qml-qtquick-controls2-tooltip.html and it says there is no such attached property.

ngraham added inline comments.Dec 18 2019, 1:31 PM
kcm/qml/ConfigurationDialog.qml
56

Then it should be given the NOTIFY property so it can be bound to

kcm/qml/main.qml
202

Gotta include the namespace, e.g.:

QQC2.toolTip.visible: hovered
QQC2.toolTip.text: i18n("blabla")
jgrulich updated this revision to Diff 71785.Dec 18 2019, 1:42 PM
jgrulich marked 2 inline comments as done.

Simplify tooltips

jgrulich marked 4 inline comments as done.Dec 18 2019, 1:43 PM
jgrulich added inline comments.
kcm/qml/ConfigurationDialog.qml
56

The Configuration class contains only static methods, which means you cannot emit signal from them without passing the object or something like that.

ngraham accepted this revision.Dec 18 2019, 1:44 PM

All right, LGTM them!

kcm/qml/ConfigurationDialog.qml
56

Maybe add a TODO comment explaining this then

This revision is now accepted and ready to land.Dec 18 2019, 1:44 PM
This revision was automatically updated to reflect the committed changes.
jgrulich marked an inline comment as done.