Review Request 119451: some machinery for look and feel package
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119451/ --- Review request for Plasma. Repository: plasma-workspace Description --- This is still nowhere near final mergeability, but rather a request for comments for early stages ;) So, what does an application that uses stuff from lf needs? * open the proper lf package, as configured * fetch files from it * use files of the default one if the provided one is incomplete * monitor for theme changes and if necessary reload the qml * some applications may even want to have an optional local config that overrides it, like the splash, but is out of scope of a central management the branch uses a little class that does all of the above (minus the last point) and uses it for now in the splash screen For now it would just be statically linked into users, since they should be all in plasma-framework for now (of course not optimal) *maybe* is stuff for libplasmaquick, but that library since locks a release of p-f with the same release of users of it, should probably me made public or else, so I'm a bit hesitant to add further stuff into it. Diffs - ksplash/ksplashqml/CMakeLists.txt 16c58a0 ksplash/ksplashqml/SplashWindow.cpp 23603f5 ksplash/ksplashqml/shellpluginloader.h 9c0f624 lookandfeelaccess/lookandfeelaccess.h PRE-CREATION lookandfeelaccess/lookandfeelaccess.cpp PRE-CREATION lookandfeelaccess/shellpluginloader.h PRE-CREATION Diff: https://git.reviewboard.kde.org/r/119451/diff/ Testing --- Thanks, Marco Martin ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119451: some machinery for look and feel package
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119451/#review63040 --- Also another problem connected to this: how to take the default icons, default fonts, colors etc from values specified inside this? those defaults should be put in a framework that can't really depend from this in any shape or form.. - Marco Martin On July 24, 2014, 9:43 a.m., Marco Martin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119451/ --- (Updated July 24, 2014, 9:43 a.m.) Review request for Plasma. Repository: plasma-workspace Description --- This is still nowhere near final mergeability, but rather a request for comments for early stages ;) So, what does an application that uses stuff from lf needs? * open the proper lf package, as configured * fetch files from it * use files of the default one if the provided one is incomplete * monitor for theme changes and if necessary reload the qml * some applications may even want to have an optional local config that overrides it, like the splash, but is out of scope of a central management the branch uses a little class that does all of the above (minus the last point) and uses it for now in the splash screen For now it would just be statically linked into users, since they should be all in plasma-framework for now (of course not optimal) *maybe* is stuff for libplasmaquick, but that library since locks a release of p-f with the same release of users of it, should probably me made public or else, so I'm a bit hesitant to add further stuff into it. Diffs - ksplash/ksplashqml/CMakeLists.txt 16c58a0 ksplash/ksplashqml/SplashWindow.cpp 23603f5 ksplash/ksplashqml/shellpluginloader.h 9c0f624 lookandfeelaccess/lookandfeelaccess.h PRE-CREATION lookandfeelaccess/lookandfeelaccess.cpp PRE-CREATION lookandfeelaccess/shellpluginloader.h PRE-CREATION Diff: https://git.reviewboard.kde.org/r/119451/diff/ Testing --- Thanks, Marco Martin ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request 119452: Rename libkworkspace for coinstallability with kde-workspace4.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119452/ --- Review request for Plasma. Repository: plasma-workspace Description --- A number of KDE 4 applications depend on libkworkspace from kde-workspace, which currently collides with libkworkspace provided by plasma-workspace. Renaming makes it easier downstream to provide the ability run these applications in a Plasma 5 session. Diffs - libkworkspace/CMakeLists.txt e417c76ad4a032e6dc2fde9aae74352303821983 Diff: https://git.reviewboard.kde.org/r/119452/diff/ Testing --- plasma-workspace, khotkeys, and powerdevil builds. plasma-desktop requires a matching header include rename. Not aware of any other frameworks-based libkworkspace consumers. Thanks, Michael Palimaka ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119451: some machinery for look and feel package
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119451/#review63037 --- lookandfeelaccess/lookandfeelaccess.cpp https://git.reviewboard.kde.org/r/119451/#comment43763 My concern is this will create more problems than it's worth. Scenario: - I add a new feature in the lock screen in 5.1 with a new rootContext variable - I update the QML to use this new feature in 5.1 - a user upgrades his system - We then reload the package (but not the binary) we get a QML error and the user can't log in again. lookandfeelaccess/shellpluginloader.h https://git.reviewboard.kde.org/r/119451/#comment43764 not used? I'd quite like to have a chat about the goals of look and feel (next Monday meeting?). It was something where you guys clearly had a plan (or at least a name) before I got more heavily involved in Plasma, and I'm really not on the same page. - David Edmundson On July 24, 2014, 9:43 a.m., Marco Martin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119451/ --- (Updated July 24, 2014, 9:43 a.m.) Review request for Plasma. Repository: plasma-workspace Description --- This is still nowhere near final mergeability, but rather a request for comments for early stages ;) So, what does an application that uses stuff from lf needs? * open the proper lf package, as configured * fetch files from it * use files of the default one if the provided one is incomplete * monitor for theme changes and if necessary reload the qml * some applications may even want to have an optional local config that overrides it, like the splash, but is out of scope of a central management the branch uses a little class that does all of the above (minus the last point) and uses it for now in the splash screen For now it would just be statically linked into users, since they should be all in plasma-framework for now (of course not optimal) *maybe* is stuff for libplasmaquick, but that library since locks a release of p-f with the same release of users of it, should probably me made public or else, so I'm a bit hesitant to add further stuff into it. Diffs - ksplash/ksplashqml/CMakeLists.txt 16c58a0 ksplash/ksplashqml/SplashWindow.cpp 23603f5 ksplash/ksplashqml/shellpluginloader.h 9c0f624 lookandfeelaccess/lookandfeelaccess.h PRE-CREATION lookandfeelaccess/lookandfeelaccess.cpp PRE-CREATION lookandfeelaccess/shellpluginloader.h PRE-CREATION Diff: https://git.reviewboard.kde.org/r/119451/diff/ Testing --- Thanks, Marco Martin ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119452: Rename libkworkspace for coinstallability with kde-workspace4.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119452/#review63043 --- Ship it! fine for me - Marco Martin On Lug. 24, 2014, 10:19 a.m., Michael Palimaka wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119452/ --- (Updated Lug. 24, 2014, 10:19 a.m.) Review request for Plasma. Repository: plasma-workspace Description --- A number of KDE 4 applications depend on libkworkspace from kde-workspace, which currently collides with libkworkspace provided by plasma-workspace. Renaming makes it easier downstream to provide the ability run these applications in a Plasma 5 session. Diffs - libkworkspace/CMakeLists.txt e417c76ad4a032e6dc2fde9aae74352303821983 Diff: https://git.reviewboard.kde.org/r/119452/diff/ Testing --- plasma-workspace, khotkeys, and powerdevil builds. plasma-desktop requires a matching header include rename. Not aware of any other frameworks-based libkworkspace consumers. Thanks, Michael Palimaka ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119451: some machinery for look and feel package
On July 24, 2014, 10:34 a.m., David Edmundson wrote: lookandfeelaccess/lookandfeelaccess.cpp, line 89 https://git.reviewboard.kde.org/r/119451/diff/1/?file=292284#file292284line89 My concern is this will create more problems than it's worth. Scenario: - I add a new feature in the lock screen in 5.1 with a new rootContext variable - I update the QML to use this new feature in 5.1 - a user upgrades his system - We then reload the package (but not the binary) we get a QML error and the user can't log in again. so do you think some more complicated things like the lockscreen souldn't be themeable? may make sense on a maintainance standpoint, but yeah, needs definition of what should be in there, what shouldn't - Marco --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119451/#review63037 --- On July 24, 2014, 9:43 a.m., Marco Martin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119451/ --- (Updated July 24, 2014, 9:43 a.m.) Review request for Plasma. Repository: plasma-workspace Description --- This is still nowhere near final mergeability, but rather a request for comments for early stages ;) So, what does an application that uses stuff from lf needs? * open the proper lf package, as configured * fetch files from it * use files of the default one if the provided one is incomplete * monitor for theme changes and if necessary reload the qml * some applications may even want to have an optional local config that overrides it, like the splash, but is out of scope of a central management the branch uses a little class that does all of the above (minus the last point) and uses it for now in the splash screen For now it would just be statically linked into users, since they should be all in plasma-framework for now (of course not optimal) *maybe* is stuff for libplasmaquick, but that library since locks a release of p-f with the same release of users of it, should probably me made public or else, so I'm a bit hesitant to add further stuff into it. Diffs - ksplash/ksplashqml/CMakeLists.txt 16c58a0 ksplash/ksplashqml/SplashWindow.cpp 23603f5 ksplash/ksplashqml/shellpluginloader.h 9c0f624 lookandfeelaccess/lookandfeelaccess.h PRE-CREATION lookandfeelaccess/lookandfeelaccess.cpp PRE-CREATION lookandfeelaccess/shellpluginloader.h PRE-CREATION Diff: https://git.reviewboard.kde.org/r/119451/diff/ Testing --- Thanks, Marco Martin ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119451: some machinery for look and feel package
On July 24, 2014, 10:34 a.m., David Edmundson wrote: lookandfeelaccess/shellpluginloader.h, line 31 https://git.reviewboard.kde.org/r/119451/diff/1/?file=292285#file292285line31 not used? that is just the header 1:1 coming from libplasmaquick, usual old problem - Marco --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119451/#review63037 --- On July 24, 2014, 9:43 a.m., Marco Martin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119451/ --- (Updated July 24, 2014, 9:43 a.m.) Review request for Plasma. Repository: plasma-workspace Description --- This is still nowhere near final mergeability, but rather a request for comments for early stages ;) So, what does an application that uses stuff from lf needs? * open the proper lf package, as configured * fetch files from it * use files of the default one if the provided one is incomplete * monitor for theme changes and if necessary reload the qml * some applications may even want to have an optional local config that overrides it, like the splash, but is out of scope of a central management the branch uses a little class that does all of the above (minus the last point) and uses it for now in the splash screen For now it would just be statically linked into users, since they should be all in plasma-framework for now (of course not optimal) *maybe* is stuff for libplasmaquick, but that library since locks a release of p-f with the same release of users of it, should probably me made public or else, so I'm a bit hesitant to add further stuff into it. Diffs - ksplash/ksplashqml/CMakeLists.txt 16c58a0 ksplash/ksplashqml/SplashWindow.cpp 23603f5 ksplash/ksplashqml/shellpluginloader.h 9c0f624 lookandfeelaccess/lookandfeelaccess.h PRE-CREATION lookandfeelaccess/lookandfeelaccess.cpp PRE-CREATION lookandfeelaccess/shellpluginloader.h PRE-CREATION Diff: https://git.reviewboard.kde.org/r/119451/diff/ Testing --- Thanks, Marco Martin ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119451: some machinery for look and feel package
On July 24, 2014, 10:34 a.m., Marco Martin wrote: I'd quite like to have a chat about the goals of look and feel (next Monday meeting?). It was something where you guys clearly had a plan (or at least a name) before I got more heavily involved in Plasma, and I'm really not on the same page. yes, we can do that monday - Marco --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119451/#review63037 --- On July 24, 2014, 9:43 a.m., Marco Martin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119451/ --- (Updated July 24, 2014, 9:43 a.m.) Review request for Plasma. Repository: plasma-workspace Description --- This is still nowhere near final mergeability, but rather a request for comments for early stages ;) So, what does an application that uses stuff from lf needs? * open the proper lf package, as configured * fetch files from it * use files of the default one if the provided one is incomplete * monitor for theme changes and if necessary reload the qml * some applications may even want to have an optional local config that overrides it, like the splash, but is out of scope of a central management the branch uses a little class that does all of the above (minus the last point) and uses it for now in the splash screen For now it would just be statically linked into users, since they should be all in plasma-framework for now (of course not optimal) *maybe* is stuff for libplasmaquick, but that library since locks a release of p-f with the same release of users of it, should probably me made public or else, so I'm a bit hesitant to add further stuff into it. Diffs - ksplash/ksplashqml/CMakeLists.txt 16c58a0 ksplash/ksplashqml/SplashWindow.cpp 23603f5 ksplash/ksplashqml/shellpluginloader.h 9c0f624 lookandfeelaccess/lookandfeelaccess.h PRE-CREATION lookandfeelaccess/lookandfeelaccess.cpp PRE-CREATION lookandfeelaccess/shellpluginloader.h PRE-CREATION Diff: https://git.reviewboard.kde.org/r/119451/diff/ Testing --- Thanks, Marco Martin ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119451: some machinery for look and feel package
On July 24, 2014, 10:34 a.m., David Edmundson wrote: lookandfeelaccess/lookandfeelaccess.cpp, line 89 https://git.reviewboard.kde.org/r/119451/diff/1/?file=292284#file292284line89 My concern is this will create more problems than it's worth. Scenario: - I add a new feature in the lock screen in 5.1 with a new rootContext variable - I update the QML to use this new feature in 5.1 - a user upgrades his system - We then reload the package (but not the binary) we get a QML error and the user can't log in again. Marco Martin wrote: so do you think some more complicated things like the lockscreen souldn't be themeable? may make sense on a maintainance standpoint, but yeah, needs definition of what should be in there, what shouldn't It probably should be themeable. My concern was changing files for an application whilst it's running. But after I think about it more, that would happen anyway. All it would take is to have a Loader in a release that opens a file that gets renamed/deleted in an upgraded version and you'll have the same problems. It probably is best to notify and update the package so the app at least has a chance to handle it. I withdraw my comment. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119451/#review63037 --- On July 24, 2014, 9:43 a.m., Marco Martin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119451/ --- (Updated July 24, 2014, 9:43 a.m.) Review request for Plasma. Repository: plasma-workspace Description --- This is still nowhere near final mergeability, but rather a request for comments for early stages ;) So, what does an application that uses stuff from lf needs? * open the proper lf package, as configured * fetch files from it * use files of the default one if the provided one is incomplete * monitor for theme changes and if necessary reload the qml * some applications may even want to have an optional local config that overrides it, like the splash, but is out of scope of a central management the branch uses a little class that does all of the above (minus the last point) and uses it for now in the splash screen For now it would just be statically linked into users, since they should be all in plasma-framework for now (of course not optimal) *maybe* is stuff for libplasmaquick, but that library since locks a release of p-f with the same release of users of it, should probably me made public or else, so I'm a bit hesitant to add further stuff into it. Diffs - ksplash/ksplashqml/CMakeLists.txt 16c58a0 ksplash/ksplashqml/SplashWindow.cpp 23603f5 ksplash/ksplashqml/shellpluginloader.h 9c0f624 lookandfeelaccess/lookandfeelaccess.h PRE-CREATION lookandfeelaccess/lookandfeelaccess.cpp PRE-CREATION lookandfeelaccess/shellpluginloader.h PRE-CREATION Diff: https://git.reviewboard.kde.org/r/119451/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/#review63049 --- src/declarativeimports/core/framesvgitem.cpp https://git.reviewboard.kde.org/r/119425/#comment43774 Having spoken to you offline this does make sense, but it needs documentation as to why it's doing this. - David Edmundson On July 24, 2014, 12:44 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 24, 2014, 12:44 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 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 119451: some machinery for look and feel package
On July 24, 2014, 10:34 a.m., David Edmundson wrote: lookandfeelaccess/lookandfeelaccess.cpp, line 89 https://git.reviewboard.kde.org/r/119451/diff/1/?file=292284#file292284line89 My concern is this will create more problems than it's worth. Scenario: - I add a new feature in the lock screen in 5.1 with a new rootContext variable - I update the QML to use this new feature in 5.1 - a user upgrades his system - We then reload the package (but not the binary) we get a QML error and the user can't log in again. Marco Martin wrote: so do you think some more complicated things like the lockscreen souldn't be themeable? may make sense on a maintainance standpoint, but yeah, needs definition of what should be in there, what shouldn't David Edmundson wrote: It probably should be themeable. My concern was changing files for an application whilst it's running. But after I think about it more, that would happen anyway. All it would take is to have a Loader in a release that opens a file that gets renamed/deleted in an upgraded version and you'll have the same problems. It probably is best to notify and update the package so the app at least has a chance to handle it. I withdraw my comment. anyways, the class is just sending a signal, if the application has particular problems in reloading at runtime, it would just ignore the signal. btw at the moment it's signaling when the theme gets changed, but not when the files of the package gets upgraded (that looks like a separate can of worms...) - Marco --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119451/#review63037 --- On July 24, 2014, 9:43 a.m., Marco Martin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119451/ --- (Updated July 24, 2014, 9:43 a.m.) Review request for Plasma. Repository: plasma-workspace Description --- This is still nowhere near final mergeability, but rather a request for comments for early stages ;) So, what does an application that uses stuff from lf needs? * open the proper lf package, as configured * fetch files from it * use files of the default one if the provided one is incomplete * monitor for theme changes and if necessary reload the qml * some applications may even want to have an optional local config that overrides it, like the splash, but is out of scope of a central management the branch uses a little class that does all of the above (minus the last point) and uses it for now in the splash screen For now it would just be statically linked into users, since they should be all in plasma-framework for now (of course not optimal) *maybe* is stuff for libplasmaquick, but that library since locks a release of p-f with the same release of users of it, should probably me made public or else, so I'm a bit hesitant to add further stuff into it. Diffs - ksplash/ksplashqml/CMakeLists.txt 16c58a0 ksplash/ksplashqml/SplashWindow.cpp 23603f5 ksplash/ksplashqml/shellpluginloader.h 9c0f624 lookandfeelaccess/lookandfeelaccess.h PRE-CREATION lookandfeelaccess/lookandfeelaccess.cpp PRE-CREATION lookandfeelaccess/shellpluginloader.h PRE-CREATION Diff: https://git.reviewboard.kde.org/r/119451/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
On July 24, 2014, 10:55 a.m., David Edmundson wrote: src/declarativeimports/core/framesvgitem.cpp, line 45 https://git.reviewboard.kde.org/r/119425/diff/2/?file=292260#file292260line45 Having spoken to you offline this does make sense, but it needs documentation as to why it's doing this. uhm, why a filter on palette change? this would work for themes that use system colors but not the other ones. theme::themechanged should cover both cases (and if it doesn't, it should) - Marco --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119425/#review63049 --- On July 24, 2014, 12:44 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 24, 2014, 12:44 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 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 24, 2014, 10:55 a.m., David Edmundson wrote: src/declarativeimports/core/framesvgitem.cpp, line 45 https://git.reviewboard.kde.org/r/119425/diff/2/?file=292260#file292260line45 Having spoken to you offline this does make sense, but it needs documentation as to why it's doing this. Marco Martin wrote: uhm, why a filter on palette change? this would work for themes that use system colors but not the other ones. theme::themechanged should cover both cases (and if it doesn't, it should) See SvgPrivate::checkColorHints(). I agree it could make sense having it in plasma theme, but this should be part of another patch. - Aleix --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119425/#review63049 --- On July 24, 2014, 12:44 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 24, 2014, 12:44 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 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/ --- (Updated July 24, 2014, 11:14 a.m.) Review request for Plasma. Changes --- Pleased David. 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 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 119451: some machinery for look and feel package
On July 24, 2014, 12:34 p.m., David Edmundson wrote: lookandfeelaccess/lookandfeelaccess.cpp, line 89 https://git.reviewboard.kde.org/r/119451/diff/1/?file=292284#file292284line89 My concern is this will create more problems than it's worth. Scenario: - I add a new feature in the lock screen in 5.1 with a new rootContext variable - I update the QML to use this new feature in 5.1 - a user upgrades his system - We then reload the package (but not the binary) we get a QML error and the user can't log in again. Marco Martin wrote: so do you think some more complicated things like the lockscreen souldn't be themeable? may make sense on a maintainance standpoint, but yeah, needs definition of what should be in there, what shouldn't David Edmundson wrote: It probably should be themeable. My concern was changing files for an application whilst it's running. But after I think about it more, that would happen anyway. All it would take is to have a Loader in a release that opens a file that gets renamed/deleted in an upgraded version and you'll have the same problems. It probably is best to notify and update the package so the app at least has a chance to handle it. I withdraw my comment. Marco Martin wrote: anyways, the class is just sending a signal, if the application has particular problems in reloading at runtime, it would just ignore the signal. btw at the moment it's signaling when the theme gets changed, but not when the files of the package gets upgraded (that looks like a separate can of worms...) David, I think your fear is without need in this case as the greeter is not a long running process, but one that gets freshly started each time the screen gets locked. - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119451/#review63037 --- On July 24, 2014, 11:43 a.m., Marco Martin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119451/ --- (Updated July 24, 2014, 11:43 a.m.) Review request for Plasma. Repository: plasma-workspace Description --- This is still nowhere near final mergeability, but rather a request for comments for early stages ;) So, what does an application that uses stuff from lf needs? * open the proper lf package, as configured * fetch files from it * use files of the default one if the provided one is incomplete * monitor for theme changes and if necessary reload the qml * some applications may even want to have an optional local config that overrides it, like the splash, but is out of scope of a central management the branch uses a little class that does all of the above (minus the last point) and uses it for now in the splash screen For now it would just be statically linked into users, since they should be all in plasma-framework for now (of course not optimal) *maybe* is stuff for libplasmaquick, but that library since locks a release of p-f with the same release of users of it, should probably me made public or else, so I'm a bit hesitant to add further stuff into it. Diffs - ksplash/ksplashqml/CMakeLists.txt 16c58a0 ksplash/ksplashqml/SplashWindow.cpp 23603f5 ksplash/ksplashqml/shellpluginloader.h 9c0f624 lookandfeelaccess/lookandfeelaccess.h PRE-CREATION lookandfeelaccess/lookandfeelaccess.cpp PRE-CREATION lookandfeelaccess/shellpluginloader.h PRE-CREATION Diff: https://git.reviewboard.kde.org/r/119451/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
On July 24, 2014, 10:55 a.m., David Edmundson wrote: src/declarativeimports/core/framesvgitem.cpp, line 45 https://git.reviewboard.kde.org/r/119425/diff/2/?file=292260#file292260line45 Having spoken to you offline this does make sense, but it needs documentation as to why it's doing this. Marco Martin wrote: uhm, why a filter on palette change? this would work for themes that use system colors but not the other ones. theme::themechanged should cover both cases (and if it doesn't, it should) Aleix Pol Gonzalez wrote: See SvgPrivate::checkColorHints(). I agree it could make sense having it in plasma theme, but this should be part of another patch. it does a repaintneeded, that is correct. but yeah, having it in theme doesn't make much sense, because technically the theme didn't change and you can't know from there if one of the svgs actually needs a repaint. so, what all of this suggests me is that the thing that would make most sense is actually use repaintneeded, and either: a) just remove from the hash the id of this texture (multiple removes still possible tough) b) event compress the clear() c) both of the above - Marco --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119425/#review63049 --- On July 24, 2014, 11: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 24, 2014, 11: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 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 24, 2014, 10:55 a.m., David Edmundson wrote: src/declarativeimports/core/framesvgitem.cpp, line 45 https://git.reviewboard.kde.org/r/119425/diff/2/?file=292260#file292260line45 Having spoken to you offline this does make sense, but it needs documentation as to why it's doing this. Marco Martin wrote: uhm, why a filter on palette change? this would work for themes that use system colors but not the other ones. theme::themechanged should cover both cases (and if it doesn't, it should) Aleix Pol Gonzalez wrote: See SvgPrivate::checkColorHints(). I agree it could make sense having it in plasma theme, but this should be part of another patch. Marco Martin wrote: it does a repaintneeded, that is correct. but yeah, having it in theme doesn't make much sense, because technically the theme didn't change and you can't know from there if one of the svgs actually needs a repaint. so, what all of this suggests me is that the thing that would make most sense is actually use repaintneeded, and either: a) just remove from the hash the id of this texture (multiple removes still possible tough) b) event compress the clear() c) both of the above I'm getting a bit lost, sorry. Why is listening to all svg's better? RepaintNeeded will emit upon signals we don't need. Note that themeChanged doesn't clean the cache and it will rather be the textures just stopping to use the old theme, because the hash is refcounted. That's why I introduced more elements to the hash key. - Aleix --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119425/#review63049 --- On July 24, 2014, 11: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 24, 2014, 11: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 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 24, 2014, 10:55 a.m., David Edmundson wrote: src/declarativeimports/core/framesvgitem.cpp, line 45 https://git.reviewboard.kde.org/r/119425/diff/2/?file=292260#file292260line45 Having spoken to you offline this does make sense, but it needs documentation as to why it's doing this. Marco Martin wrote: uhm, why a filter on palette change? this would work for themes that use system colors but not the other ones. theme::themechanged should cover both cases (and if it doesn't, it should) Aleix Pol Gonzalez wrote: See SvgPrivate::checkColorHints(). I agree it could make sense having it in plasma theme, but this should be part of another patch. Marco Martin wrote: it does a repaintneeded, that is correct. but yeah, having it in theme doesn't make much sense, because technically the theme didn't change and you can't know from there if one of the svgs actually needs a repaint. so, what all of this suggests me is that the thing that would make most sense is actually use repaintneeded, and either: a) just remove from the hash the id of this texture (multiple removes still possible tough) b) event compress the clear() c) both of the above Aleix Pol Gonzalez wrote: I'm getting a bit lost, sorry. Why is listening to all svg's better? RepaintNeeded will emit upon signals we don't need. Note that themeChanged doesn't clean the cache and it will rather be the textures just stopping to use the old theme, because the hash is refcounted. That's why I introduced more elements to the hash key. Calling clear on an empty hash is an atomic operation. Maybe it should just go back to the version in revision 1. It's much simpler and probably much faster than all this extra string appending and monitoring. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119425/#review63049 --- On July 24, 2014, 11: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 24, 2014, 11: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 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 24, 2014, 10:55 a.m., David Edmundson wrote: src/declarativeimports/core/framesvgitem.cpp, line 45 https://git.reviewboard.kde.org/r/119425/diff/2/?file=292260#file292260line45 Having spoken to you offline this does make sense, but it needs documentation as to why it's doing this. Marco Martin wrote: uhm, why a filter on palette change? this would work for themes that use system colors but not the other ones. theme::themechanged should cover both cases (and if it doesn't, it should) Aleix Pol Gonzalez wrote: See SvgPrivate::checkColorHints(). I agree it could make sense having it in plasma theme, but this should be part of another patch. Marco Martin wrote: it does a repaintneeded, that is correct. but yeah, having it in theme doesn't make much sense, because technically the theme didn't change and you can't know from there if one of the svgs actually needs a repaint. so, what all of this suggests me is that the thing that would make most sense is actually use repaintneeded, and either: a) just remove from the hash the id of this texture (multiple removes still possible tough) b) event compress the clear() c) both of the above Aleix Pol Gonzalez wrote: I'm getting a bit lost, sorry. Why is listening to all svg's better? RepaintNeeded will emit upon signals we don't need. Note that themeChanged doesn't clean the cache and it will rather be the textures just stopping to use the old theme, because the hash is refcounted. That's why I introduced more elements to the hash key. David Edmundson wrote: Calling clear on an empty hash is an atomic operation. Maybe it should just go back to the version in revision 1. It's much simpler and probably much faster than all this extra string appending and monitoring. yes. If we can assure that the following scenario will never happen, better version number one scenario as in * clear * put something back in * clear again * repeat for every single instance - Marco --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119425/#review63049 --- On July 24, 2014, 11: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 24, 2014, 11: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 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 24, 2014, 10:55 a.m., David Edmundson wrote: src/declarativeimports/core/framesvgitem.cpp, line 45 https://git.reviewboard.kde.org/r/119425/diff/2/?file=292260#file292260line45 Having spoken to you offline this does make sense, but it needs documentation as to why it's doing this. Marco Martin wrote: uhm, why a filter on palette change? this would work for themes that use system colors but not the other ones. theme::themechanged should cover both cases (and if it doesn't, it should) Aleix Pol Gonzalez wrote: See SvgPrivate::checkColorHints(). I agree it could make sense having it in plasma theme, but this should be part of another patch. Marco Martin wrote: it does a repaintneeded, that is correct. but yeah, having it in theme doesn't make much sense, because technically the theme didn't change and you can't know from there if one of the svgs actually needs a repaint. so, what all of this suggests me is that the thing that would make most sense is actually use repaintneeded, and either: a) just remove from the hash the id of this texture (multiple removes still possible tough) b) event compress the clear() c) both of the above Aleix Pol Gonzalez wrote: I'm getting a bit lost, sorry. Why is listening to all svg's better? RepaintNeeded will emit upon signals we don't need. Note that themeChanged doesn't clean the cache and it will rather be the textures just stopping to use the old theme, because the hash is refcounted. That's why I introduced more elements to the hash key. David Edmundson wrote: Calling clear on an empty hash is an atomic operation. Maybe it should just go back to the version in revision 1. It's much simpler and probably much faster than all this extra string appending and monitoring. Marco Martin wrote: yes. If we can assure that the following scenario will never happen, better version number one scenario as in * clear * put something back in * clear again * repeat for every single instance I think we can ensure that, because the painting is carried out in a different thread, so it will end up in the next frame painting. - Aleix --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119425/#review63049 --- On July 24, 2014, 11: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 24, 2014, 11: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 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 119452: Rename libkworkspace for coinstallability with kde-workspace4.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119452/ --- (Updated July 24, 2014, 3:40 p.m.) Status -- This change has been marked as submitted. Review request for Plasma. Repository: plasma-workspace Description --- A number of KDE 4 applications depend on libkworkspace from kde-workspace, which currently collides with libkworkspace provided by plasma-workspace. Renaming makes it easier downstream to provide the ability run these applications in a Plasma 5 session. Diffs - libkworkspace/CMakeLists.txt e417c76ad4a032e6dc2fde9aae74352303821983 Diff: https://git.reviewboard.kde.org/r/119452/diff/ Testing --- plasma-workspace, khotkeys, and powerdevil builds. plasma-desktop requires a matching header include rename. Not aware of any other frameworks-based libkworkspace consumers. Thanks, Michael Palimaka ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Build failed in Jenkins: plasma-desktop_master_qt5 #487
See http://build.kde.org/job/plasma-desktop_master_qt5/487/changes Changes: [kensington] Fix header include to match plasma-workspace change. -- [...truncated 2488 lines...] http://build.kde.org/job/plasma-desktop_master_qt5/ws/containments/folder/plugin/internallibkonq/konq_operations.cpp:1031:69: warning: ‘KUrl’ is deprecated [-Wdeprecated-declarations] QPairbool, QString KonqOperations::pasteInfo(const KUrl targetUrl) ^ http://build.kde.org/job/plasma-desktop_master_qt5/ws/containments/folder/plugin/internallibkonq/konq_operations.cpp: In static member function ‘static QPairbool, QString KonqOperations::pasteInfo(const KUrl)’: http://build.kde.org/job/plasma-desktop_master_qt5/ws/containments/folder/plugin/internallibkonq/konq_operations.cpp:1038:16: warning: ‘List’ is deprecated (declared at /srv/jenkins/install/linux/x86_64/g++/kf5-qt5/frameworks/kdelibs4support/inst/include/KF5/KDELibs4Support/kurl.h:143) [-Wdeprecated-declarations] KUrl::List urls = KUrl::List::fromMimeData(mimeData); ^ http://build.kde.org/job/plasma-desktop_master_qt5/ws/containments/folder/plugin/internallibkonq/konq_operations.cpp:1038:56: warning: ‘static KUrl::List KUrl::List::fromMimeData(const QMimeData*, KUrl::List::DecodeOptions, KUrl::MetaDataMap*)’ is deprecated (declared at /srv/jenkins/install/linux/x86_64/g++/kf5-qt5/frameworks/kdelibs4support/inst/include/KF5/KDELibs4Support/kurl.h:317) [-Wdeprecated-declarations] KUrl::List urls = KUrl::List::fromMimeData(mimeData); ^ http://build.kde.org/job/plasma-desktop_master_qt5/ws/containments/folder/plugin/internallibkonq/konq_operations.cpp:1041:73: warning: ‘KFileItem::KFileItem(mode_t, mode_t, const QUrl, bool)’ is deprecated (declared at /srv/jenkins/install/linux/x86_64/g++/kf5-qt5/frameworks/kio/inst/include/KF5/KIOCore/kfileitem.h:104) [-Wdeprecated-declarations] KFileItem item(KFileItem::Unknown, KFileItem::Unknown, targetUrl); ^ http://build.kde.org/job/plasma-desktop_master_qt5/ws/containments/folder/plugin/internallibkonq/konq_operations.cpp:1045:92: warning: ‘KFileItem::KFileItem(mode_t, mode_t, const QUrl, bool)’ is deprecated (declared at /srv/jenkins/install/linux/x86_64/g++/kf5-qt5/frameworks/kio/inst/include/KF5/KIOCore/kfileitem.h:104) [-Wdeprecated-declarations] const KFileItem item(KFileItem::Unknown, KFileItem::Unknown, urls.first(), true); ^ In file included from http://build.kde.org/job/plasma-desktop_master_qt5/ws/kcms/input/xcursor/themepage.h:25:0, from http://build.kde.org/job/plasma-desktop_master_qt5/ws/build/kcms/input/../../../kcms/input/kcmcursortheme.h:23, from http://build.kde.org/job/plasma-desktop_master_qt5/ws/build/kcms/input/moc_kcmcursortheme.cpp:9, from http://build.kde.org/job/plasma-desktop_master_qt5/ws/build/kcms/input/kcm_cursortheme_automoc.cpp:2: http://build.kde.org/job/plasma-desktop_master_qt5/ws/build/kcms/input/ui_themepage.h:36:18: warning: ‘KPushButton’ is deprecated (declared at /srv/jenkins/install/linux/x86_64/g++/kf5-qt5/frameworks/kdelibs4support/inst/include/KF5/KDELibs4Support/kpushbutton.h:47) [-Wdeprecated-declarations] KPushButton *installKnsButton; ^ http://build.kde.org/job/plasma-desktop_master_qt5/ws/build/kcms/input/ui_themepage.h:37:18: warning: ‘KPushButton’ is deprecated (declared at /srv/jenkins/install/linux/x86_64/g++/kf5-qt5/frameworks/kdelibs4support/inst/include/KF5/KDELibs4Support/kpushbutton.h:47) [-Wdeprecated-declarations] KPushButton *installButton; ^ http://build.kde.org/job/plasma-desktop_master_qt5/ws/build/kcms/input/ui_themepage.h:38:18: warning: ‘KPushButton’ is deprecated (declared at /srv/jenkins/install/linux/x86_64/g++/kf5-qt5/frameworks/kdelibs4support/inst/include/KF5/KDELibs4Support/kpushbutton.h:47) [-Wdeprecated-declarations] KPushButton *removeButton; ^ http://build.kde.org/job/plasma-desktop_master_qt5/ws/build/kcms/input/ui_themepage.h: In member function ‘void Ui_ThemePage::setupUi(QWidget*)’: http://build.kde.org/job/plasma-desktop_master_qt5/ws/build/kcms/input/ui_themepage.h:77:32: warning: ‘KPushButton’ is deprecated (declared at /srv/jenkins/install/linux/x86_64/g++/kf5-qt5/frameworks/kdelibs4support/inst/include/KF5/KDELibs4Support/kpushbutton.h:47) [-Wdeprecated-declarations] installKnsButton = new KPushButton(ThemePage); ^ http://build.kde.org/job/plasma-desktop_master_qt5/ws/build/kcms/input/ui_themepage.h:82:29: warning: ‘KPushButton’ is deprecated (declared at
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
Jenkins build is back to normal : plasma-desktop_master_qt5 #488
See http://build.kde.org/job/plasma-desktop_master_qt5/488/changes ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
