On Tuesday 24 of December 2013 14:05:36 David Faure wrote: > On Tuesday 24 December 2013 13:50:29 Michal Humpula wrote: > > On Tuesday 24 of December 2013 13:22:05 David Faure wrote: > > > On Tuesday 24 December 2013 12:30:56 Michal Humpula wrote: > > > > Hi there, > > > > > > > > as suggested without fully functional reviewboard, here is the little > > > > patch > > > > for discussion. > > > > > > > > Emit urlChanged only when actually changing url trough setUrl. The > > > > drawback > > > > of this is that now the fast assignment is replaced by possibly > > > > expensive > > > > string comparison. > > > > > > That's ok, this method is typically not called 10000 times per second. > > > > > > > On the other hand, when the urls are the same, the emit > > > > wont fire and whole bunch of potentially expensive operations > > > > connected > > > > to > > > > that signal will not happen. > > > > > > Yep. The patch makes a lot of sense, please commit it - at least to > > > frameworks. Do you also need it in kdelibs master? > > > > Nope, no need. Just to be sure, pushing to master branch of > > > > [email protected]:kparts > > Yes. > > Please fix the coding style first: the '{' belongs on the same line as the > if(). > > The code in frameworks/* should be very consistent now, since I reformatted > it all with astyle. So no excuse anymore for inconsistent coding style in > new code :) > > > But, if I have a green light, then I would like to change the second > > callsite of urlChanged() as well. Please, see attached patch. The > > reasoning > > is the same as with setUrl. > > Coding style: please use { ... } even for one-line statements. > > Looks OK otherwise.
It's there. Thanks! Cheers Michal _______________________________________________ Kde-frameworks-devel mailing list [email protected] https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
