Recent

Author Topic: Invite to Colorizing TSynEdit ! {Improving Editor for Editing Complex Codes}  (Read 111235 times)

Martin_fr

  • Administrator
  • Hero Member
  • *
  • Posts: 5693
    • wiki
Quote
Let's add more happiness to any new HL development, by enabling sfaOutline by-default.
Better to make it avail via Object inspector.

Wait ! do you mean that in future, Object Inspector can be used as Code Explorer ? :o
if yes, OI would be very useful for both form and code exploration.
I love this idea :-*
No, but OI should be able to show and configure FoldConfig, and markups should be components that can be added like a HL. But that will be some work to get there.

Martin_fr

  • Administrator
  • Hero Member
  • *
  • Posts: 5693
    • wiki
On the invalidation. Unfortunately no luck.

You only check, if the change happens in the same line as the caret. But text can change on any line.
Some examples
- indent selection
- unit open in 2 windows (context menu on tab, "clone to new window")
  Edit in one window, and the other window will not update correctly
- Undo/Redo
- syncro edit (changes on potentially dozens of lines)
- multi caret edit, only one caret line, but many carets
- codetools
- editor pascal macros
....

I will be very surprised if you find a solution that does not in any way pre-calculate the columns, and store them. (See my suggestion)

Martin_fr

  • Administrator
  • Hero Member
  • *
  • Posts: 5693
    • wiki
Instead of storing data for each screen line, you could also store a list of all the nodes found for the current screen.

If your list is empty (first run), then you have to fill it (in TextCanged), and invalidate all line that will need painting.

the list simply stores a copy of the node from the nested list (and the end line for that node, any other info you may need, such as color level decided)). That has the line, and the x pos.
The list includes the nodes that start off screen, but paint into the screen.

The list does not duplicate:
If line 722 has an opening node that will end in 732, then you do not need to store this node again for line 723...

If lines are folded, you do not search/store for them. But you store such nodes that start in the fold, and reach into the visible.

when lines where changed, then you need to rescan for those lines (if they are visible. (lines that start before visible, will be found by the backward search of the first visible line.
Then you can compare and invalidate.

You can use the content of the list for drawing.

To be able to skip folded lines, you need to work with screen lines.


Martin_fr

  • Administrator
  • Hero Member
  • *
  • Posts: 5693
    • wiki
I had a long look at the remaining changes. (InProcLevel, ...)

As far as I can see, they are dealing with {$ELSE} (and only $ELSE)?

If they have any other effects, without $ELSE (or even without any $IFDEF) then let me know.

Currently I am inclined not to merge them.

They break currently working functionality.
Code: Pascal  [Select]
  1. procedure foo;
  2. begin
  3. {$IFDEF A}
  4.   try
  5. {$ELSE}
  6.   //
  7. {$ENDIF}
  8.   // ...
  9. {$IFDEF A}
  10.   finally
  11.   end;
  12. {$ELSE}
  13.   //
  14. {$ENDIF}
  15. end;
"try" can no longer be folded.

I have not looked for further examples, but I am sure, I could find them. Equally I am sure that even if you fix this one, there will be others, and some will always be broken.

It is (IMHO) in the nature of ifdef, that (at least without codetools) it is impossible to interpret them correct. If you add code trying, you will simply change one error for another, but pay the price of added complexity.

For example what if instead of $ELSE, the code is {$IFDEF foo} .. {$ENDIF}{$IFDEF opposit_of_foo} .. {$ENDIF}. Or 3 or more consecutive IFDEF, of which you can not detect, if there will be exactly one, zero or one, one or more, zero or more used?

I also think that given the fact that it can always only address a subset of ifdef variations, and that there is no way to predict if users will have those variations or others, the overhead this code adds (in resource usage) is not justified.

-----------------

The only way I can thing that would improve ifdef handling, is ignoring inactive code (like the lowlighting does).

Everything else will be changing one problem for another problem, and which problem is the bigger issue will be each users personal view.

To ignore inactive code codetools is needed. Codetools can not be used inside the HL. It be possible to try a new module to archive ifdef handling (like the lowlighting, or extending that).

But this in definitely not part of this thread. (it is too big a topic of its own)

-----------------


Appart from this a few notes on the implementation.

Those criteria had no part in the decision, any changes to them will not affect the decision.

They are whatever I picked up yet, and probably incomplete

1) It appears that it is limited to 32 nest levels of ifdef, and then what? (32 bits in cardinal)

2) Smart idea to use strings. saving some memory, as copy on write means several ranges can share one string.

2a)
Code: Pascal  [Select]
  1.    while  L < EndLevelIfDef+1 do begin
  2.       SetLength(DepthProcsMade, L+1);
Set the length before the loop.
This code will reallocate the memory over and over. That can cause major slowdown.


