Re: Review Request 119455: make Button a QtControl
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119455/#review63110 --- I think this is nearly good to go. A few questions below. src/declarativeimports/core/iconitem.cpp https://git.reviewboard.kde.org/r/119455/#comment43857 Do we want to clear the current icon when this happens? If so we have to start the loadPixmap timer at the bottom of the function I think. src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml https://git.reviewboard.kde.org/r/119455/#comment43858 I was thinking this can become just property real minimumWidth: Layout.minimumWidth Also this is currently wrong; we're adding style.padding.left as the gap between icon + label, but the RowLayout spacing is units.smallSpacing, so they are different src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml https://git.reviewboard.kde.org/r/119455/#comment43860 I'd use Layout.alignment: Qt::AlignVCenter so we don't mix and match anchors layouts in a bit of code. up to you. - David Edmundson On July 24, 2014, 6:17 p.m., Marco Martin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119455/ --- (Updated July 24, 2014, 6:17 p.m.) Review request for KDE Frameworks and Plasma. Repository: plasma-framework Description --- this makes Button inherit from the QtControl and annd an accompaining plasma-looking theme now, the ugly part of the patch: iconSource is an url, so it screws up passing freedesktop compatible names (it expects names of pngs local to the qml file directory, testimony of the main platform target for controls actually being android/ios). one solution may be overriding iconSource as a normal string, but i would like the theme working also on a plain Button, so it extract only the filename from the url. Diffs - examples/applets/widgetgallery/contents/ui/Buttons.qml 9134df9 src/declarativeimports/core/iconitem.cpp 38012cc src/declarativeimports/plasmacomponents/qml/Button.qml 262e20f src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml PRE-CREATION Diff: https://git.reviewboard.kde.org/r/119455/diff/ Testing --- in both a normal plasma session or the widget gallery buttons work fine, painting is 100% identical Thanks, Marco Martin ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119455: make Button a QtControl
On July 25, 2014, 8:13 a.m., David Edmundson wrote: src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml, line 42 https://git.reviewboard.kde.org/r/119455/diff/3/?file=292514#file292514line42 I was thinking this can become just property real minimumWidth: Layout.minimumWidth Also this is currently wrong; we're adding style.padding.left as the gap between icon + label, but the RowLayout spacing is units.smallSpacing, so they are different yeah, i tried thatdoesn't seem to report correct values. I think in order to do that i need to set a fixed Layout.minimumWidth to the label, but would be a quite arbitrary value, like 10 msizes or so - Marco --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119455/#review63110 --- On July 24, 2014, 6:17 p.m., Marco Martin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119455/ --- (Updated July 24, 2014, 6:17 p.m.) Review request for KDE Frameworks and Plasma. Repository: plasma-framework Description --- this makes Button inherit from the QtControl and annd an accompaining plasma-looking theme now, the ugly part of the patch: iconSource is an url, so it screws up passing freedesktop compatible names (it expects names of pngs local to the qml file directory, testimony of the main platform target for controls actually being android/ios). one solution may be overriding iconSource as a normal string, but i would like the theme working also on a plain Button, so it extract only the filename from the url. Diffs - examples/applets/widgetgallery/contents/ui/Buttons.qml 9134df9 src/declarativeimports/core/iconitem.cpp 38012cc src/declarativeimports/plasmacomponents/qml/Button.qml 262e20f src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml PRE-CREATION Diff: https://git.reviewboard.kde.org/r/119455/diff/ Testing --- in both a normal plasma session or the widget gallery buttons work fine, painting is 100% identical Thanks, Marco Martin ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119455: make Button a QtControl
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119455/ --- (Updated July 25, 2014, 9:41 a.m.) Review request for KDE Frameworks and Plasma. Repository: plasma-framework Description --- this makes Button inherit from the QtControl and annd an accompaining plasma-looking theme now, the ugly part of the patch: iconSource is an url, so it screws up passing freedesktop compatible names (it expects names of pngs local to the qml file directory, testimony of the main platform target for controls actually being android/ios). one solution may be overriding iconSource as a normal string, but i would like the theme working also on a plain Button, so it extract only the filename from the url. Diffs (updated) - examples/applets/widgetgallery/contents/ui/Buttons.qml 9134df9 src/declarativeimports/core/iconitem.cpp 38012cc src/declarativeimports/plasmacomponents/qml/Button.qml 262e20f src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml PRE-CREATION Diff: https://git.reviewboard.kde.org/r/119455/diff/ Testing --- in both a normal plasma session or the widget gallery buttons work fine, painting is 100% identical Thanks, Marco Martin ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119455: make Button a QtControl
On July 25, 2014, 8:13 a.m., David Edmundson wrote: src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml, line 42 https://git.reviewboard.kde.org/r/119455/diff/3/?file=292514#file292514line42 I was thinking this can become just property real minimumWidth: Layout.minimumWidth Also this is currently wrong; we're adding style.padding.left as the gap between icon + label, but the RowLayout spacing is units.smallSpacing, so they are different Marco Martin wrote: yeah, i tried thatdoesn't seem to report correct values. I think in order to do that i need to set a fixed Layout.minimumWidth to the label, but would be a quite arbitrary value, like 10 msizes or so it's currently label.implicitWidth, so we'd want to still use that instead of something arbitrary? actually maybe that makes this: property real minimumWidth: Layout.preferredWidth instead of what I said. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119455/#review63110 --- On July 25, 2014, 9:41 a.m., Marco Martin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119455/ --- (Updated July 25, 2014, 9:41 a.m.) Review request for KDE Frameworks and Plasma. Repository: plasma-framework Description --- this makes Button inherit from the QtControl and annd an accompaining plasma-looking theme now, the ugly part of the patch: iconSource is an url, so it screws up passing freedesktop compatible names (it expects names of pngs local to the qml file directory, testimony of the main platform target for controls actually being android/ios). one solution may be overriding iconSource as a normal string, but i would like the theme working also on a plain Button, so it extract only the filename from the url. Diffs - examples/applets/widgetgallery/contents/ui/Buttons.qml 9134df9 src/declarativeimports/core/iconitem.cpp 38012cc src/declarativeimports/plasmacomponents/qml/Button.qml 262e20f src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml PRE-CREATION Diff: https://git.reviewboard.kde.org/r/119455/diff/ Testing --- in both a normal plasma session or the widget gallery buttons work fine, painting is 100% identical Thanks, Marco Martin ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119455: make Button a QtControl
On July 25, 2014, 8:13 a.m., David Edmundson wrote: src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml, line 42 https://git.reviewboard.kde.org/r/119455/diff/3/?file=292514#file292514line42 I was thinking this can become just property real minimumWidth: Layout.minimumWidth Also this is currently wrong; we're adding style.padding.left as the gap between icon + label, but the RowLayout spacing is units.smallSpacing, so they are different Marco Martin wrote: yeah, i tried thatdoesn't seem to report correct values. I think in order to do that i need to set a fixed Layout.minimumWidth to the label, but would be a quite arbitrary value, like 10 msizes or so David Edmundson wrote: it's currently label.implicitWidth, so we'd want to still use that instead of something arbitrary? actually maybe that makes this: property real minimumWidth: Layout.preferredWidth instead of what I said. right now is the minimum between its implicit width and that 10 characters arbitrary (rationale: not having buttons overkill large by default) instead, Layout.preferredWidth seems to be -1 - Marco --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119455/#review63110 --- On July 25, 2014, 9:41 a.m., Marco Martin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119455/ --- (Updated July 25, 2014, 9:41 a.m.) Review request for KDE Frameworks and Plasma. Repository: plasma-framework Description --- this makes Button inherit from the QtControl and annd an accompaining plasma-looking theme now, the ugly part of the patch: iconSource is an url, so it screws up passing freedesktop compatible names (it expects names of pngs local to the qml file directory, testimony of the main platform target for controls actually being android/ios). one solution may be overriding iconSource as a normal string, but i would like the theme working also on a plain Button, so it extract only the filename from the url. Diffs - examples/applets/widgetgallery/contents/ui/Buttons.qml 9134df9 src/declarativeimports/core/iconitem.cpp 38012cc src/declarativeimports/plasmacomponents/qml/Button.qml 262e20f src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml PRE-CREATION Diff: https://git.reviewboard.kde.org/r/119455/diff/ Testing --- in both a normal plasma session or the widget gallery buttons work fine, painting is 100% identical Thanks, Marco Martin ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119455: make Button a QtControl
On July 25, 2014, 8:13 a.m., David Edmundson wrote: src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml, line 42 https://git.reviewboard.kde.org/r/119455/diff/3/?file=292514#file292514line42 I was thinking this can become just property real minimumWidth: Layout.minimumWidth Also this is currently wrong; we're adding style.padding.left as the gap between icon + label, but the RowLayout spacing is units.smallSpacing, so they are different Marco Martin wrote: yeah, i tried thatdoesn't seem to report correct values. I think in order to do that i need to set a fixed Layout.minimumWidth to the label, but would be a quite arbitrary value, like 10 msizes or so David Edmundson wrote: it's currently label.implicitWidth, so we'd want to still use that instead of something arbitrary? actually maybe that makes this: property real minimumWidth: Layout.preferredWidth instead of what I said. Marco Martin wrote: right now is the minimum between its implicit width and that 10 characters arbitrary (rationale: not having buttons overkill large by default) instead, Layout.preferredWidth seems to be -1 but handling that is what line 116 is for Math.max(theme.mSize(theme.defaultFont).width*12, style.minimumWidth); - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119455/#review63110 --- On July 25, 2014, 9:41 a.m., Marco Martin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119455/ --- (Updated July 25, 2014, 9:41 a.m.) Review request for KDE Frameworks and Plasma. Repository: plasma-framework Description --- this makes Button inherit from the QtControl and annd an accompaining plasma-looking theme now, the ugly part of the patch: iconSource is an url, so it screws up passing freedesktop compatible names (it expects names of pngs local to the qml file directory, testimony of the main platform target for controls actually being android/ios). one solution may be overriding iconSource as a normal string, but i would like the theme working also on a plain Button, so it extract only the filename from the url. Diffs - examples/applets/widgetgallery/contents/ui/Buttons.qml 9134df9 src/declarativeimports/core/iconitem.cpp 38012cc src/declarativeimports/plasmacomponents/qml/Button.qml 262e20f src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml PRE-CREATION Diff: https://git.reviewboard.kde.org/r/119455/diff/ Testing --- in both a normal plasma session or the widget gallery buttons work fine, painting is 100% identical Thanks, Marco Martin ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119455: make Button a QtControl
On July 25, 2014, 8:13 a.m., David Edmundson wrote: src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml, line 42 https://git.reviewboard.kde.org/r/119455/diff/3/?file=292514#file292514line42 I was thinking this can become just property real minimumWidth: Layout.minimumWidth Also this is currently wrong; we're adding style.padding.left as the gap between icon + label, but the RowLayout spacing is units.smallSpacing, so they are different Marco Martin wrote: yeah, i tried thatdoesn't seem to report correct values. I think in order to do that i need to set a fixed Layout.minimumWidth to the label, but would be a quite arbitrary value, like 10 msizes or so David Edmundson wrote: it's currently label.implicitWidth, so we'd want to still use that instead of something arbitrary? actually maybe that makes this: property real minimumWidth: Layout.preferredWidth instead of what I said. Marco Martin wrote: right now is the minimum between its implicit width and that 10 characters arbitrary (rationale: not having buttons overkill large by default) instead, Layout.preferredWidth seems to be -1 David Edmundson wrote: but handling that is what line 116 is for Math.max(theme.mSize(theme.defaultFont).width*12, style.minimumWidth); that's max, not min, is the part that sets as implicit width a certain size, so if buttons have short text, they have all the same size, so nice grid and all that (but not enforced by Layout, so they can still be squshed a bit by the client code, if needed) - Marco --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119455/#review63110 --- On July 25, 2014, 9:41 a.m., Marco Martin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119455/ --- (Updated July 25, 2014, 9:41 a.m.) Review request for KDE Frameworks and Plasma. Repository: plasma-framework Description --- this makes Button inherit from the QtControl and annd an accompaining plasma-looking theme now, the ugly part of the patch: iconSource is an url, so it screws up passing freedesktop compatible names (it expects names of pngs local to the qml file directory, testimony of the main platform target for controls actually being android/ios). one solution may be overriding iconSource as a normal string, but i would like the theme working also on a plain Button, so it extract only the filename from the url. Diffs - examples/applets/widgetgallery/contents/ui/Buttons.qml 9134df9 src/declarativeimports/core/iconitem.cpp 38012cc src/declarativeimports/plasmacomponents/qml/Button.qml 262e20f src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml PRE-CREATION Diff: https://git.reviewboard.kde.org/r/119455/diff/ Testing --- in both a normal plasma session or the widget gallery buttons work fine, painting is 100% identical Thanks, Marco Martin ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119455: make Button a QtControl
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119455/#review63127 --- src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml https://git.reviewboard.kde.org/r/119455/#comment43882 Not valid anymore. src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml https://git.reviewboard.kde.org/r/119455/#comment43881 This causes a behavioural change: look at line 102 of the old Button.qml the minimumWidth was the size of the full text + margins. - David Edmundson On July 25, 2014, 9:41 a.m., Marco Martin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119455/ --- (Updated July 25, 2014, 9:41 a.m.) Review request for KDE Frameworks and Plasma. Repository: plasma-framework Description --- this makes Button inherit from the QtControl and annd an accompaining plasma-looking theme now, the ugly part of the patch: iconSource is an url, so it screws up passing freedesktop compatible names (it expects names of pngs local to the qml file directory, testimony of the main platform target for controls actually being android/ios). one solution may be overriding iconSource as a normal string, but i would like the theme working also on a plain Button, so it extract only the filename from the url. Diffs - examples/applets/widgetgallery/contents/ui/Buttons.qml 9134df9 src/declarativeimports/core/iconitem.cpp 38012cc src/declarativeimports/plasmacomponents/qml/Button.qml 262e20f src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml PRE-CREATION Diff: https://git.reviewboard.kde.org/r/119455/diff/ Testing --- in both a normal plasma session or the widget gallery buttons work fine, painting is 100% identical Thanks, Marco Martin ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119455: make Button a QtControl
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119455/ --- (Updated July 25, 2014, 10:55 a.m.) Review request for KDE Frameworks and Plasma. Repository: plasma-framework Description --- this makes Button inherit from the QtControl and annd an accompaining plasma-looking theme now, the ugly part of the patch: iconSource is an url, so it screws up passing freedesktop compatible names (it expects names of pngs local to the qml file directory, testimony of the main platform target for controls actually being android/ios). one solution may be overriding iconSource as a normal string, but i would like the theme working also on a plain Button, so it extract only the filename from the url. Diffs (updated) - examples/applets/widgetgallery/contents/ui/Buttons.qml 9134df9 src/declarativeimports/core/iconitem.cpp 38012cc src/declarativeimports/plasmacomponents/qml/Button.qml 262e20f src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml PRE-CREATION Diff: https://git.reviewboard.kde.org/r/119455/diff/ Testing --- in both a normal plasma session or the widget gallery buttons work fine, painting is 100% identical Thanks, Marco Martin ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119455: make Button a QtControl
On July 25, 2014, 10:44 a.m., David Edmundson wrote: src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml, line 76 https://git.reviewboard.kde.org/r/119455/diff/4/?file=292603#file292603line76 This causes a behavioural change: look at line 102 of the old Button.qml the minimumWidth was the size of the full text + margins. right, so let's use just implicitWidth.. last version seems to produce same results for sizing - Marco --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119455/#review63127 --- On July 25, 2014, 10:55 a.m., Marco Martin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119455/ --- (Updated July 25, 2014, 10:55 a.m.) Review request for KDE Frameworks and Plasma. Repository: plasma-framework Description --- this makes Button inherit from the QtControl and annd an accompaining plasma-looking theme now, the ugly part of the patch: iconSource is an url, so it screws up passing freedesktop compatible names (it expects names of pngs local to the qml file directory, testimony of the main platform target for controls actually being android/ios). one solution may be overriding iconSource as a normal string, but i would like the theme working also on a plain Button, so it extract only the filename from the url. Diffs - examples/applets/widgetgallery/contents/ui/Buttons.qml 9134df9 src/declarativeimports/core/iconitem.cpp 38012cc src/declarativeimports/plasmacomponents/qml/Button.qml 262e20f src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml PRE-CREATION Diff: https://git.reviewboard.kde.org/r/119455/diff/ Testing --- in both a normal plasma session or the widget gallery buttons work fine, painting is 100% identical Thanks, Marco Martin ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119455: make Button a QtControl
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119455/#review63130 --- Ship it! Ship It! - David Edmundson On July 25, 2014, 10:55 a.m., Marco Martin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119455/ --- (Updated July 25, 2014, 10:55 a.m.) Review request for KDE Frameworks and Plasma. Repository: plasma-framework Description --- this makes Button inherit from the QtControl and annd an accompaining plasma-looking theme now, the ugly part of the patch: iconSource is an url, so it screws up passing freedesktop compatible names (it expects names of pngs local to the qml file directory, testimony of the main platform target for controls actually being android/ios). one solution may be overriding iconSource as a normal string, but i would like the theme working also on a plain Button, so it extract only the filename from the url. Diffs - examples/applets/widgetgallery/contents/ui/Buttons.qml 9134df9 src/declarativeimports/core/iconitem.cpp 38012cc src/declarativeimports/plasmacomponents/qml/Button.qml 262e20f src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml PRE-CREATION Diff: https://git.reviewboard.kde.org/r/119455/diff/ Testing --- in both a normal plasma session or the widget gallery buttons work fine, painting is 100% identical Thanks, Marco Martin ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119455: make Button a QtControl
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119455/ --- (Updated July 25, 2014, 11:05 a.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks and Plasma. Repository: plasma-framework Description --- this makes Button inherit from the QtControl and annd an accompaining plasma-looking theme now, the ugly part of the patch: iconSource is an url, so it screws up passing freedesktop compatible names (it expects names of pngs local to the qml file directory, testimony of the main platform target for controls actually being android/ios). one solution may be overriding iconSource as a normal string, but i would like the theme working also on a plain Button, so it extract only the filename from the url. Diffs - examples/applets/widgetgallery/contents/ui/Buttons.qml 9134df9 src/declarativeimports/core/iconitem.cpp 38012cc src/declarativeimports/plasmacomponents/qml/Button.qml 262e20f src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml PRE-CREATION Diff: https://git.reviewboard.kde.org/r/119455/diff/ Testing --- in both a normal plasma session or the widget gallery buttons work fine, painting is 100% identical Thanks, Marco Martin ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request 119455: make Button a QtControl
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119455/ --- Review request for KDE Frameworks and Plasma. Repository: plasma-framework Description --- this makes Button inherit from the QtControl and annd an accompaining plasma-looking theme now, the ugly part of the patch: iconSource is an url, so it screws up passing freedesktop compatible names (it expects names of pngs local to the qml file directory, testimony of the main platform target for controls actually being android/ios). one solution may be overriding iconSource as a normal string, but i would like the theme working also on a plain Button, so it extract only the filename from the url. Diffs - src/declarativeimports/core/iconitem.cpp 38012cc src/declarativeimports/plasmacomponents/qml/Button.qml 262e20f src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml PRE-CREATION Diff: https://git.reviewboard.kde.org/r/119455/diff/ Testing --- in both a normal plasma session or the widget gallery buttons work fine, painting is 100% identical Thanks, Marco Martin ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119455: make Button a QtControl
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119455/#review63086 --- Exciting! Might this eventually support a ButtonStyle.qml in the plasma theme that overrides the svg-based ButtonStyle if it's present? That way we could take the already developed Breeze ButtonStyle.qml and just ship it in the plasma theme. - Andrew Lake On July 24, 2014, 3:55 p.m., Marco Martin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119455/ --- (Updated July 24, 2014, 3:55 p.m.) Review request for KDE Frameworks and Plasma. Repository: plasma-framework Description --- this makes Button inherit from the QtControl and annd an accompaining plasma-looking theme now, the ugly part of the patch: iconSource is an url, so it screws up passing freedesktop compatible names (it expects names of pngs local to the qml file directory, testimony of the main platform target for controls actually being android/ios). one solution may be overriding iconSource as a normal string, but i would like the theme working also on a plain Button, so it extract only the filename from the url. Diffs - src/declarativeimports/core/iconitem.cpp 38012cc src/declarativeimports/plasmacomponents/qml/Button.qml 262e20f src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml PRE-CREATION Diff: https://git.reviewboard.kde.org/r/119455/diff/ Testing --- in both a normal plasma session or the widget gallery buttons work fine, painting is 100% identical Thanks, Marco Martin ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119455: make Button a QtControl
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119455/#review63087 --- Concerning the icon names: for the Desktops Effects KCM the names are working without me doing anything. So there must be a solution hidden in the widgets emulating style. - Martin Gräßlin On July 24, 2014, 5:55 p.m., Marco Martin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119455/ --- (Updated July 24, 2014, 5:55 p.m.) Review request for KDE Frameworks and Plasma. Repository: plasma-framework Description --- this makes Button inherit from the QtControl and annd an accompaining plasma-looking theme now, the ugly part of the patch: iconSource is an url, so it screws up passing freedesktop compatible names (it expects names of pngs local to the qml file directory, testimony of the main platform target for controls actually being android/ios). one solution may be overriding iconSource as a normal string, but i would like the theme working also on a plain Button, so it extract only the filename from the url. Diffs - src/declarativeimports/core/iconitem.cpp 38012cc src/declarativeimports/plasmacomponents/qml/Button.qml 262e20f src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml PRE-CREATION Diff: https://git.reviewboard.kde.org/r/119455/diff/ Testing --- in both a normal plasma session or the widget gallery buttons work fine, painting is 100% identical Thanks, Marco Martin ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119455: make Button a QtControl
On July 24, 2014, 4:18 p.m., Andrew Lake wrote: Exciting! Might this eventually support a ButtonStyle.qml in the plasma theme that overrides the svg-based ButtonStyle if it's present? That way we could take the already developed Breeze ButtonStyle.qml and just ship it in the plasma theme. perhaps. that would be a step after when the set is more or less complete tough one thing that makes me a bit hesitant tough is how too much logic still needs to be in the styles, very easy to break stuff - Marco --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119455/#review63086 --- On July 24, 2014, 3:55 p.m., Marco Martin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119455/ --- (Updated July 24, 2014, 3:55 p.m.) Review request for KDE Frameworks and Plasma. Repository: plasma-framework Description --- this makes Button inherit from the QtControl and annd an accompaining plasma-looking theme now, the ugly part of the patch: iconSource is an url, so it screws up passing freedesktop compatible names (it expects names of pngs local to the qml file directory, testimony of the main platform target for controls actually being android/ios). one solution may be overriding iconSource as a normal string, but i would like the theme working also on a plain Button, so it extract only the filename from the url. Diffs - src/declarativeimports/core/iconitem.cpp 38012cc src/declarativeimports/plasmacomponents/qml/Button.qml 262e20f src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml PRE-CREATION Diff: https://git.reviewboard.kde.org/r/119455/diff/ Testing --- in both a normal plasma session or the widget gallery buttons work fine, painting is 100% identical Thanks, Marco Martin ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119455: make Button a QtControl
On July 24, 2014, 4:20 p.m., Martin Gräßlin wrote: Concerning the icon names: for the Desktops Effects KCM the names are working without me doing anything. So there must be a solution hidden in the widgets emulating style. yes, the native style figures out that internally, creates an internal qaction, and then uses the icon of the qaction. it has a couple of drawbacks tough, it's internal api, so may break anytime, and returns a qicon, while i actually need the name, to use the icon from the plasma theme when available - Marco --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119455/#review63087 --- On July 24, 2014, 3:55 p.m., Marco Martin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119455/ --- (Updated July 24, 2014, 3:55 p.m.) Review request for KDE Frameworks and Plasma. Repository: plasma-framework Description --- this makes Button inherit from the QtControl and annd an accompaining plasma-looking theme now, the ugly part of the patch: iconSource is an url, so it screws up passing freedesktop compatible names (it expects names of pngs local to the qml file directory, testimony of the main platform target for controls actually being android/ios). one solution may be overriding iconSource as a normal string, but i would like the theme working also on a plain Button, so it extract only the filename from the url. Diffs - src/declarativeimports/core/iconitem.cpp 38012cc src/declarativeimports/plasmacomponents/qml/Button.qml 262e20f src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml PRE-CREATION Diff: https://git.reviewboard.kde.org/r/119455/diff/ Testing --- in both a normal plasma session or the widget gallery buttons work fine, painting is 100% identical Thanks, Marco Martin ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119455: make Button a QtControl
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119455/ --- (Updated July 24, 2014, 4:31 p.m.) Review request for KDE Frameworks and Plasma. Changes --- better rendering for the menu arrow, support checked buttons Repository: plasma-framework Description --- this makes Button inherit from the QtControl and annd an accompaining plasma-looking theme now, the ugly part of the patch: iconSource is an url, so it screws up passing freedesktop compatible names (it expects names of pngs local to the qml file directory, testimony of the main platform target for controls actually being android/ios). one solution may be overriding iconSource as a normal string, but i would like the theme working also on a plain Button, so it extract only the filename from the url. Diffs (updated) - examples/applets/widgetgallery/contents/ui/Buttons.qml 9134df9 src/declarativeimports/core/iconitem.cpp 38012cc src/declarativeimports/plasmacomponents/qml/Button.qml 262e20f src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml PRE-CREATION Diff: https://git.reviewboard.kde.org/r/119455/diff/ Testing --- in both a normal plasma session or the widget gallery buttons work fine, painting is 100% identical Thanks, Marco Martin ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119455: make Button a QtControl
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119455/#review63091
---
src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml
https://git.reviewboard.kde.org/r/119455/#comment43833
If you change Row to RowLayout this could be worked out for you.
Layouts also have attached LayoutProperties on them. IMHO this makes the
width calculation in the Button super easier too (just Layout.fillWidth: true)
src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml
https://git.reviewboard.kde.org/r/119455/#comment43834
This isn't very declarative, is there a reason we can't do:
Button
{
style: ButtonStyle{}
property alias minimumWidth: style.minimumWidth
}
and in this file
property alias minimumWidth: buttonContent.minimumWidth
same for height
src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml
https://git.reviewboard.kde.org/r/119455/#comment43836
if you want fd-o icons, generally you use iconName instead of iconSource.
We may need some changes for source compatibility, but that should be in
the Button rather than the style I think.
src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml
https://git.reviewboard.kde.org/r/119455/#comment43835
For the style I think we just want to do:
source: control.iconName || control.iconSource
as I think IconItem internally can handle both and choose the right thing
src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml
https://git.reviewboard.kde.org/r/119455/#comment43837
style.labelImplicitWidth doesn't exist?
- David Edmundson
On July 24, 2014, 4:31 p.m., Marco Martin wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119455/
---
(Updated July 24, 2014, 4:31 p.m.)
Review request for KDE Frameworks and Plasma.
Repository: plasma-framework
Description
---
this makes Button inherit from the QtControl and annd an accompaining
plasma-looking theme
now, the ugly part of the patch:
iconSource is an url, so it screws up passing freedesktop compatible names
(it expects names of pngs local to the qml file directory, testimony of the
main platform target for controls actually being android/ios). one solution
may be overriding iconSource as a normal string, but i would like the theme
working also on a plain Button, so it extract only the filename from the url.
Diffs
-
examples/applets/widgetgallery/contents/ui/Buttons.qml 9134df9
src/declarativeimports/core/iconitem.cpp 38012cc
src/declarativeimports/plasmacomponents/qml/Button.qml 262e20f
src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml
PRE-CREATION
Diff: https://git.reviewboard.kde.org/r/119455/diff/
Testing
---
in both a normal plasma session or the widget gallery buttons work fine,
painting is 100% identical
Thanks,
Marco Martin
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119455: make Button a QtControl
On July 24, 2014, 4:56 p.m., David Edmundson wrote:
src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml, line 41
https://git.reviewboard.kde.org/r/119455/diff/2/?file=292510#file292510line41
This isn't very declarative, is there a reason we can't do:
Button
{
style: ButtonStyle{}
property alias minimumWidth: style.minimumWidth
}
and in this file
property alias minimumWidth: buttonContent.minimumWidth
same for height
as far i know, the only way to access the style is with like minimumWidth:
__style.minimumWidth
was to avoid the private property since the __
if we are sure __style is not going to be removed, sine, but i would be not so
sure of that
On July 24, 2014, 4:56 p.m., David Edmundson wrote:
src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml, line 58
https://git.reviewboard.kde.org/r/119455/diff/2/?file=292510#file292510line58
if you want fd-o icons, generally you use iconName instead of
iconSource.
We may need some changes for source compatibility, but that should be
in the Button rather than the style I think.
I'll have to override iconsource to be an alias of iconname then, since all the
users use only iconSource
- Marco
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119455/#review63091
---
On July 24, 2014, 4:31 p.m., Marco Martin wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119455/
---
(Updated July 24, 2014, 4:31 p.m.)
Review request for KDE Frameworks and Plasma.
Repository: plasma-framework
Description
---
this makes Button inherit from the QtControl and annd an accompaining
plasma-looking theme
now, the ugly part of the patch:
iconSource is an url, so it screws up passing freedesktop compatible names
(it expects names of pngs local to the qml file directory, testimony of the
main platform target for controls actually being android/ios). one solution
may be overriding iconSource as a normal string, but i would like the theme
working also on a plain Button, so it extract only the filename from the url.
Diffs
-
examples/applets/widgetgallery/contents/ui/Buttons.qml 9134df9
src/declarativeimports/core/iconitem.cpp 38012cc
src/declarativeimports/plasmacomponents/qml/Button.qml 262e20f
src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml
PRE-CREATION
Diff: https://git.reviewboard.kde.org/r/119455/diff/
Testing
---
in both a normal plasma session or the widget gallery buttons work fine,
painting is 100% identical
Thanks,
Marco Martin
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119455: make Button a QtControl
On July 24, 2014, 4:56 p.m., David Edmundson wrote: src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml, line 83 https://git.reviewboard.kde.org/r/119455/diff/2/?file=292510#file292510line83 style.labelImplicitWidth doesn't exist? gah, leftover - Marco --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119455/#review63091 --- On July 24, 2014, 4:31 p.m., Marco Martin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119455/ --- (Updated July 24, 2014, 4:31 p.m.) Review request for KDE Frameworks and Plasma. Repository: plasma-framework Description --- this makes Button inherit from the QtControl and annd an accompaining plasma-looking theme now, the ugly part of the patch: iconSource is an url, so it screws up passing freedesktop compatible names (it expects names of pngs local to the qml file directory, testimony of the main platform target for controls actually being android/ios). one solution may be overriding iconSource as a normal string, but i would like the theme working also on a plain Button, so it extract only the filename from the url. Diffs - examples/applets/widgetgallery/contents/ui/Buttons.qml 9134df9 src/declarativeimports/core/iconitem.cpp 38012cc src/declarativeimports/plasmacomponents/qml/Button.qml 262e20f src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml PRE-CREATION Diff: https://git.reviewboard.kde.org/r/119455/diff/ Testing --- in both a normal plasma session or the widget gallery buttons work fine, painting is 100% identical Thanks, Marco Martin ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119455: make Button a QtControl
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119455/ --- (Updated July 24, 2014, 6:17 p.m.) Review request for KDE Frameworks and Plasma. Repository: plasma-framework Description --- this makes Button inherit from the QtControl and annd an accompaining plasma-looking theme now, the ugly part of the patch: iconSource is an url, so it screws up passing freedesktop compatible names (it expects names of pngs local to the qml file directory, testimony of the main platform target for controls actually being android/ios). one solution may be overriding iconSource as a normal string, but i would like the theme working also on a plain Button, so it extract only the filename from the url. Diffs (updated) - examples/applets/widgetgallery/contents/ui/Buttons.qml 9134df9 src/declarativeimports/core/iconitem.cpp 38012cc src/declarativeimports/plasmacomponents/qml/Button.qml 262e20f src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml PRE-CREATION Diff: https://git.reviewboard.kde.org/r/119455/diff/ Testing --- in both a normal plasma session or the widget gallery buttons work fine, painting is 100% identical Thanks, Marco Martin ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
