Re: Review Request 122320: use xcb-screen count instead of qguiapplication.screens
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122320/ --- (Updated Feb. 4, 2015, 8:59 p.m.) Status -- This change has been marked as submitted. Review request for kde-workspace, Martin Gräßlin and Thomas Lübking. Repository: plasma-workspace Description --- this patch makes kcminit behave like in kde4: it uses proper xcb screen count which may be different from QGuiApplication::screens().count(). for example when i connext external monitor via vga to my laptop, xcb screen count is still '1', while QGuiApplication::screens().count() returns '2'. switching from QGuiApplication to QCoreApplication still wasn't possible because modules like 'mouse' need gui initialized and would crash if kcminit uses QCoreApplication. Diffs - startkde/kcminit/CMakeLists.txt b17951f startkde/kcminit/main.cpp 1008966 Diff: https://git.reviewboard.kde.org/r/122320/diff/ Testing --- i have built kcminit on ubuntu vivid alpha 32-bit, replaced binaries and libraries in the system and successfuly could run kcminit_startup and reboot also went fine. Thanks, Nick Shaforostoff
Re: Review Request 122320: use xcb-screen count instead of qguiapplication.screens
On Фев. 4, 2015, 9:16 д.п., Martin Gräßlin wrote: startkde/kcminit/CMakeLists.txt, line 6 https://git.reviewboard.kde.org/r/122320/diff/4/?file=346734#file346734line6 you forgot to git add the config-xcb.h.cmake i know. its contents is the following: /* Define if you have XCB at all */ #cmakedefine XCB_FOUND are there any other issues apart this one? - Nick --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122320/#review75358 --- On Фев. 4, 2015, 12:31 д.п., Nick Shaforostoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122320/ --- (Updated Фев. 4, 2015, 12:31 д.п.) Review request for kde-workspace, Martin Gräßlin and Thomas Lübking. Repository: plasma-workspace Description --- this patch makes kcminit behave like in kde4: it uses proper xcb screen count which may be different from QGuiApplication::screens().count(). for example when i connext external monitor via vga to my laptop, xcb screen count is still '1', while QGuiApplication::screens().count() returns '2'. switching from QGuiApplication to QCoreApplication still wasn't possible because modules like 'mouse' need gui initialized and would crash if kcminit uses QCoreApplication. Diffs - startkde/kcminit/CMakeLists.txt b17951f startkde/kcminit/main.cpp 1008966 Diff: https://git.reviewboard.kde.org/r/122320/diff/ Testing --- i have built kcminit on ubuntu vivid alpha 32-bit, replaced binaries and libraries in the system and successfuly could run kcminit_startup and reboot also went fine. Thanks, Nick Shaforostoff
Re: Review Request 122320: use xcb-screen count instead of qguiapplication.screens
On Лют. 3, 2015, 7:08 до полудня, Martin Gräßlin wrote: components/CMakeLists.txt, line 2 https://git.reviewboard.kde.org/r/122320/diff/3/?file=346480#file346480line2 that seems unrelated change. sorry, i forgot to remove it -- i had to comment it because i had compile errors there On Лют. 3, 2015, 7:08 до полудня, Martin Gräßlin wrote: startkde/kcminit/CMakeLists.txt, line 16 https://git.reviewboard.kde.org/r/122320/diff/3/?file=346481#file346481line16 you find optional, but link required. OSX devs won't be happy with that change ;-) you need to do something like: if (XCB_XCB_FOUND) target_link_libraries(kdeinit_kcminit XCB::XCB) endif() it compiles even if i remove find_package(XCB) -- XCB::XCB variable is just empty then On Лют. 3, 2015, 7:08 до полудня, Martin Gräßlin wrote: startkde/kcminit/main.cpp, line 26 https://git.reviewboard.kde.org/r/122320/diff/3/?file=346482#file346482line26 the ifdef is wrong: HAVE_X11 is defined as 1 if Xlib is found. It doesn't say anything about whether XCB is found. Please introduce a dedicated ifdef for it. ok, and a dedicated config-xcb.h generated by cmake. will do it this evening - Nick --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122320/#review75256 --- On Лют. 2, 2015, 9:15 після полудня, Nick Shaforostoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122320/ --- (Updated Лют. 2, 2015, 9:15 після полудня) Review request for kde-workspace, Martin Gräßlin and Thomas Lübking. Repository: plasma-workspace Description --- this patch makes kcminit behave like in kde4: it uses proper xcb screen count which may be different from QGuiApplication::screens().count(). for example when i connext external monitor via vga to my laptop, xcb screen count is still '1', while QGuiApplication::screens().count() returns '2'. switching from QGuiApplication to QCoreApplication still wasn't possible because modules like 'mouse' need gui initialized and would crash if kcminit uses QCoreApplication. Diffs - components/CMakeLists.txt 42c820f startkde/kcminit/CMakeLists.txt b17951f startkde/kcminit/main.cpp 1008966 Diff: https://git.reviewboard.kde.org/r/122320/diff/ Testing --- i have built kcminit on ubuntu vivid alpha 32-bit, replaced binaries and libraries in the system and successfuly could run kcminit_startup and reboot also went fine. Thanks, Nick Shaforostoff
Re: Review Request 122320: use xcb-screen count instead of qguiapplication.screens
On Лют. 3, 2015, 7:08 до полудня, Martin Gräßlin wrote: startkde/kcminit/CMakeLists.txt, line 16 https://git.reviewboard.kde.org/r/122320/diff/3/?file=346481#file346481line16 you find optional, but link required. OSX devs won't be happy with that change ;-) you need to do something like: if (XCB_XCB_FOUND) target_link_libraries(kdeinit_kcminit XCB::XCB) endif() Nick Shaforostoff wrote: it compiles even if i remove find_package(XCB) -- XCB::XCB variable is just empty then update: probably this is because XCB is already found by some upper CMakeLists.txt. i will adjust the code this evening - Nick --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122320/#review75256 --- On Лют. 2, 2015, 9:15 після полудня, Nick Shaforostoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122320/ --- (Updated Лют. 2, 2015, 9:15 після полудня) Review request for kde-workspace, Martin Gräßlin and Thomas Lübking. Repository: plasma-workspace Description --- this patch makes kcminit behave like in kde4: it uses proper xcb screen count which may be different from QGuiApplication::screens().count(). for example when i connext external monitor via vga to my laptop, xcb screen count is still '1', while QGuiApplication::screens().count() returns '2'. switching from QGuiApplication to QCoreApplication still wasn't possible because modules like 'mouse' need gui initialized and would crash if kcminit uses QCoreApplication. Diffs - components/CMakeLists.txt 42c820f startkde/kcminit/CMakeLists.txt b17951f startkde/kcminit/main.cpp 1008966 Diff: https://git.reviewboard.kde.org/r/122320/diff/ Testing --- i have built kcminit on ubuntu vivid alpha 32-bit, replaced binaries and libraries in the system and successfuly could run kcminit_startup and reboot also went fine. Thanks, Nick Shaforostoff
Re: Review Request 122320: use xcb-screen count instead of qguiapplication.screens
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122320/ --- (Updated Feb. 2, 2015, 9:15 p.m.) Review request for kde-workspace, Martin Gräßlin and Thomas Lübking. Changes --- -declare XCB an optional dependency -add a check of current qt platform (xcb/wayland) Repository: plasma-workspace Description --- this patch makes kcminit behave like in kde4: it uses proper xcb screen count which may be different from QGuiApplication::screens().count(). for example when i connext external monitor via vga to my laptop, xcb screen count is still '1', while QGuiApplication::screens().count() returns '2'. switching from QGuiApplication to QCoreApplication still wasn't possible because modules like 'mouse' need gui initialized and would crash if kcminit uses QCoreApplication. Diffs (updated) - components/CMakeLists.txt 42c820f startkde/kcminit/CMakeLists.txt b17951f startkde/kcminit/main.cpp 1008966 Diff: https://git.reviewboard.kde.org/r/122320/diff/ Testing --- i have built kcminit on ubuntu vivid alpha 32-bit, replaced binaries and libraries in the system and successfuly could run kcminit_startup and reboot also went fine. Thanks, Nick Shaforostoff
Re: Review Request 122320: use xcb-screen count instead of qguiapplication.screens
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122320/ --- (Updated Jan. 31, 2015, 11:07 p.m.) Review request for kde-workspace, Martin Gräßlin and Thomas Lübking. Changes --- added xcb_connection_has_error() check Repository: plasma-workspace Description --- this patch makes kcminit behave like in kde4: it uses proper xcb screen count which may be different from QGuiApplication::screens().count(). for example when i connext external monitor via vga to my laptop, xcb screen count is still '1', while QGuiApplication::screens().count() returns '2'. switching from QGuiApplication to QCoreApplication still wasn't possible because modules like 'mouse' need gui initialized and would crash if kcminit uses QCoreApplication. Diffs (updated) - startkde/kcminit/CMakeLists.txt b17951f startkde/kcminit/main.cpp 1008966 Diff: https://git.reviewboard.kde.org/r/122320/diff/ Testing --- i have built kcminit on ubuntu vivid alpha 32-bit, replaced binaries and libraries in the system and successfuly could run kcminit_startup and reboot also went fine. 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
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
Review Request 122320: use xcb-screen count instead of qguiapplication.screens
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122320/ --- Review request for kde-workspace, Martin Gräßlin and Thomas Lübking. Repository: plasma-workspace Description --- this patch makes kcminit behave like in kde4: it uses proper xcb screen count which may be different from QGuiApplication::screens().count(). for example when i connext external monitor via vga to my laptop, xcb screen count is still '1', while QGuiApplication::screens().count() returns '2'. switching from QGuiApplication to QCoreApplication still wasn't possible because modules like 'mouse' need gui initialized and would crash if kcminit uses QCoreApplication. Diffs - startkde/kcminit/CMakeLists.txt b17951f startkde/kcminit/main.cpp 1008966 Diff: https://git.reviewboard.kde.org/r/122320/diff/ Testing --- i have built kcminit on ubuntu vivid alpha 32-bit, replaced binaries and libraries in the system and successfuly could run kcminit_startup and reboot also went fine. 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, 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 Січ. 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
--- 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
Review Request: make Speller::spellCheckerFound() return true when the dict for the current language is found (important when there was no dict for default language)
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103729/ --- Review request for kdelibs. Description --- right now in cases when the dictionary for default language cannot be found Speller::spellCheckerFound() always returns false, even after a langcode for the existing dictionary is passed via setCurrentLanguage(). This patch sets spellCheckerFound to true if the dict is found for the language being set, and false in another case. Actually, the patch makes spellCheckerFound just a cached value of d-dict-isValid(). in the bug 256896 you can find users reporting this problem in addition to another one (regarding using 'language_COUNTRY' dictionaries when only 'language' is specified) This addresses bug 256896. http://bugs.kde.org/show_bug.cgi?id=256896 Diffs - kdeui/sonnet/highlighter.cpp 3f478f0 Diff: http://git.reviewboard.kde.org/r/103729/diff/diff Testing --- Thanks, Nick Shaforostoff
Re: Review Request: check if enough disk space available before even starting to copy each file
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103412/ --- (Updated Dec. 16, 2011, 1:28 p.m.) Review request for kdelibs. Changes --- i changed the patch to only retrive available space info for fast drives. for NFS it remains in TODO status (post 4.8). Description --- this is simple fix for 243160. It gets free space info for the dst partition, then after each successful file copy it decreases an internally kept m_freeSpace value. this will help us avoid situations when user copies a 4gb long file onto his disk, then finds out it has not enough space available. i hope that i correctly understood kio copy job mechanism and done error reporting right. TODO (from what was asked in the bug): *checking for single file size limit on vfat. *if the total size is larger than free space, warn user beforehand immediately (right now it does the copying until it finds that the next file cannot be copied completely) both these require new dialogs with user visible strings and are subject to be added after 4.8. also as a bonus i changed m_overwriteList to be qset instead of qlist to make lookup operations faster. This addresses bug 243160. http://bugs.kde.org/show_bug.cgi?id=243160 Diffs (updated) - kio/kio/copyjob.cpp eff7825 Diff: http://git.reviewboard.kde.org/r/103412/diff/diff Testing --- files get copied fine. if i copy a bunch of files including one big file (created with dd if=/dev/zero of=file.out bs=1MB count=300), then the copying process stops when it gets to this big file (i have small disk on my virtual machine, only 400 mb free) Thanks, Nick Shaforostoff
Review Request: check if enough disk space available before even starting to copy each file
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103412/ --- Review request for kdelibs. Description --- this is simple fix for 243160. It gets free space info for the dst partition, then after each successful file copy it decreases an internally kept m_freeSpace value. this will help us avoid situations when user copies a 4gb long file onto his disk, then finds out it has not enough space available. i hope that i correctly understood kio copy job mechanism and done error reporting right. TODO (from what was asked in the bug): *checking for single file size limit on vfat. *if the total size is larger than free space, warn user beforehand immediately (right now it does the copying until it finds that the next file cannot be copied completely) both these require new dialogs with user visible strings and are subject to be added after 4.8. also as a bonus i changed m_overwriteList to be qset instead of qlist to make lookup operations faster. This addresses bug 243160. http://bugs.kde.org/show_bug.cgi?id=243160 Diffs - kio/kio/copyjob.cpp eff7825 Diff: http://git.reviewboard.kde.org/r/103412/diff/diff Testing --- files get copied fine. if i copy a bunch of files including one big file (created with dd if=/dev/zero of=file.out bs=1MB count=300), then the copying process stops when it gets to this big file (i have small disk on my virtual machine, only 400 mb free) Thanks, Nick Shaforostoff
KRandomSequence and 64-bit
Hi all. I see that KRandomSequence uses longs everywhere. long on 64-bit gcc systems is 8 bytes long, while int is always 4. maybe this is ok overall, but e.g. it uses KRandom::random() return value inside, which is int. so i suppose this can actually make the sequence less random. i propose to change the code to use ints internally, add getInt(), setSeed(int) methods and deprecate getLong and others (specifying in their documentation that they cast long to int and vice versa), and remove them completely in KDE 5.0
Review Request: optimize kcolorscheme consting time: avoid calculating luma for the same colors multiple times
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103348/ --- Review request for kdelibs. Description --- pretty straightforward and small optimization of kde apps startup time: avoid calculating luma for the same colors multiple times. callgrind shows it is called 4503 times instead of 12087 at startup of kwrite, which is 1.5%. Diffs - kdeui/colors/kcolorutils.cpp efe8cdd Diff: http://git.reviewboard.kde.org/r/103348/diff/diff Testing --- kwrite starts fine with the new kdeui lib Thanks, Nick Shaforostoff
Re: Review Request: optimize kcolorscheme consting time: avoid calculating luma for the same colors multiple times
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103348/#review8762 --- I have an account called shaforo. i could push to kdepimlibs with it, but i couldn't push to kdelibs. is it because of lack of permissions? - Nick Shaforostoff On Dec. 6, 2011, 5:53 p.m., Nick Shaforostoff wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103348/ --- (Updated Dec. 6, 2011, 5:53 p.m.) Review request for kdelibs. Description --- pretty straightforward and small optimization of kde apps startup time: avoid calculating luma for the same colors multiple times. callgrind shows it is called 4503 times instead of 12087 at startup of kwrite, which is 1.5%. Diffs - kdeui/colors/kcolorutils.cpp efe8cdd Diff: http://git.reviewboard.kde.org/r/103348/diff/diff Testing --- kwrite starts fine with the new kdeui lib Thanks, Nick Shaforostoff
Re: Review Request: optimize kcolorscheme consting time: avoid calculating luma for the same colors multiple times
On Dec. 6, 2011, 5:58 p.m., Nick Shaforostoff wrote: I have an account called shaforo. i could push to kdepimlibs with it, but i couldn't push to kdelibs. is it because of lack of permissions? Alexander Potashev wrote: You need to clone the `kdelibs` repo from `g...@git.kde.org:kdelibs.git`. Check this in `kdelibs/.git/config` on your machine. Albert Astals Cid wrote: kdelibs master is frozen because of the frameworks effort. You either commit it to the frameworks branch and wait until it's released or convince us this is a bugfix (which seems to be) and commit it to the 4.7 branch. ok, so if nobody objects i will push it to 4.7 branch tomorrow - Nick --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103348/#review8762 --- On Dec. 6, 2011, 5:53 p.m., Nick Shaforostoff wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103348/ --- (Updated Dec. 6, 2011, 5:53 p.m.) Review request for kdelibs. Description --- pretty straightforward and small optimization of kde apps startup time: avoid calculating luma for the same colors multiple times. callgrind shows it is called 4503 times instead of 12087 at startup of kwrite, which is 1.5%. Diffs - kdeui/colors/kcolorutils.cpp efe8cdd Diff: http://git.reviewboard.kde.org/r/103348/diff/diff Testing --- kwrite starts fine with the new kdeui lib Thanks, Nick Shaforostoff