Martin_fr

  • Administrator
  • Hero Member
  • *
  • Posts: 5693
    • wiki
That said, feel free to open the hl ifdef on a separate thread. for discussion what may still be possible.

Pascal

  • Hero Member
  • *****
  • Posts: 832
Martin_fr,

is everything implemented now? It does not seem to work for me as the vertical lines should start at the x offest of the first token of the line.

For example at the if:

Code: Pascal  [Select]
  1. if condition then begin
  2.   statement;
  3.   if condition then with variable do begin
  4.     if condition then
  5.       statement
  6.     else
  7.       statement;
  8.     statement;
  9.   end;
  10. end else begin
  11.   statements;
  12. end;

Instead they start at the x offset of the token.

Procedure and function should also be colored.

Regards
Pascal
laz trunk - fpc trunk 32bit - Windows 10 Pro x64 (1803)

x2nie

  • Sr. Member
  • ****
  • Posts: 478
  • Impossible=I don't know the way
    • impossible is nothing - www.x2nie.com
I had a long look at the remaining changes. (InProcLevel, ...)

As far as I can see, they are dealing with {$ELSE} (and only $ELSE)?

If they have any other effects, without $ELSE (or even without any $IFDEF) then let me know.
@Martin:
EndLevelIfDef + InIfdefProcsMade is what you think to deal with {$ifdef}


InProcLevel, InProcNeck is to detect whether the current node is sub-procedure (function,procedure,operator).


