kossebau added a comment.
In D25323#562989 <https://phabricator.kde.org/D25323#562989>, @meven wrote: > In D25323#562908 <https://phabricator.kde.org/D25323#562908>, @kossebau wrote: > > > 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 <https://phabricator.kde.org/D25328> as a naive proposal. Thanks. Sadly no detailed insight into syntax highlighting, so hoping on its maintainers :) REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D25323 To: meven, kossebau, cullmann, vkrause Cc: 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
