Hi,

To take the discussion of the bug tracker.

My current intention on this issue would be to solve the additional issues, such as double scan on Paste, or ensuring LoadFromFile only does a single scan for all lines in one go)

This would mean the only Problem remaining is:
If a user writes his own code, calling Lines.Add (or even Lines.Insert) in a loop. And does so without calling Lines.BeginUpdate before.

While I admit that such a behaviour would add a lot of calculation overhead, with the current SynEdit implementation, I would also see it partly as a user error, since other Components also provide BeginUpdate or similar Methods.
Still it should be fixed.
But other changes could affect it (scan only on demand), and the amount of work, and the risk of breaking thinks are IMHO outweighing the current benefits.

So once the above is closed, I would target that as "post 1.2" (simply meaning whenever some one has time to do it)

Or do you think think this should be handled as a more severe issue?

Best Regards
Martin

2009/3/4 Martin Friebe :

IMHO, if the user does not call Lines.BeginUpdate before doing Lines.Add or Lines.Insert, then the performance penality is his own fault, not something that must be fixed.

Vincent

Vincent Snijders wrote

IMHO, if the user does not call Lines.BeginUpdate before doing Lines.Add or Lines.Insert, then the performance penality is his own fault, not something that must be fixed.

Absolutely agree.

Best regards,
Paul Ishenin.

Calling BeginUpdate is not enough, they must disable/unset the highlighter to avoid the double parse.

Cheers,
Fl?vio

As I said, I need to check, if BeginUpdate currently works(I haven't verified, so I don't know if it does). Until that is done the bug stays as it is.

*If* BeginUpdate works, then the content of my original mail should be applicable.

Best Regards
Martin

To avoid any misunderstanding:

*If* / *Once* BeginUpdate works, then you can insert a batch of say 100 lines (with Lines.Add or Lines.Insert). It will scan 101 lines (because yes, 1 line needs to be rescanned)
I do not think we need to worry about the performance between 100 or 101 Lines? (Not saying this may not be "fixed", but it would have a very low priority "post 1.2")

If you insert a single line, it will scan 2 Lines. True that is not good neither. But it will still be so fast that no human can tell the difference.

So I will fix BeginUpdate, *if* it is broken (and I will try to get that done for 0.9.28)

But making the changes according to the original description will be deferred for later (unless a nice clean patch comes along). It may also be that it will become unnecessary due to other changes that may be made in future.

Do you think fixing BeginUpdate will improve the Situation enough for now?

Best Regards
Martin

Oops, only now I saw you said "once you fix the code to do load and paste in one go".
Sorry :-$

IMO this is pretty good enough, yes. (Not sure it's possible, though) The important thing is to fix the paste performance; I didn't mean to be pedantic with the code ;-)

Cheers,
Fl?vio

Calling BeginUpdate is not enough, they must disable/unset the highlighter to avoid the double parse.

I found it sufficient when the highlighter only parses the current line(s) while painting the text. When the highlighter information is reduced to a flag, indicating continuation lines in multi-line comments, this flag can be stored together with the informaton about the line begin in the text (line number table). Then the determination of the line positions can be done by the highlighter/parser base class. Later, when text is displayed, the continuation flag of the current line is passed to the highlighter, so that only the requested line has to be parsed.

Things can become more complicated, of course, when e.g. matching pairs (begin-end, parentheses...) shall be highlighted dynamically.

DoDi

The SynHighlighterPas unfortenately needs to keep some state across lines => some of this is needed for folding.

You can fold on a procedure, except if it is in:
- interface (just the declaration)
- "type" declaration: "a = procedure of object"
There are many many other examples like this....

This may only be known if you know the state of the previous line(s). Actually in the worst case that can be a 100 lines above, if someone put as many empty, or comment lines in between.

Yet I agree, the highlighter shouldn't scan on every change, it should scan only if the info is required => When a line is painted, all unscanned lines to this line must be scanned.
That's a different issue from what we currently discuss. But it is also not that simple, because folding relies on highlighting. If the vertical scrollbar is painted, then all folding info down to the end of file is needed (to calculate the amount of actual visible lines). I have ideas how to still reduce scanning, but one step at a time......

