Review Request 118961: Treat video thumbnails the same as image thumbnails

Tue Jul 1 15:44:37 BST 2014

> On June 26, 2014, 7:15 p.m., Emmanuel Pescosta wrote:
> > Thanks for the patch!
> > 
> > Applying a shadow frame to video thumbnails really makes sense, but I think we should go even further and apply it to other thumbnails as well.
> 
> Diego Soenens wrote:
>     Which ones are you thinking of? There are some that shouldn't have it I think like .exe and .ico files because more often than not they have transparency.
>     Also, what would be the best way to discriminate between them (checking dozens of mime types would get ugly really fast)?
>     
>     Sorry I'm new to all this :).
> 
> Emmanuel Pescosta wrote:
>     > they have transparency.
>     Transparency is no problem, we only use the bounding rect for the shadow effect.
>     
>     I think of PDFs, (odf, txt, ...) documets and so on - almost all thumbnails.
>     IMHO mixing flat thumbails with shadow applied thumbnails looks bad (hee before.png).
>     
>     So why not applying it to all thumbnails? 
>     
>     > checking dozens of mime types would get ugly really fast
>     Yep whitelisting a lot of types is a no-go, but if we apply the shadow to all
>     thumbnails we only need to check if the current item is a directory.
>     
>     > Sorry I'm new to all this
>     Np, feel free to ask questions and just write down your thoughts ;)
>     
>     We should better wait for Frank.
> 
> Diego Soenens wrote:
>     > So why not applying it to all thumbnails?
>     
>     Most ThumbCreator's return the ThumbCreator::DrawFrame flag in their flags() function to draw a 1px frame around the image to give them a depth/3d-ish effect for example.
>     This causes the preview to appear blurry around the edge when combined with the shadow frame (see http://i.imgur.com/TfRqFmN.png).
>     I guess those preview plugins could be updated afterwards to change the flag they return to ThumbCreator::None?
> 
> Emmanuel Pescosta wrote:
>     > Most ThumbCreator's return the ThumbCreator::DrawFrame flag in their flags() function to draw a 1px frame around the image to give them a depth/3d-ish effect for example.
>     In my opinion this should be changed. A thumbnail shoud only be an image of the actual data without a frame because drawing shadows and so on should definitely be 
>     part of the application. This "always" draw an frame around some thumbnails is also bad in the information panel.
>     
>     But please wait for Franks opinion ;)
> 
> Frank Reininghaus wrote:
>     I'm not sure if we should apply shadows to all thumbnails. We even got negative feedback about the shadows for images from users who work a lot with icons and other images which have a lot of transparency. Shadows make less sense for this kind of image than for, e.g., photos. I think that adding shadows to video thumbnails might make sense though - thanks for working on it!
>     
>     I'm not familiar with the ThumbCreator flags - where does this drawing of the 1 pixel frame happen?
> 
> Diego Soenens wrote:
>     Yeah I agree.
>     
>     > I'm not familiar with the ThumbCreator flags - where does this drawing of the 1 pixel frame happen?
>     
>     It seems to be handled in kde-runtime/kioslave/thumbnail/thumbnail.cpp (line 287).
> 
> Emmanuel Pescosta wrote:
>     > We even got negative feedback about the shadows for images from users who work a lot with icons
>     Ok good point. But what about shadows for documents, e.g. PDFs or ODTs? 
>     I think shadow also makes sense in these cases.
>     
>     IMHO we should apply shadows to all different kinds of thumbnails and only exclude icons and other problematic types.
>     (Example - other FM with shadows on documents: http://www.stboston.com/wp-content/uploads/2013/10/Mac-OSX-Finder.png)
> 
> Diego Soenens wrote:
>     Would QPixmap::hasAlphaChannel() work as a way to check if the thumbnail has transparency?
>     If so, maybe the best way to go about it would be to check if the item is not a directory and hasAlphaChannel() returns false?
>     
>     Also, quick question regarding ReviewBoard: does Close -> Submitted commit the patch or will I need to setup git?
>     Or is it up to the reviewer(s) to do that? The documentation is rather vague about the whole process :/.
> 
> Emmanuel Pescosta wrote:
>     > Would QPixmap::hasAlphaChannel() work as a way to check if the thumbnail has transparency?
>     Using the alpha channel of an image to decide if shadow shoud be applied or not is a little bit problematic, because there are a bunch of image formats which support transparency (e.g. png) and should have a shadow effect.
>     
>     So a blacklist for problematic image types would be the better solution here.
>     
>     > does Close -> Submitted commit the patch or will I need to setup git?
>     No, it only closes this review request. You have to push the patch (this review request will be automatically set to submitted when you add the corresponding "REVIEW: 118961" commit hook).
>     
>     Given that you have no git account yet, we'll push it for you ;)
> 
> Emmanuel Pescosta wrote:
>     Hmm and what if we apply it to all images and use the transparency information instead of the image size to apply the shadow effect?
>     So we have no mixing of shadow and no shadow anymore and the user can immediately see if the image has transparent areas, so no
>     more solid borders on transparent images (current version does this for pngs, ...).
>     
>     This can be achieved really easily with the QPainter composition mode.

Removing the border completely from transparent images might be problematic because the border also helps to identify the "clickable area".

BTW, I found one report where there was a discussion about the shadows: https://bugs.kde.org/show_bug.cgi?id=295526

- Frank

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118961/#review61039
-----------------------------------------------------------

On June 26, 2014, 6:59 p.m., Diego Soenens wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118961/
> -----------------------------------------------------------
> 
> (Updated June 26, 2014, 6:59 p.m.)
> 
> 
> Review request for Dolphin.
> 
> 
> Repository: kde-baseapps
> 
> 
> Description
> -------
> 
> A minor (but very noticeable) inconsistency that has been bugging me in Dolphin is the fact that video thumbnails don't get the nice shadow frame added that image thumbnails have.
> Video snapshots are essentially images, and often times these two file types will be in the same folder. So to me it doesn't make sense to treat them differently.
> 
> 
> Diffs
> -----
> 
>   dolphin/src/kitemviews/kfileitemmodelrolesupdater.cpp 0865d40 
> 
> Diff: https://git.reviewboard.kde.org/r/118961/diff/
> 
> 
> Testing
> -------
> 
> Works fine (see before/after screenshots).
> 
> 
> File Attachments
> ----------------
> 
> Before
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/06/26/11ed57e4-04af-4ff0-9963-7a183dfcb035__before.png
> After
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/06/26/ae8c87c1-fb61-48ee-ba3c-11070752edb3__after.png
> 
> 
> Thanks,
> 
> Diego Soenens
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20140701/c7861e0f/attachment.htm>