Review Request: Internal viewer: improve handling of unknown MIME types

[Jonathan Marten]

> On Dec. 25, 2011, 2:44 a.m., Raphael Kubo da Costa wrote:
> > part/arkviewer.cpp, lines 109-111
> > <http://git.reviewboard.kde.org/r/103110/diff/1/?file=40842#file40842line109>
> >
> >     - It'd be good to use i18nc() here to give translators some context.
> >     - Semantic markup would be good too.
> >     - Don't you need to close the <qt> tag (if it is needed at all)?
> >     - I feel that information such as the mimetype should be kept away from the user, or at most visible in a "more details" part of the dialog. Do you have strong feelings for showing the mimetype here?

I18N context and semantic markup added.

I think the MIME type should be shown here, there is no other way to obtain it.  The only message boxes that have a "Details" option are detailedError and detailedSorry, they can only have a single button, and neither of those is really appropriate here.  Even if it were possible, I'm not sure that having a "Details" option that shows a single line of text helps the user much.

Would it be better if the message showed the MIME type comment instead of the name?


> On Dec. 25, 2011, 2:44 a.m., Raphael Kubo da Costa wrote:
> > part/arkviewer.cpp, lines 146-151
> > <http://git.reviewboard.kde.org/r/103110/diff/1/?file=40842#file40842line146>
> >
> >     This sounds like a good idea, but it is not directly related to the purpose of this request. I'd rather review it (and have it committed) separately.

OK, will submit a separate review.  Thought it may be useful because many other applications (e.g. KMail, Konqueror as file manager when viewing an archive) prompt before opening any file in an external application.


> On Dec. 25, 2011, 2:44 a.m., Raphael Kubo da Costa wrote:
> > part/arkviewer.cpp, line 180
> > <http://git.reviewboard.kde.org/r/103110/diff/1/?file=40842#file40842line180>
> >
> >     Why have the calls to QFile::remove been changed? It looks like the file is not being removed when it is previewed by the internal viewer anymore?

It is removed in dialogClosed(), the other cases are taken care of by the single remove here.


> On Dec. 25, 2011, 2:44 a.m., Raphael Kubo da Costa wrote:
> > part/arkviewer.cpp, line 275
> > <http://git.reviewboard.kde.org/r/103110/diff/1/?file=40842#file40842line275>
> >
> >     Why?

Only that it looks simpler - no performance benefit because findByUrl converts to a KUrl internally.


> On Dec. 25, 2011, 2:44 a.m., Raphael Kubo da Costa wrote:
> > part/arkviewer.cpp, lines 154-155
> > <http://git.reviewboard.kde.org/r/103110/diff/1/?file=40842#file40842line154>
> >
> >     Does this change from KRun::runUrl bring any benefits?

Only that we already have the MIME and service types, so there is no need for runUrl to determine them again.


- Jonathan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/103110/#review9246
-----------------------------------------------------------


On Dec. 29, 2011, 10:04 p.m., Jonathan Marten wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103110/
> -----------------------------------------------------------
> 
> (Updated Dec. 29, 2011, 10:04 p.m.)
> 
> 
> Review request for KDE Utils and Raphael Kubo da Costa.
> 
> 
> Description
> -------
> 
> Archives often contain files which contain and are viewable as plain text, but either resolve to a MIME type that does not have a suitable viewer KPart available, or the default MIME type application/octet-stream.  Trying to view such a file is simply rejected - and there is no "Open with" operation, so it is not easily possible to open the file in KWrite or another application.
> 
> This change gives the user the option, in this case, of viewing the file as plain text - whether this is will result in a sensible display for the file is their decision.  If they want to do that, ArkViewer::viewInInternalViewer() is forced to display the file as text/plain and not to try to detect the MIME type from the file name.
> 
> The size of the internal viewer window is also saved and restored, as per the TODO comment.
> 
> This change does introduce some new GUI strings, so it will not be able to be committed until after the KDE SC 4.8 release.
> 
> 
> Diffs
> -----
> 
>   part/arkviewer.h b810e17 
>   part/arkviewer.cpp 9bfd651 
> 
> Diff: http://git.reviewboard.kde.org/r/103110/diff/diff
> 
> 
> Testing
> -------
> 
> Built ark with these changes, attempted to preview archive file contents for a variety of file types.  The extremely annoying "The internal viewer cannot preview this file" error message is now a thing of the past.
> 
> 
> Thanks,
> 
> Jonathan Marten
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-utils-devel/attachments/20111229/c09d33da/attachment-0001.html>