Best Regards
Martin

No problem, it is always good to sort out any doubt. From your description your problem may be something different though....

Do you refer to paste, as done by SynEdit internally (not by externally calling lines.Insert) ?

Because the current paste (if you press ctrl-v in the IDE, having 10 lines in the Clipboard) isn't affected by this bug. (I looked at it in the debugger)
It is affected by 2 other things:
1) A bug, that paste insert 10 empty lines, sans 10 empty lines, puts the content in, scans the content.
- The 10 lines Content a scanned in a single call to ScanFrom => so ScanFrom needs only once to rescan a single known line, and then does 10 lines without any duplication scanning.
- The same applies to scanning the 10 empty lines. It is *one* extra call to ScanFrom (which should be avoided)

2) A problem in the design of SynEdit's Ranges
If the code you insert has unbalanced keyword pairs (begin without end / "(*" without "*)" / sometimes normal "(" without ")" / .... ) then each call to ScanFrom (the good one, that scans the content) will always Scan to the very end of file!!!
This is because The Ranges, contain info how many open "begins" there are. If that info changes, then all Ranges to the end of File must be updated (Folding needs this info, too)

I do have ideas on how I may be able to improve that, so I haven't verified they will work as expected. => and it will be a lot of work to fix this kind of thing. So that is definitely an item for "post 1.2"

