sql language style issue with \"

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.)

mpheath

@PeterJones

Based on 8.7.8 release master

diff --git a/PowerEditor/src/NppBigSwitch.cpp b/PowerEditor/src/NppBigSwitch.cpp
index 19b26bda2..77d1bcc3b 100644
--- a/PowerEditor/src/NppBigSwitch.cpp
+++ b/PowerEditor/src/NppBigSwitch.cpp
@@ -4144,6 +4144,12 @@ LRESULT Notepad_plus::process(HWND hwnd, UINT message, WPARAM wParam, LPARAM lPa
 			return TRUE;
 		}
 
+		case NPPM_INTERNAL_SQLBACKSLASHESCAPE:
+		{
+			const bool kbBackSlash = NppParameters::getInstance().getNppGUI()._backSlashIsEscapeCharacterForSql;
+			_pEditView->execute(SCI_SETPROPERTY, reinterpret_cast<WPARAM>("sql.backslash.escapes"), reinterpret_cast<LPARAM>(kbBackSlash ? "1" : "0"));
+		}
+
 		default:
 		{
 			if (message == WDN_NOTIFY)
diff --git a/PowerEditor/src/WinControls/Preference/preferenceDlg.cpp b/PowerEditor/src/WinControls/Preference/preferenceDlg.cpp
index dbf20b4b1..cbdba9381 100644
--- a/PowerEditor/src/WinControls/Preference/preferenceDlg.cpp
+++ b/PowerEditor/src/WinControls/Preference/preferenceDlg.cpp
@@ -3900,6 +3900,8 @@ intptr_t CALLBACK LanguageSubDlg::run_dlgProc(UINT message, WPARAM wParam, LPARA
 				case IDC_CHECK_BACKSLASHISESCAPECHARACTERFORSQL:
 				{
 					nppGUI._backSlashIsEscapeCharacterForSql = isCheckedOrNot(IDC_CHECK_BACKSLASHISESCAPECHARACTERFORSQL);
+					HWND grandParent = ::GetParent(_hParent);
+					::SendMessage(grandParent, NPPM_INTERNAL_SQLBACKSLASHESCAPE, 0, 0);
 					return TRUE;
 				}
 
diff --git a/PowerEditor/src/resource.h b/PowerEditor/src/resource.h
index 842a04afb..71655deae 100644
--- a/PowerEditor/src/resource.h
+++ b/PowerEditor/src/resource.h
@@ -747,6 +747,7 @@
 	#define NPPM_INTERNAL_CHECKDOCSTATUS                (NOTEPADPLUS_USER_INTERNAL + 106)
 	#define NPPM_INTERNAL_HIDEMENURIGHTSHORTCUTS        (NOTEPADPLUS_USER_INTERNAL + 107)
 	#define NPPM_INTERNAL_RELOADFUNCTIONLIST            (NOTEPADPLUS_USER_INTERNAL + 108)
+	#define NPPM_INTERNAL_SQLBACKSLASHESCAPE            (NOTEPADPLUS_USER_INTERNAL + 109)

It is not an instant visual change in the current buffer. It changes once the preferences dialog closes. Changing buffer to another buffer can show another SQL document with opposite backslash escape setting as the blank document is still an issue with the property change being sent to the blank document. So while it does not fix the core issue, the preferences dialog can be opened again and check the checkbox again to send the message to apply to the current buffer that needs the change of property.

PeterJones

@mpheath ,

Thanks for that idea. Merging your idea, with my idea of checking of whether or not a given buffer is SQL before applying the property, I am able to loop through all buffers, and for any that are SQL, apply the updated property.

When I run it, it seems to work live (when the checkbox is toggled) when I have one ore more SQL files in one or both views (or with files only in one view)

If you could test the appropriate artifact from this build, and make sure that it’s consistent, and not just “working” for me, I’d appreciate it.

(Future readers: the artifacts for a given build aren’t around for more than 30 days, due to github retention policy; don’t bother trying to follow this link in the future)

mpheath

@PeterJones

I just tried the Notepad++.MSVC.x64.Release artifact and am impressed. It changes directly in the editor pane with checking and unchecking the checkbox. I tried cloned view and works good with both panes. I see NPPM_ACTIVATEDOC, that I presume it is helping to refresh the property change live.

I have tested multiple Notepad++ restarts, checkbox changes and buffer changes. Tests are so good, that it should IMO fix and close the issue ticket #16244.

Well done!