sql language style issue with \"

Gary Cunningham

@rdipardo This code is PLSQL and is defined under S as sql:

I think that the issue comes only for strings that terminate with a Backslash.

Gary Cunningham

@rdipardo

Case can be easily replicated:

rdipardo

@Gary-Cunningham

Syntax highlighting comes from a third-party library; their bug tracker is at https://github.com/ScintillaOrg/lexilla/issues

mpheath

@Gary-Cunningham

In PLSQL with '\', the \ would be escaping the ' string terminator so the ' character becomes literal and the string styling continues past the ' and terminates at the next instance of unescaped '.

The SQL lexer is informing you that the syntax is not correct based on the styling shown.

Differences in syntax:

	single backslash	double backslash	checkbox state
PLSQL	`'\\'`	`'\\\\'`	checked
SQLite	`'\'`	`'\\'`	unchecked

A backslash in SQLite is not a special character though in PLSQL it is an escape character .

PLSQL: Checkbox checked to style with \ escape syntax. To escape a backslash is to double it.

-- PLSQL syntax

-- ABC\\
select * from tablea
where col1 = 'ABC\\\\';

-- ABC\
select * from tablea
where col1 = 'ABC\\';

-- ABC
select * from tablea
where col1 = 'ABC';

SQLite: Checkbox unchecked as \ is a literal character in a string.

-- SQLite syntax

-- ABC\\
select * from tablea
where col1 = 'ABC\\';

-- ABC\
select * from tablea
where col1 = 'ABC\';

-- ABC
select * from tablea
where col1 = 'ABC';

Gary Cunningham

@mpheath Okay, bad example:

This may be a better one.

I have raised a bug as suggested by @rdipardo
Many thanks for all feedback

PeterJones

I don’t think there’s a bug in Scintilla’s handling of it:

In all these examples, SciTE has sql.backslash.escapes=1 on the left; it has it 0 on the right:

@mpheath’s “SQLite”:
@mpheath’s “PSQL”:
- this isn’t an interesting test, because the example text always has dual slashes, which means that you’d never see a difference between the option being on or off
@Gary-Cunningham’s “better one”:

That proves that in SciTE (and thus Scintilla/Lexilla’s LexSQL code), the option is doing the right thing. Thus, there is no bug to report to the Lexilla project. Since it’s already been reported, I am sure the Lexilla team will promptly reject it as invalid.

In Notepad++, if I toggle the option on the “better one” text, nothing happens! It should be treating them differently. So I thought maybe it required restarting Notepad++ (even though I didn’t think it used to require that): if I set that preference so it’s checkmarked, then exit Notepad++ and restart, the option goes back off. This really looks like a bug in Notepad++'s usage of the LexSQL, not a bug in LexSQL

I brought up PythonScript console, and even though Notepad++'s preferences show Treat backslash as escape character for SQL is off, getting the value of the property shows that Notepad++ has actually told Lexilla to always have that property on.

And if I manually change that property using the plugin, the actual property and lexer do what I expect:

vs

Thus, I am quite confident the bug is in Notepad++: it is not honoring its preference, and is instead always just setting the LexSQL property sql.backslash.escapes=1 regardless of the property.

addenda: here is the text version of the “better one” , for easy copy/paste

select 'ABCDE' from dual where 1=1;
select 'AB\CDE' from dual where 1=1;
select 'ABCDE\' from dual where 1=1;
select 'ABCDE' from dual where 1=1;
select 'AB\CDE' from dual where 1=1;

Alan Kilborn

I’m not sure about all the ins-and-outs of the SQL backslash stuff (as I don’t have a need), but I do notice that, when the setting in the Preferences is changed, if you’ve currently got a SQL tab selected, you have to switch to a different tab, then switch back to see something change. This is definitely a problem in the N++ logic.

EDIT:

Here’s what I did:

using Notepad++ 8.7.5
copied the text at the end of Peter’s POST into a new N++ tab
changed the “language” on the tab to SQL using the N++ status bar
opened Preferences and saw Treat backslash… was currently uncheckmarked

at this point I glanced back at the sql tab and saw this in N++ (which was unchanged from the moment I changed the language to sql):

Then I:

checkmarked the Treat backslash… checkbox
noticed no change in the contents of the active tab in N++ (still the sql tab)
made the active tab a different tab
made the sql tab active again

at this point I observed:

PeterJones

Update:

I looked at the Changes, and it showed v8.5.1 as the most-recent change to SQL, so I tried the following experiment:

If I go back to v8.5.0, when I toggle the option nothing happens, but if I restart, it keeps the state of the option, and the

vs

=====

If I swap to v8.5.1, it works the same. So it wasn’t the change in v8.5.1 that did it. :-(

So trying to search for the version that changed the behavior:

version	behavior
8.7.7	Ignores preference, always treats `\` as escape
8.5	Honors preference after restart
8.5.1	Honors preference after restart
8.6	Honors preference after restart
8.7	Ignores preference, always treats `\` as escape
8.6.5	Honors preference after restart
8.6.6	Honors preference after restart
8.6.7	Honors preference after restart
8.6.8	Honors preference after restart
8.6.9	Honors preference after restart

At this point, @Alan-Kilborn posted, so I retried v8.7 with toggling to license.txt and back to my SQL: For v8.7, if I switch to another tab and back, it does start following the preference for that instance. If I have the option off, so the “better one” is rendered as @Gary-Cunningham wants it, and restart Notepad++, it continues to hold the preference; however, if i turn the option on, then exit, when I restart Notepad++, it does not show the preference being on, but it treats it as if it is: I then have to toggle on the option and back off, then switch to a new tab then back to the SQL, for it to treat the option as if it’s off, like it says.

Going back to v8.6.9, if I toggle to a new 1 tab and back between toggling the preference, I no longer need to restart Notepad++, but it’s annoying to have to toggle to a different tab (ie, force a re-render).

But regardless of the toggle-to-different-tab or not, v8.6.9 annd v8.7.0 work differently when you first run Notepad++, depending on the state of that preference. And v8.7.0 was when the Indentation preferences were split off to a different page from the Langauge preferences, so it may have been something to do with that split…

PeterJones

For the main issue (escaping), I believe I have some Steps to Reproduce. Before putting in a bug report, I’d like confirmation that I’ve got the StR correct:

Create the file escape.sql

select 'ABCDE' from dual where 1=1;
select 'AB\CDE' from dual where 1=1;
select 'ABCDE\' from dual where 1=1;
select 'ABCDE' from dual where 1=1;
select 'AB\CDE' from dual where 1=1;

The expectation is that

state	screenshot	desc
`\` is escape char		`\'` will not end the string
`\` is normal char		`\'` will end the string

Steps to Reproduce

escape.sql created per above
Fresh portable unzip of v8.7 (or newer), delete config.xml , and open escape.sql ⇒ Rendering proves it defaults to treating \ as escape
Check Settings > Preferences > Language > Treat Backslash as escape character for SQL ⇒ is not checkmarked, even though the behavior obviously shows it is treating it as escape ⇒ this is the BUG
Toggle the option on.
Ctrl+W then Ctrl+Shift+T to reload/re-render the file ⇒ no change in rendering
Toggle the option off
Ctrl+W then Ctrl+Shift+T to reload/re-render the file ⇒ now the rendering changes and matches the preference, so it is now treating \ as normal character, like it should.
Exit Notepad++ (it should save the preference state)
Relaunch Notepad++ with the same file ⇒ now it has the right state when launched.
Turn the option back on.
Ctrl+W then Ctrl+Shift+T to reload/re-render the file ⇒ the rendering changes and matches the preference to treat \ as escape
Exit Notepad++ (it should save the preference state)
Relaunch Notepad++ with the same file ⇒ it’s treating \ as escape
Check Settings > Preferences > Language > Treat Backslash… and notice that like in step 2, it’s showing as unchecked, even though the preference state was saved as checked, and it’s behaving as if it’s checked

Expected Behavior (what worked in an older version)

escape.sql created per above
Fresh portable unzip of v8.6.9, delete config.xml , and open escape.sql ⇒ Defaults to treating \ as escape
Check Settings > Preferences > Language > Treat Backslash as escape character for SQL ⇒ it is checkmarked by default ⇒ this is the expected/good behavior
Toggle that option off
Ctrl+W then Ctrl+Shift+T to reload/re-render the file
Now it’s treating \ as normal character
With the option off, exit Notepad++, and restart Notepad++, opening the same file
This time, it renders \ as normal character, and Settings > Preferences > Language > Treat Backslash… is not checkmarked, as expected ⇒ this is the expected/good behavior

PeterJones

@Alan-Kilborn said in sql language style issue with \":

using Notepad++ 8.7.5

Confirmed: with v8.7.5 and earlier, changing tab is sufficient. But when I try with v8.7.6, v8.7.7, or v8.7.8-RC2, I have to close the tab and re-open it (the Ctrl+W, Ctrl+Shift+T from my StR) in order to get it to re-render: apparently, the Syntax Highlighter “efficiency” improvements made it harder to get the lexer to see the new state.

I’m going to create the issue for this preference highlighting-refresh first. Then I will later (hopefully after someone confirms my StR) create the issue for the preference not showing the stored state correctly in versions newer than v8.6.9

update: created issue #16244 for the preference refresh

PeterJones

Since no one tried my StR, but they show the problem 100% of the time for me, I put in Issue #16249 for the preference checkmark not being remembered after restart, and having the rendering not being the same as what the preference shows.

mpheath

@PeterJones Definitely a problem I see with the checkbox initial state and the set of the property.

The checkbox initial state needs 1 line of code to make it work. Seems overlooked as to why the code to do this does not currently exist.

diff --git a/PowerEditor/src/WinControls/Preference/preferenceDlg.cpp b/PowerEditor/src/WinControls/Preference/preferenceDlg.cpp
index 13409f709..a1cb7e7f6 100644
--- a/PowerEditor/src/WinControls/Preference/preferenceDlg.cpp
+++ b/PowerEditor/src/WinControls/Preference/preferenceDlg.cpp
@@ -3790,6 +3790,8 @@ intptr_t CALLBACK LanguageSubDlg::run_dlgProc(UINT message, WPARAM wParam, LPARA
 			::EnableWindow(::GetDlgItem(_hSelf, IDC_BUTTON_REMOVE), FALSE);
 			::EnableWindow(::GetDlgItem(_hSelf, IDC_BUTTON_RESTORE), FALSE);
 
+			::SendDlgItemMessage(_hSelf, IDC_CHECK_BACKSLASHISESCAPECHARACTERFORSQL, BM_SETCHECK, nppGUI._backSlashIsEscapeCharacterForSql ? TRUE : FALSE, 0);
+
 			return TRUE;
 		}

The property setting change of sql.backslash.escapes internally is ignored as PythonScript editor.getProperty('sql.backslash.escapes') shows no change in value. It’s like Lexilla is not receiving from the internally called SCI_SETPROPERTY to change the value. Yet, PythonScript can change the property so is a mystery to why the internal call is being ignored. setSqlLexer() is called on buffer change and _backSlashIsEscapeCharacterForSql value changes so do not know yet what is causing the failure to apply the property. I have been busy to find the cause and have little to show for it.

PeterJones

@mpheath said in sql language style issue with \":

Seems overlooked as to why the code to do this does not currently exist.

Ahh, I think I found it: essentially that line currently exists in IndentationSubDlg::run_dlgProc()'s WM_INITDIALOG rather than in the
LanguageSubDlg::run_dlgProc()'s WM_INITDIALOG where you put it.

When Indentation preferences was split off from the Language preferences in 439bbb0, it accidentally included the IDC_CHECK_BACKSLASHISESCAPECHARACTERFORSQL.

—

Update: submitted PR #16253 to move that line

mpheath

@PeterJones

Nice fix on initial checkbox state.

By process of elimination, may need to create branches with some checkouts of before and after features were added, compile and test. That should narrow it down.

v8.7.5 release tests OK with changing the property.

1st commit after v8.7.5 release compiled shows the buffer change problem with the property.

Enhance large files with syntax highlighting performance

apparently, the Syntax Highlighter “efficiency” improvements made it harder to get the lexer to see the new state.

Appears to be a correct. “efficiency” and “optimization” is terms that makes me go hmm with these large file improvements.

PeterJones

@mpheath said in sql language style issue with \":

1st commit after v8.7.5 release compiled shows the buffer change problem with the property.

Wow, didn’t take 'em long to make the refresh worse after that version.

I was really surprised earlier, when single-stepping through the code, that even though the SETPROPERTY message is sent, Lexilla doesn’t refresh… I am wondering whether it has something to do with all the SETMODEEVENTMASK calls that were added for “efficiency”.

However, even if it were to go back to v8.7.5 behavior for toggling the preference, is that sufficient? Because even ver<=v8.7.5 required toggling to a different tab and back – ie, forcing a manual refresh of the scintilla display; and really, I don’t think a manual refresh should be necessary.

Is there a way to force Scintilla to redraw on command? What I’m thinking is that if there were a way, we could send that SCI message immediately after updating the preference and/or closing the dialog. I might see if I can play around with that tomorrow. (it wouldn’t be a degredation in “efficiency” at that point, since it would only be on-demand when the preference was toggled) – but being able to see it update as soon as the preference is toggled would really improve the usability of that preference.

mpheath

@PeterJones said in sql language style issue with \":

@mpheath said in sql language style issue with \":

1st commit after v8.7.5 release compiled shows the buffer change problem with the property.

Wow, didn’t take 'em long to make the refresh worse after that version.

I was really surprised earlier, when single-stepping through the code, that even though the SETPROPERTY message is sent, Lexilla doesn’t refresh… I am wondering whether it has something to do with all the SETMODEEVENTMASK calls that were added for “efficiency”.

In the latest master I tried SC_MODEVENTMASKALL and noticed no difference with the property. I would need to try it with the 1st commit after v8.7.5 release to confirm.

However, even if it were to go back to v8.7.5 behavior for toggling the preference, is that sufficient? Because even ver<=v8.7.5 required toggling to a different tab and back – ie, forcing a manual refresh of the scintilla display; and really, I don’t think a manual refresh should be necessary.

The preferences dialog just sets values to variables. There is the caret line background that is instant so perhaps the property could be instant too. If it could execute SET_PROPERTY in the dialog then it would be as good as PythonScript editor.setPropertry() instant behaviour, so if possible, sounds good if is acceptable by the maintainer.

Is there a way to force Scintilla to redraw on command? What I’m thinking is that if there were a way, we could send that SCI message immediately after updating the preference and/or closing the dialog. I might see if I can play around with that tomorrow. (it wouldn’t be a degredation in “efficiency” at that point, since it would only be on-demand when the preference was toggled) – but being able to see it update as soon as the preference is toggled would really improve the usability of that preference.

The variable changes in the preferences dialog. When the tab changes, that event causes the setSqlLexer() function to be called so the variable value is used there to call SET_PROPERTY. The later call to SET_PROPERTY needs to occur in the preferences dialog to get instant change that you seem to be asking for. The requested refresh is done calling SET_PROPERTY same as editor.setProperty(). AFAIK, no extra magic SCI_REFRESH is needed.

mpheath

@PeterJones

It is the temporary blank document being used on change of buffer then reassigned the actual document that seems to be causing the issue with the property not changing.

https://github.com/notepad-plus-plus/notepad-plus-plus/blob/829cd9d11904a307230be92afb4023baa1e2606a/PowerEditor/src/ScintillaComponent/ScintillaEditView.cpp#L2379

		execute(SCI_SETDOCPOINTER, 0, getBlankDocument());

@xomx mentioned an issue with blank-document temporary substitute in a PR #16245

If you comment out most of the code block

https://github.com/notepad-plus-plus/notepad-plus-plus/blob/829cd9d11904a307230be92afb4023baa1e2606a/PowerEditor/src/ScintillaComponent/ScintillaEditView.cpp#L2351-L2387

and just leave uncommented

https://github.com/notepad-plus-plus/notepad-plus-plus/blob/829cd9d11904a307230be92afb4023baa1e2606a/PowerEditor/src/ScintillaComponent/ScintillaEditView.cpp#L2356-L2362

		execute(SCI_SETMODEVENTMASK, MODEVENTMASK_OFF);
		execute(SCI_SETDOCPOINTER, 0, _currentBuffer->getDocument());
		execute(SCI_SETMODEVENTMASK, MODEVENTMASK_ON);

		// Due to execute(SCI_CLEARDOCUMENTSTYLE); in defineDocType() function
		// defineDocType() function should be called here, but not be after the fold info loop
		defineDocType(_currentBuffer->getLangType());

Compile and then < v8.7.5 like behavior is restored and the sql.backslash.escapes property can be changed.

Edit: The idea of setting the property directly from the preferences dialog might get around this blank document substitute issue as the actual document is loaded at that time.

Though it is not at buffer change time so may fail from the preferences dialog. May need testing to make sure if the idea might work.

PeterJones

@mpheath said in sql language style issue with \":

Compile and then < v8.7.5 like behavior is restored and the sql.backslash.escapes property can be changed.

Yeah, I’m not sure what other issues would be introduced by getting rid of all of those logics and treating everything as if isFirstActiveBuffer.

And, even if it did, the <=v8.7.5 behavior is still not ideal, because it still requires switching to a different buffer and back to see that the preference has changed – that’s better than having to close the file, but it’s still not a good user experience.

Edit: The idea of setting the property directly from the preferences dialog might get around this blank document substitute issue as the actual document is loaded at that time.

Yes, that is what I’m starting to explore at this point

mpheath

@PeterJones

Changing the property in preferences would probably be like how with PythonScript needs to use a callback. Changing buffers in Notepad++ use of callback is currently broken with the empty document substitute. So another sql document probably would not get the property change. This is why in the last edit I mentioned about possible failure in the idea of directly using preferences to change the property.

PeterJones

@mpheath ,

Hmm… so a roadblock, probably due to my unfamiliarity with the source code…

I think I need access to the object instance for the active editor. But as far as I can tell, the preferencesDlg.cpp doesn’t have access to that, or to any other object that I can find that would allow me to get the current editor as a property/method. I actually need that to both find out what the current lexer is (because, obviously, we don’t need to change the SQL-specific attribute if SQL isn’t the language) and to be able to have access to the execute() method to actually set that.

So far, I haven’t found examples of anything in the preferencesDlg.cpp that needs that or similar object, so I cannot see a way it’s already been done – and I doubt Don would like it if I started bringing in new headers/objects willy-nilly, especially since I don’t understand all the implications of such.

I’ll keep looking on my own, but if anyone here knows some other action in the preferences dialog that needs to interact with the active editor object, I’d appreciate it being pointed out.

update: or is the expectation that things would be done through messages? I see that the DarkMode preferences send messages, so maybe I’ll try something similar for this one…
update2: … oh, but while messages can get me the langType of the current buffer, it still doesn’t give me access to the hwnd for the buffer’s scintilla component, so I still cannot set the property from in there. :-( I may have bitten off more than I can chew with trying to go down this route. (I had originally assumed that the active editor object was something global enough that I could get it from the checkbox-changed processing.)