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 119425: Cache the textures created for the fast path
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119425/ --- (Updated July 25, 2014, 10:14 a.m.) Review request for Plasma. Changes --- Adopt some changes by David. Escentially uses the QImage itself to get the image hash rather than using the image description, so it keeps things simpler. Also we don't need to worry about themes changing, because this will take care of itself. Repository: plasma-framework Description --- Create a cache that has pointers to all the textures that we've generated, so in case we have one already created, we can re-use it. Diffs (updated) - src/declarativeimports/core/framesvgitem.cpp 323b06b src/declarativeimports/core/iconitem.cpp 38012cc src/declarativeimports/core/svgitem.cpp eccff55 src/declarativeimports/core/svgtexturenode.h 21b9b2f src/desktoptheme/air/widgets/lineedit.svgz 8b3a123 Diff: https://git.reviewboard.kde.org/r/119425/diff/ Testing --- see the qDebug (to be removed before commit). plasmashell 2 out $ grep s_cache out | grep : miss | wc -l 342 $ grep s_cache out | grep : hit | wc -l 126 So still having 3 times more hits than miss, so there's big room for improvement. Good news is that with this, we get a ~25% of memory and bandwidth save, in a per-item basis. Thanks, Aleix Pol Gonzalez ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119425: Cache the textures created for the fast path
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119425/#review63124 --- Ship it! src/declarativeimports/core/framesvgitem.cpp https://git.reviewboard.kde.org/r/119425/#comment43878 This comment is now wrong. So sorry! Also (pedantry) it's not a hash; it's a static counter inside the QImage constructor/detatch so it's super fast. - David Edmundson On July 25, 2014, 10:14 a.m., Aleix Pol Gonzalez wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119425/ --- (Updated July 25, 2014, 10:14 a.m.) Review request for Plasma. Repository: plasma-framework Description --- Create a cache that has pointers to all the textures that we've generated, so in case we have one already created, we can re-use it. Diffs - src/declarativeimports/core/framesvgitem.cpp 323b06b src/declarativeimports/core/iconitem.cpp 38012cc src/declarativeimports/core/svgitem.cpp eccff55 src/declarativeimports/core/svgtexturenode.h 21b9b2f src/desktoptheme/air/widgets/lineedit.svgz 8b3a123 Diff: https://git.reviewboard.kde.org/r/119425/diff/ Testing --- see the qDebug (to be removed before commit). plasmashell 2 out $ grep s_cache out | grep : miss | wc -l 342 $ grep s_cache out | grep : hit | wc -l 126 So still having 3 times more hits than miss, so there's big room for improvement. Good news is that with this, we get a ~25% of memory and bandwidth save, in a per-item basis. Thanks, Aleix Pol Gonzalez ___ 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 119425: Cache the textures created for the fast path
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119425/#review63128 --- Ship it! Ship It! - Marco Martin On July 25, 2014, 10:14 a.m., Aleix Pol Gonzalez wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119425/ --- (Updated July 25, 2014, 10:14 a.m.) Review request for Plasma. Repository: plasma-framework Description --- Create a cache that has pointers to all the textures that we've generated, so in case we have one already created, we can re-use it. Diffs - src/declarativeimports/core/framesvgitem.cpp 323b06b src/declarativeimports/core/iconitem.cpp 38012cc src/declarativeimports/core/svgitem.cpp eccff55 src/declarativeimports/core/svgtexturenode.h 21b9b2f src/desktoptheme/air/widgets/lineedit.svgz 8b3a123 Diff: https://git.reviewboard.kde.org/r/119425/diff/ Testing --- see the qDebug (to be removed before commit). plasmashell 2 out $ grep s_cache out | grep : miss | wc -l 342 $ grep s_cache out | grep : hit | wc -l 126 So still having 3 times more hits than miss, so there's big room for improvement. Good news is that with this, we get a ~25% of memory and bandwidth save, in a per-item basis. Thanks, Aleix Pol Gonzalez ___ 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 119425: Cache the textures created for the fast path
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119425/ --- (Updated July 25, 2014, 10:57 a.m.) Status -- This change has been marked as submitted. Review request for Plasma. Repository: plasma-framework Description --- Create a cache that has pointers to all the textures that we've generated, so in case we have one already created, we can re-use it. Diffs - src/declarativeimports/core/framesvgitem.cpp 323b06b src/declarativeimports/core/iconitem.cpp 38012cc src/declarativeimports/core/svgitem.cpp eccff55 src/declarativeimports/core/svgtexturenode.h 21b9b2f src/desktoptheme/air/widgets/lineedit.svgz 8b3a123 Diff: https://git.reviewboard.kde.org/r/119425/diff/ Testing --- see the qDebug (to be removed before commit). plasmashell 2 out $ grep s_cache out | grep : miss | wc -l 342 $ grep s_cache out | grep : hit | wc -l 126 So still having 3 times more hits than miss, so there's big room for improvement. Good news is that with this, we get a ~25% of memory and bandwidth save, in a per-item basis. Thanks, Aleix Pol Gonzalez ___ 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 119464: port ToolButton to QtControls
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119464/ --- Review request for KDE Frameworks and Plasma. Repository: plasma-framework Description --- this ports ToolButton. It's a different style than Button since it's way more complicated. The ugly thing here is that QtControls ToolButton doesn't have a flat property, it should probably be upstreamed, at least for compatibility with the qwidgets ToolButton Diffs - src/declarativeimports/plasmacomponents/qml/ToolButton.qml 6dcbf7b src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml d92a662 src/declarativeimports/plasmacomponents/qml/styles/ComboBoxStyle.qml 20caef6 src/declarativeimports/plasmacomponents/qml/styles/ToolButtonStyle.qml PRE-CREATION Diff: https://git.reviewboard.kde.org/r/119464/diff/ Testing --- Thanks, Marco Martin ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119425: Cache the textures created for the fast path
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119425/#review63152 --- To me, at least on Nvidia seems that graphics gets completely broken for popup contents. I think it should watch for widow change and delete the cache/rebuild the textures in that case - Marco Martin On July 25, 2014, 10:57 a.m., Aleix Pol Gonzalez wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119425/ --- (Updated July 25, 2014, 10:57 a.m.) Review request for Plasma. Repository: plasma-framework Description --- Create a cache that has pointers to all the textures that we've generated, so in case we have one already created, we can re-use it. Diffs - src/declarativeimports/core/framesvgitem.cpp 323b06b src/declarativeimports/core/iconitem.cpp 38012cc src/declarativeimports/core/svgitem.cpp eccff55 src/declarativeimports/core/svgtexturenode.h 21b9b2f src/desktoptheme/air/widgets/lineedit.svgz 8b3a123 Diff: https://git.reviewboard.kde.org/r/119425/diff/ Testing --- see the qDebug (to be removed before commit). plasmashell 2 out $ grep s_cache out | grep : miss | wc -l 342 $ grep s_cache out | grep : hit | wc -l 126 So still having 3 times more hits than miss, so there's big room for improvement. Good news is that with this, we get a ~25% of memory and bandwidth save, in a per-item basis. Thanks, Aleix Pol Gonzalez ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119425: Cache the textures created for the fast path
On July 25, 2014, 1:55 p.m., Marco Martin wrote: To me, at least on Nvidia seems that graphics gets completely broken for popup contents. I think it should watch for widow change and delete the cache/rebuild the textures in that case Do you have a picture? Here (intel) it looks fine. At least I tried moving the panel from a screen to the other and it seemed to stay put, with all the tooltips and such. - Aleix --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119425/#review63152 --- On July 25, 2014, 10:57 a.m., Aleix Pol Gonzalez wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119425/ --- (Updated July 25, 2014, 10:57 a.m.) Review request for Plasma. Repository: plasma-framework Description --- Create a cache that has pointers to all the textures that we've generated, so in case we have one already created, we can re-use it. Diffs - src/declarativeimports/core/framesvgitem.cpp 323b06b src/declarativeimports/core/iconitem.cpp 38012cc src/declarativeimports/core/svgitem.cpp eccff55 src/declarativeimports/core/svgtexturenode.h 21b9b2f src/desktoptheme/air/widgets/lineedit.svgz 8b3a123 Diff: https://git.reviewboard.kde.org/r/119425/diff/ Testing --- see the qDebug (to be removed before commit). plasmashell 2 out $ grep s_cache out | grep : miss | wc -l 342 $ grep s_cache out | grep : hit | wc -l 126 So still having 3 times more hits than miss, so there's big room for improvement. Good news is that with this, we get a ~25% of memory and bandwidth save, in a per-item basis. Thanks, Aleix Pol Gonzalez ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119425: Cache the textures created for the fast path
On July 25, 2014, 1:55 p.m., Marco Martin wrote: To me, at least on Nvidia seems that graphics gets completely broken for popup contents. I think it should watch for widow change and delete the cache/rebuild the textures in that case Aleix Pol Gonzalez wrote: Do you have a picture? Here (intel) it looks fine. At least I tried moving the panel from a screen to the other and it seemed to stay put, with all the tooltips and such. http://wstaw.org/m/2014/07/25/plasma-desktopvE1748.png can try to bisect to see if it really came up from that patch or before - Marco --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119425/#review63152 --- On July 25, 2014, 10:57 a.m., Aleix Pol Gonzalez wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119425/ --- (Updated July 25, 2014, 10:57 a.m.) Review request for Plasma. Repository: plasma-framework Description --- Create a cache that has pointers to all the textures that we've generated, so in case we have one already created, we can re-use it. Diffs - src/declarativeimports/core/framesvgitem.cpp 323b06b src/declarativeimports/core/iconitem.cpp 38012cc src/declarativeimports/core/svgitem.cpp eccff55 src/declarativeimports/core/svgtexturenode.h 21b9b2f src/desktoptheme/air/widgets/lineedit.svgz 8b3a123 Diff: https://git.reviewboard.kde.org/r/119425/diff/ Testing --- see the qDebug (to be removed before commit). plasmashell 2 out $ grep s_cache out | grep : miss | wc -l 342 $ grep s_cache out | grep : hit | wc -l 126 So still having 3 times more hits than miss, so there's big room for improvement. Good news is that with this, we get a ~25% of memory and bandwidth save, in a per-item basis. Thanks, Aleix Pol Gonzalez ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119425: Cache the textures created for the fast path
On July 25, 2014, 1:55 p.m., Marco Martin wrote: To me, at least on Nvidia seems that graphics gets completely broken for popup contents. I think it should watch for widow change and delete the cache/rebuild the textures in that case Aleix Pol Gonzalez wrote: Do you have a picture? Here (intel) it looks fine. At least I tried moving the panel from a screen to the other and it seemed to stay put, with all the tooltips and such. Marco Martin wrote: http://wstaw.org/m/2014/07/25/plasma-desktopvE1748.png can try to bisect to see if it really came up from that patch or before in the example above, i loaded it in plasmoidviewer, then resized to switc some times between full and popup - Marco --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119425/#review63152 --- On July 25, 2014, 10:57 a.m., Aleix Pol Gonzalez wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119425/ --- (Updated July 25, 2014, 10:57 a.m.) Review request for Plasma. Repository: plasma-framework Description --- Create a cache that has pointers to all the textures that we've generated, so in case we have one already created, we can re-use it. Diffs - src/declarativeimports/core/framesvgitem.cpp 323b06b src/declarativeimports/core/iconitem.cpp 38012cc src/declarativeimports/core/svgitem.cpp eccff55 src/declarativeimports/core/svgtexturenode.h 21b9b2f src/desktoptheme/air/widgets/lineedit.svgz 8b3a123 Diff: https://git.reviewboard.kde.org/r/119425/diff/ Testing --- see the qDebug (to be removed before commit). plasmashell 2 out $ grep s_cache out | grep : miss | wc -l 342 $ grep s_cache out | grep : hit | wc -l 126 So still having 3 times more hits than miss, so there's big room for improvement. Good news is that with this, we get a ~25% of memory and bandwidth save, in a per-item basis. Thanks, Aleix Pol Gonzalez ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119425: Cache the textures created for the fast path
On July 25, 2014, 1:55 p.m., Marco Martin wrote: To me, at least on Nvidia seems that graphics gets completely broken for popup contents. I think it should watch for widow change and delete the cache/rebuild the textures in that case Aleix Pol Gonzalez wrote: Do you have a picture? Here (intel) it looks fine. At least I tried moving the panel from a screen to the other and it seemed to stay put, with all the tooltips and such. Marco Martin wrote: http://wstaw.org/m/2014/07/25/plasma-desktopvE1748.png can try to bisect to see if it really came up from that patch or before Marco Martin wrote: in the example above, i loaded it in plasmoidviewer, then resized to switc some times between full and popup yep, reliably doesn't happen anymore on f6e4cb790ec815cad0cb7be40f7145f08b034693 that's the one before this has been berged - Marco --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119425/#review63152 --- On July 25, 2014, 10:57 a.m., Aleix Pol Gonzalez wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119425/ --- (Updated July 25, 2014, 10:57 a.m.) Review request for Plasma. Repository: plasma-framework Description --- Create a cache that has pointers to all the textures that we've generated, so in case we have one already created, we can re-use it. Diffs - src/declarativeimports/core/framesvgitem.cpp 323b06b src/declarativeimports/core/iconitem.cpp 38012cc src/declarativeimports/core/svgitem.cpp eccff55 src/declarativeimports/core/svgtexturenode.h 21b9b2f src/desktoptheme/air/widgets/lineedit.svgz 8b3a123 Diff: https://git.reviewboard.kde.org/r/119425/diff/ Testing --- see the qDebug (to be removed before commit). plasmashell 2 out $ grep s_cache out | grep : miss | wc -l 342 $ grep s_cache out | grep : hit | wc -l 126 So still having 3 times more hits than miss, so there's big room for improvement. Good news is that with this, we get a ~25% of memory and bandwidth save, in a per-item basis. Thanks, Aleix Pol Gonzalez ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119425: Cache the textures created for the fast path
On July 25, 2014, 1:55 p.m., Marco Martin wrote: To me, at least on Nvidia seems that graphics gets completely broken for popup contents. I think it should watch for widow change and delete the cache/rebuild the textures in that case Aleix Pol Gonzalez wrote: Do you have a picture? Here (intel) it looks fine. At least I tried moving the panel from a screen to the other and it seemed to stay put, with all the tooltips and such. Marco Martin wrote: http://wstaw.org/m/2014/07/25/plasma-desktopvE1748.png can try to bisect to see if it really came up from that patch or before Marco Martin wrote: in the example above, i loaded it in plasmoidviewer, then resized to switc some times between full and popup Marco Martin wrote: yep, reliably doesn't happen anymore on f6e4cb790ec815cad0cb7be40f7145f08b034693 that's the one before this has been berged This seems to *partly* fix the thing, but not completely http://paste.opensuse.org/9869702 - Marco --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119425/#review63152 --- On July 25, 2014, 10:57 a.m., Aleix Pol Gonzalez wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119425/ --- (Updated July 25, 2014, 10:57 a.m.) Review request for Plasma. Repository: plasma-framework Description --- Create a cache that has pointers to all the textures that we've generated, so in case we have one already created, we can re-use it. Diffs - src/declarativeimports/core/framesvgitem.cpp 323b06b src/declarativeimports/core/iconitem.cpp 38012cc src/declarativeimports/core/svgitem.cpp eccff55 src/declarativeimports/core/svgtexturenode.h 21b9b2f src/desktoptheme/air/widgets/lineedit.svgz 8b3a123 Diff: https://git.reviewboard.kde.org/r/119425/diff/ Testing --- see the qDebug (to be removed before commit). plasmashell 2 out $ grep s_cache out | grep : miss | wc -l 342 $ grep s_cache out | grep : hit | wc -l 126 So still having 3 times more hits than miss, so there's big room for improvement. Good news is that with this, we get a ~25% of memory and bandwidth save, in a per-item basis. Thanks, Aleix Pol Gonzalez ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119425: Cache the textures created for the fast path
On July 25, 2014, 1:55 p.m., Marco Martin wrote: To me, at least on Nvidia seems that graphics gets completely broken for popup contents. I think it should watch for widow change and delete the cache/rebuild the textures in that case Aleix Pol Gonzalez wrote: Do you have a picture? Here (intel) it looks fine. At least I tried moving the panel from a screen to the other and it seemed to stay put, with all the tooltips and such. Marco Martin wrote: http://wstaw.org/m/2014/07/25/plasma-desktopvE1748.png can try to bisect to see if it really came up from that patch or before Marco Martin wrote: in the example above, i loaded it in plasmoidviewer, then resized to switc some times between full and popup Marco Martin wrote: yep, reliably doesn't happen anymore on f6e4cb790ec815cad0cb7be40f7145f08b034693 that's the one before this has been berged Marco Martin wrote: This seems to *partly* fix the thing, but not completely http://paste.opensuse.org/9869702 another possible way could be to index textures by their window - Marco --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119425/#review63152 --- On July 25, 2014, 10:57 a.m., Aleix Pol Gonzalez wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119425/ --- (Updated July 25, 2014, 10:57 a.m.) Review request for Plasma. Repository: plasma-framework Description --- Create a cache that has pointers to all the textures that we've generated, so in case we have one already created, we can re-use it. Diffs - src/declarativeimports/core/framesvgitem.cpp 323b06b src/declarativeimports/core/iconitem.cpp 38012cc src/declarativeimports/core/svgitem.cpp eccff55 src/declarativeimports/core/svgtexturenode.h 21b9b2f src/desktoptheme/air/widgets/lineedit.svgz 8b3a123 Diff: https://git.reviewboard.kde.org/r/119425/diff/ Testing --- see the qDebug (to be removed before commit). plasmashell 2 out $ grep s_cache out | grep : miss | wc -l 342 $ grep s_cache out | grep : hit | wc -l 126 So still having 3 times more hits than miss, so there's big room for improvement. Good news is that with this, we get a ~25% of memory and bandwidth save, in a per-item basis. Thanks, Aleix Pol Gonzalez ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request 119465: Have separate texture hashes for each window
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119465/ --- Review request for Plasma. Repository: plasma-framework Description --- Apparently in nvidia we get corruptions when a texture created for a window is used in another one. With this patch we tell the texture has changed when we move it from a window to another, so it's re-created and we keep textures for all windows separately. This way we ensure they don't mix. Diffs - src/declarativeimports/core/framesvgitem.h 0b39c70 src/declarativeimports/core/framesvgitem.cpp ebac29f Diff: https://git.reviewboard.kde.org/r/119465/diff/ Testing --- Still works here, I hope Marco can confirm it fixes the problem. Thanks, Aleix Pol Gonzalez ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119465: Have separate texture hashes for each window
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119465/#review63164 --- Ship it! on this system it does indeed fix the rendering issues. can anyone else with an nvidia system (or other kinds of drivers) test as well? - Marco Martin On July 25, 2014, 2:28 p.m., Aleix Pol Gonzalez wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119465/ --- (Updated July 25, 2014, 2:28 p.m.) Review request for Plasma. Repository: plasma-framework Description --- Apparently in nvidia we get corruptions when a texture created for a window is used in another one. With this patch we tell the texture has changed when we move it from a window to another, so it's re-created and we keep textures for all windows separately. This way we ensure they don't mix. Diffs - src/declarativeimports/core/framesvgitem.h 0b39c70 src/declarativeimports/core/framesvgitem.cpp ebac29f Diff: https://git.reviewboard.kde.org/r/119465/diff/ Testing --- Still works here, I hope Marco can confirm it fixes the problem. Thanks, Aleix Pol Gonzalez ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119465: Have separate texture hashes for each window
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119465/#review63166 --- The problem is not the different window, but probably the different OpenGL context. QtQuick uses with the threaded renderer an OpenGL context per QWindow. Marco could confirm that by enforcing the main thread renderer and verify that this also fixes the problem. If that's the case I'd rather go for verifying that it's the correct OpenGL context than the QWindow. - Martin Gräßlin On July 25, 2014, 4:28 p.m., Aleix Pol Gonzalez wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119465/ --- (Updated July 25, 2014, 4:28 p.m.) Review request for Plasma. Repository: plasma-framework Description --- Apparently in nvidia we get corruptions when a texture created for a window is used in another one. With this patch we tell the texture has changed when we move it from a window to another, so it's re-created and we keep textures for all windows separately. This way we ensure they don't mix. Diffs - src/declarativeimports/core/framesvgitem.h 0b39c70 src/declarativeimports/core/framesvgitem.cpp ebac29f Diff: https://git.reviewboard.kde.org/r/119465/diff/ Testing --- Still works here, I hope Marco can confirm it fixes the problem. Thanks, Aleix Pol Gonzalez ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119464: port ToolButton to QtControls
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119464/#review63169 --- src/declarativeimports/plasmacomponents/qml/ToolButton.qml https://git.reviewboard.kde.org/r/119464/#comment43931 Just curious, any reason to not use ToolButton here? http://qt-project.org/doc/qt-5/qml-qtquick-controls-toolbutton.html - Andrew Lake On July 25, 2014, 1:47 p.m., Marco Martin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119464/ --- (Updated July 25, 2014, 1:47 p.m.) Review request for KDE Frameworks and Plasma. Repository: plasma-framework Description --- this ports ToolButton. It's a different style than Button since it's way more complicated. The ugly thing here is that QtControls ToolButton doesn't have a flat property, it should probably be upstreamed, at least for compatibility with the qwidgets ToolButton Diffs - src/declarativeimports/plasmacomponents/qml/ToolButton.qml 6dcbf7b src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml d92a662 src/declarativeimports/plasmacomponents/qml/styles/ComboBoxStyle.qml 20caef6 src/declarativeimports/plasmacomponents/qml/styles/ToolButtonStyle.qml PRE-CREATION Diff: https://git.reviewboard.kde.org/r/119464/diff/ Testing --- Thanks, Marco Martin ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119464: port ToolButton to QtControls
On July 25, 2014, 3:57 p.m., Andrew Lake wrote: src/declarativeimports/plasmacomponents/qml/ToolButton.qml, line 30 https://git.reviewboard.kde.org/r/119464/diff/1/?file=292702#file292702line30 Just curious, any reason to not use ToolButton here? http://qt-project.org/doc/qt-5/qml-qtquick-controls-toolbutton.html I'm inheriting button so reuses some of he custom stuff, since i seen that toolbutton in qtcontrols just inherits hit button.. anyways would be easy enough to inherit instead the qtcontrols toolbutton and duplicate the custom stuff needed for compatibility instead - Marco --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119464/#review63169 --- On July 25, 2014, 1:47 p.m., Marco Martin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119464/ --- (Updated July 25, 2014, 1:47 p.m.) Review request for KDE Frameworks and Plasma. Repository: plasma-framework Description --- this ports ToolButton. It's a different style than Button since it's way more complicated. The ugly thing here is that QtControls ToolButton doesn't have a flat property, it should probably be upstreamed, at least for compatibility with the qwidgets ToolButton Diffs - src/declarativeimports/plasmacomponents/qml/ToolButton.qml 6dcbf7b src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml d92a662 src/declarativeimports/plasmacomponents/qml/styles/ComboBoxStyle.qml 20caef6 src/declarativeimports/plasmacomponents/qml/styles/ToolButtonStyle.qml PRE-CREATION Diff: https://git.reviewboard.kde.org/r/119464/diff/ Testing --- Thanks, Marco Martin ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119465: Have separate texture hashes for each window
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119465/#review63172 --- src/declarativeimports/core/framesvgitem.cpp https://git.reviewboard.kde.org/r/119465/#comment43938 when a qquickwindow gets deleted.. does it delete all the QSGTextures created with createTextureFromImage? - Marco Martin On July 25, 2014, 2:28 p.m., Aleix Pol Gonzalez wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119465/ --- (Updated July 25, 2014, 2:28 p.m.) Review request for Plasma. Repository: plasma-framework Description --- Apparently in nvidia we get corruptions when a texture created for a window is used in another one. With this patch we tell the texture has changed when we move it from a window to another, so it's re-created and we keep textures for all windows separately. This way we ensure they don't mix. Diffs - src/declarativeimports/core/framesvgitem.h 0b39c70 src/declarativeimports/core/framesvgitem.cpp ebac29f Diff: https://git.reviewboard.kde.org/r/119465/diff/ Testing --- Still works here, I hope Marco can confirm it fixes the problem. Thanks, Aleix Pol Gonzalez ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119465: Have separate texture hashes for each window
On July 25, 2014, 3:43 p.m., Martin Gräßlin wrote: The problem is not the different window, but probably the different OpenGL context. QtQuick uses with the threaded renderer an OpenGL context per QWindow. Marco could confirm that by enforcing the main thread renderer and verify that this also fixes the problem. If that's the case I'd rather go for verifying that it's the correct OpenGL context than the QWindow. yep, with export QSG_RENDER_LOOP=basic seems rendering is correct - Marco --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119465/#review63166 --- On July 25, 2014, 2:28 p.m., Aleix Pol Gonzalez wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119465/ --- (Updated July 25, 2014, 2:28 p.m.) Review request for Plasma. Repository: plasma-framework Description --- Apparently in nvidia we get corruptions when a texture created for a window is used in another one. With this patch we tell the texture has changed when we move it from a window to another, so it's re-created and we keep textures for all windows separately. This way we ensure they don't mix. Diffs - src/declarativeimports/core/framesvgitem.h 0b39c70 src/declarativeimports/core/framesvgitem.cpp ebac29f Diff: https://git.reviewboard.kde.org/r/119465/diff/ Testing --- Still works here, I hope Marco can confirm it fixes the problem. Thanks, Aleix Pol Gonzalez ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119464: port ToolButton to QtControls
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119464/ --- (Updated July 25, 2014, 4:52 p.m.) Review request for KDE Frameworks and Plasma. Repository: plasma-framework Description --- this ports ToolButton. It's a different style than Button since it's way more complicated. The ugly thing here is that QtControls ToolButton doesn't have a flat property, it should probably be upstreamed, at least for compatibility with the qwidgets ToolButton Diffs (updated) - src/declarativeimports/plasmacomponents/qml/styles/ComboBoxStyle.qml 20caef6 src/declarativeimports/plasmacomponents/qml/ToolButton.qml 6dcbf7b src/declarativeimports/plasmacomponents/qml/styles/ToolButtonStyle.qml PRE-CREATION src/declarativeimports/plasmacomponents/qml/styles/ButtonStyle.qml d92a662 Diff: https://git.reviewboard.kde.org/r/119464/diff/ Testing --- Thanks, Marco Martin ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119465: Have separate texture hashes for each window
On July 25, 2014, 4:36 p.m., Marco Martin wrote: src/declarativeimports/core/framesvgitem.cpp, line 49 https://git.reviewboard.kde.org/r/119465/diff/1/?file=292708#file292708line49 when a qquickwindow gets deleted.. does it delete all the QSGTextures created with createTextureFromImage? No. Ownership is always transferred to the recipient. (and this is using QSharedPointer to then manage them) - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119465/#review63172 --- On July 25, 2014, 2:28 p.m., Aleix Pol Gonzalez wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119465/ --- (Updated July 25, 2014, 2:28 p.m.) Review request for Plasma. Repository: plasma-framework Description --- Apparently in nvidia we get corruptions when a texture created for a window is used in another one. With this patch we tell the texture has changed when we move it from a window to another, so it's re-created and we keep textures for all windows separately. This way we ensure they don't mix. Diffs - src/declarativeimports/core/framesvgitem.h 0b39c70 src/declarativeimports/core/framesvgitem.cpp ebac29f Diff: https://git.reviewboard.kde.org/r/119465/diff/ Testing --- Still works here, I hope Marco can confirm it fixes the problem. Thanks, Aleix Pol Gonzalez ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119465: Have separate texture hashes for each window
On July 25, 2014, 3:43 p.m., Martin Gräßlin wrote: The problem is not the different window, but probably the different OpenGL context. QtQuick uses with the threaded renderer an OpenGL context per QWindow. Marco could confirm that by enforcing the main thread renderer and verify that this also fixes the problem. If that's the case I'd rather go for verifying that it's the correct OpenGL context than the QWindow. Marco Martin wrote: yep, with export QSG_RENDER_LOOP=basic seems rendering is correct I don't think comparing contexts is possible here. Qt has a wrapper round the OpenGL context, QOpenGLContext. You have one of those per window, even if underneath there is just one OpenGL context. It's possible to get the shareContext but according to the docs that can return 0 on some platforms at which point we're playing dangerously. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119465/#review63166 --- On July 25, 2014, 2:28 p.m., Aleix Pol Gonzalez wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119465/ --- (Updated July 25, 2014, 2:28 p.m.) Review request for Plasma. Repository: plasma-framework Description --- Apparently in nvidia we get corruptions when a texture created for a window is used in another one. With this patch we tell the texture has changed when we move it from a window to another, so it's re-created and we keep textures for all windows separately. This way we ensure they don't mix. Diffs - src/declarativeimports/core/framesvgitem.h 0b39c70 src/declarativeimports/core/framesvgitem.cpp ebac29f Diff: https://git.reviewboard.kde.org/r/119465/diff/ Testing --- Still works here, I hope Marco can confirm it fixes the problem. Thanks, Aleix Pol Gonzalez ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119465: Have separate texture hashes for each window
On jul. 25, 2014, 4:36 p.m., Marco Martin wrote: src/declarativeimports/core/framesvgitem.cpp, line 49 https://git.reviewboard.kde.org/r/119465/diff/1/?file=292708#file292708line49 when a qquickwindow gets deleted.. does it delete all the QSGTextures created with createTextureFromImage? David Edmundson wrote: No. Ownership is always transferred to the recipient. (and this is using QSharedPointer to then manage them) The texture will get deleted when the node is. When we change window the texture gets destroyed (or de-referenced, since it's a qsharedpointer) - Aleix --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119465/#review63172 --- On jul. 25, 2014, 2:28 p.m., Aleix Pol Gonzalez wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119465/ --- (Updated jul. 25, 2014, 2:28 p.m.) Review request for Plasma. Repository: plasma-framework Description --- Apparently in nvidia we get corruptions when a texture created for a window is used in another one. With this patch we tell the texture has changed when we move it from a window to another, so it's re-created and we keep textures for all windows separately. This way we ensure they don't mix. Diffs - src/declarativeimports/core/framesvgitem.h 0b39c70 src/declarativeimports/core/framesvgitem.cpp ebac29f Diff: https://git.reviewboard.kde.org/r/119465/diff/ Testing --- Still works here, I hope Marco can confirm it fixes the problem. Thanks, Aleix Pol Gonzalez ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119465: Have separate texture hashes for each window
On jul. 25, 2014, 3:43 p.m., Martin Gräßlin wrote: The problem is not the different window, but probably the different OpenGL context. QtQuick uses with the threaded renderer an OpenGL context per QWindow. Marco could confirm that by enforcing the main thread renderer and verify that this also fixes the problem. If that's the case I'd rather go for verifying that it's the correct OpenGL context than the QWindow. Marco Martin wrote: yep, with export QSG_RENDER_LOOP=basic seems rendering is correct David Edmundson wrote: I don't think comparing contexts is possible here. Qt has a wrapper round the OpenGL context, QOpenGLContext. You have one of those per window, even if underneath there is just one OpenGL context. It's possible to get the shareContext but according to the docs that can return 0 on some platforms at which point we're playing dangerously. Therefore it's correct that the patch is too conservative, no? - Aleix --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119465/#review63166 --- On jul. 25, 2014, 2:28 p.m., Aleix Pol Gonzalez wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119465/ --- (Updated jul. 25, 2014, 2:28 p.m.) Review request for Plasma. Repository: plasma-framework Description --- Apparently in nvidia we get corruptions when a texture created for a window is used in another one. With this patch we tell the texture has changed when we move it from a window to another, so it's re-created and we keep textures for all windows separately. This way we ensure they don't mix. Diffs - src/declarativeimports/core/framesvgitem.h 0b39c70 src/declarativeimports/core/framesvgitem.cpp ebac29f Diff: https://git.reviewboard.kde.org/r/119465/diff/ Testing --- Still works here, I hope Marco can confirm it fixes the problem. Thanks, Aleix Pol Gonzalez ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
