D17241: WIP:Disable highlighting after 512 characters on a line.

[Kåre Särs]

sars added a comment.


  I think a partially highlighted line is better than a totally non-highlighted 
one. And I think that the user is more likely to instinctively guess correctly 
why the end of the line is not highlighted than if the line is not highlighted 
at all.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D17241

To: cullmann, vkrause, dhaumann, mwolff, sars
Cc: zetazeta, mwolff, brauch, kwrite-devel, kde-frameworks-devel, LeGast00n, 
domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D17241: WIP:Disable highlighting after 512 characters on a line.

[Christoph Cullmann]

cullmann added a comment.


  Hmm, would a partially highlighted line really preferable?

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D17241

To: cullmann, vkrause, dhaumann, mwolff, sars
Cc: zetazeta, mwolff, brauch, kwrite-devel, kde-frameworks-devel, LeGast00n, 
domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D17241: WIP:Disable highlighting after 512 characters on a line.

[Milian Wolff]

mwolff added a comment.


  I personally dislike this too.

INLINE COMMENTS

> katerenderer.cpp:408
> +auto  = renderRanges.pushNewRange();
>  for (int i = 0; i < al.count(); ++i)
>  if (al[i].length > 0 && al[i].attributeValue > 0) {

couldn't the al.count be limited here? if I understand the patch correctly, we 
throw out all highlighting if we encounter too many ranges. but could we 
instead just skip the items that are beyond the limit?

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D17241

To: cullmann, vkrause, dhaumann, mwolff, sars
Cc: zetazeta, mwolff, brauch, kwrite-devel, kde-frameworks-devel, LeGast00n, 
domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D17241: WIP:Disable highlighting after 512 characters on a line.

[Christoph Cullmann]

cullmann added a comment.


  4daee27406cad0001f30bca4bf551495e0624d23 

  
  >
  =
  
  - addConfigEntry(ConfigEntry(LineLengthLimit, "Line Length Limit", QString(), 
4096));
  
  +addConfigEntry(ConfigEntry(LineLengthLimit, "Line Length Limit", 
QString(), 1));

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D17241

To: cullmann, vkrause, dhaumann, mwolff, sars
Cc: zetazeta, mwolff, brauch, kwrite-devel, kde-frameworks-devel, LeGast00n, 
domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D17241: WIP:Disable highlighting after 512 characters on a line.

[Christoph Cullmann]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R39:1b7a2c69e492: WIP:Disable highlighting after 512 
characters on a line. (authored by cullmann).

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17241?vs=59550=59899

REVISION DETAIL
  https://phabricator.kde.org/D17241

AFFECTED FILES
  src/completion/katecompletiondelegate.cpp
  src/render/katerenderer.cpp
  src/render/katerenderer.h
  src/render/katerenderrange.cpp
  src/render/katerenderrange.h

To: cullmann, vkrause, dhaumann, mwolff, sars
Cc: zetazeta, mwolff, brauch, kwrite-devel, kde-frameworks-devel, LeGast00n, 
domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D17241: WIP:Disable highlighting after 512 characters on a line.

[Christoph Cullmann]

cullmann added a comment.


  Then let's try that. Even for all cases without hitting the limit the new 
code is faster.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D17241

To: cullmann, vkrause, dhaumann, mwolff, sars
Cc: zetazeta, mwolff, brauch, kwrite-devel, kde-frameworks-devel, LeGast00n, 
domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D17241: WIP:Disable highlighting after 512 characters on a line.

[Dominik Haumann]

dhaumann accepted this revision.
dhaumann added a comment.


  +1

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D17241

To: cullmann, vkrause, dhaumann, mwolff, sars
Cc: zetazeta, mwolff, brauch, kwrite-devel, kde-frameworks-devel, LeGast00n, 
domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D17241: WIP:Disable highlighting after 512 characters on a line.

[Christoph Cullmann]

cullmann added a comment.


  I am not sure about the warning.
  The line length limit default should be increased it think, we could just try 
something like 1 for a start.
  I would propose we try this without a warning and see if we get negative 
feedback.
  Normally this should only kick in situations were before we slowed down to 
"unusable".
  Would that be ok?

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D17241

To: cullmann, vkrause, dhaumann, mwolff, sars
Cc: zetazeta, mwolff, brauch, kwrite-devel, kde-frameworks-devel, LeGast00n, 
domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D17241: WIP:Disable highlighting after 512 characters on a line.

[Kåre Särs]

sars added a comment.


  I tried the patch and it improved the performance really much! :) I was able 
