> On Aug. 27, 2015, 12:01 p.m., Mark Gaiser wrote: > > ksmserver/screenlocker/greeter/greeterapp.cpp, line 140 > > <https://git.reviewboard.kde.org/r/124947/diff/1/?file=399227#file399227line140> > > > > It might be nice to check if this pointer isn't 0 after the cast. > > > > Also, coding style. > > qobject_cast<KQuickAddons::QuickViewSharedEngine*> > > > > instead of > > qobject_cast<KQuickAddons::QuickViewSharedEngine *> > > > > Or the other way around, but then all the others need to change ;) > > Not quite sure which one is appropiate here, the style guideline isn't > > clear on this. I think the one with the space is the adviced way (which > > would mean that the rest needs to change, not this line)
Yes in theory one needs to check. In practice the slot is only invoked from a signal emitted from the view. So yes in theory I agree, in practice I don't think it's needed. > On Aug. 27, 2015, 12:01 p.m., Mark Gaiser wrote: > > ksmserver/screenlocker/greeter/greeterapp.cpp, line 165 > > <https://git.reviewboard.kde.org/r/124947/diff/1/?file=399227#file399227line165> > > > > Just a suggestion, not an issue. Can't this (and m_views where this one > > is inserted later on) be a smart pointer? That makes it go out of scope > > instead of relying on qDeleteAll(m_views) which is done in the destructor. > > Both methods work just fine, but i - personal preference - prefer smart > > pointers :) I also prefer smart pointers, but I don't think it's a good idea in this case. I do not like to needlessly change security relevant code for things that work. On Aug. 27, 2015, 12:01 p.m., Martin Gräßlin wrote: > > I can't really give a +1.. I just don't know this or the implications it > > might have. > > So just a style review then :) Concerning the coding style: I don't want to mix it with the changes. If you look closely you will notice that in all outlined cases the coding style did not change. The coding style of that class is inconsistent. If you think it should be fixed I suggest you run astyle on it after my changes are in. - Martin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124947/#review84451 ----------------------------------------------------------- On Aug. 27, 2015, 10:34 a.m., Martin Gräßlin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/124947/ > ----------------------------------------------------------- > > (Updated Aug. 27, 2015, 10:34 a.m.) > > > Review request for Plasma and David Edmundson. > > > Repository: plasma-workspace > > > Description > ------- > > In a multi-screen setup we have multiple views showing the same Qml > scene. Let's share the engine for all views. > > > Diffs > ----- > > ksmserver/screenlocker/greeter/CMakeLists.txt > 4fb679f838a80f11eb8e4b4f5cb5ef04dc39c0f7 > ksmserver/screenlocker/greeter/greeterapp.h > ed278e492a9a237f65f4c1600360f84fb25bdad7 > ksmserver/screenlocker/greeter/greeterapp.cpp > b500ba44c2b483d7372ca46840152c90ef5f798c > > Diff: https://git.reviewboard.kde.org/r/124947/diff/ > > > Testing > ------- > > This makes creating a second view basically free (from timing perspective) > > > Thanks, > > Martin Gräßlin > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
