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

[Méven Car]

meven added a comment.


  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.
  
  > 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 <https://phabricator.kde.org/D25328> as a naive proposal.
  
  In D25323#562911 <https://phabricator.kde.org/D25323#562911>, @kossebau wrote:
  
  > 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.

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20191115/53040ff7/attachment.htm>