The best way to experience is MacOSAll.pp ( in the FPC source dir packages\univint\src\MacOSAll.pas)
and type on the top of file (outside any comment of course) "begin" (not the time it takes for the "n") :( :( :(

The only good news in this is, that I believe the it can be partly improved by refactoring the highlighter itself => I think I can implement a faster Scanning.

Best Regards
Martin

Sorry, I meant when you paste _at EOL_.

-Fl?vio

Sorry, I meant when you paste _at EOL_.

Dang, I meant _EOF_.
I should have made it clear before that the problem as I see only/always involves the last line/EOF.

Best regards,
Fl?vio

I found it sufficient when the highlighter only parses the current line(s) while painting the text.

By the original design the highlighter parses only more than one line if the Range
is changed by typing.
There is no logical reason to change this behavior, folding or not doesn't matter.
But a lot of people have been made changes, which never understood nor even have tried
to understood how the editor is working.
One can only wonder why people are so reluctant to use their brain for what it has been created, thinking.

Martin

Some of the highlighting (actually folding) added (by me) needed more complexity.

I doubt, that more complexity is needed.

The cost on normal operation can be ignored.

A highlighter should be lightning fast even on an underpowered netbook, as it was designed to work fast on far weaker computers..

Martin

That (AFAIK) is again different from the you modify a line by inserting text into the line, then the highlighter should scan based on the stored info what the range was at the start of line (*).
The Range found after Highligher did scan, is compared with the Range on the next Line, and scanning stops.

Some of the highlighting (actually folding) added (by me) needed more complexity. I understood (or believe I did) the existing scheme, but had to break it. As a consequence, at the End of the Range an extra line may have to be checked. The cost on normal operation can be ignored. (No one can type this fast / any code calling functions like this in a loop, should do BeginUpdate)

(*) For this it does not matter, if "Ranges[LineIndex]" is:
- the Range as on the Start of this line
- the Range as on the end of this line (because if you need the start-range you get it from the previous line)

------------------------------
The issue at hand was originally described as:
The Range at the end of the very last line is not stored. If you add a line below the end of file, you must scan both lines:
- the old last line to get the range for the start of the next (new) line
- the new line (and actually there is no reason to scan that new line, the result is discarded => yet currently it is scanned)

According to the submitter Lines.BeginUpdate doesn't work (I have not verified this).
*IF* that is the case the currently the following
BeginUpdate

  for a := 1 to 100 do Lines.Add( xxxxx );
  EndUpdate

would scan each line twice.
=> It should not. If BeginUpdate works, they are all scanned once in a single go.

So in truth the problem at hand is/are:
- if BeginUpdate does work or does not work.
- Scanning the last line and the discarding the result (see my next mail)

Best Regards
Martin

There is another way to avoid double scanning of the last line.

- Allow the (current/old) last line to be scanned, if a line is appended
- But Do not scan the last line of the text, on normal Scans (since the result is not used anyway)

 From ScanFrom
    fHighlighter.SetLine(FTheLinesView[Result], Result);
    fHighlighter.NextToEol;
    inc(Result);
    if Result = FTheLinesView.Count then
      break;

As you can see if Result hits Count, the result of NextToEOL is never used

    fHighlighter.SetLine(FTheLinesView[Result], Result);
    inc(Result);
    if Result = FTheLinesView.Count then
      break;
    fHighlighter.NextToEol;

There are still cases where this is less good than, storing the EndRange instead of the StartRange. but anyway ...

Best Regards
Martin

It is still way faster than a person can ever type.

if one does things more complicated as needed one will also likely introduce more bugs.
A few minutes more thinking often saves hours of work, in my experience.

Martin

Actually you are right. The problem lies in the folding module.

But anyway, even on a low power computer, scanning just one line more will not be noticeable.
That is why (until folding gets improved) the ScanFrom does a tiny bit more work.

The problem is that some folds end in the previous line;=> that means the token indicating the end, is in the line *below* the folds end. Yes it still can (and will) be solved in a better way. But as I said, it shouldn't have an impact. It is still way faster than a person can ever type.

Of course I don't know the original, I only looked at it during the past 6 month, and can only comment on the impat changes in that time have made

As far as I can see it still is. And while I can follow the theoretical point of the issue, I don't see a practical case where it would make a difference.

Anyway, some parts of this issue are easy to fix (if they are broken at all, which I still haven't checked).

Best Regards
Martin

You seem to have misread me. There is a diff between "doing things more complicated than needed" and "adding more complex functionality than previous present".
(I don't want to go down the road and start arguing which functionality is needed or useful, or ....)

The fact is, that I choose an implementation that was simpler (in design and implementation), so it had a lower risk of being buggy, and was quicker to implement. But the reason was not that it was simpler to do, the reason was that the propper solution will become easier, once some of the refactoring (next sentence) has been done.

It goes against the good principle of "do it right or not at all". But SynEdit as it stands for now, has become so far a way from good design, that it will take a long time to change this (I am in the process of trying). I simply decided, that this should not stop me from also adding features.

Martin

Slightly different answer:

I do not only spent minutes, but probably even hours on thinking. Yet I am aware that I can never explore all tought alone. I can not even explore a sufficient percentage of the available thought as long as I do this on my own.
Hence I will always read, and think about any constructive critic.

As for the issue at hand, here are my thought (up till now), and feel free to add any input, which may improve them.

Currently FoldInfo (referring to available FoldPositions) is stored on the ranges. (I do not know who introduced it, or if it was right at the time, nor does it make sense for me now, to try and judge this). If carefully done FoldInfo could be stored outside the Ranges (on an AVL tree similar to the one that stores which folds are collapsed). The highlighter may still be an appropriate place to detect them, Since it does already do some scanning. (Yet that is an isue of it's own, deserving it's own thoughts)

- This could reduce the amount of different Ranges, and therefore reduce the likelihood of having to Scan many lines before getting in Sync with existing ranges.
- this would also allow for separate FoldTrees for pascal begin/end; for {$SECTION} user defined (which can overlap); for {$IFDEF}

This hasn't been started yet. I am still thinking about it.

Best Regards
Martin

The original design allows to have an unlimited amount of nested Highlighters,
which could be used to solve a lot of problems in a simple way. Upon a lot of requests for a scriptable Highligter I have even published a lexer that could be scripted, and shows nicely how easy nesting is and how to provide a lot of information by simply storing references, that one can walk up and down.

Martin

BeginUpdate is indeed broken for SynEdit...

Good news is, that due to other requirements, the initial request/optimization to store ranges at the end of line has been implemented in SVN.

Best Regards
Martin

Dear Moderator, please kindly remove my email from the mailing list. Thank you very much

Yoppy