Re: Review Request 122500: Don't delete widgets we don't own when changing styles
On Feb. 9, 2015, 5:34 p.m., Hugo Pereira Da Costa wrote: Although I have no objection against the change, I must admit I don't understand what's wrong with the current code, nor the actual description of the patch. registerWidget may take an existing widget as a parameter. If so, we don't want to delete it if I understand my own code right, _widget is not the argument passed as the registerWidget method. It is an internal member, created parentless, at first accepted call to registerWidget. So as such it is not 'explicitly' owned by anyone, and implicitly owned by us. And then, what is wrong with deleting it in our destructor ? is it because, though parentless it might get deleted elsewhere ? or because of a thread issue ? What do I miss ? (PS: the reason behind this interal _widget member, is that you can not track palettechanged events on a widget, via event filter, once you set it your own 'altered' palette: it won't recive these events anymore. So: eventFilter must be installed on either qApp (which is then getting the event for _every_ widget, for which we did not alter the palette, which is quite a lot), or on a widget for which we are sure the pallette is not altered. Hence: our own). David Edmundson wrote: oh, you're absolutely right... I just had my valgrind traces and in my haste didn't see the difference between _widget and widget. The crash was in QApplication trying to update the palette on _widget after you change styles in the style KCM. https://paste.kde.org/pvhhfielh According to valgrind, the widget it was trying to update was very much the one deleted in the PaletteHelper destructor. I'll try replacing just delete _widget with _widget-deleteLater() and see if that crash still happens; it might be the more relevant part of the fix. It's generally a bad idea to directly delete a QObject in anything that might be called from a slot. Thanks. Hugo Pereira Da Costa wrote: Clear enough. I agree with the delete - deleteLater change. Keep me posted ! Does the crash you report above happen every time ? (and is there a bug report ?), At least here i could not reproduce so far. There's a bug report on Netrunner's internal issue tracker. It's only when going from qtcurve-breeze that systemsetting crashes (yes, that way round, even though it seems backwards. There is a preview of some widgets with the Breeze theme in systemsettings which is the app that crashes so it makes sense that some Breeze stuff ) It seems QtCurve handles the same dbus call that makes apps switch theme to switch palette ast the same time. changing to deleteLater() seems to work. I'll update this. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122500/#review75715 --- On Feb. 9, 2015, 2:33 p.m., David Edmundson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122500/ --- (Updated Feb. 9, 2015, 2:33 p.m.) Review request for Plasma and Hugo Pereira Da Costa. Repository: breeze Description --- Don't delete widgets we don't own when changing styles registerWidget may take an existing widget as a parameter. If so, we don't want to delete it when our paletteHelper is deleted for example if we change style. Diffs - kstyle/breezepalettehelper.cpp 31c32c3 Diff: https://git.reviewboard.kde.org/r/122500/diff/ Testing --- Thanks, David Edmundson ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 122500: Don't delete widgets we don't own when changing styles
On Feb. 9, 2015, 5:34 p.m., Hugo Pereira Da Costa wrote: Although I have no objection against the change, I must admit I don't understand what's wrong with the current code, nor the actual description of the patch. registerWidget may take an existing widget as a parameter. If so, we don't want to delete it if I understand my own code right, _widget is not the argument passed as the registerWidget method. It is an internal member, created parentless, at first accepted call to registerWidget. So as such it is not 'explicitly' owned by anyone, and implicitly owned by us. And then, what is wrong with deleting it in our destructor ? is it because, though parentless it might get deleted elsewhere ? or because of a thread issue ? What do I miss ? (PS: the reason behind this interal _widget member, is that you can not track palettechanged events on a widget, via event filter, once you set it your own 'altered' palette: it won't recive these events anymore. So: eventFilter must be installed on either qApp (which is then getting the event for _every_ widget, for which we did not alter the palette, which is quite a lot), or on a widget for which we are sure the pallette is not altered. Hence: our own). David Edmundson wrote: oh, you're absolutely right... I just had my valgrind traces and in my haste didn't see the difference between _widget and widget. The crash was in QApplication trying to update the palette on _widget after you change styles in the style KCM. https://paste.kde.org/pvhhfielh According to valgrind, the widget it was trying to update was very much the one deleted in the PaletteHelper destructor. I'll try replacing just delete _widget with _widget-deleteLater() and see if that crash still happens; it might be the more relevant part of the fix. It's generally a bad idea to directly delete a QObject in anything that might be called from a slot. Thanks. Clear enough. I agree with the delete - deleteLater change. Keep me posted ! Does the crash you report above happen every time ? (and is there a bug report ?), At least here i could not reproduce so far. - Hugo --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122500/#review75715 --- On Feb. 9, 2015, 2:33 p.m., David Edmundson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122500/ --- (Updated Feb. 9, 2015, 2:33 p.m.) Review request for Plasma and Hugo Pereira Da Costa. Repository: breeze Description --- Don't delete widgets we don't own when changing styles registerWidget may take an existing widget as a parameter. If so, we don't want to delete it when our paletteHelper is deleted for example if we change style. Diffs - kstyle/breezepalettehelper.cpp 31c32c3 Diff: https://git.reviewboard.kde.org/r/122500/diff/ Testing --- Thanks, David Edmundson ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 122500: Don't delete widgets we don't own when changing styles
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122500/#review75715 --- Although I have no objection against the change, I must admit I don't understand what's wrong with the current code, nor the actual description of the patch. registerWidget may take an existing widget as a parameter. If so, we don't want to delete it if I understand my own code right, _widget is not the argument passed as the registerWidget method. It is an internal member, created parentless, at first accepted call to registerWidget. So as such it is not 'explicitly' owned by anyone, and implicitly owned by us. And then, what is wrong with deleting it in our destructor ? is it because, though parentless it might get deleted elsewhere ? or because of a thread issue ? What do I miss ? (PS: the reason behind this interal _widget member, is that you can not track palettechanged events on a widget, via event filter, once you set it your own 'altered' palette: it won't recive these events anymore. So: eventFilter must be installed on either qApp (which is then getting the event for _every_ widget, for which we did not alter the palette, which is quite a lot), or on a widget for which we are sure the pallette is not altered. Hence: our own). kstyle/breezepalettehelper.cpp https://git.reviewboard.kde.org/r/122500/#comment52340 Shouldn't this be as this isn't a QWidget (since 'this' actually is a qobject) - Hugo Pereira Da Costa On Feb. 9, 2015, 2:33 p.m., David Edmundson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122500/ --- (Updated Feb. 9, 2015, 2:33 p.m.) Review request for Plasma and Hugo Pereira Da Costa. Repository: breeze Description --- Don't delete widgets we don't own when changing styles registerWidget may take an existing widget as a parameter. If so, we don't want to delete it when our paletteHelper is deleted for example if we change style. Diffs - kstyle/breezepalettehelper.cpp 31c32c3 Diff: https://git.reviewboard.kde.org/r/122500/diff/ Testing --- Thanks, David Edmundson ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 122500: Don't delete widgets we don't own when changing styles
On Feb. 9, 2015, 5:34 p.m., Hugo Pereira Da Costa wrote: Although I have no objection against the change, I must admit I don't understand what's wrong with the current code, nor the actual description of the patch. registerWidget may take an existing widget as a parameter. If so, we don't want to delete it if I understand my own code right, _widget is not the argument passed as the registerWidget method. It is an internal member, created parentless, at first accepted call to registerWidget. So as such it is not 'explicitly' owned by anyone, and implicitly owned by us. And then, what is wrong with deleting it in our destructor ? is it because, though parentless it might get deleted elsewhere ? or because of a thread issue ? What do I miss ? (PS: the reason behind this interal _widget member, is that you can not track palettechanged events on a widget, via event filter, once you set it your own 'altered' palette: it won't recive these events anymore. So: eventFilter must be installed on either qApp (which is then getting the event for _every_ widget, for which we did not alter the palette, which is quite a lot), or on a widget for which we are sure the pallette is not altered. Hence: our own). oh, you're absolutely right... I just had my valgrind traces and in my haste didn't see the difference between _widget and widget. The crash was in QApplication trying to update the palette on _widget after you change styles in the style KCM. https://paste.kde.org/pvhhfielh According to valgrind, the widget it was trying to update was very much the one deleted in the PaletteHelper destructor. I'll try replacing just delete _widget with _widget-deleteLater() and see if that crash still happens; it might be the more relevant part of the fix. It's generally a bad idea to directly delete a QObject in anything that might be called from a slot. Thanks. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122500/#review75715 --- On Feb. 9, 2015, 2:33 p.m., David Edmundson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122500/ --- (Updated Feb. 9, 2015, 2:33 p.m.) Review request for Plasma and Hugo Pereira Da Costa. Repository: breeze Description --- Don't delete widgets we don't own when changing styles registerWidget may take an existing widget as a parameter. If so, we don't want to delete it when our paletteHelper is deleted for example if we change style. Diffs - kstyle/breezepalettehelper.cpp 31c32c3 Diff: https://git.reviewboard.kde.org/r/122500/diff/ Testing --- Thanks, David Edmundson ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
