> On Jan. 2, 2012, 10:50 p.m., David Faure wrote: > > kparts/browseropenorsavequestion.cpp, line 287 > > <http://git.reviewboard.kde.org/r/103617/diff/1/?file=45307#file45307line287> > > > > I don't get what this call changes, here (with no filename passed to > > the method). It resolves aliases? But that doesn't matter for > > mime->is(...). So this seems superfluous.
Ahh... We no longer store the result of doing KMimeType::mimeType(...) in the constructor and hence we do it here again to get the mime-type pointer ? KMimeType::mimeType explicitly says not to store the returned value, but this class for some reason did just that. Or is it okay to ignore KMimeType::mimeType's documentation ? > On Jan. 2, 2012, 10:50 p.m., David Faure wrote: > > kparts/browseropenorsavequestion.cpp, line 336 > > <http://git.reviewboard.kde.org/r/103617/diff/1/?file=45307#file45307line336> > > > > I'm not generally opposed to early returns, but this one seems > > dangerous. It's easy to overlook it when adding new unrelated code at the > > end of this method. I preferred the initial if(). That is just a habit. We already did this the first time around. :) I will return it the way it was. > On Jan. 2, 2012, 10:50 p.m., David Faure wrote: > > kparts/browseropenorsavequestion.cpp, line 343 > > <http://git.reviewboard.kde.org/r/103617/diff/1/?file=45307#file45307line343> > > > > The " mime ? " test is superflous, this code is only run when mime is > > not null. Should be cleaned up -- unless the goal was to use the exact same > > code as the other similar setText line, but then this code should be > > factored out, rather than duplicated. > > > > > > > > This code change shows mimetype comments rather than mimetype names, > > too. That's fine I suppose, it's just missing from the overall description > > of this change. Don't forget it in the commit log ;) My actual intent was to make both the ctor and setSuggestedFileName guess mime-types and display the results the same way. Unfortunately, in the process I got bitten by the usual copy/paste bug here. :) Fixed. For the record this one is supposed display the raw mime-type only when mime->comment() returns empty. :) I will also update the commit log to reflect this change. - Dawit ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103617/#review9484 ----------------------------------------------------------- On Jan. 2, 2012, 8:41 p.m., Dawit Alemayehu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/103617/ > ----------------------------------------------------------- > > (Updated Jan. 2, 2012, 8:41 p.m.) > > > Review request for kdelibs. > > > Description > ------- > > * Factored out the code that is used to determine the actual mime-type from > either another mime-type or a filename. > * Avoid storing the KMimeType::Ptr returned by KMimeType::mimeType as stated > in its documentation. > > > Diffs > ----- > > kparts/browseropenorsavequestion.cpp 092198f > > Diff: http://git.reviewboard.kde.org/r/103617/diff/diff > > > Testing > ------- > > > Thanks, > > Dawit Alemayehu > >
