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.
Details
Details
Renders fine for any kind of text/plain MIME types and sub formats.
Diff Detail
Diff Detail
- Repository
- R320 KIO Extras
- Branch
- addsyntaxhighlighttotextpreview
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 9022 Build 9040: arc lint + arc unit
Comment Actions
After:
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).
| 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. | |
| 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 | |
| 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). | |
| 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. | |
Comment Actions
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.
Comment Actions
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.
Comment Actions
@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?
Comment Actions
Current After:
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:
| 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? | |
| 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. | |
| 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... | |