FLUID syntax highlighting bug with quotes

[jafcobend]

The syntax highlighter in FLUID gets confused when a quote (") is the first character of a line. It essentially believes its the end quote, when it must always be a starting quote.

--- a/fluid/CodeEditor.cxx
+++ b/fluid/CodeEditor.cxx
@@ -238,7 +238,7 @@ void CodeEditor::style_parse(const char *text, char *style, int length) {
 	length --;
 	col += 2;
 	continue;
-      } else if (*text == '\"') {
+      } else if (*text == '\"' && col) {
         // End quote...
 	*style++ = current;
 	col ++;

This is in v1.3.5. It would also be nice to push the improved highlighter code in FLUID into the "designing a simple text editor" example. Its good to demo what works, not what doesn't. :-)

[erco77]

Hmm, not sure that's the solution.

I tried the recommended mod, and I can still get it to fail with a simple two line code block in the code editor:

const char *s = 
"This is a test";

..where "This is a test"; shows in normal mode instead of blue, with the quoted line flush left.

Replication:
If I alternate between adding a space before the flush-left quote and deleting it
with backspace, the issue persists.

It seems like the problem is that in the beginning of the for() loop in style_parse(), the
*style is starting out in quote mode ('D') instead of in normal mode ('A').

And this seems due to code outside of style_parse() that determines what *style starts out set to, which looks at the line above to see what style should be carried over to the current line being analyzed.

I'll see if I can dig a little more, and when a fix is determined, see if that can be pulled over to the editor demo too.

Let me know if I'm missing something.

[erco77]

I'm thinking this might be the fix; updates the stale style buffer state from previous line
in this one case where an edit just occurred; not sure if this same 'stale state' lies elsewhere,
but pretty sure stale info in the style buffer being used to start style parsing is the cause:

--- a/fluid/CodeEditor.cxx
+++ b/fluid/CodeEditor.cxx
@@ -336,6 +336,9 @@ void CodeEditor::style_update(int pos, int nInserted, int nDeleted,
     text  = editor->mBuffer->text_range(start, end);
     style = editor->mStyleBuffer->text_range(start, end);
 
+    style[0] = last;  // Update beginning of style buffer with end of last line (issue #135)
+                      // Ensures we don't start parsing with possibly stale style buffer info
+
     style_parse(text, style, end - start);

To use this mod, undo your change and use this in its place; can you confirm if it solves or not for your test case?
I've only been using my test case so far, not sure what other cases there might be.

[erco77]

I think there's more stale style issues; while experimenting with /* .. */ comment blocks, I still see more stale info coming in, so continuing to investigate.

[jafcobend]

Are you using the source from v1.3.5? That's what my patch is against. You are partly right about *style pointing to "D". BUT that is as it should be! A starting quote gets styled as D, same as the continuing text and the closing quote. Where things go wrong is assuming that a quote already styled as D is the terminating quote, hence end of the style. When the quote occurs one or more chars from the start of the line the highlighting code had enough info to make the correct decisions. So the only place it failed is when the quote was literally the first char on the line, hence the "&& col".

Using my patch on my copy of the 1.3.5 source makes the quote highlighting work and I never had trouble with the /* ... */ remarks, crossing lines. Now in the demo code I mentioned the mutli-line remark and quote handling are broken.

This is what my FLUID code editor does:

That was typed in in the order seen in the image and I went back and did some edits and open and closed the window. The formatting remained true. Its working for me and this is the patch I'm using.

[jafcobend]

Oh! One other note: style_update() only passes text/style to style_parse() from the previous line if it determines its needed, like multi-line remarks. Otherwise it only passes the current line. In some cases it will also pass the remainder of the text & style buffers.

[erco77]

Ah, I think I see what you're saying. I guess you're right then.

OK, let me do a few more tests.. have to admit I'm not fully understanding what's going on in there, you've probably got a better handle on it, sounds like.

I thought style_parse() was being called to recalculate the style starting with 'text' it's passed, and since it starts parsing with the ("), it seemed like 'A' would be the 'before' style, and the first (") would snap it into 'D' mode.
But when the mode starts as 'D', yeah, it hits that second 'End quote' condition for the first quote, forcing it to 'A' mode, and your mod prevents that if col == 0.

The implemented logic is weird to me, because then it never gets a chance to parse the opening (") for either the start or end quote conditions when parsing text[0], but I guess it's supposed to work that way, since it's apparently pre-determined its in quote mode, and will effectively skip over both quote conditions for that leading quote.

I'm gonna stare at the code a little more to see why it would work that way.. the design escapes me at present.

[jafcobend]

I confess I haven't spent much more than an hour looking at it and that sitting next to my wife while she's watching TV. Not my most productive situation. But the code seems fairly well documented. The basic flow as I see it is:

1. The editor's change event fires, calling CodeEditor::style_update()
2. CodeEditor::style_update() finds the beginning and ending of the line, making adjustments as mentioned above, and then calls CodeEditor::style_parse() with slices of the style and text buffer for the line.
3. style_parse() parses and sets style info for the buffer slices passed to it.

So, in the case of quotes we can always expect that style[0] and text[0] are first character of the line (col=0) and corresponding style. C++ syntax dictates that a quote in that column is always going to be the beginning of a quoted string. At least in my version of g++. So I prevent it from being interpreted as an ending quote.

If the quote occurs in col >=1 then current is likely to be 'A', basically anything other than 'D', and will be handled correctly. There might be a better way to manage current... but that will take a bunch more time to analyze and I don't have it at the moment.

I did do some std::cerr << ... to verify the flow. Gotta learn gdb...

[erco77]

I am still getting weird behavior with multiline block comments (nothing to do with your mod).
Here's a video showing what might be a related issue with the handling of /*.

[jafcobend]

Interesting! I hadn't tried that variation of a /*...*/ edit. Looks like that section at the end of style_update, that starts with if (start==end || last != style[end - start - 1]) needs to be changed to fire when an EOL format is deleted.

[jafcobend]

And, yes, I verified that I have that issue too.

[erco77]

I tried an experiment to rewrite the parser; made a fork.
Try cloning and git checkout issue135, build, and try the resulting fluid to see if it solves, well, hopefully everything.

Some new features:

• Multiline directives (e.g. #define's with trailing 's should highlight correctly on subsequent lines)
• Strings within directives will still highlight as strings (they didn't previously)
• Syntax highlighting on {}'s removed because it looked clownish.. most code editors don't highlight them (but some do)

Curious if you have luck. I started with branch-1.3 when I created the branch, so it should be just like 1.3 only with my mods.

[jafcobend]

Looks like you need to escape your backslash in the comment about trailing ones. I'm sure you weren't concerned with trailing apostrophe esses. :-)

I gave it a quick test spin and I couldn't break it. Well, except for one teensy minor thing: Leaving a quote off the end of a quoted string the highlight spills over to the next line. My g++ will bark about that missing quote, so it seems that the syntax highlight should end with the line, unless other commonly used C++ compilers accept multi-line strings. I've been contemplating various syntactic elements and I think without a trailing '\' the only C++ format that crosses lines is the /* ... */ remark format.

I suppose one could always debate whether the directive highlight should extend beyond the directive. You turned on quote highlighting in there but what about other C++ syntactic elements? For example:

#define MyWonderMacro(WIN) \
WIN *appwin; \
int main(int argc, char **argv) { \
  appwin = new WIN(); \
  appwin->show(argc, argv); \
  return Fl::run(); \
}

OK not very wonderful, but you often see some significant C/C++ code tucked in macros. C++ templates reduce some of that. So... do you highlight the whole as language syntax or paint it in the directive color?

I think my preference would be to limit the directive highlight to the directive itself, and paint the remainder with standard syntax highlights. But others probably have different takes on that.

[jafcobend]

A follow on comment: I suppose my thoughts on directive highlights in FLUID is probably irrelevant since its not likely that anyone would do anything that complex within FLUID. I know beyond initial form/window layout I have to bail out simply because it can't facilitate what I need to do.

[erco77]

• Re: escaped backslash in comments, oh, thought I removed those comments before I committed; will update
• Speaking of apostrophe esses, I think I'm not highlighting single quoted chars. Need to check on that
• Re: directives, sure, I can have it just 'directive highlight' the keyword, and switch to regular syntax highlighting; should be easy.
• Missing quotes should "spill" IMHO as it's legal to allow strings to run multiple lines with trailing \'s even in C
• FLTK has a check on missing quotes/braces when you try to close the code editor window.

RE: your follow up, oh, I've done multiline #define's in fluid, I have some pretty crazy large apps written in fluid that abuse the macro parser, no doubt about it.

The reason I'm jumping on this is I spent a bit of time on fluid adding the external editor option, so I've been in the editor stuff before. The internals of fluid is a whole other thing.. well designed I have to say.

[jafcobend]

No, FLUID didn't highlight character constants using single-quotes. I was going to say something but figured I caused enough trouble just trying to help out. ;-) I'm all for highlighting those.

As far as missing quotes: as long as that is the behavior you want you're golden. I'll have to play with g++ to see why it grumps. Oh! Its probably because I left the trailing backslashes out. :-D Its been a while since I've tried.

I'm glad you could jump on it and felt compelled to do so. I started in that same direction (a rewrite) but simply had to back-burner it and do other things that needed doing. I'm new to FLTK but I like what I've seen and have committed to using it for my GUI dev. I would also agree its well designed but oddly not very OOP oriented. Perhaps for legacy reasons.

[jafcobend]

Ah rats. OK since we've gone this far I gotta bring this up too: If the directive syntax highlight ends at the end of the directive you'll probably want to highlight the < ... > combination too, and that gets a bit trickier since that really only applies to #include. Trying to handle it anywhere else in the editor's content breaks things.

[erco77]

Currently looking at this for a multiline #define with syntax highlighting on the code inside it:

I added string style highlighting for the <stdio.h> part of #include's.

Guess I'll leave out single quote highlighting for now; as you say, fluid didn't have it before, and it'd mean defining a new style type. Could be added easily though.

I'll commit as soon as I give the code a cleanup pass, and some more exercising with different code.

[erco77]

Pushed a few commits to the issue135 branch for my fork at: https://github.com/erco77/fltk

Cleaned up the style_parse() code so it's all pretty and easy to grok.

Has the features described above.. hope I didn't break anything.
I need to exercise more code through it, but it seemed to be pretty good with what I could find lying around and tossing into the code editor. I'll try to give it a careful "Denny's Code Review" where I put some undivided attention just reading the code to look for mistakes places for improvement.

[Albrecht-S]

Great job so far AFAICT (didn't test yet), thank you very much. What I'd be much more interested in though would be improved syntax highlighting in test/editor and in FLTK 1.4 rather than in 1.3 which your current work seems to be based on (or did I miss anything?). test/editor would also be much simpler to test (at least for me).

Do you think that your new code would be portable to 1.4 (fluid) and the test/editor demo? That would be awesome...

[erco77]

@Albrecht-S yes, it'll easily go into 1.4; CodeEditor.cxx didn't change much at all between 1.3.x -> 1.4.x (other than the big whitespace mods).

I'm not sure I can commit this to 1.3.x anyway because it adds two files, StyleParseInfo.{cxx,h}, which would affect the IDE files in 1.3, and I ain't goin there, lol. For 1.4.x it's just a simple mod to the fluid/{Makefile,CMakeLists.txt} to add new files (I think), so cmake would pick that up to autogen the ide files, so that's all good.

Not sure about the test/editor demo; the way I broke it out into separate files might be too "complicated" for a demo. Perhaps the editor demo could be simplified; I didn't look at it, but maybe it could be changed from syntax highlighting C/C++ code to something simpler, as it might be a bit confusing for a beginner looking for inspiration for syntax highlighting with what is almost a full on C code parser. My currently pre-caffeinated mind can't offer an alternative, but there's probably something simpler it could do, like maybe highlight html tags, or maybe a syslog parser that highlights lines with "error:" and "warning:", and we could include a small sample log. I dunno, just thinkin out loud.

[Albrecht-S]

For 1.4.x it's just a simple mod to the fluid/{Makefile,CMakeLists.txt} to add new files (I think)

Yep. And you'd only have to add the .cxx file (although the header could be added as well for IDE's that can show it in their file views).

[erco77]

@albrecht: Question: To apply to 1.4.x, since my fork already has an issue135 branch off of branch-1.3, should I (a) checkout master from my fork, create a new "issue135-1.4" branch, and port to that, then do a pull to merge with FLTK?
Or (b) just create a branch off fltk/fltk master called issue135 and apply to that. Seems like the latter might be "easier".

[erco77]

For now I've done the former; created an issue135-1.4.x branch off master on my fork, and forward ported to that.

So if you want a 1.4 thing to try out/code review, that's the place to look.

[jafcobend]

I'll be test driving the highlighter inside fluid on the 1.3 branch. Sorry I want to work with stable code for my projects. So far its working as expected. Thanks @erco77 for the effort.

[Albrecht-S]

@erco77, @jafcobend:
Since PR #150 has been merged, can we close this issue now?

[erco77]

I should probably additionally merge this into the 1.3.x branch to solve the OP's original problem.

Not sure if I should apply this fix to the text editor demo though; perhaps the editor demo should be /simplified/ so as to just highlight /* and // comments or something, to keep the demonstration of highlighting simple.. it is perhaps a bit of a reach to use a full on C highlighter.

[jafcobend]

@Albrecht-S: @erco77's fix worked for me. I've considered it fixed for a while now. :-)

[Albrecht-S]

I should probably additionally merge this into the 1.3.x branch ...

Or maybe not. This is not a serious bug and we don't intend to fix all bugs in 1.3. Just in case a fix would maybe introduce other bugs we should IMHO not backport this to 1.3.

Not sure if I should apply this fix to the text editor demo though ...

The docs in chapter "Designing a Simple Text Editor" show in paragraph "Syntax Highlighting" much code including the entire style_parse() function. This code has obviously not been updated regularly, there are some parts missing/incomplete/outdated. I'd say, whatever we do, we should synchronize the code and the docs at some time. I'm also not sure if it's useful to add the entire new functionality to the editor demo. Maybe it would be a compromise to

• do only a small fix but not to update the entire code or
• remove the function from the docs and refer to the real code in the source distro and
• fix the syntax highlighting code by adding separate files/functions that can be shared with fluid

or something like that. In the simplest case we'd just document that the documentation is incomplete and that the real code in the source distro should be consulted (to avoid too much code duplication in the docs).

[Albrecht-S]

I'm closing this issue now after the code changes have been fixed (this issue) and I opened a new "documentation" issue (#189).