> On May 19, 2014, 5:57 p.m., David Edmundson wrote: > > freespacenotifier/freespacenotifier.cpp, line 35 > > <https://git.reviewboard.kde.org/r/118205/diff/1/?file=273471#file273471line35> > > > > is there no pretty header?
Ah yes, wrong autocomplete :) > On May 19, 2014, 5:57 p.m., David Edmundson wrote: > > freespacenotifier/freespacenotifier.cpp, line 61 > > <https://git.reviewboard.kde.org/r/118205/diff/1/?file=273471#file273471line61> > > > > does this leak now? No, now it's used properly, closing it also deleteLaters it. The deref() call should actually be private in KNotification > On May 19, 2014, 5:57 p.m., David Edmundson wrote: > > freespacenotifier/freespacenotifier.cpp, line 66 > > <https://git.reviewboard.kde.org/r/118205/diff/1/?file=273471#file273471line66> > > > > set it to null too > > otherwise you can crash. > > > > Personally I'd also rename to m_ for everything, and consider using a > > QWeakPointer Well it's a destructor...where would it crash? As for m_ - yes, I plan to, but I wanted this review to not contain bazillion unrelated changes. > On May 19, 2014, 5:57 p.m., David Edmundson wrote: > > freespacenotifier/freespacenotifier.cpp, line 133 > > <https://git.reviewboard.kde.org/r/118205/diff/1/?file=273471#file273471line133> > > > > is it really called this? Yes, for some reason...I may rename it in some later commit. - Martin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/118205/#review58157 ----------------------------------------------------------- On May 19, 2014, 4:44 p.m., Martin Klapetek wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/118205/ > ----------------------------------------------------------- > > (Updated May 19, 2014, 4:44 p.m.) > > > Review request for Plasma. > > > Repository: plasma-workspace > > > Description > ------- > > Currently it's done purely as a notification, which doesn't make much sense. > This patch turns it into a regular SNI. The actions were moved from the > notification popup to the SNI menu. > > > Diffs > ----- > > freespacenotifier/freespacenotifier.h d3c0351 > freespacenotifier/freespacenotifier.cpp cdf9bc5 > > Diff: https://git.reviewboard.kde.org/r/118205/diff/ > > > Testing > ------- > > Works properly. > > > File Attachments > ---------------- > > Screenshot (fonts are broken for some reason here) > > https://git.reviewboard.kde.org/media/uploaded/files/2014/05/19/a918c329-5679-4aeb-9b2e-af260c0bb4d6__fsn_sni.png > > > Thanks, > > Martin Klapetek > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
