It doesn't make sense to have additional applet configuration, especially when
the configuration is not related only to the applet.
BUG: 407561
ngraham |
Plasma |
It doesn't make sense to have additional applet configuration, especially when
the configuration is not related only to the applet.
BUG: 407561
Automatic diff as part of commit; lint not applicable. |
Automatic diff as part of commit; unit tests not applicable. |
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 |
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. |
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. |
All right, LGTM them!
kcm/qml/ConfigurationDialog.qml | ||
---|---|---|
56 | Maybe add a TODO comment explaining this then |