[text thumbnail] Force Syntax Highligthing when no definition for file was found
AbandonedPublic

Authored by meven on Nov 15 2019, 10:54 AM.

Details

Summary

By default KSyntaxHighlighter does not highlight text when it has not found a valid definition.
In this case force KSyntaxHighlighter to highlight the text.

Alternative to D21295

Diff Detail

Repository
R320 KIO Extras
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 18820
Build 18838: arc lint + arc unit
meven created this revision.Nov 15 2019, 10:54 AM
Restricted Application added projects: Dolphin, Frameworks. · View Herald TranscriptNov 15 2019, 10:54 AM
Restricted Application added subscribers: kfm-devel, kde-frameworks-devel. · View Herald Transcript
meven requested review of this revision.Nov 15 2019, 10:54 AM

Thanks for looking at the issue. No time to look closer the next days, but curious about this partial change (which has been discussed before and discarded):
changing QColor ( 245, 245, 245 ); // light-grey background to highlightingTheme.editorColor(KSyntaxHighlighting::Theme::EditorColorRole::BackgroundColor) implies, one cannot use KSyntaxHighlighting to render text highlighted e.g. for a print-out on a paper (or only for a PDF). Compare e.g. the example https://phabricator.kde.org/source/syntax-highlighting/browse/master/examples/codepdfprinter/.
Is this change for background needed to make that rehighlight approach working?

For the rest, a not existing definition might be something other users of KSyntaxHighlighting might run into as well, so that use-case should be ideally supported by concepts in their API already (or at least ve documented how one is supposed to deal with that case), The proposed work-around code here does not look long-term stable, calling rehighlight seems to just work by chance currently to gain whatever effect (which effect does it have actually?), But as code for human readers it makes little sense. Having to have some non-code comment makes that even more clear something in KSyntaxHighlighting API is not supporting us here.

Also am I wonfering how this relates to the bug report you referred to? Can you tell what effect your code change has on the symptoms reported in https://bugs.kde.org/show_bug.cgi?id=409380#c0 ?

meven added a comment.Nov 15 2019, 2:10 PM

Thanks for looking at the issue. No time to look closer the next days, but curious about this partial change (which has been discussed before and discarded):
changing QColor ( 245, 245, 245 ); // light-grey background to highlightingTheme.editorColor(KSyntaxHighlighting::Theme::EditorColorRole::BackgroundColor) implies, one cannot use KSyntaxHighlighting to render text highlighted e.g. for a print-out on a paper (or only for a PDF). Compare e.g. the example https://phabricator.kde.org/source/syntax-highlighting/browse/master/examples/codepdfprinter/.

I don't think it implies this, and I don't see the point here, textCreator makes thumbnails for text files only anyway.
The change is just so that the background follows the user theme and avoids an hardcoded value that is not good practice.

Is this change for background needed to make that rehighlight approach working?

No, but IMO, this ought to be changed.

For the rest, a not existing definition might be something other users of KSyntaxHighlighting might run into as well, so that use-case should be ideally supported by concepts in their API already (or at least ve documented how one is supposed to deal with that case), The proposed work-around code here does not look long-term stable, calling rehighlight seems to just work by chance currently to gain whatever effect (which effect does it have actually?),

https://doc.qt.io/qt-5/qsyntaxhighlighter.html#rehighlight is part of QSyntaxHighlighter, it is pretty stable, and is documented as Reapplies the highlighting to the whole document precisely what is needed.

But as code for human readers it makes little sense. Having to have some non-code comment makes that even more clear something in KSyntaxHighlighting API is not supporting us here.

I totally agree the change might need to be in KSyntaxHighlighting instead, and this patch was meant as a discussion opener.
I would be happy to offer a patch to KSyntaxHighlighting instead, I would just welcome some suggestion from KSyntaxHighlighting maintainer how to handle this.
I opened D25328 as a naive proposal.

Also am I wonfering how this relates to the bug report you referred to? Can you tell what effect your code change has on the symptoms reported in https://bugs.kde.org/show_bug.cgi?id=409380#c0 ?

I misinterpreted the bug, I thought it was about kio-extras. My bad.
The fix here is in fact a downstream workaround for this bug as you noticed.

meven edited the summary of this revision. (Show Details)Nov 15 2019, 2:10 PM

Thanks for looking at the issue. No time to look closer the next days, but curious about this partial change (which has been discussed before and discarded):
changing QColor ( 245, 245, 245 ); // light-grey background to highlightingTheme.editorColor(KSyntaxHighlighting::Theme::EditorColorRole::BackgroundColor) implies, one cannot use KSyntaxHighlighting to render text highlighted e.g. for a print-out on a paper (or only for a PDF). Compare e.g. the example https://phabricator.kde.org/source/syntax-highlighting/browse/master/examples/codepdfprinter/.

I don't think it implies this, and I don't see the point here, textCreator makes thumbnails for text files only anyway.
The change is just so that the background follows the user theme and avoids an hardcoded value that is not good practice.

See below why here a hardcoded color can make sense, at least to some.

Is this change for background needed to make that rehighlight approach working?

No, but IMO, this ought to be changed.

So far people agreed that having a consistent paper-like background made sense, also when there were different proposals in the last year:
a) thumbnails are kind of representing print-out (see also shape border), so like PDFs & other docs do not change "paper" background color on style change
b) currently thumbnail pixmaps are cached, both in processes as well as on disc, so on any theme change the background will be outdated

And I still think that holds. But you are free to challenge that, as young generations ought to do ;) Not a maintainer, just stated my 2 cent here.

For the rest, a not existing definition might be something other users of KSyntaxHighlighting might run into as well, so that use-case should be ideally supported by concepts in their API already (or at least ve documented how one is supposed to deal with that case), The proposed work-around code here does not look long-term stable, calling rehighlight seems to just work by chance currently to gain whatever effect (which effect does it have actually?),

https://doc.qt.io/qt-5/qsyntaxhighlighter.html#rehighlight is part of QSyntaxHighlighter, it is pretty stable, and is documented as Reapplies the highlighting to the whole document precisely what is needed.

What I mean is that it is strange this has to be done explicitly here only in this case, and that it still works, given we just checked that syntaxHighlighter has no definitions. This seems rather random.

But as code for human readers it makes little sense. Having to have some non-code comment makes that even more clear something in KSyntaxHighlighting API is not supporting us here.

I totally agree the change might need to be in KSyntaxHighlighting instead, and this patch was meant as a discussion opener.
I would be happy to offer a patch to KSyntaxHighlighting instead, I would just welcome some suggestion from KSyntaxHighlighting maintainer how to handle this.
I opened D25328 as a naive proposal.

Thanks. Sadly no detailed insight into syntax highlighting, so hoping on its maintainers :)

See D25891 (not tested by myself, only saw it) for another approach to this. So seems in general setTheme() always needs an explicit rehighlight() afterwards (but why only in some cases)?
AbstractHighlighter::setTheme claims that "Subclasses can re-implement this method to e.g. trigger re-highlighing or to do general palette color setup. ", so SyntaxHighlighter actually is missing to comply here?

meven abandoned this revision.Dec 14 2019, 7:22 AM

In favor of D25891