Re: Review Request 119406: Always take the painter based path for composeOverBorder
On July 22, 2014, 10:50 p.m., Eike Hein wrote: Two issues: - Existing Dialog instances don't get the corner mask applied correctly when switching from transparent to opague:  - Compiler warning: framesvgitem.cpp:119:10: warning: unused parameter ‘composeOverBorder’ [-Wunused-parameter] void updateTexture(const QSize size, const QString elementId, bool composeOverBorder) happens only from this patch? - Marco --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119406/#review62911 --- On July 22, 2014, 2:24 p.m., David Edmundson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119406/ --- (Updated July 22, 2014, 2:24 p.m.) Review request for KDE Frameworks and Plasma. Repository: plasma-framework Description --- Always take the painter based path for composeOverBorder We previously only supported compose-over-border when the centre was not set to tile. A recent change to the breeze theme put everything into tiling, even if some things used compose-over-border, which broke opaque widgets. Given that creating an opacityMask loads most of the image anyway, we can make use of the FrameSVG painter path and avoid any additional code complexity here. Diffs - src/declarativeimports/core/framesvgitem.cpp a5fe315 Diff: https://git.reviewboard.kde.org/r/119406/diff/ Testing --- Thanks, David Edmundson ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 119406: Always take the painter based path for composeOverBorder
On July 22, 2014, 10:50 p.m., Eike Hein wrote: Two issues: - Existing Dialog instances don't get the corner mask applied correctly when switching from transparent to opague:  - Compiler warning: framesvgitem.cpp:119:10: warning: unused parameter ‘composeOverBorder’ [-Wunused-parameter] void updateTexture(const QSize size, const QString elementId, bool composeOverBorder) Marco Martin wrote: happens only from this patch? Seems to be reproducible in v5.0 in the following situation: start a scene i.e tests/dialog.qml enable compositing resize to something not in the cache disable compositing - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119406/#review62911 --- On July 22, 2014, 2:24 p.m., David Edmundson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119406/ --- (Updated July 22, 2014, 2:24 p.m.) Review request for KDE Frameworks and Plasma. Repository: plasma-framework Description --- Always take the painter based path for composeOverBorder We previously only supported compose-over-border when the centre was not set to tile. A recent change to the breeze theme put everything into tiling, even if some things used compose-over-border, which broke opaque widgets. Given that creating an opacityMask loads most of the image anyway, we can make use of the FrameSVG painter path and avoid any additional code complexity here. Diffs - src/declarativeimports/core/framesvgitem.cpp a5fe315 Diff: https://git.reviewboard.kde.org/r/119406/diff/ Testing --- Thanks, David Edmundson ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 119406: Always take the painter based path for composeOverBorder
On July 22, 2014, 10:50 p.m., Eike Hein wrote: Two issues: - Existing Dialog instances don't get the corner mask applied correctly when switching from transparent to opague:  - Compiler warning: framesvgitem.cpp:119:10: warning: unused parameter ‘composeOverBorder’ [-Wunused-parameter] void updateTexture(const QSize size, const QString elementId, bool composeOverBorder) Marco Martin wrote: happens only from this patch? David Edmundson wrote: Seems to be reproducible in v5.0 in the following situation: start a scene i.e tests/dialog.qml enable compositing resize to something not in the cache disable compositing ok, so either kwindowsystem not notifying or a wrong response to the signal - Marco --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119406/#review62911 --- On July 22, 2014, 2:24 p.m., David Edmundson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119406/ --- (Updated July 22, 2014, 2:24 p.m.) Review request for KDE Frameworks and Plasma. Repository: plasma-framework Description --- Always take the painter based path for composeOverBorder We previously only supported compose-over-border when the centre was not set to tile. A recent change to the breeze theme put everything into tiling, even if some things used compose-over-border, which broke opaque widgets. Given that creating an opacityMask loads most of the image anyway, we can make use of the FrameSVG painter path and avoid any additional code complexity here. Diffs - src/declarativeimports/core/framesvgitem.cpp a5fe315 Diff: https://git.reviewboard.kde.org/r/119406/diff/ Testing --- Thanks, David Edmundson ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 119406: Always take the painter based path for composeOverBorder
On July 22, 2014, 10:50 p.m., Eike Hein wrote: Two issues: - Existing Dialog instances don't get the corner mask applied correctly when switching from transparent to opague:  - Compiler warning: framesvgitem.cpp:119:10: warning: unused parameter ‘composeOverBorder’ [-Wunused-parameter] void updateTexture(const QSize size, const QString elementId, bool composeOverBorder) Marco Martin wrote: happens only from this patch? David Edmundson wrote: Seems to be reproducible in v5.0 in the following situation: start a scene i.e tests/dialog.qml enable compositing resize to something not in the cache disable compositing Marco Martin wrote: ok, so either kwindowsystem not notifying or a wrong response to the signal The cause seems to be alphaMask() coming out wrong. I added a frameSvg-alphaMask().save(/tmp/mask.png); and the mask we're using (via .mask()) to send to KWindowSystem is broken and mostly translucent instead of being primarily black. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119406/#review62911 --- On July 22, 2014, 2:24 p.m., David Edmundson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119406/ --- (Updated July 22, 2014, 2:24 p.m.) Review request for KDE Frameworks and Plasma. Repository: plasma-framework Description --- Always take the painter based path for composeOverBorder We previously only supported compose-over-border when the centre was not set to tile. A recent change to the breeze theme put everything into tiling, even if some things used compose-over-border, which broke opaque widgets. Given that creating an opacityMask loads most of the image anyway, we can make use of the FrameSVG painter path and avoid any additional code complexity here. Diffs - src/declarativeimports/core/framesvgitem.cpp a5fe315 Diff: https://git.reviewboard.kde.org/r/119406/diff/ Testing --- Thanks, David Edmundson ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 119406: Always take the painter based path for composeOverBorder
On July 22, 2014, 10:50 p.m., Eike Hein wrote: Two issues: - Existing Dialog instances don't get the corner mask applied correctly when switching from transparent to opague:  - Compiler warning: framesvgitem.cpp:119:10: warning: unused parameter ‘composeOverBorder’ [-Wunused-parameter] void updateTexture(const QSize size, const QString elementId, bool composeOverBorder) Marco Martin wrote: happens only from this patch? David Edmundson wrote: Seems to be reproducible in v5.0 in the following situation: start a scene i.e tests/dialog.qml enable compositing resize to something not in the cache disable compositing Marco Martin wrote: ok, so either kwindowsystem not notifying or a wrong response to the signal David Edmundson wrote: The cause seems to be alphaMask() coming out wrong. I added a frameSvg-alphaMask().save(/tmp/mask.png); and the mask we're using (via .mask()) to send to KWindowSystem is broken and mostly translucent instead of being primarily black. ah, i see the theme doesn't have an element for the mask, for some version so it's using the normal frame as the mask as well, that's a bit less predictable. adding it in the theme, and let's see if it changes anything - Marco --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119406/#review62911 --- On July 22, 2014, 2:24 p.m., David Edmundson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119406/ --- (Updated July 22, 2014, 2:24 p.m.) Review request for KDE Frameworks and Plasma. Repository: plasma-framework Description --- Always take the painter based path for composeOverBorder We previously only supported compose-over-border when the centre was not set to tile. A recent change to the breeze theme put everything into tiling, even if some things used compose-over-border, which broke opaque widgets. Given that creating an opacityMask loads most of the image anyway, we can make use of the FrameSVG painter path and avoid any additional code complexity here. Diffs - src/declarativeimports/core/framesvgitem.cpp a5fe315 Diff: https://git.reviewboard.kde.org/r/119406/diff/ Testing --- Thanks, David Edmundson ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 119406: Always take the painter based path for composeOverBorder
On July 22, 2014, 10:50 p.m., Eike Hein wrote: Two issues: - Existing Dialog instances don't get the corner mask applied correctly when switching from transparent to opague:  - Compiler warning: framesvgitem.cpp:119:10: warning: unused parameter ‘composeOverBorder’ [-Wunused-parameter] void updateTexture(const QSize size, const QString elementId, bool composeOverBorder) Marco Martin wrote: happens only from this patch? David Edmundson wrote: Seems to be reproducible in v5.0 in the following situation: start a scene i.e tests/dialog.qml enable compositing resize to something not in the cache disable compositing Marco Martin wrote: ok, so either kwindowsystem not notifying or a wrong response to the signal David Edmundson wrote: The cause seems to be alphaMask() coming out wrong. I added a frameSvg-alphaMask().save(/tmp/mask.png); and the mask we're using (via .mask()) to send to KWindowSystem is broken and mostly translucent instead of being primarily black. Marco Martin wrote: ah, i see the theme doesn't have an element for the mask, for some version so it's using the normal frame as the mask as well, that's a bit less predictable. adding it in the theme, and let's see if it changes anything try it now, i added a dedicated black mask prefix to the dialog elements - Marco --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119406/#review62911 --- On July 22, 2014, 2:24 p.m., David Edmundson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119406/ --- (Updated July 22, 2014, 2:24 p.m.) Review request for KDE Frameworks and Plasma. Repository: plasma-framework Description --- Always take the painter based path for composeOverBorder We previously only supported compose-over-border when the centre was not set to tile. A recent change to the breeze theme put everything into tiling, even if some things used compose-over-border, which broke opaque widgets. Given that creating an opacityMask loads most of the image anyway, we can make use of the FrameSVG painter path and avoid any additional code complexity here. Diffs - src/declarativeimports/core/framesvgitem.cpp a5fe315 Diff: https://git.reviewboard.kde.org/r/119406/diff/ Testing --- Thanks, David Edmundson ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 119406: Always take the painter based path for composeOverBorder
On July 22, 2014, 10:50 p.m., Eike Hein wrote: Two issues: - Existing Dialog instances don't get the corner mask applied correctly when switching from transparent to opague:  - Compiler warning: framesvgitem.cpp:119:10: warning: unused parameter ‘composeOverBorder’ [-Wunused-parameter] void updateTexture(const QSize size, const QString elementId, bool composeOverBorder) Marco Martin wrote: happens only from this patch? David Edmundson wrote: Seems to be reproducible in v5.0 in the following situation: start a scene i.e tests/dialog.qml enable compositing resize to something not in the cache disable compositing Marco Martin wrote: ok, so either kwindowsystem not notifying or a wrong response to the signal David Edmundson wrote: The cause seems to be alphaMask() coming out wrong. I added a frameSvg-alphaMask().save(/tmp/mask.png); and the mask we're using (via .mask()) to send to KWindowSystem is broken and mostly translucent instead of being primarily black. Marco Martin wrote: ah, i see the theme doesn't have an element for the mask, for some version so it's using the normal frame as the mask as well, that's a bit less predictable. adding it in the theme, and let's see if it changes anything Marco Martin wrote: try it now, i added a dedicated black mask prefix to the dialog elements well that won't fix the root issue, just breeze. It seems there's a race and it builds the alphamask from the transparent components rather than the new opaque ones. I tried making sure dialog updates after framesvg has processed the textureChanged signal, but that's not quite enough. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119406/#review62911 --- On July 22, 2014, 2:24 p.m., David Edmundson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119406/ --- (Updated July 22, 2014, 2:24 p.m.) Review request for KDE Frameworks and Plasma. Repository: plasma-framework Description --- Always take the painter based path for composeOverBorder We previously only supported compose-over-border when the centre was not set to tile. A recent change to the breeze theme put everything into tiling, even if some things used compose-over-border, which broke opaque widgets. Given that creating an opacityMask loads most of the image anyway, we can make use of the FrameSVG painter path and avoid any additional code complexity here. Diffs - src/declarativeimports/core/framesvgitem.cpp a5fe315 Diff: https://git.reviewboard.kde.org/r/119406/diff/ Testing --- Thanks, David Edmundson ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 119406: Always take the painter based path for composeOverBorder
On July 22, 2014, 10:50 p.m., Eike Hein wrote: Two issues: - Existing Dialog instances don't get the corner mask applied correctly when switching from transparent to opague:  - Compiler warning: framesvgitem.cpp:119:10: warning: unused parameter ‘composeOverBorder’ [-Wunused-parameter] void updateTexture(const QSize size, const QString elementId, bool composeOverBorder) Marco Martin wrote: happens only from this patch? David Edmundson wrote: Seems to be reproducible in v5.0 in the following situation: start a scene i.e tests/dialog.qml enable compositing resize to something not in the cache disable compositing Marco Martin wrote: ok, so either kwindowsystem not notifying or a wrong response to the signal David Edmundson wrote: The cause seems to be alphaMask() coming out wrong. I added a frameSvg-alphaMask().save(/tmp/mask.png); and the mask we're using (via .mask()) to send to KWindowSystem is broken and mostly translucent instead of being primarily black. Marco Martin wrote: ah, i see the theme doesn't have an element for the mask, for some version so it's using the normal frame as the mask as well, that's a bit less predictable. adding it in the theme, and let's see if it changes anything Marco Martin wrote: try it now, i added a dedicated black mask prefix to the dialog elements David Edmundson wrote: well that won't fix the root issue, just breeze. It seems there's a race and it builds the alphamask from the transparent components rather than the new opaque ones. I tried making sure dialog updates after framesvg has processed the textureChanged signal, but that's not quite enough. hmm, in theory it should be correct just wether the new mask is taken after the repaintNeeded signal. will indagate on that - Marco --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119406/#review62911 --- On July 22, 2014, 2:24 p.m., David Edmundson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119406/ --- (Updated July 22, 2014, 2:24 p.m.) Review request for KDE Frameworks and Plasma. Repository: plasma-framework Description --- Always take the painter based path for composeOverBorder We previously only supported compose-over-border when the centre was not set to tile. A recent change to the breeze theme put everything into tiling, even if some things used compose-over-border, which broke opaque widgets. Given that creating an opacityMask loads most of the image anyway, we can make use of the FrameSVG painter path and avoid any additional code complexity here. Diffs - src/declarativeimports/core/framesvgitem.cpp a5fe315 Diff: https://git.reviewboard.kde.org/r/119406/diff/ Testing --- Thanks, David Edmundson ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 119406: Always take the painter based path for composeOverBorder
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119406/ --- (Updated July 23, 2014, 1:53 p.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks and Plasma. Repository: plasma-framework Description --- Always take the painter based path for composeOverBorder We previously only supported compose-over-border when the centre was not set to tile. A recent change to the breeze theme put everything into tiling, even if some things used compose-over-border, which broke opaque widgets. Given that creating an opacityMask loads most of the image anyway, we can make use of the FrameSVG painter path and avoid any additional code complexity here. Diffs - src/declarativeimports/core/framesvgitem.cpp a5fe315 Diff: https://git.reviewboard.kde.org/r/119406/diff/ Testing --- Thanks, David Edmundson ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 119406: Always take the painter based path for composeOverBorder
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119406/#review62888 --- +1, this escentially means that it now will use the same code as it used in 5.0, so no regressions at least. It means though, that everything that has composeOverBorders will be rendered by the CPU rather than the GPU. - Aleix Pol Gonzalez On July 22, 2014, 2:24 p.m., David Edmundson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119406/ --- (Updated July 22, 2014, 2:24 p.m.) Review request for KDE Frameworks and Plasma. Repository: plasma-framework Description --- Always take the painter based path for composeOverBorder We previously only supported compose-over-border when the centre was not set to tile. A recent change to the breeze theme put everything into tiling, even if some things used compose-over-border, which broke opaque widgets. Given that creating an opacityMask loads most of the image anyway, we can make use of the FrameSVG painter path and avoid any additional code complexity here. Diffs - src/declarativeimports/core/framesvgitem.cpp a5fe315 Diff: https://git.reviewboard.kde.org/r/119406/diff/ Testing --- Thanks, David Edmundson ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 119406: Always take the painter based path for composeOverBorder
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119406/#review62911 --- Two issues: - Existing Dialog instances don't get the corner mask applied correctly when switching from transparent to opague:  - Compiler warning: framesvgitem.cpp:119:10: warning: unused parameter ‘composeOverBorder’ [-Wunused-parameter] void updateTexture(const QSize size, const QString elementId, bool composeOverBorder) - Eike Hein On July 22, 2014, 2:24 p.m., David Edmundson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119406/ --- (Updated July 22, 2014, 2:24 p.m.) Review request for KDE Frameworks and Plasma. Repository: plasma-framework Description --- Always take the painter based path for composeOverBorder We previously only supported compose-over-border when the centre was not set to tile. A recent change to the breeze theme put everything into tiling, even if some things used compose-over-border, which broke opaque widgets. Given that creating an opacityMask loads most of the image anyway, we can make use of the FrameSVG painter path and avoid any additional code complexity here. Diffs - src/declarativeimports/core/framesvgitem.cpp a5fe315 Diff: https://git.reviewboard.kde.org/r/119406/diff/ Testing --- Thanks, David Edmundson ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
