<table><tr><td style="">kossebau added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D25323">View Revision</a></tr></table><br /><div><div><blockquote style="border-left: 3px solid #8C98B8;
color: #6B748C;
font-style: italic;
margin: 4px 0 12px 0;
padding: 8px 12px;
background-color: #F8F9FC;">
<div style="font-style: normal;
padding-bottom: 4px;">In <a href="https://phabricator.kde.org/D25323#562989" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D25323#562989</a>, <a href="https://phabricator.kde.org/p/meven/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@meven</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><blockquote style="border-left: 3px solid #8C98B8;
color: #6B748C;
font-style: italic;
margin: 4px 0 12px 0;
padding: 8px 12px;
background-color: #F8F9FC;">
<div style="font-style: normal;
padding-bottom: 4px;">In <a href="https://phabricator.kde.org/D25323#562908" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D25323#562908</a>, <a href="https://phabricator.kde.org/p/kossebau/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@kossebau</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><p>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):<br />
changing <tt style="background: #ebebeb; font-size: 13px;">QColor ( 245, 245, 245 ); // light-grey background </tt> to <tt style="background: #ebebeb; font-size: 13px;">highlightingTheme.editorColor(KSyntaxHighlighting::Theme::EditorColorRole::BackgroundColor)</tt> 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 <a href="https://phabricator.kde.org/source/syntax-highlighting/browse/master/examples/codepdfprinter/" class="remarkup-link" target="_blank" rel="noreferrer">https://phabricator.kde.org/source/syntax-highlighting/browse/master/examples/codepdfprinter/</a>.</p></div>
</blockquote>
<p>I don't think it implies this, and I don't see the point here, textCreator makes thumbnails for text files only anyway.<br />
The change is just so that the background follows the user theme and avoids an hardcoded value that is not good practice.</p></div>
</blockquote>
<p>See below why here a hardcoded color can make sense, at least to some.</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>Is this change for background needed to make that rehighlight approach working?</p></blockquote>
<p>No, but IMO, this ought to be changed.</p></blockquote>
<p>So far people agreed that having a consistent paper-like background made sense, also when there were different proposals in the last year:<br />
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<br />
b) currently thumbnail pixmaps are cached, both in processes as well as on disc, so on any theme change the background will be outdated</p>
<p>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.</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>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 <tt style="background: #ebebeb; font-size: 13px;">rehighlight</tt> seems to just work by chance currently to gain whatever effect (which effect does it have actually?),</p></blockquote>
<p><a href="https://doc.qt.io/qt-5/qsyntaxhighlighter.html#rehighlight" class="remarkup-link" target="_blank" rel="noreferrer">https://doc.qt.io/qt-5/qsyntaxhighlighter.html#rehighlight</a> is part of QSyntaxHighlighter, it is pretty stable, and is documented as <tt style="background: #ebebeb; font-size: 13px;">Reapplies the highlighting to the whole document</tt> precisely what is needed.</p></blockquote>
<p>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.</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>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.</p></blockquote>
<p>I totally agree the change might need to be in KSyntaxHighlighting instead, and this patch was meant as a discussion opener.<br />
I would be happy to offer a patch to KSyntaxHighlighting instead, I would just welcome some suggestion from KSyntaxHighlighting maintainer how to handle this.<br />
I opened <a href="https://phabricator.kde.org/D25328" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D25328</a> as a naive proposal.</p></blockquote>
<p>Thanks. Sadly no detailed insight into syntax highlighting, so hoping on its maintainers :)</p></div></div><br /><div><strong>REPOSITORY</strong><div><div>R320 KIO Extras</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D25323">https://phabricator.kde.org/D25323</a></div></div><br /><div><strong>To: </strong>meven, kossebau, cullmann, vkrause<br /><strong>Cc: </strong>kde-frameworks-devel, kfm-devel, pberestov, iasensio, fprice, LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov<br /></div>