Re: Review Request 115147: Fix non-X11 implementation of KPassivePopup placement
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115147/ --- (Updated Jan. 21, 2014, 11:12 a.m.) Status -- This change has been discarded. Review request for KDE Frameworks. Repository: knotifications Description --- Fix non-X11 implementation of KPassivePopup placement Diffs - src/kpassivepopup.cpp b41cb8dc8b3372346bd12c0413bf6bfa9a6fd00d tests/kpassivepopuptest.h bc0dedc71b4bef8dbc5c89b79c82fb920929cced Diff: https://git.reviewboard.kde.org/r/115147/diff/ Testing --- Builds when I comment out find_package(X11) in the CMakeLists.txt file. Thanks, Alex Merry ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 115147: Fix non-X11 implementation of KPassivePopup placement
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115147/ --- (Updated Jan. 20, 2014, 2:03 p.m.) Review request for KDE Frameworks. Repository: knotifications Description --- Fix non-X11 implementation of KPassivePopup placement Diffs - src/kpassivepopup.cpp b41cb8dc8b3372346bd12c0413bf6bfa9a6fd00d Diff: https://git.reviewboard.kde.org/r/115147/diff/ Testing (updated) --- Builds when I comment out find_package(X11) in the CMakeLists.txt file. Thanks, Alex Merry ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Review Request 115147: Fix non-X11 implementation of KPassivePopup placement
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115147/ --- Review request for KDE Frameworks. Repository: knotifications Description --- Fix non-X11 implementation of KPassivePopup placement Diffs - src/kpassivepopup.cpp b41cb8dc8b3372346bd12c0413bf6bfa9a6fd00d Diff: https://git.reviewboard.kde.org/r/115147/diff/ Testing --- Thanks, Alex Merry ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 115147: Fix non-X11 implementation of KPassivePopup placement
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115147/#review47781 --- src/kpassivepopup.cpp https://git.reviewboard.kde.org/r/115147/#comment33875 why is the QScreen include needed? Looks like the changes don't use it src/kpassivepopup.cpp https://git.reviewboard.kde.org/r/115147/#comment33874 is this comment still valid? QXcbWindow knows the concept of foreign windows. So maybe the code could just be simplified and the if - else completely removed? - Martin Gräßlin On Jan. 20, 2014, 3:03 p.m., Alex Merry wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115147/ --- (Updated Jan. 20, 2014, 3:03 p.m.) Review request for KDE Frameworks. Repository: knotifications Description --- Fix non-X11 implementation of KPassivePopup placement Diffs - src/kpassivepopup.cpp b41cb8dc8b3372346bd12c0413bf6bfa9a6fd00d Diff: https://git.reviewboard.kde.org/r/115147/diff/ Testing --- Builds when I comment out find_package(X11) in the CMakeLists.txt file. Thanks, Alex Merry ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 115147: Fix non-X11 implementation of KPassivePopup placement
On Jan. 20, 2014, 2:17 p.m., Martin Gräßlin wrote: src/kpassivepopup.cpp, line 37 https://git.reviewboard.kde.org/r/115147/diff/1/?file=234805#file234805line37 why is the QScreen include needed? Looks like the changes don't use it The line window-screen()-availableGeometry(); (477) requires it. On Jan. 20, 2014, 2:17 p.m., Martin Gräßlin wrote: src/kpassivepopup.cpp, lines 471-473 https://git.reviewboard.kde.org/r/115147/diff/1/?file=234805#file234805line471 is this comment still valid? QXcbWindow knows the concept of foreign windows. So maybe the code could just be simplified and the if - else completely removed? It has foreign windows, it just doesn't fetch any geometry information for them. Unless that's been added since I last looked (which was after the Qt 5.2 feature freeze). - Alex --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115147/#review47781 --- On Jan. 20, 2014, 2:03 p.m., Alex Merry wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115147/ --- (Updated Jan. 20, 2014, 2:03 p.m.) Review request for KDE Frameworks. Repository: knotifications Description --- Fix non-X11 implementation of KPassivePopup placement Diffs - src/kpassivepopup.cpp b41cb8dc8b3372346bd12c0413bf6bfa9a6fd00d Diff: https://git.reviewboard.kde.org/r/115147/diff/ Testing --- Builds when I comment out find_package(X11) in the CMakeLists.txt file. Thanks, Alex Merry ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 115147: Fix non-X11 implementation of KPassivePopup placement
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115147/ --- (Updated Jan. 20, 2014, 2:22 p.m.) Review request for KDE Frameworks. Changes --- Make the tests link as well. Repository: knotifications Description --- Fix non-X11 implementation of KPassivePopup placement Diffs (updated) - src/kpassivepopup.cpp b41cb8dc8b3372346bd12c0413bf6bfa9a6fd00d tests/kpassivepopuptest.h bc0dedc71b4bef8dbc5c89b79c82fb920929cced Diff: https://git.reviewboard.kde.org/r/115147/diff/ Testing --- Builds when I comment out find_package(X11) in the CMakeLists.txt file. Thanks, Alex Merry ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 115147: Fix non-X11 implementation of KPassivePopup placement
On Jan. 20, 2014, 3:17 p.m., Martin Gräßlin wrote: src/kpassivepopup.cpp, lines 471-473 https://git.reviewboard.kde.org/r/115147/diff/1/?file=234805#file234805line471 is this comment still valid? QXcbWindow knows the concept of foreign windows. So maybe the code could just be simplified and the if - else completely removed? Alex Merry wrote: It has foreign windows, it just doesn't fetch any geometry information for them. Unless that's been added since I last looked (which was after the Qt 5.2 feature freeze). ok that might make sense. I guess we cannot depend on KWindowSystem, right? - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115147/#review47781 --- On Jan. 20, 2014, 3:22 p.m., Alex Merry wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115147/ --- (Updated Jan. 20, 2014, 3:22 p.m.) Review request for KDE Frameworks. Repository: knotifications Description --- Fix non-X11 implementation of KPassivePopup placement Diffs - src/kpassivepopup.cpp b41cb8dc8b3372346bd12c0413bf6bfa9a6fd00d tests/kpassivepopuptest.h bc0dedc71b4bef8dbc5c89b79c82fb920929cced Diff: https://git.reviewboard.kde.org/r/115147/diff/ Testing --- Builds when I comment out find_package(X11) in the CMakeLists.txt file. Thanks, Alex Merry ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 115147: Fix non-X11 implementation of KPassivePopup placement
On Jan. 20, 2014, 2:17 p.m., Martin Gräßlin wrote: src/kpassivepopup.cpp, lines 471-473 https://git.reviewboard.kde.org/r/115147/diff/1/?file=234805#file234805line471 is this comment still valid? QXcbWindow knows the concept of foreign windows. So maybe the code could just be simplified and the if - else completely removed? Alex Merry wrote: It has foreign windows, it just doesn't fetch any geometry information for them. Unless that's been added since I last looked (which was after the Qt 5.2 feature freeze). Martin Gräßlin wrote: ok that might make sense. I guess we cannot depend on KWindowSystem, right? We can and do (which is why KPassivePopup is here and not in KGuiAddons). In particular, the X11 path uses NETWinInfo. I made https://git.reviewboard.kde.org/r/115157/ as an alternative using KWindowInfo. - Alex --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115147/#review47781 --- On Jan. 20, 2014, 2:22 p.m., Alex Merry wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115147/ --- (Updated Jan. 20, 2014, 2:22 p.m.) Review request for KDE Frameworks. Repository: knotifications Description --- Fix non-X11 implementation of KPassivePopup placement Diffs - src/kpassivepopup.cpp b41cb8dc8b3372346bd12c0413bf6bfa9a6fd00d tests/kpassivepopuptest.h bc0dedc71b4bef8dbc5c89b79c82fb920929cced Diff: https://git.reviewboard.kde.org/r/115147/diff/ Testing --- Builds when I comment out find_package(X11) in the CMakeLists.txt file. Thanks, Alex Merry ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 115147: Fix non-X11 implementation of KPassivePopup placement
On Jan. 20, 2014, 3:17 p.m., Martin Gräßlin wrote: src/kpassivepopup.cpp, lines 471-473 https://git.reviewboard.kde.org/r/115147/diff/1/?file=234805#file234805line471 is this comment still valid? QXcbWindow knows the concept of foreign windows. So maybe the code could just be simplified and the if - else completely removed? Alex Merry wrote: It has foreign windows, it just doesn't fetch any geometry information for them. Unless that's been added since I last looked (which was after the Qt 5.2 feature freeze). Martin Gräßlin wrote: ok that might make sense. I guess we cannot depend on KWindowSystem, right? Alex Merry wrote: We can and do (which is why KPassivePopup is here and not in KGuiAddons). In particular, the X11 path uses NETWinInfo. I made https://git.reviewboard.kde.org/r/115157/ as an alternative using KWindowInfo. I think the KWindowInfo one is nicer as it doesn't have the workaround with the screen fallback - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115147/#review47781 --- On Jan. 20, 2014, 3:22 p.m., Alex Merry wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115147/ --- (Updated Jan. 20, 2014, 3:22 p.m.) Review request for KDE Frameworks. Repository: knotifications Description --- Fix non-X11 implementation of KPassivePopup placement Diffs - src/kpassivepopup.cpp b41cb8dc8b3372346bd12c0413bf6bfa9a6fd00d tests/kpassivepopuptest.h bc0dedc71b4bef8dbc5c89b79c82fb920929cced Diff: https://git.reviewboard.kde.org/r/115147/diff/ Testing --- Builds when I comment out find_package(X11) in the CMakeLists.txt file. Thanks, Alex Merry ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
