D26041: Move applet configuration to KCM
This revision was automatically updated to reflect the committed changes. jgrulich marked an inline comment as done. Closed by commit R116:7601936e3642: Move applet configuration to KCM (authored by jgrulich). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D26041?vs=71785=71788#toc REPOSITORY R116 Plasma Network Management Applet CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D26041?vs=71785=71788 REVISION DETAIL https://phabricator.kde.org/D26041 AFFECTED FILES applet/contents/config/config.qml applet/contents/config/main.xml applet/contents/ui/configGeneral.qml applet/contents/ui/main.qml kcm/CMakeLists.txt kcm/qml/AddConnectionDialog.qml kcm/qml/ConfigurationDialog.qml kcm/qml/Dialog.qml kcm/qml/main.qml To: jgrulich, ngraham, #plasma Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D26041: Move applet configuration to KCM
ngraham accepted this revision. ngraham added a comment. This revision is now accepted and ready to land. All right, LGTM them! INLINE COMMENTS > jgrulich wrote in ConfigurationDialog.qml:55 > The Configuration class contains only static methods, which means you cannot > emit signal from them without passing the object or something like that. Maybe add a TODO comment explaining this then REPOSITORY R116 Plasma Network Management Applet BRANCH kcm-config REVISION DETAIL https://phabricator.kde.org/D26041 To: jgrulich, ngraham, #plasma Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D26041: Move applet configuration to KCM
jgrulich marked 4 inline comments as done. jgrulich added inline comments. INLINE COMMENTS > ngraham wrote in ConfigurationDialog.qml:55 > Then it should be given the NOTIFY property so it can be bound to The Configuration class contains only static methods, which means you cannot emit signal from them without passing the object or something like that. REPOSITORY R116 Plasma Network Management Applet REVISION DETAIL https://phabricator.kde.org/D26041 To: jgrulich, ngraham, #plasma Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D26041: Move applet configuration to KCM
jgrulich updated this revision to Diff 71785. jgrulich marked 2 inline comments as done. jgrulich added a comment. Simplify tooltips REPOSITORY R116 Plasma Network Management Applet CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D26041?vs=71707=71785 BRANCH kcm-config REVISION DETAIL https://phabricator.kde.org/D26041 AFFECTED FILES applet/contents/config/config.qml applet/contents/config/main.xml applet/contents/ui/configGeneral.qml applet/contents/ui/main.qml kcm/CMakeLists.txt kcm/qml/AddConnectionDialog.qml kcm/qml/ConfigurationDialog.qml kcm/qml/Dialog.qml kcm/qml/main.qml To: jgrulich, ngraham, #plasma Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D26041: Move applet configuration to KCM
ngraham added inline comments.
INLINE COMMENTS
> jgrulich wrote in ConfigurationDialog.qml:55
> I did it this way, because if I use checked: foo, then it reports warning
> about non-NOTIFYable property.
Then it should be given the NOTIFY property so it can be bound to
> jgrulich wrote in main.qml:205
> 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.
Gotta include the namespace, e.g.:
QQC2.toolTip.visible: hovered
QQC2.toolTip.text: i18n("blabla")
REPOSITORY
R116 Plasma Network Management Applet
REVISION DETAIL
https://phabricator.kde.org/D26041
To: jgrulich, ngraham, #plasma
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas,
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed,
jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D26041: Move applet configuration to KCM
jgrulich marked 10 inline comments as done. jgrulich added inline comments. INLINE COMMENTS > ngraham wrote in ConfigurationDialog.qml:55 > this shouldn't need to be in `Component.onCompleted:`; just set `checked` > directly as a binding: `checked: configuration.unlockModemOnDetection` I did it this way, because if I use checked: foo, then it reports warning about non-NOTIFYable property. > ngraham wrote in main.qml:205 > Use the attached ToolTip property instead of creating a whole object; it's > lighter that way 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. REPOSITORY R116 Plasma Network Management Applet REVISION DETAIL https://phabricator.kde.org/D26041 To: jgrulich, ngraham, #plasma Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D26041: Move applet configuration to KCM
jgrulich updated this revision to Diff 71707. jgrulich added a comment. Fix review comments REPOSITORY R116 Plasma Network Management Applet CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D26041?vs=71657=71707 BRANCH kcm-config REVISION DETAIL https://phabricator.kde.org/D26041 AFFECTED FILES applet/contents/config/config.qml applet/contents/config/main.xml applet/contents/ui/configGeneral.qml applet/contents/ui/main.qml kcm/CMakeLists.txt kcm/qml/AddConnectionDialog.qml kcm/qml/ConfigurationDialog.qml kcm/qml/Dialog.qml kcm/qml/main.qml To: jgrulich, ngraham, #plasma Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D26041: Move applet configuration to KCM
ngraham added a comment.
Lovely. Works great, and much better UI. I have just a few UI and code
suggestions:
INLINE COMMENTS
> ConfigurationDialog.qml:23
> +import QtQuick.Dialogs 1.2
> +import QtQuick.Controls 2.5 as QtControls
> +import org.kde.kirigami 2.5 as Kirigami
`as QQC2`
> ConfigurationDialog.qml:38
> +implicitHeight: 200
> +implicitWidth: 300
> +
A bit wider would be better so the window's title (which is quite long) doesn't
get elided
> ConfigurationDialog.qml:48
> +Kirigami.FormLayout {
> +anchors.left: parent.left
> +anchors.right: parent.right
a top padding of one gridUnit might make this look nicer
> ConfigurationDialog.qml:55
> +onClicked: okButton.enabled = true
> +Component.onCompleted: checked =
> configuration.unlockModemOnDetection
> +}
this shouldn't need to be in `Component.onCompleted:`; just set `checked`
directly as a binding: `checked: configuration.unlockModemOnDetection`
> ConfigurationDialog.qml:62
> +onClicked: okButton.enabled = true
> +Component.onCompleted: checked =
> configuration.manageVirtualConnections
> +}
ditto
> ConfigurationDialog.qml:71
> +right: parent.right
> +margins: Math.round(units.gridUnit / 2)
> +}
use `units.smallSpacing` instead
> ConfigurationDialog.qml:73
> +}
> +spacing: Math.round(units.gridUnit / 2)
> +
ditto
> main.qml:196
> +left: parent.left
> +margins: Math.round(units.gridUnit / 3)
> +}
`units.smallSpacing`
> main.qml:198
> +}
> +spacing: Math.round(units.gridUnit / 2)
> +
ditto
> main.qml:205
> +
> +QQC2.ToolTip {
> +text: i18n("Configuration")
Use the attached ToolTip property instead of creating a whole object; it's
lighter that way
REPOSITORY
R116 Plasma Network Management Applet
REVISION DETAIL
https://phabricator.kde.org/D26041
To: jgrulich, ngraham, #plasma
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas,
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed,
jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D26041: Move applet configuration to KCM
jgrulich created this revision. Herald added a project: Plasma. Herald added a subscriber: plasma-devel. jgrulich requested review of this revision. REVISION SUMMARY It doesn't make sense to have additional applet configuration, especially when the configuration is not related only to the applet. REPOSITORY R116 Plasma Network Management Applet BRANCH kcm-config REVISION DETAIL https://phabricator.kde.org/D26041 AFFECTED FILES applet/contents/config/config.qml applet/contents/config/main.xml applet/contents/ui/configGeneral.qml applet/contents/ui/main.qml kcm/CMakeLists.txt kcm/qml/AddConnectionDialog.qml kcm/qml/ConfigurationDialog.qml kcm/qml/Dialog.qml kcm/qml/main.qml To: jgrulich Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