to edit a line that contained over a million characters!
  
  Unfortunately I can't comment on the code too much, as I don't know the code 
enough.
  
  Some general stuff:
  
  1. Should there be a warning/information-message about why the highlighting 
is disabled?
  2. Should the line length limit be increased?
  
  Great work!

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D17241

To: cullmann, vkrause, dhaumann, mwolff, sars
Cc: zetazeta, mwolff, brauch, kwrite-devel, kde-frameworks-devel, LeGast00n, 
domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D17241: WIP:Disable highlighting after 512 characters on a line.

[Christoph Cullmann]

cullmann added a comment.


  For the outer vector, yes.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D17241

To: cullmann, vkrause, dhaumann, mwolff, sars
Cc: zetazeta, mwolff, brauch, kwrite-devel, kde-frameworks-devel, LeGast00n, 
domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D17241: WIP:Disable highlighting after 512 characters on a line.

[Dominik Haumann]

dhaumann added a comment.


  Could this be pushed even further by using QVarLengthArray?

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D17241

To: cullmann, vkrause, dhaumann, mwolff, sars
Cc: zetazeta, mwolff, brauch, kwrite-devel, kde-frameworks-devel, LeGast00n, 
domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D17241: WIP:Disable highlighting after 512 characters on a line.

[Christoph Cullmann]

cullmann updated this revision to Diff 59550.
cullmann added a comment.


  Remove a MASS of heap allocations.
  We allocated the range on the heap and then inside we really even allocated 
all 16 byte KTextEditor::Range objects again on the heap.

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17241?vs=59548=59550

REVISION DETAIL
  https://phabricator.kde.org/D17241

AFFECTED FILES
  src/completion/katecompletiondelegate.cpp
  src/render/katerenderer.cpp
  src/render/katerenderer.h
  src/render/katerenderrange.cpp
  src/render/katerenderrange.h

To: cullmann, vkrause, dhaumann, mwolff, sars
Cc: zetazeta, mwolff, brauch, kwrite-devel, kde-frameworks-devel, LeGast00n, 
domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D17241: WIP:Disable highlighting after 512 characters on a line.

[Christoph Cullmann]

cullmann edited the summary of this revision.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D17241

To: cullmann, vkrause, dhaumann, mwolff, sars
Cc: zetazeta, mwolff, brauch, kwrite-devel, kde-frameworks-devel, LeGast00n, 
domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D17241: WIP:Disable highlighting after 512 characters on a line.

[Christoph Cullmann]

cullmann marked an inline comment as done.
cullmann added a comment.


  Here is my try on this.
  Instead of limiting the line length, we just limit the number of attributes 
we are allowed to display.
  That only hits stuff != selection.
  e.g. for the example out of https://bugs.kde.org/show_bug.cgi?id=345775 you 
still see selection but not other stuff.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D17241

To: cullmann, vkrause, dhaumann, mwolff, sars
Cc: zetazeta, mwolff, brauch, kwrite-devel, kde-frameworks-devel, LeGast00n, 
domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D17241: WIP:Disable highlighting after 512 characters on a line.

[Christoph Cullmann]

cullmann updated this revision to Diff 59548.

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17241?vs=48461=59548

REVISION DETAIL
  https://phabricator.kde.org/D17241

AFFECTED FILES
  src/render/katerenderer.cpp

To: cullmann, vkrause, dhaumann, mwolff, sars
Cc: zetazeta, mwolff, brauch, kwrite-devel, kde-frameworks-devel, LeGast00n, 
domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D17241: WIP:Disable highlighting after 512 characters on a line.

[Christoph Cullmann]

cullmann commandeered this revision.
cullmann edited reviewers, added: sars; removed: cullmann.
cullmann added a comment.


  I have drafted a small limit to attributes like I proposed.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D17241

