Review Request: Broken layout in KEditToolbar after restoring defaults

[ahartmetz at gmail.com]

-----------------------------------------------------------
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
> 
>