Re: Review Request 122270: port kcminit away from kdelibs4support
On Jan. 29, 2015, 2:36 nachm., Martin Gräßlin wrote: I'm surprised that you pushed the change although the review was not finished and you hadn't a shipit on any of the versions. Nick, though it does not seem as if you had introduced it, the QGuiApplication::screens().count() 1 check is, as has been pointed out in this review several times, still wrong for sure. Do you intend to keep working on this code? (to get rid of QGuiApplication and perhaps the ini read) - Thomas --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122270/#review74982 --- On Jan. 29, 2015, 11:58 vorm., Nick Shaforostoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122270/ --- (Updated Jan. 29, 2015, 11:58 vorm.) Review request for kde-workspace, Aleix Pol Gonzalez, Martin Gräßlin, and Lukáš Tinkl. Repository: plasma-workspace Description --- Now kcminit is linked with less libraries - startup time improved I also suggest always setting KDE_MULTIHEAD=true to eliminate ini file access during startup and to be able to stop linking against QtGui Diffs - startkde/kcminit/CMakeLists.txt ffae38c startkde/kcminit/main.h 1140b77 startkde/kcminit/main.cpp 4724323 Diff: https://git.reviewboard.kde.org/r/122270/diff/ Testing --- compiled, ran 'kcminit --list' and kcminit AAA Thanks, Nick Shaforostoff
Re: Review Request 122270: port kcminit away from kdelibs4support
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122270/#review74973
---
I still think this is wrong as QGuiApplication::screens().count() gives the
count of the xrandr screens and not of X11 screens.
startkde/kcminit/main.cpp
https://git.reviewboard.kde.org/r/122270/#comment51927
nitpick coding style: whitespaces around the == and the { goes together
tisth the if()
startkde/kcminit/main.cpp
https://git.reviewboard.kde.org/r/122270/#comment51928
drop the else case and move the variable definition outside the ifdef block?
- Martin Gräßlin
On Jan. 29, 2015, 2:19 a.m., Nick Shaforostoff wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122270/
---
(Updated Jan. 29, 2015, 2:19 a.m.)
Review request for kde-workspace, Aleix Pol Gonzalez, Martin Gräßlin, and
Lukáš Tinkl.
Repository: plasma-workspace
Description
---
Now kcminit is linked with less libraries - startup time improved
I also suggest always setting KDE_MULTIHEAD=true to eliminate ini file access
during startup and to be able to stop linking against QtGui
Diffs
-
startkde/kcminit/CMakeLists.txt ffae38c
startkde/kcminit/main.h 1140b77
startkde/kcminit/main.cpp 4724323
Diff: https://git.reviewboard.kde.org/r/122270/diff/
Testing
---
compiled, ran 'kcminit --list' and kcminit AAA
Thanks,
Nick Shaforostoff
Re: Review Request 122270: port kcminit away from kdelibs4support
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122270/ --- (Updated Jan. 29, 2015, 11:58 a.m.) Status -- This change has been marked as submitted. Review request for kde-workspace, Aleix Pol Gonzalez, Martin Gräßlin, and Lukáš Tinkl. Repository: plasma-workspace Description --- Now kcminit is linked with less libraries - startup time improved I also suggest always setting KDE_MULTIHEAD=true to eliminate ini file access during startup and to be able to stop linking against QtGui Diffs - startkde/kcminit/CMakeLists.txt ffae38c startkde/kcminit/main.h 1140b77 startkde/kcminit/main.cpp 4724323 Diff: https://git.reviewboard.kde.org/r/122270/diff/ Testing --- compiled, ran 'kcminit --list' and kcminit AAA Thanks, Nick Shaforostoff
Re: Review Request 122270: port kcminit away from kdelibs4support
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122270/#review74982 --- I'm surprised that you pushed the change although the review was not finished and you hadn't a shipit on any of the versions. - Martin Gräßlin On Jan. 29, 2015, 12:58 p.m., Nick Shaforostoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122270/ --- (Updated Jan. 29, 2015, 12:58 p.m.) Review request for kde-workspace, Aleix Pol Gonzalez, Martin Gräßlin, and Lukáš Tinkl. Repository: plasma-workspace Description --- Now kcminit is linked with less libraries - startup time improved I also suggest always setting KDE_MULTIHEAD=true to eliminate ini file access during startup and to be able to stop linking against QtGui Diffs - startkde/kcminit/CMakeLists.txt ffae38c startkde/kcminit/main.h 1140b77 startkde/kcminit/main.cpp 4724323 Diff: https://git.reviewboard.kde.org/r/122270/diff/ Testing --- compiled, ran 'kcminit --list' and kcminit AAA Thanks, Nick Shaforostoff
Re: Review Request 122270: port kcminit away from kdelibs4support
On Січ. 29, 2015, 2:36 після полудня, Martin Gräßlin wrote: I'm surprised that you pushed the change although the review was not finished and you hadn't a shipit on any of the versions. Thomas Lübking wrote: Nick, though it does not seem as if you had introduced it, the QGuiApplication::screens().count() 1 check is, as has been pointed out in this review several times, still wrong for sure. Do you intend to keep working on this code? (to get rid of QGuiApplication and perhaps the ini read) sorry, should i revert the commit? yes, i intend to create a new review request with 'QGuiApplication::screens().count() 1' replacement using xcb routines. - Nick --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122270/#review74982 --- On Січ. 29, 2015, 11:58 до полудня, Nick Shaforostoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122270/ --- (Updated Січ. 29, 2015, 11:58 до полудня) Review request for kde-workspace, Aleix Pol Gonzalez, Martin Gräßlin, and Lukáš Tinkl. Repository: plasma-workspace Description --- Now kcminit is linked with less libraries - startup time improved I also suggest always setting KDE_MULTIHEAD=true to eliminate ini file access during startup and to be able to stop linking against QtGui Diffs - startkde/kcminit/CMakeLists.txt ffae38c startkde/kcminit/main.h 1140b77 startkde/kcminit/main.cpp 4724323 Diff: https://git.reviewboard.kde.org/r/122270/diff/ Testing --- compiled, ran 'kcminit --list' and kcminit AAA Thanks, Nick Shaforostoff
Re: Review Request 122270: port kcminit away from kdelibs4support
On Січ. 29, 2015, 2:36 після полудня, Martin Gräßlin wrote: I'm surprised that you pushed the change although the review was not finished and you hadn't a shipit on any of the versions. Thomas Lübking wrote: Nick, though it does not seem as if you had introduced it, the QGuiApplication::screens().count() 1 check is, as has been pointed out in this review several times, still wrong for sure. Do you intend to keep working on this code? (to get rid of QGuiApplication and perhaps the ini read) Nick Shaforostoff wrote: sorry, should i revert the commit? yes, i intend to create a new review request with 'QGuiApplication::screens().count() 1' replacement using xcb routines. i had a look at kcminit from KDE4 branch and indeed it used ScreenCount(QX11Info::display()), which can be substituted simply via xcb according to http://xcb.freedesktop.org/xlibtoxcbtranslationguide/ - Nick --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122270/#review74982 --- On Січ. 29, 2015, 11:58 до полудня, Nick Shaforostoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122270/ --- (Updated Січ. 29, 2015, 11:58 до полудня) Review request for kde-workspace, Aleix Pol Gonzalez, Martin Gräßlin, and Lukáš Tinkl. Repository: plasma-workspace Description --- Now kcminit is linked with less libraries - startup time improved I also suggest always setting KDE_MULTIHEAD=true to eliminate ini file access during startup and to be able to stop linking against QtGui Diffs - startkde/kcminit/CMakeLists.txt ffae38c startkde/kcminit/main.h 1140b77 startkde/kcminit/main.cpp 4724323 Diff: https://git.reviewboard.kde.org/r/122270/diff/ Testing --- compiled, ran 'kcminit --list' and kcminit AAA Thanks, Nick Shaforostoff
Re: Review Request 122270: port kcminit away from kdelibs4support
On Jan. 27, 2015, 7:59 a.m., Martin Gräßlin wrote: startkde/kcminit/main.cpp, lines 250-254 https://git.reviewboard.kde.org/r/122270/diff/1/?file=345342#file345342line250 I do not like this. If the only need is to check whether it's X11 multi-head, it should open an xcb_connection_t - if that fails we don't have an X-Server. If it succeeds we can check the number of screens. Nick Shaforostoff wrote: understood, but the question remains: is kcminit the best place to do multihead-related stuff? Thomas Lübking wrote: It's required by KApplication and while that's in kde4support, it's still there (as well as KDE4 clients) So yes, we need that everywhere - for now. Clients seem to default the env to false, so uncoditionally setting it true is wrong for sure. It'd rather be multihead = (screenCount 1); (ignoring the ini) what however would be a feature loss. = Proposal users should be able to pre-control the variable in eg. ~/.kde/env (or wherever the Plasma 2 equivalent is) If the variable is set to false, it remains like that. If not (ie. it's not set or set true resp. anything but 0/false) it becomes true if the screen count 1 Spares the ini lookup, but I don't think you get around xcb_connection_t *c; int screen_count = xcb_setup_roots_length(xcb_get_setup(c)); without risking to really break stuff (the suggested approach still causes a transitional break) Nick Shaforostoff wrote: reading ini file is not more than 0.5 second with 'cold cache'. getting rid of kdelibs4support for kcminit is 2 seconds gain. direct xcb request would require copying some 30 lines of code from xcp qpa plugin (= a no go). direct xcb request would require copying some 30 lines of code from xcp qpa plugin that should be less and can be done without copying code form xcb qpa. Feel free to look at kwin/main_x11.cpp to see how it can be done. As I did the port to xcb I can safely grant a relicense if that's needed. - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122270/#review74806 --- On Jan. 28, 2015, 1:47 a.m., Nick Shaforostoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122270/ --- (Updated Jan. 28, 2015, 1:47 a.m.) Review request for kde-workspace, Aleix Pol Gonzalez, Martin Gräßlin, and Lukáš Tinkl. Repository: plasma-workspace Description --- Now kcminit is linked with less libraries - startup time improved I also suggest always setting KDE_MULTIHEAD=true to eliminate ini file access during startup and to be able to stop linking against QtGui Diffs - startkde/kcminit/CMakeLists.txt ffae38c startkde/kcminit/main.h 1140b77 startkde/kcminit/main.cpp 4724323 Diff: https://git.reviewboard.kde.org/r/122270/diff/ Testing --- compiled, ran 'kcminit --list' and kcminit AAA Thanks, Nick Shaforostoff
Re: Review Request 122270: port kcminit away from kdelibs4support
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122270/#review74892 --- startkde/kcminit/main.cpp https://git.reviewboard.kde.org/r/122270/#comment51890 this is still unconditionally executed on all platforms. It's missing a qpa plugin check. - Martin Gräßlin On Jan. 28, 2015, 1:47 a.m., Nick Shaforostoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122270/ --- (Updated Jan. 28, 2015, 1:47 a.m.) Review request for kde-workspace, Aleix Pol Gonzalez, Martin Gräßlin, and Lukáš Tinkl. Repository: plasma-workspace Description --- Now kcminit is linked with less libraries - startup time improved I also suggest always setting KDE_MULTIHEAD=true to eliminate ini file access during startup and to be able to stop linking against QtGui Diffs - startkde/kcminit/CMakeLists.txt ffae38c startkde/kcminit/main.h 1140b77 startkde/kcminit/main.cpp 4724323 Diff: https://git.reviewboard.kde.org/r/122270/diff/ Testing --- compiled, ran 'kcminit --list' and kcminit AAA Thanks, Nick Shaforostoff
Re: Review Request 122270: port kcminit away from kdelibs4support
On Jan. 27, 2015, 6:59 vorm., Martin Gräßlin wrote: startkde/kcminit/main.cpp, lines 250-254 https://git.reviewboard.kde.org/r/122270/diff/1/?file=345342#file345342line250 I do not like this. If the only need is to check whether it's X11 multi-head, it should open an xcb_connection_t - if that fails we don't have an X-Server. If it succeeds we can check the number of screens. Nick Shaforostoff wrote: understood, but the question remains: is kcminit the best place to do multihead-related stuff? Thomas Lübking wrote: It's required by KApplication and while that's in kde4support, it's still there (as well as KDE4 clients) So yes, we need that everywhere - for now. Clients seem to default the env to false, so uncoditionally setting it true is wrong for sure. It'd rather be multihead = (screenCount 1); (ignoring the ini) what however would be a feature loss. = Proposal users should be able to pre-control the variable in eg. ~/.kde/env (or wherever the Plasma 2 equivalent is) If the variable is set to false, it remains like that. If not (ie. it's not set or set true resp. anything but 0/false) it becomes true if the screen count 1 Spares the ini lookup, but I don't think you get around xcb_connection_t *c; int screen_count = xcb_setup_roots_length(xcb_get_setup(c)); without risking to really break stuff (the suggested approach still causes a transitional break) Nick Shaforostoff wrote: reading ini file is not more than 0.5 second with 'cold cache'. getting rid of kdelibs4support for kcminit is 2 seconds gain. direct xcb request would require copying some 30 lines of code from xcp qpa plugin (= a no go). Martin Gräßlin wrote: direct xcb request would require copying some 30 lines of code from xcp qpa plugin that should be less and can be done without copying code form xcb qpa. Feel free to look at kwin/main_x11.cpp to see how it can be done. As I did the port to xcb I can safely grant a relicense if that's needed. Nick Shaforostoff wrote: aree you talking about this code? int primaryScreen = 0; xcb_connection_t *c = xcb_connect(nullptr, primaryScreen); const int number_of_screens = xcb_setup_roots_length(xcb_get_setup(c)); xcb_connect(c); its results differ from QGuiApplication::screens().size(). for me it still returns 1 when i have external monitor connected to my laptop. see void QXcbConnection::updateScreens() in src/plugins/platforms/xcb/qxcbconnection.cpp unless my memory is really broken, qga::screens returns a list of panels, ie. also for the normal xrandr setup. on your multiscreen setup, does the running kwin operate on both screens and eg. allow you to move a window from one screen to another? - Thomas --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122270/#review74806 --- On Jan. 28, 2015, 12:47 vorm., Nick Shaforostoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122270/ --- (Updated Jan. 28, 2015, 12:47 vorm.) Review request for kde-workspace, Aleix Pol Gonzalez, Martin Gräßlin, and Lukáš Tinkl. Repository: plasma-workspace Description --- Now kcminit is linked with less libraries - startup time improved I also suggest always setting KDE_MULTIHEAD=true to eliminate ini file access during startup and to be able to stop linking against QtGui Diffs - startkde/kcminit/CMakeLists.txt ffae38c startkde/kcminit/main.h 1140b77 startkde/kcminit/main.cpp 4724323 Diff: https://git.reviewboard.kde.org/r/122270/diff/ Testing --- compiled, ran 'kcminit --list' and kcminit AAA Thanks, Nick Shaforostoff
Re: Review Request 122270: port kcminit away from kdelibs4support
On Jan. 27, 2015, 6:59 a.m., Martin Gräßlin wrote: startkde/kcminit/main.cpp, lines 250-254 https://git.reviewboard.kde.org/r/122270/diff/1/?file=345342#file345342line250 I do not like this. If the only need is to check whether it's X11 multi-head, it should open an xcb_connection_t - if that fails we don't have an X-Server. If it succeeds we can check the number of screens. Nick Shaforostoff wrote: understood, but the question remains: is kcminit the best place to do multihead-related stuff? Thomas Lübking wrote: It's required by KApplication and while that's in kde4support, it's still there (as well as KDE4 clients) So yes, we need that everywhere - for now. Clients seem to default the env to false, so uncoditionally setting it true is wrong for sure. It'd rather be multihead = (screenCount 1); (ignoring the ini) what however would be a feature loss. = Proposal users should be able to pre-control the variable in eg. ~/.kde/env (or wherever the Plasma 2 equivalent is) If the variable is set to false, it remains like that. If not (ie. it's not set or set true resp. anything but 0/false) it becomes true if the screen count 1 Spares the ini lookup, but I don't think you get around xcb_connection_t *c; int screen_count = xcb_setup_roots_length(xcb_get_setup(c)); without risking to really break stuff (the suggested approach still causes a transitional break) Nick Shaforostoff wrote: reading ini file is not more than 0.5 second with 'cold cache'. getting rid of kdelibs4support for kcminit is 2 seconds gain. direct xcb request would require copying some 30 lines of code from xcp qpa plugin (= a no go). Martin Gräßlin wrote: direct xcb request would require copying some 30 lines of code from xcp qpa plugin that should be less and can be done without copying code form xcb qpa. Feel free to look at kwin/main_x11.cpp to see how it can be done. As I did the port to xcb I can safely grant a relicense if that's needed. aree you talking about this code? int primaryScreen = 0; xcb_connection_t *c = xcb_connect(nullptr, primaryScreen); const int number_of_screens = xcb_setup_roots_length(xcb_get_setup(c)); xcb_connect(c); its results differ from QGuiApplication::screens().size(). for me it still returns 1 when i have external monitor connected to my laptop. see void QXcbConnection::updateScreens() in src/plugins/platforms/xcb/qxcbconnection.cpp - Nick --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122270/#review74806 --- On Jan. 28, 2015, 12:47 a.m., Nick Shaforostoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122270/ --- (Updated Jan. 28, 2015, 12:47 a.m.) Review request for kde-workspace, Aleix Pol Gonzalez, Martin Gräßlin, and Lukáš Tinkl. Repository: plasma-workspace Description --- Now kcminit is linked with less libraries - startup time improved I also suggest always setting KDE_MULTIHEAD=true to eliminate ini file access during startup and to be able to stop linking against QtGui Diffs - startkde/kcminit/CMakeLists.txt ffae38c startkde/kcminit/main.h 1140b77 startkde/kcminit/main.cpp 4724323 Diff: https://git.reviewboard.kde.org/r/122270/diff/ Testing --- compiled, ran 'kcminit --list' and kcminit AAA Thanks, Nick Shaforostoff
Re: Review Request 122270: port kcminit away from kdelibs4support
On Jan. 27, 2015, 7:59 a.m., Martin Gräßlin wrote: startkde/kcminit/main.cpp, lines 250-254 https://git.reviewboard.kde.org/r/122270/diff/1/?file=345342#file345342line250 I do not like this. If the only need is to check whether it's X11 multi-head, it should open an xcb_connection_t - if that fails we don't have an X-Server. If it succeeds we can check the number of screens. Nick Shaforostoff wrote: understood, but the question remains: is kcminit the best place to do multihead-related stuff? Thomas Lübking wrote: It's required by KApplication and while that's in kde4support, it's still there (as well as KDE4 clients) So yes, we need that everywhere - for now. Clients seem to default the env to false, so uncoditionally setting it true is wrong for sure. It'd rather be multihead = (screenCount 1); (ignoring the ini) what however would be a feature loss. = Proposal users should be able to pre-control the variable in eg. ~/.kde/env (or wherever the Plasma 2 equivalent is) If the variable is set to false, it remains like that. If not (ie. it's not set or set true resp. anything but 0/false) it becomes true if the screen count 1 Spares the ini lookup, but I don't think you get around xcb_connection_t *c; int screen_count = xcb_setup_roots_length(xcb_get_setup(c)); without risking to really break stuff (the suggested approach still causes a transitional break) Nick Shaforostoff wrote: reading ini file is not more than 0.5 second with 'cold cache'. getting rid of kdelibs4support for kcminit is 2 seconds gain. direct xcb request would require copying some 30 lines of code from xcp qpa plugin (= a no go). Martin Gräßlin wrote: direct xcb request would require copying some 30 lines of code from xcp qpa plugin that should be less and can be done without copying code form xcb qpa. Feel free to look at kwin/main_x11.cpp to see how it can be done. As I did the port to xcb I can safely grant a relicense if that's needed. Nick Shaforostoff wrote: aree you talking about this code? int primaryScreen = 0; xcb_connection_t *c = xcb_connect(nullptr, primaryScreen); const int number_of_screens = xcb_setup_roots_length(xcb_get_setup(c)); xcb_connect(c); its results differ from QGuiApplication::screens().size(). for me it still returns 1 when i have external monitor connected to my laptop. see void QXcbConnection::updateScreens() in src/plugins/platforms/xcb/qxcbconnection.cpp Thomas Lübking wrote: unless my memory is really broken, qga::screens returns a list of panels, ie. also for the normal xrandr setup. on your multiscreen setup, does the running kwin operate on both screens and eg. allow you to move a window from one screen to another? If you want to test a multi-head system try using Xephyr: Xephyr -screen 1024x768x24 -screen 1024x768x24 :99 in that case you should get number_of_screens to be 2. I'm quite sure about it as I used it this week to test multi-head related changes in kwin and got the correct number of kwin instances running. - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122270/#review74806 --- On Jan. 28, 2015, 1:47 a.m., Nick Shaforostoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122270/ --- (Updated Jan. 28, 2015, 1:47 a.m.) Review request for kde-workspace, Aleix Pol Gonzalez, Martin Gräßlin, and Lukáš Tinkl. Repository: plasma-workspace Description --- Now kcminit is linked with less libraries - startup time improved I also suggest always setting KDE_MULTIHEAD=true to eliminate ini file access during startup and to be able to stop linking against QtGui Diffs - startkde/kcminit/CMakeLists.txt ffae38c startkde/kcminit/main.h 1140b77 startkde/kcminit/main.cpp 4724323 Diff: https://git.reviewboard.kde.org/r/122270/diff/ Testing --- compiled, ran 'kcminit --list' and kcminit AAA Thanks, Nick Shaforostoff
Re: Review Request 122270: port kcminit away from kdelibs4support
On Jan. 27, 2015, 6:59 a.m., Martin Gräßlin wrote: startkde/kcminit/main.cpp, lines 250-254 https://git.reviewboard.kde.org/r/122270/diff/1/?file=345342#file345342line250 I do not like this. If the only need is to check whether it's X11 multi-head, it should open an xcb_connection_t - if that fails we don't have an X-Server. If it succeeds we can check the number of screens. Nick Shaforostoff wrote: understood, but the question remains: is kcminit the best place to do multihead-related stuff? Thomas Lübking wrote: It's required by KApplication and while that's in kde4support, it's still there (as well as KDE4 clients) So yes, we need that everywhere - for now. Clients seem to default the env to false, so uncoditionally setting it true is wrong for sure. It'd rather be multihead = (screenCount 1); (ignoring the ini) what however would be a feature loss. = Proposal users should be able to pre-control the variable in eg. ~/.kde/env (or wherever the Plasma 2 equivalent is) If the variable is set to false, it remains like that. If not (ie. it's not set or set true resp. anything but 0/false) it becomes true if the screen count 1 Spares the ini lookup, but I don't think you get around xcb_connection_t *c; int screen_count = xcb_setup_roots_length(xcb_get_setup(c)); without risking to really break stuff (the suggested approach still causes a transitional break) Nick Shaforostoff wrote: reading ini file is not more than 0.5 second with 'cold cache'. getting rid of kdelibs4support for kcminit is 2 seconds gain. direct xcb request would require copying some 30 lines of code from xcp qpa plugin (= a no go). Martin Gräßlin wrote: direct xcb request would require copying some 30 lines of code from xcp qpa plugin that should be less and can be done without copying code form xcb qpa. Feel free to look at kwin/main_x11.cpp to see how it can be done. As I did the port to xcb I can safely grant a relicense if that's needed. Nick Shaforostoff wrote: aree you talking about this code? int primaryScreen = 0; xcb_connection_t *c = xcb_connect(nullptr, primaryScreen); const int number_of_screens = xcb_setup_roots_length(xcb_get_setup(c)); xcb_connect(c); its results differ from QGuiApplication::screens().size(). for me it still returns 1 when i have external monitor connected to my laptop. see void QXcbConnection::updateScreens() in src/plugins/platforms/xcb/qxcbconnection.cpp Thomas Lübking wrote: unless my memory is really broken, qga::screens returns a list of panels, ie. also for the normal xrandr setup. on your multiscreen setup, does the running kwin operate on both screens and eg. allow you to move a window from one screen to another? Martin Gräßlin wrote: If you want to test a multi-head system try using Xephyr: Xephyr -screen 1024x768x24 -screen 1024x768x24 :99 in that case you should get number_of_screens to be 2. I'm quite sure about it as I used it this week to test multi-head related changes in kwin and got the correct number of kwin instances running. Thomas Lübking: on your multiscreen setup, does the running kwin operate on both screens and eg. allow you to move a window from one screen to another? in kde5, when i connect external monitor, it has black background (in kde4 it immediately gets background image), immediately everything becomes slow: at first top shows that 4 'migration' processes are taking cpu time, then after a while kded5 eats 100% cpu time (it goes crazy with PowerDevilUPowerBackend::brightnessValueMax() and PowerDevilUPowerBackend::brightnessValue()) -- see bug 337674. so, in kde5, when i move a window to external monitor, only then it gets background image. - Nick --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122270/#review74806 --- On Jan. 28, 2015, 12:47 a.m., Nick Shaforostoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122270/ --- (Updated Jan. 28, 2015, 12:47 a.m.) Review request for kde-workspace, Aleix Pol Gonzalez, Martin Gräßlin, and Lukáš Tinkl. Repository: plasma-workspace Description --- Now kcminit is linked with less libraries - startup time improved I also suggest always setting KDE_MULTIHEAD=true to eliminate ini file access during startup and to be able to stop linking against QtGui Diffs - startkde/kcminit/CMakeLists.txt ffae38c startkde/kcminit/main.h 1140b77
Re: Review Request 122270: port kcminit away from kdelibs4support
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122270/ --- (Updated Jan. 29, 2015, 1:19 a.m.) Review request for kde-workspace, Aleix Pol Gonzalez, Martin Gräßlin, and Lukáš Tinkl. Changes --- added a qpa check (xcb) Repository: plasma-workspace Description --- Now kcminit is linked with less libraries - startup time improved I also suggest always setting KDE_MULTIHEAD=true to eliminate ini file access during startup and to be able to stop linking against QtGui Diffs (updated) - startkde/kcminit/CMakeLists.txt ffae38c startkde/kcminit/main.h 1140b77 startkde/kcminit/main.cpp 4724323 Diff: https://git.reviewboard.kde.org/r/122270/diff/ Testing --- compiled, ran 'kcminit --list' and kcminit AAA Thanks, Nick Shaforostoff
Re: Review Request 122270: port kcminit away from kdelibs4support
On Jan. 27, 2015, 6:59 a.m., Martin Gräßlin wrote: startkde/kcminit/main.cpp, lines 250-254 https://git.reviewboard.kde.org/r/122270/diff/1/?file=345342#file345342line250 I do not like this. If the only need is to check whether it's X11 multi-head, it should open an xcb_connection_t - if that fails we don't have an X-Server. If it succeeds we can check the number of screens. Nick Shaforostoff wrote: understood, but the question remains: is kcminit the best place to do multihead-related stuff? Thomas Lübking wrote: It's required by KApplication and while that's in kde4support, it's still there (as well as KDE4 clients) So yes, we need that everywhere - for now. Clients seem to default the env to false, so uncoditionally setting it true is wrong for sure. It'd rather be multihead = (screenCount 1); (ignoring the ini) what however would be a feature loss. = Proposal users should be able to pre-control the variable in eg. ~/.kde/env (or wherever the Plasma 2 equivalent is) If the variable is set to false, it remains like that. If not (ie. it's not set or set true resp. anything but 0/false) it becomes true if the screen count 1 Spares the ini lookup, but I don't think you get around xcb_connection_t *c; int screen_count = xcb_setup_roots_length(xcb_get_setup(c)); without risking to really break stuff (the suggested approach still causes a transitional break) Nick Shaforostoff wrote: reading ini file is not more than 0.5 second with 'cold cache'. getting rid of kdelibs4support for kcminit is 2 seconds gain. direct xcb request would require copying some 30 lines of code from xcp qpa plugin (= a no go). Martin Gräßlin wrote: direct xcb request would require copying some 30 lines of code from xcp qpa plugin that should be less and can be done without copying code form xcb qpa. Feel free to look at kwin/main_x11.cpp to see how it can be done. As I did the port to xcb I can safely grant a relicense if that's needed. Nick Shaforostoff wrote: aree you talking about this code? int primaryScreen = 0; xcb_connection_t *c = xcb_connect(nullptr, primaryScreen); const int number_of_screens = xcb_setup_roots_length(xcb_get_setup(c)); xcb_connect(c); its results differ from QGuiApplication::screens().size(). for me it still returns 1 when i have external monitor connected to my laptop. see void QXcbConnection::updateScreens() in src/plugins/platforms/xcb/qxcbconnection.cpp Thomas Lübking wrote: unless my memory is really broken, qga::screens returns a list of panels, ie. also for the normal xrandr setup. on your multiscreen setup, does the running kwin operate on both screens and eg. allow you to move a window from one screen to another? Martin Gräßlin wrote: If you want to test a multi-head system try using Xephyr: Xephyr -screen 1024x768x24 -screen 1024x768x24 :99 in that case you should get number_of_screens to be 2. I'm quite sure about it as I used it this week to test multi-head related changes in kwin and got the correct number of kwin instances running. Nick Shaforostoff wrote: Thomas Lübking: on your multiscreen setup, does the running kwin operate on both screens and eg. allow you to move a window from one screen to another? in kde5, when i connect external monitor, it has black background (in kde4 it immediately gets background image), immediately everything becomes slow: at first top shows that 4 'migration' processes are taking cpu time, then after a while kded5 eats 100% cpu time (it goes crazy with PowerDevilUPowerBackend::brightnessValueMax() and PowerDevilUPowerBackend::brightnessValue()) -- see bug 337674. so, in kde5, when i move a window to external monitor, only then it gets background image. update: after i connect external monitor, migration/0..3 processes start taking cpu time permanently, only until I drag konsole window onto external motinor. after this the perceived slowness disappears, but kded5 starts using 100% cpu (the reported bug) - Nick --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122270/#review74806 --- On Jan. 28, 2015, 12:47 a.m., Nick Shaforostoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122270/ --- (Updated Jan. 28, 2015, 12:47 a.m.) Review request for kde-workspace, Aleix Pol Gonzalez, Martin Gräßlin, and Lukáš Tinkl. Repository: plasma-workspace Description
Re: Review Request 122270: port kcminit away from kdelibs4support
On Jan. 27, 2015, 6:59 vorm., Martin Gräßlin wrote: startkde/kcminit/main.cpp, lines 250-254 https://git.reviewboard.kde.org/r/122270/diff/1/?file=345342#file345342line250 I do not like this. If the only need is to check whether it's X11 multi-head, it should open an xcb_connection_t - if that fails we don't have an X-Server. If it succeeds we can check the number of screens. Nick Shaforostoff wrote: understood, but the question remains: is kcminit the best place to do multihead-related stuff? Thomas Lübking wrote: It's required by KApplication and while that's in kde4support, it's still there (as well as KDE4 clients) So yes, we need that everywhere - for now. Clients seem to default the env to false, so uncoditionally setting it true is wrong for sure. It'd rather be multihead = (screenCount 1); (ignoring the ini) what however would be a feature loss. = Proposal users should be able to pre-control the variable in eg. ~/.kde/env (or wherever the Plasma 2 equivalent is) If the variable is set to false, it remains like that. If not (ie. it's not set or set true resp. anything but 0/false) it becomes true if the screen count 1 Spares the ini lookup, but I don't think you get around xcb_connection_t *c; int screen_count = xcb_setup_roots_length(xcb_get_setup(c)); without risking to really break stuff (the suggested approach still causes a transitional break) Nick Shaforostoff wrote: reading ini file is not more than 0.5 second with 'cold cache'. getting rid of kdelibs4support for kcminit is 2 seconds gain. direct xcb request would require copying some 30 lines of code from xcp qpa plugin (= a no go). Martin Gräßlin wrote: direct xcb request would require copying some 30 lines of code from xcp qpa plugin that should be less and can be done without copying code form xcb qpa. Feel free to look at kwin/main_x11.cpp to see how it can be done. As I did the port to xcb I can safely grant a relicense if that's needed. Nick Shaforostoff wrote: aree you talking about this code? int primaryScreen = 0; xcb_connection_t *c = xcb_connect(nullptr, primaryScreen); const int number_of_screens = xcb_setup_roots_length(xcb_get_setup(c)); xcb_connect(c); its results differ from QGuiApplication::screens().size(). for me it still returns 1 when i have external monitor connected to my laptop. see void QXcbConnection::updateScreens() in src/plugins/platforms/xcb/qxcbconnection.cpp Thomas Lübking wrote: unless my memory is really broken, qga::screens returns a list of panels, ie. also for the normal xrandr setup. on your multiscreen setup, does the running kwin operate on both screens and eg. allow you to move a window from one screen to another? Martin Gräßlin wrote: If you want to test a multi-head system try using Xephyr: Xephyr -screen 1024x768x24 -screen 1024x768x24 :99 in that case you should get number_of_screens to be 2. I'm quite sure about it as I used it this week to test multi-head related changes in kwin and got the correct number of kwin instances running. Nick Shaforostoff wrote: Thomas Lübking: on your multiscreen setup, does the running kwin operate on both screens and eg. allow you to move a window from one screen to another? in kde5, when i connect external monitor, it has black background (in kde4 it immediately gets background image), immediately everything becomes slow: at first top shows that 4 'migration' processes are taking cpu time, then after a while kded5 eats 100% cpu time (it goes crazy with PowerDevilUPowerBackend::brightnessValueMax() and PowerDevilUPowerBackend::brightnessValue()) -- see bug 337674. so, in kde5, when i move a window to external monitor, only then it gets background image. Nick Shaforostoff wrote: update: after i connect external monitor, migration/0..3 processes start taking cpu time permanently, only until I drag konsole window onto external motinor. after this the perceived slowness disappears, but kded5 starts using 100% cpu (the reported bug) I canno really say anything about the kded5 behaviour or why plasma does not add/extend a desktop, but - staying on topic - the ability to drag konsole window onto external monitor is a clear indication that this is NOT a zaphod mode - the only valid value for multihead in that setup is false. To create a zaphod/multihead setup, you'll have to configure xorg.conf or - simpler - use xephyr w/t the command Martin posted. FYI: the zaphod mode is the original, pre-xinerama, pre-xrandr, multiscreen setup. Each screen has a root window of its own, thus needs a WM for its own and it's not possible to move windows around. In theory you could run KDE on the one screen and
Re: Review Request 122270: port kcminit away from kdelibs4support
On Січ. 27, 2015, 6:59 до полудня, Martin Gräßlin wrote: startkde/kcminit/main.cpp, lines 250-254 https://git.reviewboard.kde.org/r/122270/diff/1/?file=345342#file345342line250 I do not like this. If the only need is to check whether it's X11 multi-head, it should open an xcb_connection_t - if that fails we don't have an X-Server. If it succeeds we can check the number of screens. understood, but the question remains: is kcminit the best place to do multihead-related stuff? - Nick --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122270/#review74806 --- On Січ. 27, 2015, 3:10 до полудня, Nick Shaforostoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122270/ --- (Updated Січ. 27, 2015, 3:10 до полудня) Review request for kde-workspace, Aleix Pol Gonzalez, Martin Gräßlin, and Lukáš Tinkl. Repository: plasma-workspace Description --- Now kcminit is linked with less libraries - startup time improved I also suggest always setting KDE_MULTIHEAD=true to eliminate ini file access during startup and to be able to stop linking against QtGui Diffs - startkde/kcminit/CMakeLists.txt ffae38c startkde/kcminit/main.h 1140b77 startkde/kcminit/main.cpp 4724323 Diff: https://git.reviewboard.kde.org/r/122270/diff/ Testing --- compiled, ran 'kcminit --list' and kcminit AAA Thanks, Nick Shaforostoff
Re: Review Request 122270: port kcminit away from kdelibs4support
On Jan. 27, 2015, 6:59 vorm., Martin Gräßlin wrote: startkde/kcminit/main.cpp, lines 250-254 https://git.reviewboard.kde.org/r/122270/diff/1/?file=345342#file345342line250 I do not like this. If the only need is to check whether it's X11 multi-head, it should open an xcb_connection_t - if that fails we don't have an X-Server. If it succeeds we can check the number of screens. Nick Shaforostoff wrote: understood, but the question remains: is kcminit the best place to do multihead-related stuff? It's required by KApplication and while that's in kde4support, it's still there (as well as KDE4 clients) So yes, we need that everywhere - for now. Clients seem to default the env to false, so uncoditionally setting it true is wrong for sure. It'd rather be multihead = (screenCount 1); (ignoring the ini) what however would be a feature loss. = Proposal users should be able to pre-control the variable in eg. ~/.kde/env (or wherever the Plasma 2 equivalent is) If the variable is set to false, it remains like that. If not (ie. it's not set or set true resp. anything but 0/false) it becomes true if the screen count 1 Spares the ini lookup, but I don't think you get around xcb_connection_t *c; int screen_count = xcb_setup_roots_length(xcb_get_setup(c)); without risking to really break stuff (the suggested approach still causes a transitional break) - Thomas --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122270/#review74806 --- On Jan. 28, 2015, 12:47 vorm., Nick Shaforostoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122270/ --- (Updated Jan. 28, 2015, 12:47 vorm.) Review request for kde-workspace, Aleix Pol Gonzalez, Martin Gräßlin, and Lukáš Tinkl. Repository: plasma-workspace Description --- Now kcminit is linked with less libraries - startup time improved I also suggest always setting KDE_MULTIHEAD=true to eliminate ini file access during startup and to be able to stop linking against QtGui Diffs - startkde/kcminit/CMakeLists.txt ffae38c startkde/kcminit/main.h 1140b77 startkde/kcminit/main.cpp 4724323 Diff: https://git.reviewboard.kde.org/r/122270/diff/ Testing --- compiled, ran 'kcminit --list' and kcminit AAA Thanks, Nick Shaforostoff
Re: Review Request 122270: port kcminit away from kdelibs4support
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122270/ --- (Updated Jan. 28, 2015, 12:47 a.m.) Review request for kde-workspace, Aleix Pol Gonzalez, Martin Gräßlin, and Lukáš Tinkl. Repository: plasma-workspace Description --- Now kcminit is linked with less libraries - startup time improved I also suggest always setting KDE_MULTIHEAD=true to eliminate ini file access during startup and to be able to stop linking against QtGui Diffs (updated) - startkde/kcminit/CMakeLists.txt ffae38c startkde/kcminit/main.h 1140b77 startkde/kcminit/main.cpp 4724323 Diff: https://git.reviewboard.kde.org/r/122270/diff/ Testing --- compiled, ran 'kcminit --list' and kcminit AAA Thanks, Nick Shaforostoff
Re: Review Request 122270: port kcminit away from kdelibs4support
On Jan. 27, 2015, 6:59 a.m., Martin Gräßlin wrote: startkde/kcminit/main.cpp, lines 250-254 https://git.reviewboard.kde.org/r/122270/diff/1/?file=345342#file345342line250 I do not like this. If the only need is to check whether it's X11 multi-head, it should open an xcb_connection_t - if that fails we don't have an X-Server. If it succeeds we can check the number of screens. Nick Shaforostoff wrote: understood, but the question remains: is kcminit the best place to do multihead-related stuff? Thomas Lübking wrote: It's required by KApplication and while that's in kde4support, it's still there (as well as KDE4 clients) So yes, we need that everywhere - for now. Clients seem to default the env to false, so uncoditionally setting it true is wrong for sure. It'd rather be multihead = (screenCount 1); (ignoring the ini) what however would be a feature loss. = Proposal users should be able to pre-control the variable in eg. ~/.kde/env (or wherever the Plasma 2 equivalent is) If the variable is set to false, it remains like that. If not (ie. it's not set or set true resp. anything but 0/false) it becomes true if the screen count 1 Spares the ini lookup, but I don't think you get around xcb_connection_t *c; int screen_count = xcb_setup_roots_length(xcb_get_setup(c)); without risking to really break stuff (the suggested approach still causes a transitional break) reading ini file is not more than 0.5 second with 'cold cache'. getting rid of kdelibs4support for kcminit is 2 seconds gain. direct xcb request would require copying some 30 lines of code from xcp qpa plugin (= a no go). - Nick --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122270/#review74806 --- On Jan. 28, 2015, 12:47 a.m., Nick Shaforostoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122270/ --- (Updated Jan. 28, 2015, 12:47 a.m.) Review request for kde-workspace, Aleix Pol Gonzalez, Martin Gräßlin, and Lukáš Tinkl. Repository: plasma-workspace Description --- Now kcminit is linked with less libraries - startup time improved I also suggest always setting KDE_MULTIHEAD=true to eliminate ini file access during startup and to be able to stop linking against QtGui Diffs - startkde/kcminit/CMakeLists.txt ffae38c startkde/kcminit/main.h 1140b77 startkde/kcminit/main.cpp 4724323 Diff: https://git.reviewboard.kde.org/r/122270/diff/ Testing --- compiled, ran 'kcminit --list' and kcminit AAA Thanks, Nick Shaforostoff
Re: Review Request 122270: port kcminit away from kdelibs4support
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122270/#review74800 --- googling for KDE_MULTIHEAD revealed the real world use case for it. http://www.kde-forum.org/artikel/16920/kde-multihead-environment-variable.html we still can avoid qtgui linkage and possibli ini file read by adding a cmdline option or generally move KDE_MULTIHEAD setup to another place (close to it's users?) - Nick Shaforostoff On Jan. 27, 2015, 3:10 a.m., Nick Shaforostoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122270/ --- (Updated Jan. 27, 2015, 3:10 a.m.) Review request for kde-workspace, Aleix Pol Gonzalez, Martin Gräßlin, and Lukáš Tinkl. Repository: plasma-workspace Description --- Now kcminit is linked with less libraries - startup time improved I also suggest always setting KDE_MULTIHEAD=true to eliminate ini file access during startup and to be able to stop linking against QtGui Diffs - startkde/kcminit/CMakeLists.txt ffae38c startkde/kcminit/main.h 1140b77 startkde/kcminit/main.cpp 4724323 Diff: https://git.reviewboard.kde.org/r/122270/diff/ Testing --- compiled, ran 'kcminit --list' and kcminit AAA Thanks, Nick Shaforostoff
Review Request 122270: port kcminit away from kdelibs4support
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122270/ --- Review request for kde-workspace, Aleix Pol Gonzalez, Martin Gräßlin, and Lukáš Tinkl. Repository: plasma-workspace Description --- Now kcminit is linked with less libraries - startup time improved I also suggest always setting KDE_MULTIHEAD=true to eliminate ini file access during startup and to be able to stop linking against QtGui Diffs - startkde/kcminit/CMakeLists.txt ffae38c startkde/kcminit/main.h 1140b77 startkde/kcminit/main.cpp 4724323 Diff: https://git.reviewboard.kde.org/r/122270/diff/ Testing --- compiled, ran 'kcminit --list' and kcminit AAA Thanks, Nick Shaforostoff
Re: Review Request 122270: port kcminit away from kdelibs4support
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122270/#review74806 --- startkde/kcminit/main.cpp https://git.reviewboard.kde.org/r/122270/#comment51849 this should probably go into a if (isX11()) block startkde/kcminit/main.cpp https://git.reviewboard.kde.org/r/122270/#comment51848 this inverses logic for non X11 startkde/kcminit/main.cpp https://git.reviewboard.kde.org/r/122270/#comment51850 I do not like this. If the only need is to check whether it's X11 multi-head, it should open an xcb_connection_t - if that fails we don't have an X-Server. If it succeeds we can check the number of screens. - Martin Gräßlin On Jan. 27, 2015, 4:10 a.m., Nick Shaforostoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122270/ --- (Updated Jan. 27, 2015, 4:10 a.m.) Review request for kde-workspace, Aleix Pol Gonzalez, Martin Gräßlin, and Lukáš Tinkl. Repository: plasma-workspace Description --- Now kcminit is linked with less libraries - startup time improved I also suggest always setting KDE_MULTIHEAD=true to eliminate ini file access during startup and to be able to stop linking against QtGui Diffs - startkde/kcminit/CMakeLists.txt ffae38c startkde/kcminit/main.h 1140b77 startkde/kcminit/main.cpp 4724323 Diff: https://git.reviewboard.kde.org/r/122270/diff/ Testing --- compiled, ran 'kcminit --list' and kcminit AAA Thanks, Nick Shaforostoff
