----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103617/#review9484 -----------------------------------------------------------
kparts/browseropenorsavequestion.cpp <http://git.reviewboard.kde.org/r/103617/#comment7770> 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. kparts/browseropenorsavequestion.cpp <http://git.reviewboard.kde.org/r/103617/#comment7771> 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(). kparts/browseropenorsavequestion.cpp <http://git.reviewboard.kde.org/r/103617/#comment7772> 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 ;) - David Faure 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 > >
