[text thumbnailer] Use KSyntaxHighlighting for text rendering
ClosedPublic

Authored by kossebau on Thu, Feb 28, 11:00 PM.

Details

Summary

Makes previews of text files a bit easier to recognize, both due to the
highlighting helping with reading as well as color pattern adding to
recognizability of files by their preview.

Test Plan

Renders fine for any kind of text/plain MIME types and sub formats.

Diff Detail

Repository
R320 KIO Extras
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
kossebau created this revision.Thu, Feb 28, 11:00 PM
Restricted Application added projects: Dolphin, Frameworks. · View Herald TranscriptThu, Feb 28, 11:00 PM
Restricted Application added subscribers: kfm-devel, kde-frameworks-devel. · View Herald Transcript
kossebau requested review of this revision.Thu, Feb 28, 11:00 PM

Before:


After:

There is surely room for more improvements with this thumbnailer, but the syntax highlighting already adds a lot for me (though I only use it with the tooltip myself, not actual thumbnails).

cfeck added a subscriber: cfeck.Thu, Feb 28, 11:13 PM
cfeck added inline comments.
thumbnail/textcreator.cpp
169

KSyntaxHighlighting::Theme also provides a background color. Can that be used instead of the hardcoded 245? I cannot remember reason why I disabled (or never enabled) the QPalette code; I suggest to remove it.

kossebau added inline comments.Thu, Feb 28, 11:33 PM
thumbnail/textcreator.cpp
169

Good idea. Hm, highlightingTheme.backgroundColor(KSyntaxHighlighting::Theme::Normal) returns black for me, and there is a comment for Theme::backgroundColor(TextStyle) const saying
"0 is returned for styles that do not specify a background color, use the default background color in that case. "
No clue yet what is meant by "default background color". Not sure I want to use the Theme::editorColor(EditorColorRole role) const with BackgroundColor?

kossebau added inline comments.
thumbnail/textcreator.cpp
169

@vkrause Can you help here and tell what one is intended to use?

169

@cfeck: I would do the remove of the qpalette in a separate commit then later.

vkrause added inline comments.Fri, Mar 1, 7:52 AM
thumbnail/textcreator.cpp
169

I think default background color here refers to what your palette gives you. The background color defined by highlighting is typically only used for small text blocks like for alerts ("TODO", "HACK", etc).

kossebau added inline comments.Fri, Mar 1, 1:49 PM
thumbnail/textcreator.cpp
169

@vkrause Thanks for the comment. With that in mind and coffee in blood and in daylight I get the SH api dox a bit better. I might think about doing a proposal for improvement for the next person hitting that without coffee perhaps ;)

@cfeck From what I have seen, I would then stay with the hardcoded background color. Actually I wonder why it it not pure #fff as one has(?) with any rich textdocument usually. But not the intention of this very patch, so I leave that out.

kossebau updated this revision to Diff 52899.Fri, Mar 1, 1:49 PM

use original different x & y margins, to gain more chars rendered, as before

kossebau added a comment.EditedFri, Mar 1, 1:52 PM

New after:

Hm, meh, now I see that textDocument.drawContents(&painter); does not stay within the page size the document was given... let's see whether I can do some clipping... Update: which then though also flaws the impression of having more chars rendered again, which is not the case.
Guess I will just role back to the initial version.

kossebau updated this revision to Diff 52901.Fri, Mar 1, 2:31 PM

Changes:

  • clip when drawing the text document, so our "page" margins stay clean
  • for that reuse vars some more

Somehow the old custom margins look nicer to me, so I kept the logic in the
code.
Seems QTextDocument draws the text with a bit more line-spacing, but not
sure if additional code to influence that is worth it.

@vkrause : Any idea why I get org.kde.ksyntaxhighlighting: Repository got deleted while a highlighter is still active! with this code? The KSyntaxHighlighting::SyntaxHighlighter instance and the QTextDocument instance are both created on the stack in the scope of the method, while the KSyntaxHighlighting::Repository is a member of the class.
Any idea why would a highlighter outlives the repoository, or what else would trigger that warning?

So, from my side I am fine with the current patch. While the change to QTextDocument (& syntax highlighting?) results in a marginal bigger linespacing and as result up to one line less text rendered in the preview, I find the newer linespacing actually better to read and by my samples found the missing line not to be important to get which file this is and what content. So I have stopped further experiments to restore the old linespacing.

Given only positive comments so far & the unclear maintainership of kio-extra, I would then proceed to a ship-in-7-days-unless-someone-objects. Of course welcoming any +1 or Accept before :;)

Before:


Current After:

cfeck accepted this revision.Mon, Mar 4, 10:26 PM
This revision is now accepted and ready to land.Mon, Mar 4, 10:26 PM
This revision was automatically updated to reflect the committed changes.
dhaumann added inline comments.
thumbnail/textcreator.cpp
169

I saw this review request just now: Using a hardcoded theme is not a good idea. I suggest to use a solution based on the background color. We have the following in our example codeeditor:

setTheme((palette().color(QPalette::Base).lightness() < 128)
    ? m_repository.defaultTheme(KSyntaxHighlighting::Repository::DarkTheme)
    : m_repository.defaultTheme(KSyntaxHighlighting::Repository::LightTheme));

Could a similar approach be used here as well?

kossebau added inline comments.Sun, Mar 10, 8:10 PM
thumbnail/textcreator.cpp
169

For the thumbnail the code is rendering on a paper-like canvas, which is hard-coded as well (see lines above with QColor ( 245, 245, 245 ); // light-grey background). To simulate a print-out, for what I guess or would have done myself.
So with that using also a hard-coded theme should be fine, or?

dhaumann added inline comments.Sun, Mar 10, 8:40 PM
thumbnail/textcreator.cpp
169

Ah, I thought in a dark color theme the preview is also dark - which does not seem to be the case...