> 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. > > David Edmundson wrote: > 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.
My valgrind trace when it was crashing. https://paste.kde.org/p8cxylcox - David ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122500/#review75715 ----------------------------------------------------------- On Feb. 10, 2015, 1:25 p.m., David Edmundson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/122500/ > ----------------------------------------------------------- > > (Updated Feb. 10, 2015, 1:25 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