To: cullmann, vkrause, dhaumann, mwolff, sars
Cc: zetazeta, mwolff, brauch, kwrite-devel, kde-frameworks-devel, LeGast00n, 
domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D17241: WIP:Disable highlighting after 512 characters on a line.

[Christoph Cullmann]

cullmann requested changes to this revision.
cullmann added a comment.
This revision now requires changes to proceed.


  I think we shall count the number of ranges on the line to judge the limit, 
otherwise this is a sound idea.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D17241

To: sars, cullmann, vkrause, dhaumann, mwolff
Cc: zetazeta, mwolff, brauch, kwrite-devel, kde-frameworks-devel, domson, 
michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D17241: WIP:Disable highlighting after 512 characters on a line.

[Christoph Cullmann]

cullmann added a comment.


  Sorry to chime in that late ;=)
  
  Wouldn't it make more sense, to limit the number of ranges per line?
  I think the real issue is that we can't handle  highlightings per line, 
not that the raw number of letters is that much an issue.
  
  This would relax the constraint for most use cases.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D17241

To: sars, cullmann, vkrause, dhaumann, mwolff
Cc: zetazeta, mwolff, brauch, kwrite-devel, kde-frameworks-devel, hase, 
michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D17241: WIP:Disable highlighting after 512 characters on a line.

[Kåre Särs]

sars added a comment.


  @dhaumann OK the limit is too low for Kile that is clear. Visual Studio Code 
is limiting the highlighting on a line to 1 characters.
  
  I tried to set the limit to 1, but that was very noticeably slow. 
Selecting a whole line took multiple seconds, which is probably the reason why 
we have had 4096 as the wrap limit ;)
  
  Is it the whole idea of limiting the highlights or just the too low limit 
that you object to?
  
  The main hotspots I see in my perf/hotspot profiling is 
RenderRangeList::advanceTo(...) in  KateRenderer::decorationsForLine() and in 
KateRenderer::paintTextLine() the hotspot is QTextLayout::draw() (especially 
the one with "additionalFormats").
  
  In both places I don't see (right now at least) very many possibilities to 
optimize.
  
  I think the main problem is that we draw the whole line at once even tho we 
only see just a tiny bit of it (when we have long lines).
  
  "Fixing" this problem would probably require that we also start to draw the 
lines in chunks...

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D17241

To: sars, cullmann, vkrause, dhaumann, mwolff
Cc: zetazeta, mwolff, brauch, kwrite-devel, kde-frameworks-devel, hase, 
michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D17241: WIP:Disable highlighting after 512 characters on a line.

[Dominik Haumann]

dhaumann added a comment.


  I'm not convinced this is a good idea: Kile users writing LaTeX tend to write 
one paragraph into a single line. They easily hit the 512 character limit. With 
this patch, we'd break syntax highlighting (completely?) for Kile.
  
  Stupidly asking: Can't this be opzimized evrn further?

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D17241

To: sars, cullmann, vkrause, dhaumann, mwolff
Cc: zetazeta, mwolff, brauch, kwrite-devel, kde-frameworks-devel, hase, 
michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D17241: WIP:Disable highlighting after 512 characters on a line.

[Kåre Särs]

sars marked an inline comment as done and an inline comment as not done.
sars added a comment.


  The new limit is now limiting the number of characters highlighted on a line. 
It is only the rest of the line that is not highlighted. This is IMO nicer as 
you don't see the non-highlighted part at the beginning of the line. The 
highlighting makes it a bit slower, so I decreased the limit to 512.
  
  I have also made the limit into a parameter so that it can be only 100 
characters for the minimap highlighting.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D17241

To: sars, cullmann, vkrause, dhaumann, mwolff
Cc: zetazeta, mwolff, brauch, kwrite-devel, kde-frameworks-devel, hase, 
michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D17241: WIP:Disable highlighting after 512 characters on a line.

[Kåre Särs]

sars retitled this revision from "WIP:Disable highlighting for lines longer 
than 1024 characters." to "WIP:Disable highlighting after 512 characters on a 
line.".
sars edited the summary of this revision.
sars edited the test plan for this revision.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D17241

To: sars, cullmann, vkrause, dhaumann, mwolff
Cc: zetazeta, mwolff, brauch, kwrite-devel, kde-frameworks-devel, hase, 
michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann