Review Request: Factor out and reuse the code that guesses mime-type from filename

[Dawit Alemayehu]

> 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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20120103/0286148f/attachment.htm>