There must be reasons of why I introduce several procs/properties (which unfortunatelly I just can't remember them right now).
But you can use the file supplied (demo.pas, demo.js, demo.xml) to visually benchmark once you merge a partial works.
So, whenever you avoid any part of my synpassyn.pas, try your synpassyn.pas to deal/show the demo.pas, you may see the problems.


@Pascal:
yes, what you asked, can only be solved when we reached the next step guided by Martin_fr yesterday:
Instead of storing data for each screen line, you could also store a list of all the nodes found for the current screen.
...
That has the line, and the x pos.
The list includes the nodes that start off screen, but paint into the screen.
The obstacle of previous progress of fixing the vertical line is the speed will be amazingly very slow.
This speed problem will sacrifice the usability, meaning it will make synedit with markup-coloring useless because of the speed degradation.
Today, the speed bottleneck has been fixed by martin, so we can just continue the progress.
(in english: what you requested is indeed in our todo)
When you were logged in, you can see attachments.
Lazarus Trunk @ Windows7 64bit, XP 32bit, Debian under VirtualMachine

x2nie

  • Sr. Member
  • ****
  • Posts: 478
  • Impossible=I don't know the way
    • impossible is nothing - www.x2nie.com
@martin:
your last changes can not deal with this:
Code: Pascal  [Select]
  1. {$IFDEF BCB}
  2. procedure TBitmap32.Draw(const DstRect, SrcRect: TRect; hSrc: Cardinal);
  3. {$ELSE}
  4. procedure TBitmap32.Draw(const DstRect, SrcRect: TRect; hSrc: HDC);
  5. {$ENDIF}
  6. begin
  7.   (FBackend as IDeviceContextSupport).Draw(DstRect, SrcRect, hSrc);
  8. end;
The visual problem: second "procedure" should be drawn as black not as red.
reddish "procedure" indicates that she is sub-procedure, black is first level proc.


Wait, above code is not imaginated, its real code taken from famous gr32.pas.
meaning the code is valid, and there are many similar style of {$ifdef} usage like this.
meaning : you should support the style.


your current change will make all below procedures (of that $ifdef) as red, while actually all above procedure should be black because of they are not sub-proc.
When you were logged in, you can see attachments.
Lazarus Trunk @ Windows7 64bit, XP 32bit, Debian under VirtualMachine

Martin_fr

  • Administrator
  • Hero Member
  • *
  • Posts: 5693
    • wiki
I will look at your example code. But

@martin:
your last changes can not deal with this:
Code: Pascal  [Select]
  1. {$IFDEF BCB}
  2. procedure TBitmap32.Draw(const DstRect, SrcRect: TRect; hSrc: Cardinal);
  3. {$ELSE}
  4. procedure TBitmap32.Draw(const DstRect, SrcRect: TRect; hSrc: HDC);
  5. {$ENDIF}
  6. begin
  7.   (FBackend as IDeviceContextSupport).Draw(DstRect, SrcRect, hSrc);
  8. end;
The visual problem: second "procedure" should be drawn as black not as red.
reddish "procedure" indicates that she is sub-procedure, black is first level proc.

Wait, above code is not imaginated, its real code taken from famous gr32.pas.
meaning the code is valid, and there are many similar style of {$ifdef} usage like this.
meaning : you should support the style.

There are many issues with ifdef. from folding, highlight, top-info-line,.... all based on this.

But the issue here is, your fix picks a random constellation in which this happens, works around for that one constellation, leaves the problem for others, and introduces new problems.

An I pointed out in the try-except, some folds stop working.

As for "random" how is your example more important than say
Code: Pascal  [Select]
  1. {$IFDEF windows}
  2. procedure TBitmap32.Draw(const DstRect, SrcRect: TRect; hSrc: Cardinal);
  3. {$ENDIF}
  4. {$IFDEF linux}
  5. procedure TBitmap32.Draw(const DstRect, SrcRect: TRect; hSrc: HDC);
  6. {$ENDIF}
  7. {$IFDEF darwin}
  8. procedure TBitmap32.Draw(const DstRect, SrcRect: TRect; hSrc: HDC);
  9. {$ENDIF}
  10. begin
  11.   (FBackend as IDeviceContextSupport).Draw(DstRect, SrcRect, hSrc);
  12. end;
Same issue. Only not fixable by any approach inside the highlighter.

If on the other and your argument is that an ifdef/else double procedure declaration is so common, it deserves special handling... Well that can be talked about (maybe, still thinking there may be better ways). But you code goes much further than this, it records ALL nodes in any ifdef. A lot of overhead. (and causing the try except issue).

Also why favour the $else block? In the IDE where either the ifdef or else block will be marked as inactive, this can be very confusing. Imagine the IDE lowlighting the else block (because it is inactive), yet the markup drawing all lines according to the else block. That contraticts itself.

Overhead is an issue too, at least if not optional. I know that your code is fast enough, and memory is cheap.
But here are some cases to consider. Many people are on laptop. If they do not use a feature they do not want their batteries to be drained for it. So if it is just one specific case, then make it just that case. Also make it optional. (e.g. bound to the markup being enabled)

If (and this is a very big IF) the HL ever gets any such code, then it must be very well defined what case(s) exactly it covers, and most important why an exception for that case was made.
So the risk will be kept minimal, that it ends in adding more and more fixes in the HL for something that can not be fixed in the HL.

-------------
Better approach, would be to use info from the lowlighting. That comes from codetools, so it only works in the IDE. And that means it needs a subclassed HL, special for that.
But even that couldn't fix everything.

There may also be a way to solve this outside the HL. But I haven't given it much thought yet.

---------------
About nested procs. you can identify them by checking the TopPascalCodeFoldBlockType, or not? So that needs no extra data to be stored.


« Last Edit: April 14, 2016, 04:26:17 am by Martin_fr »

Martin_fr

  • Administrator
  • Hero Member
  • *
  • Posts: 5693
    • wiki
Here is another example. All levels out correct in the current HL.

Code: Pascal  [Select]
  1. procedure foo;
  2. begin
  3.   {$ifdef a}
  4.     try
  5.   {$else}
  6.     //
  7.   {$endif}
  8.     //
  9.   {$ifNdef a}
  10.     //
  11.   {$else}
  12.     except
  13.     end;  // wrong procedure end
  14.   {$endif}
  15. end;
  16.  

with your code added. it thinks the procedure ends at the line I commented.

the "try" could also be any other block.

So as I said, you fix one issue by introducing another....
That said, of course there are many thinks that only work with your fix.
But it only proofs you cant highlight all ifdefs correct.

x2nie

  • Sr. Member
  • ****
  • Posts: 478
  • Impossible=I don't know the way
    • impossible is nothing - www.x2nie.com
So as I said, you fix one issue by introducing another....
That said, of course there are many thinks that only work with your fix.
But it only proofs you cant highlight all ifdefs correct.
It sad to say, but :-[ apparently (yes) I support a block of pascal syntax and go (I hit and run).
the actual situation:
1. I can't remember in which syntax I introduce them (procs + properties)
2. I didn't able to use unit test that days.


So, sorry. but let me know whenever you passed the unit-test for your code.

When you were logged in, you can see attachments.
Lazarus Trunk @ Windows7 64bit, XP 32bit, Debian under VirtualMachine

x2nie

  • Sr. Member
  • ****
  • Posts: 478
  • Impossible=I don't know the way
    • impossible is nothing - www.x2nie.com

Quote
There may also be a way to solve this outside the HL. But I haven't given it much thought yet.


Better approach, would be to use info from the lowlighting. That comes from codetools,
Yes


Quote
so it only works in the IDE.
No. It should be also work outside IDE, somehow.


Quote
And that means it needs a subclassed HL, special for that.
I think not the HL, but the codetools itself must be subclassed into smaller, which only support $Ifdef.
The idea is to allow user to develop their own pascal syntax editor, with minimum requirements.
The user should be allow to manage list of $ifdef easily.




Quote
But even that couldn't fix everything.
No worry for now, we fix only the known bugs.



When you were logged in, you can see attachments.
Lazarus Trunk @ Windows7 64bit, XP 32bit, Debian under VirtualMachine

x2nie

  • Sr. Member
  • ****
  • Posts: 478
  • Impossible=I don't know the way
    • impossible is nothing - www.x2nie.com
Here is another example. All levels out correct in the current HL.

...

with your code added. it thinks the procedure ends at the line I commented.

the "try" could also be any other block.
Yes, I can reproduce that bug.

and now I remember my reason:
Q:if opening node is inside $ifdef, then which one used ?
A: the last !

{$ifdef foo} for .. do {$else} while...do {$end} ----> the last is "while..do"
{$ifdef baa} try {else} (*don try*) {$end} -----> the last is (*dont try*)
{$ifdef gii} (*raise error here*) {$else} except..end {$end} ----------------- the last is "except..end"
Your code explained:
Code: Pascal  [Select]
  1. procedure foo;
  2. begin  
  3. {$ifdef a}
  4.     try // -------------- first. will be used if there is no $else  
  5. {$else}
  6.     (*dont try*) //------------------- second. always used as valid node.
  7.          //                                        here the 'first' will be reset as before $ifdef  
  8. {$endif}
  9.     // --- code outside $ifdef block will use the last of both. which in this situation is : '(*dont try*)'
  10.     // note: in current situation, there is no 'TRY', because there is $else that removes it
  11.   {$ifNdef a}  
  12.  //// -------------- first. will be used if there is no $else
  13.   {$else}
  14.       //------------------- second. always used as valid node.
  15.          //                                        here the 'first' will be reset as before $ifdef
  16.     except ///----------------------------- invalid except, so is recognized as identifier rather than reserved word
  17.     end;  // wrong procedure end.   // in this situation, here is no 'try', so it's recognized as paired of 'begin'
  18.   {$endif}
  19. end;
  20.  
As You said, we can't highlight all situation of $ifdef, because we don't know which const is defined in {$ifdef __}.



As about codetools, I think I don't care if we will subclass the HL or CodeTools,
the matter is somehow HL can decide ( by user intervention) of which one of $ifdef is used,
.. not always the last one (as I can only did currently).
« Last Edit: April 14, 2016, 06:55:09 am by x2nie »
When you were logged in, you can see attachments.
Lazarus Trunk @ Windows7 64bit, XP 32bit, Debian under VirtualMachine

x2nie

  • Sr. Member
  • ****
  • Posts: 478
  • Impossible=I don't know the way
    • impossible is nothing - www.x2nie.com
---------------
About nested procs. you can identify them by checking the TopPascalCodeFoldBlockType, or not? So that needs no extra data to be stored.
I am trying that approach,


one think I remember is to distinct this 2 different meaning between this:
Code: Pascal  [Select]
  1.  
  2. Procedure First (n : longint); forward; // <----- NO NEED BEGIN HERE
  3.  
  4.  
  5. Procedure Second;
  6. begin
  7.   WriteLn ('In second. Calling first...');
  8.   First (1);
  9. end;  
  10.  
  11. Procedure First (n : longint); /// REAL BEGIN
  12. begin
  13.   WriteLn ('First received : ',n);
  14. end;      



and this another one:
Code: Pascal  [Select]
  1. Procedure First ; /// DIRECT BEGIN
  2.   Function Second(P : PChar) : Longint;
  3.   begin
  4.   WriteLn ('In second.');
  5.   end;
  6. begin
  7.   WriteLn ('In First. Calling second...');
  8.   First (1);
  9. end;  
« Last Edit: April 14, 2016, 07:26:14 am by x2nie »
When you were logged in, you can see attachments.
Lazarus Trunk @ Windows7 64bit, XP 32bit, Debian under VirtualMachine

Martin_fr

  • Administrator
  • Hero Member
  • *
  • Posts: 5693
    • wiki
Quote
So, sorry. but let me know whenever you passed the unit-test for your code
It passed for all that has been merged.

Quote
Quote from: Martin_fr on Today at 04:24:26 am
Quote
    About nested procs. you can identify them by checking the TopPascalCodeFoldBlockType, or not? So that needs no extra data to be stored.
I am trying that approach,
Please use a branch in your git, based on the pas hl, that is now in svn.

Otherwise I will never be able to merge changes. (Or you need to send patches based on the current svn)

Quote
Code: Pascal  [Select]
  1.  
  2. Procedure First (n : longint); forward;
This is probably a bug in the current HL. forward should cancel the fold block.
(I might look into that myself, should be easy)

That (and the delay I had on my side) is one of the things that made merging so much work:
Your work contained:
- fixes, that where not just for the markup
- refactor (move to base class), that was not for the markup
- markup changes.

The markup benefits from all of them, but still the first 2 are independent.

Submitting fixes should be separate.