WIP:Disable highlighting for lines longer than 1024 characters.
Needs ReviewPublic

Authored by sars on Thu, Nov 29, 7:56 PM.

Details

Summary

A lot of time is spent in decorationsForLine() when we have long lines.

Disabling all highlighting for lines longer than 1024 improves the rendering speed very much, but again it is not so nice to not have highlighting.

Diff Detail

Repository
R39 KTextEditor
Branch
disable_hl2
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 5833
Build 5851: arc lint + arc unit
sars created this revision.Thu, Nov 29, 7:56 PM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptThu, Nov 29, 7:56 PM
Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel. · View Herald Transcript
sars requested review of this revision.Thu, Nov 29, 7:56 PM
brauch added a subscriber: brauch.Thu, Nov 29, 8:04 PM

Shouldn't this change simultaneously remove the line length limit ...?

sars retitled this revision from Disable highlighting for lines longer than 1024 characters. to WIP: Disable highlighting for lines longer than 1024 characters..Thu, Nov 29, 8:06 PM
sars edited the summary of this revision. (Show Details)
sars added a comment.Thu, Nov 29, 8:29 PM

If we get it working properly we could make the limit much higher. I tried with 30000 characters and it still worked very good. (except when selecting the line)

sars updated this revision to Diff 46632.Sat, Dec 1, 5:23 PM

Also disable highlighting in selections for lines that are longe than 1024

sars updated this revision to Diff 46633.Sat, Dec 1, 5:41 PM
sars retitled this revision from WIP: Disable highlighting for lines longer than 1024 characters. to Disable highlighting for lines longer than 1024 characters..
sars edited the summary of this revision. (Show Details)
sars added a reviewer: dhaumann.

(Remove the unrelated change in src/view/kateview.cpp)

This version now disables the highlighting for lines longer than 1024 characters also when the line is selected. The selection highlight is shown tho.

What is the way forward? Do we apply this? Should we modify the line length limit?

I have now tested with a file with 97000 columns and I can start to see some slowdown but it is still usable.

I think we could raise the limit to 50000, but also add a notification if the file contains lines longer than 1024.

mwolff requested changes to this revision.Tue, Dec 4, 9:33 AM
mwolff added a subscriber: mwolff.

what sven said, we should also remove the code to disable highlighting altogether when the line limit is reached, no?

src/render/katerenderer.cpp
387

put the 1024 into a constant and use it here and below such that we ensure the number stays in sync

also, don't we have a setting for the line length limit? shouldn't that be used instead here?

400

this style-change should be submitted independently of this code review

This revision now requires changes to proceed.Tue, Dec 4, 9:33 AM
sars added a comment.Tue, Dec 4, 11:30 AM

I agree, we need to do something about the line length limit (that only wraps the lines at the limit when opening the file).

The question is: do we totally remove the option and limit or only the option and raise the limit.

You can still edit the document at 200 000 characters, but it starts to get sluggish. At 500 000 it takes almost a second to insert a space (on my computer). If we keep the limit, we probably could be one of the few editor that could open and read files with millions of characters on a line.

If we keep the limit what would it be? 100000, 65536, 50000 or what?

Or do we keep the option and just raise the default limit?

I'm currently working on adding a notice message about the disabled highlighting.

There also seems to be a test for the long lines that needs to be updated.

sars retitled this revision from Disable highlighting for lines longer than 1024 characters. to WIP:Disable highlighting for lines longer than 1024 characters..Tue, Dec 4, 11:30 AM
sars updated this revision to Diff 46957.Thu, Dec 6, 1:44 PM
sars edited the summary of this revision. (Show Details)

Add a message to inform about why the lines are not highlighted.
Add a note about disabled highlighting to the wrapped lines warning.
Increase the default line length limit to 100 000 characters per line.

sars added a comment.Thu, Dec 6, 1:55 PM

The highlighting limit is now returned in a function in KateRenderer as it is used also in katedocument.cpp for the warning/information message.

src/render/katerenderer.cpp
400

I figured that since I'm indenting the whole section and touching the line anyways, that fixing the style does not hurt. ;)

(Phabricator is not showing the white-space change)

dhaumann added a comment.EditedSat, Dec 8, 4:39 PM

Is it correct that highlighting in the document (i.e. KSyntaxHighlighting) still takes place for the entire line, and this change simply only changes the fact that KateRenderer stops using the highlighting info after 100000 columns? And the next line is highlighted normally again, right?

Prior to accepting this patch, would it be possible to fix this differently, i.e. by profiling and fixing this properly instead of applying a workaround?

src/buffer/katetextbuffer.cpp
71

Hm, isn't the line length limit configurable in kateconfig.cpp? There you will find a key called

const char KEY_LINE_LENGTH_LIMIT[] = "Line Length Limit";

that is used to read/write the config file.

I think if this is changed here to 100000, it should be changed to the same value in KateDocumentConfig::readValue():

setLineLengthLimit(config.readEntry(KEY_LINE_LENGTH_LIMIT, 4096));

And I would even suggest to rename as follows:

  • const char KEY_LINE_LENGTH_LIMIT[] = "Line Length Limit"; + const char KEY_LINE_LENGTH_LIMIT[] = "Line Length Limitation";

Since then, all users will automatically get a reset here. Without this change, all users will stay on the 4096 limit.

Guys this is great work. Can we please make the limits configurable? Each developer will have a different sweet spot. I don't want to burden this revision, maybe we can make a bug about configuring the limits and leave that part to someone confortable with the configs.

sars updated this revision to Diff 47146.Sat, Dec 8, 8:24 PM
  • Use a bit different config key to force a value reset for the limit.
  • Fix reading the config value.
  • Update the line-highlight disabled info message and position.
sars marked 2 inline comments as done.Sat, Dec 8, 8:53 PM

@dhaumann: You are right, the highlighting of the document still takes place and it is only KateRenderer that stops using the highlighting info for all lines longer than 1024 characters. All shorter lines are highlighted.

So the length limit option is still the same: Wrapping long lines on opening a document, just 25 times longer ;)

The disabled high-lighting is a workaround, but if we don't bring in some radical changes it will choke at some point anyways. So I would say that the target is to get the limits as high as possible.

I saw that Visualstudio code uses a similar limit for highlighting lines, but I don't know if that disables the highlighting totally....

I found this workaround by running Kate in Hotspot ;) I can continue a bit to see if I can get the 1024 limit raised...

sars added a comment.Sat, Dec 8, 8:59 PM

@zetazeta I'm not sure the highlighting length limit is worth an option as the editing of the document is not effected. I it is just a bit inconvenient that the highlighting disappears...