Community
    • Login

    sql language style issue with \"

    Scheduled Pinned Locked Moved Help wanted · · · – – – · · ·
    30 Posts 6 Posters 2.7k Views
    Loading More Posts
    • Oldest to Newest
    • Newest to Oldest
    • Most Votes
    Reply
    • Reply as topic
    Log in to reply
    This topic has been deleted. Only users with topic management privileges can see it.
    • Alan KilbornA
      Alan Kilborn
      last edited by 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):

      b2e3598d-a5b7-4f7b-8682-d338167691d4-image.png

      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:

      110929bf-7511-4086-a721-b94e583ab80e-image.png

      PeterJonesP 1 Reply Last reply Reply Quote 1
      • PeterJonesP
        PeterJones @PeterJones
        last edited by 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

        1a103976-f23e-4a42-bd40-fe7d5ab2f5d5-image.png

        vs

        1619716b-043d-4016-bfee-01faec332e34-image.png

        =====

        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…

        1 Reply Last reply Reply Quote 0
        • PeterJonesP
          PeterJones
          last edited by 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 f6ce04cf-fdd4-4a3d-841d-4d2df6d3c667-image.png \' will not end the string
          \ is normal char 6403309f-a980-4b57-a954-5d2987b498c7-image.png \' will end the string

          Steps to Reproduce

          1. escape.sql created per above
          2. Fresh portable unzip of v8.7 (or newer), delete config.xml , and open escape.sql ⇒ Rendering proves it defaults to treating \ as escape
          3. 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
          4. Toggle the option on.
          5. Ctrl+W then Ctrl+Shift+T to reload/re-render the file ⇒ no change in rendering
          6. Toggle the option off
          7. 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.
          8. Exit Notepad++ (it should save the preference state)
          9. Relaunch Notepad++ with the same file ⇒ now it has the right state when launched.
          10. Turn the option back on.
          11. Ctrl+W then Ctrl+Shift+T to reload/re-render the file ⇒ the rendering changes and matches the preference to treat \ as escape
          12. Exit Notepad++ (it should save the preference state)
          13. Relaunch Notepad++ with the same file ⇒ it’s treating \ as escape
          14. 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)

          1. escape.sql created per above
          2. Fresh portable unzip of v8.6.9, delete config.xml , and open escape.sql ⇒ Defaults to treating \ as escape
          3. Check Settings > Preferences > Language > Treat Backslash as escape character for SQL ⇒ it is checkmarked by default ⇒ this is the expected/good behavior
          4. Toggle that option off
          5. Ctrl+W then Ctrl+Shift+T to reload/re-render the file
          6. Now it’s treating \ as normal character
          7. With the option off, exit Notepad++, and restart Notepad++, opening the same file
          8. This time, it renders \ as normal character, and Settings > Preferences > Language > Treat Backslash… is not checkmarked, as expected ⇒ this is the expected/good behavior
          1 Reply Last reply Reply Quote 0
          • PeterJonesP
            PeterJones @Alan Kilborn
            last edited by 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

            1 Reply Last reply Reply Quote 3
            • PeterJonesP
              PeterJones
              last edited by 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.

              mpheathM 1 Reply Last reply Reply Quote 2
              • mpheathM
                mpheath @PeterJones
                last edited by

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

                PeterJonesP 1 Reply Last reply Reply Quote 2
                • PeterJonesP
                  PeterJones @mpheath
                  last edited by 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

                  mpheathM 1 Reply Last reply Reply Quote 1
                  • mpheathM
                    mpheath @PeterJones
                    last edited by 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.

                    PeterJonesP 1 Reply Last reply Reply Quote 1
                    • PeterJonesP
                      PeterJones @mpheath
                      last edited by

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

                      mpheathM 2 Replies Last reply Reply Quote 1
                      • mpheathM
                        mpheath @PeterJones
                        last edited by

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

                        1 Reply Last reply Reply Quote 0
                        • mpheathM
                          mpheath @PeterJones
                          last edited by 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.

                          PeterJonesP 1 Reply Last reply Reply Quote 1
                          • PeterJonesP
                            PeterJones @mpheath
                            last edited by

                            @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

                            mpheathM 1 Reply Last reply Reply Quote 0
                            • mpheathM
                              mpheath @PeterJones
                              last edited by

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

                              PeterJonesP 1 Reply Last reply Reply Quote 0
                              • PeterJonesP
                                PeterJones @mpheath
                                last edited by 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.)

                                mpheathM 1 Reply Last reply Reply Quote 0
                                • mpheathM
                                  mpheath @PeterJones
                                  last edited by 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.

                                  PeterJonesP 1 Reply Last reply Reply Quote 1
                                  • PeterJonesP
                                    PeterJones @mpheath
                                    last edited by 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)

                                    mpheathM 1 Reply Last reply Reply Quote 1
                                    • mpheathM
                                      mpheath @PeterJones
                                      last edited by 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!

                                      1 Reply Last reply Reply Quote 0
                                      • First post
                                        Last post
                                      The Community of users of the Notepad++ text editor.
                                      Powered by NodeBB | Contributors