DevHeads.net

Review Request: KDIalog::screenRect: workaround for faulty QDesktopWidget::geometry

Review request for kdelibs.

Summary
This patch changes KDialog::screenRect to call QDesktopWidget::screenGeometry(widget) instead of QDesktopWidget::geometry(), which returns the wrong geometry if a second monitor has been connected and disconnected.
This fixes a bug described here:
<a href="http://article.gmane.org/gmane.comp.kde.devel.core/71875" title="http://article.gmane.org/gmane.comp.kde.devel.core/71875">http://article.gmane.org/gmane.comp.kde.devel.core/71875</a>
The testcase and results can be found here:
<a href="http://article.gmane.org/gmane.comp.kde.devel.core/71911" title="http://article.gmane.org/gmane.comp.kde.devel.core/71911">http://article.gmane.org/gmane.comp.kde.devel.core/71911</a>

Behaviour of QDesktopWidget::geometry() vs. QDesktopWidget::screenGeometry(widget) are equivalent in case of non-faulty ::geometry() and fix the problem in case of faulty ::geometry(). So I wouldn't consider this a workaround that needs to be maintained/changed back once (if) the bug in Qt has been fixed. On the contrary: using ::screenGeometry(widget) is more verbose than ::geometry(), which make more sense in the context of KDialog::screenRect(QWidget* widget,int screen) IMHO

Diffs
kdeui/dialogs/kdialog.cpp 0cabb85

Diff: <a href="http://git.reviewboard.kde.org/r/102671/diff" title="http://git.reviewboard.kde.org/r/102671/diff">http://git.reviewboard.kde.org/r/102671/diff</a>

Testing
Tested successfully with 4.7.0 on Fedora 15 and current master

Thanks,

Thomas

Comments

Re: Review Request: KDIalog::screenRect: workaround for faulty Q

By Thomas Gahr at 09/23/2011 - 07:23

(Updated Sept. 23, 2011, 12:23 p.m.)

Review request for kdelibs.

Changes
more testing

Summary
This patch changes KDialog::screenRect to call QDesktopWidget::screenGeometry(widget) instead of QDesktopWidget::geometry(), which returns the wrong geometry if a second monitor has been connected and disconnected.
This fixes a bug described here:
<a href="http://article.gmane.org/gmane.comp.kde.devel.core/71875" title="http://article.gmane.org/gmane.comp.kde.devel.core/71875">http://article.gmane.org/gmane.comp.kde.devel.core/71875</a>
The testcase and results can be found here:
<a href="http://article.gmane.org/gmane.comp.kde.devel.core/71911" title="http://article.gmane.org/gmane.comp.kde.devel.core/71911">http://article.gmane.org/gmane.comp.kde.devel.core/71911</a>

Behaviour of QDesktopWidget::geometry() vs. QDesktopWidget::screenGeometry(widget) are equivalent in case of non-faulty ::geometry() and fix the problem in case of faulty ::geometry(). So I wouldn't consider this a workaround that needs to be maintained/changed back once (if) the bug in Qt has been fixed. On the contrary: using ::screenGeometry(widget) is more verbose than ::geometry(), which make more sense in the context of KDialog::screenRect(QWidget* widget,int screen) IMHO

Diffs
kdeui/dialogs/kdialog.cpp 0cabb85

Diff: <a href="http://git.reviewboard.kde.org/r/102671/diff" title="http://git.reviewboard.kde.org/r/102671/diff">http://git.reviewboard.kde.org/r/102671/diff</a>

Testing (updated)
Tested successfully with 4.7.0 on Fedora 15 and current master... and on branch KDE/4.7 - as expected everything works fine

Thanks,

Thomas

Re: Review Request: KDIalog::screenRect: workaround for faulty Q

By Thomas Gahr at 09/24/2011 - 08:58

(Updated Sept. 24, 2011, 1:58 p.m.)

Review request for kdelibs.

Changes
Use availableGeometry(widget) instead of screenRect(widget) to be consistent with the if()-branch above

Summary
This patch changes KDialog::screenRect to call QDesktopWidget::screenGeometry(widget) instead of QDesktopWidget::geometry(), which returns the wrong geometry if a second monitor has been connected and disconnected.
This fixes a bug described here:
<a href="http://article.gmane.org/gmane.comp.kde.devel.core/71875" title="http://article.gmane.org/gmane.comp.kde.devel.core/71875">http://article.gmane.org/gmane.comp.kde.devel.core/71875</a>
The testcase and results can be found here:
<a href="http://article.gmane.org/gmane.comp.kde.devel.core/71911" title="http://article.gmane.org/gmane.comp.kde.devel.core/71911">http://article.gmane.org/gmane.comp.kde.devel.core/71911</a>

Behaviour of QDesktopWidget::geometry() vs. QDesktopWidget::screenGeometry(widget) are equivalent in case of non-faulty ::geometry() and fix the problem in case of faulty ::geometry(). So I wouldn't consider this a workaround that needs to be maintained/changed back once (if) the bug in Qt has been fixed. On the contrary: using ::screenGeometry(widget) is more verbose than ::geometry(), which make more sense in the context of KDialog::screenRect(QWidget* widget,int screen) IMHO

Diffs (updated)
kdeui/dialogs/kdialog.cpp 0cabb85

Diff: <a href="http://git.reviewboard.kde.org/r/102671/diff" title="http://git.reviewboard.kde.org/r/102671/diff">http://git.reviewboard.kde.org/r/102671/diff</a>

Testing
Tested successfully with 4.7.0 on Fedora 15 and current master... and on branch KDE/4.7 - as expected everything works fine

Thanks,

Thomas

Re: Review Request: KDIalog::screenRect: workaround for faulty Q

By Olivier Goffart at 09/24/2011 - 07:57

kdeui/dialogs/kdialog.cpp
<http://git.reviewboard.kde.org/r/102671/#comment6004>

Are you sure it should not be availableGeometry(widget) so it is consistant with the other case? (or maybe the other case is wrong as well, wr really want to center on the center of the screen, not its available geometry).

- Olivier

On Sept. 23, 2011, 12:23 p.m., Thomas Gahr wrote:

Re: Review Request: KDIalog::screenRect: workaround for faulty Q

By Thomas Gahr at 09/24/2011 - 08:57

Hm. Well if we want to be equivalent to the old behaviour, screenGeometry(widget) is the way to go.
Looking at the case "desktop->isVirtualDesktop() && cg.readEntry( "XineramaEnabled", true ) && cg.readEntry( "XineramaPlacementEnabled", true )" it does make more sense to use availableGeometry(widget) instead
of geometry() of screenGeometry(widget). I can imagine that the difference went unnoticed so far because for 99% of all users the difference lies within ~30 pixels due to the default panel setup.
I'll change the diff to use availableGeometry, we can always choose to use the other version if there are any valid objections to this approach.

- Thomas

On Sept. 23, 2011, 12:23 p.m., Thomas Gahr wrote:

Re: Review Request: KDIalog::screenRect: workaround for faulty Q

By Thomas Gahr at 09/23/2011 - 07:22

This has _basically_ been approved in <a href="http://lists.kde.org/?l=kde-core-devel&amp;m=131616667329975&amp;w=2" title="http://lists.kde.org/?l=kde-core-devel&amp;m=131616667329975&amp;w=2">http://lists.kde.org/?l=kde-core-devel&amp;m=131616667329975&amp;w=2</a> by Aaron - dunno if the patch has been applied yet or will be applied at all... :/

- Thomas

On Sept. 20, 2011, 12:18 p.m., Thomas Gahr wrote: