[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:       ahartmetz () gmail ! com
Date:       2009-08-11 19:01:05
Message-ID: 20090811190105.28789.18902 () localhost
[Download RAW message or body]


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


Nitpick: Pointer checks are fine to establish identity. But only if the pointed-to \
objects are both alive...

- maelcum


On 2009-08-10 12:24:09, Parker Coates wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1252/
> -----------------------------------------------------------
> 
> (Updated 2009-08-10 12:24:09)
> 
> 
> Review request for kdelibs.
> 
> 
> 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]