Review Request: Improved Text Thumbnailer

[Peter Penz]

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


>From my point of view it would be OK to commit the patch. Because of the font rendering improvements during the last years the glyphs are also rendered well for very small sizes -> IMO the ASCII pixmap is not needed anymore. However I'm not the maintainer of the class and cannot give an official "Ship It", but if there are no objections until next Monday I'd suggest to commit it.

One comment to the implementation: No binary compatibility is required for thumbnailer classes -> m_splitter can be removed.

- Peter


On 2009-05-25 04:47:19, Christoph Feck wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/760/
> -----------------------------------------------------------
> 
> (Updated 2009-05-25 04:47:19)
> 
> 
> Review request for Dolphin and kdelibs.
> 
> 
> Summary
> -------
> 
> I originally posted this to k-c-d, but I did not get any feedback. I fixed the "fixed pitch" issue compared to the initial patch.
> 
> 
> Here is the original description:
> 
> Looking at bug 169381 "Text files preview does not support international 
> symbols" I wondered, if it wouldn't be better to just let Qt render the text, 
> instead of hardcoding a primitive bitmap font.
> 
> What I present here is an initial patch to fix that bug, but I have a few 
> questions/issues, so please give feedback.
> 
> * Not sure if KEncodingDetector should be used here, and if I used it 
> correctly. The idea was that the text thumbnailer should handle any text, but 
> I do not know what types of files (UTF-16?) trigger that thumbnailer.
> 
> * I added code to use standard Base/Text colors, but as all mime icons are 
> white, I commented out that, and used the old hardcoded colors.
> 
> * The font size is set to 7 pixels, but grows up to 10 pixels for larger 
> icons. People that prefer pixel sharp fonts will probably have antialiasing 
> disabled, so please try it with your font setup, if it is actually readable.
> 
> * Not sure if that is a bug fix, or a new feature (4.3 or 4.4 ... )
> 
> 
> This addresses bug 169381.
>     https://bugs.kde.org/show_bug.cgi?id=169381
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdebase/runtime/kioslave/thumbnail/CMakeLists.txt 972572 
>   /trunk/KDE/kdebase/runtime/kioslave/thumbnail/textcreator.h 972572 
>   /trunk/KDE/kdebase/runtime/kioslave/thumbnail/textcreator.cpp 972572 
> 
> Diff: http://reviewboard.kde.org/r/760/diff
> 
> 
> Testing
> -------
> 
> Compiles against trunk, works okay for the text files I have. You may need to delete .thumbnails cache to see the effect.
> 
> 
> Thanks,
> 
> Christoph
> 
>