[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-core-devel
Subject:    Re: Review Request: Broken layout in KEditToolbar after restoring
From:       "Parker Coates" <parker.coates () gmail ! com>
Date:       2009-08-10 12:24:09
Message-ID: 20090810122409.13726.12235 () localhost
[Download RAW message or body]


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/1252/
-----------------------------------------------------------

(Updated 2009-08-10 12:24:09.521247)


Review request for kdelibs.


Changes
-------

Added reference to bug. Thanks, Dario. I did do a Bugzilla search, but apparently I \
didn't choose my terms well enough as I repeatedly got no results.


Summary
-------

The issue:
1. Open up any KDE app with a toolbar.
2. Settings > Configure Toolbars...
3. Click the Defaults button and confirm.
4. Notice that the dialog layout is (most likely, but not always) broken.

While trying to track down this issue, I made a rather small patch (below) to \
kedittoolbar.cpp. The patch fixed the issue, but I didn't understand why. After \
reverting the patch and doing some more digging, I learned the call to \
KDialog::setMainWidget(QWidget*) wasn't doing anything. So I looked into that code \
and found that the method was returning early if the new widget pointer was the same \
as the old. I slapped in a few kDebug lines and, low and behold, the new widget was \
being given the exact same address as the old one, so setMainWidget was convinced \
that the widget hadn't actually changed and didn't bother setting up the layout. Of \
course address assignment depends on the memory manager, so the issue isn't 100% \
repeatable as sometimes the new widget gets a different address.

The attached patch to KDialog removes this check. My understanding is that it's \
generally bad practice to use pointer addresses to establish identity. I also can't \
think of too many legitimate use cases that would require repeated calls to \
setMainWidget with the same widget and would need this check for performance reasons. \
But I could be wrong. Provided the patch is reasonable, I think it makes sense to \
backport to the 4.3 branch.

Another alternative would be to make KDialog's mMainWidget a QPointer, but that \
change seemed more invasive, and I'm still pretty terrified of breaking things in \
kdelibs. :)

The patch to KEditToolbar is no longer strictly necessary, but it does fix an \
somewhat annoying flicker that occurs when the main widget is replaced, so I'd still \
like to commit it. Of course, it would be a separate commit and probably doesn't need \
to be backported.


This addresses bug 180064.
    https://bugs.kde.org/show_bug.cgi?id=180064


Diffs
-----

  trunk/KDE/kdelibs/kdeui/dialogs/kdialog.cpp 1008090 
  trunk/KDE/kdelibs/kdeui/dialogs/kedittoolbar.cpp 1008090 

Diff: http://reviewboard.kde.org/r/1252/diff


Testing
-------


Thanks,

Parker


[prev in list] [next in list] [prev in thread] [next in thread]