Opened 4 years ago

Closed 4 years ago

#17705 closed enhancement (fixed)

Upgrade wxSTC to Scintilla 3.7.2

Reported by: NewPagodi Owned by: awi
Priority: normal Milestone:
Component: wxStyledText Version: dev-latest
Keywords: scintilla Cc:
Blocked By: Blocking:
Patch: yes

Description

This patch upgrades the scintilla library used by wxSTC to 3.7.0 (released 16 October 2016). This update adds 8 new methods and changed the signature of 2 others as described below.

Here are the steps that were taken as described in the readme for updating:

1) copy the scintilla files - done
2) examine Scintilla.iface - done (more details in step6)
3) Identify new source files - no new files
4) Examine Platform.h - no changes
5) Adjust the version number in wxStyledTextCtrl::GetLibraryVersionInfo() - done
6a) Edit the gen_iface.py file.

  • The following were changed to match similarly named methods:
    • GetMarginBackN -> GetMarginBackground
    • SetMarginBackN -> SetMarginBackground
  • In addition, there was already a manually declared method SetMargins(int,int) that did a different task. To avoid confusion, I renamed 2 of the new methods as follows:
    • GetMargins -> GetMarginCount
    • SetMargins-> SetMarginCount
  • Also 2 methods had an additional parameter added to their signatures. To prevent any existing code from being broken, I set the second parameter to have a default value of 0.
    • BraceMatch was changed from, 'int BraceMatch(int pos);' to 'int BraceMatch(int pos, int maxReStyle=0);'
    • GetPropertyInt was changed from, 'int GetPropertyInt(const wxString& key) const;' to 'int GetPropertyInt(const wxString &key, int defaultValue=0) const;'

6b) None of the new methods are suitable for commands
7) Run gen_iface.py - done
8) Any other new methods should be checked - none
9) Add documentation code - all new methods have documentation in Scintilla.iface. In addition documentation was added for some methods that previously lacked it. If this patch is approved, I'll redo #17680 to use the documentation from the .iface instead of what I wrote in that patch.
10) Apply the fix UniConversion.h - done
11) Build and test - done.


One of the new methods, SetMouseWheelCaptures, is supposed to change the behavior of the mouse wheel when the cursor is not over the scintilla editor. In the scintilla source, this is implimented on the windows platform with the following code:

case WM_MOUSEWHEEL:
    if (!mouseWheelCaptures) {
        // if the mouse wheel is not captured, test if the mouse
        // pointer is over the editor window and if not, don't
        // handle the message but pass it on.
        RECT rc;
        GetWindowRect(MainHWND(), &rc);
        POINT pt;
        pt.x = GET_X_LPARAM(lParam);
        pt.y = GET_Y_LPARAM(lParam);
        if (!PtInRect(&rc, pt))
            return ::DefWindowProc(MainHWND(), iMessage, wParam, lParam);
    }	}
...

To match this behavior, I added the following to the beginning of OnMouseWheel in stc.cpp.in:

void wxStyledTextCtrl::OnMouseWheel(wxMouseEvent& evt)
{
    if ( !GetMouseWheelCaptures() ) {
        if ( !GetRect().Contains(evt.GetPosition()) ) {
            wxWindow* parent = GetParent();
            if (parent != NULL) {
                wxMouseEvent newevt(evt);
                newevt.SetPosition(
                    parent->ScreenToClient(ClientToScreen(evt.GetPosition())));
                parent->ProcessWindowEvent(newevt);
            }
            return;
        }
    }
...

Since GetMouseWheelCaptures() returns true by default, this code will default to the previous behavior unless a user specifically calls SetMouseWheelCaptures(false). I'm not sure if this is the best way to throw the mouse event to the parent, but it does result in the behavior described in the comment for the windows code. If there is a better way to do this, I'm of course willing to implement that instead.


There is one additional change that should probably be mentioned. This version of the scintilla library replaces the 4gl lexer with an abl one. I'm not familiar with that language, so I can't give any specifics of how the new lexer is different. In the unlikely event that a user has existing code using that lexer, they may have to change the constants from things like 'wxSTC_4GL_NUMBER' to 'wxSTC_ABL_NUMBER' to compile the code with this new version. I don't think there is anything that can be done to keep from breaking code and requiring those changes in this case.

Attachments (4)

stc370.patch download (170.0 KB) - added by NewPagodi 4 years ago.
stc372.patch download (272.1 KB) - added by NewPagodi 4 years ago.
update to scintilla 3.7.2. This supercedes to previous patch
stc372-part2.patch download (129.7 KB) - added by NewPagodi 4 years ago.
winveradd.patch download (1.1 KB) - added by NewPagodi 4 years ago.

Download all attachments as: .zip

Change History (15)

Changed 4 years ago by NewPagodi

comment:1 Changed 4 years ago by NewPagodi

After taking a second look at the new Scintilla.iface, I noticed that the second parameter in StartStyling is no longer used. That parameter was needed when some of the bits in a style byte were used for style information and the rest were used for indicators. The docstring in the iface

# Set the current styling position to start.
# The unused parameter is no longer used and should be set to 0.
fun void StartStyling=2032(position start, int unused)

So I've made an additional edit to gen_iface.py to provide a default value of 0 for the second parameter so that the method declaration is changed from:

void StartStyling(int pos, int mask);

to:

void StartStyling(int start, int unused=0);

This way again no existing code will be broken, but going forward users will not need to provide the extra parameter that does nothing. I've updated the patch accordingly.

comment:2 Changed 4 years ago by NewPagodi

  • Summary changed from Upgrade wxSTC to Scintilla 3.7.0 to Upgrade wxSTC to Scintilla 3.7.2

I'm attaching a new patch to update to scintilla 3.7.2 (30 December 2016)

In addition to everything above, for this new patch: I have done the following:

1) copy the scintilla files - done

2) examine Scintilla.iface - between 3.7.0 and 3.72 the following changes were made

  • 4 new methods
  • 1 new event generated when the user right clicks a margin.
    • I named this event wxEVT_STC_MARGIN_RIGHT_CLICK with underscores between the words. I didn't know if it was better to use this style to match how click events are named in most of the other controls or to name it wxEVT_STC_MARGINRIGHTCLICK to match the already existing event wxEVT_STC_MARGINCLICK. I hope I made the right choice.
    • To generate this event, I added an event handler to for right clicks to wxSTC. I copied the style for the handler from the existing one for left clicks. I based the behavior the handler does on the code in ScintillaWin.cxx for processing right click messages.
    • The scintilla notes for this release say "This only occurs when popup menus are turned off.". Unless I'm completely missing something, that's not correct in the code for the windows port that I based the behavior on, and so it is possible with wxSTC to generate wxEVT_STC_MARGIN_RIGHT_CLICK events even if popup menus are enabled. I could add a check before the event is thrown, but I don't see the harm in having both right click events and popup menus.
  • a new lexer for EDIFACT messages was added
  • the signature of UsePopUp was changed from taking a bool parameter to an int.
    • That shouldn't be a problem with any existing code.
    • I did have to slightly change the ScintillaWX::DoContextMenu(Point pt) method to accomodate the new behavior.
  • 2 new indicator styles
  • 3 (previously deprecated) functions have been removed.

3) Identify new source files

  • lexers\LexEDIFACT.cxx
  • src\SparseVector.h
  • because of the new lexer file, I had to edit \build\bakefiles\scintilla.bkl and so the make files will need to be regenerated.


4) Examine Platform.h

  • the only change is to a pragma used for the qt port


5) Adjust the version number in wxStyledTextCtrl::GetLibraryVersionInfo()

  • done


6a) Edit the gen_iface.py file.

  • none of the new methods required adjustmnets.
  • the scintilla team has removed structures TextToFind,TextRange, and RangeToFormat and replaced them with structurs whose names start with sci_. This this only required making a few search/replaces gen_iface.py (and also in stc.cpp.in) to update code that used the old strucure names.
  • since they have been removed from scintilla, the lines

'GetUsePalette' : (None, 0, 0, 0),
'SetUsePalette' : (None, 0, 0, 0),

were remove from the method override map


6b) None of the new methods are suitable for commands

7) Run gen_iface.py

  • done


8) Any other new methods should be checked

  • none


9) Add documentation code

  • all new methods have documentation in Scintilla.iface.
  • stc.interface.h.in was edited to list the wxEVT_STC_MARGIN_RIGHT_CLICK event.


10) Apply the fix UniConversion.h

  • done


11) Build and test

  • the 4 new methods, the changed method UsePopUp, and the new indicator styles work as expected.
  • the new event is generated with the correct data (although as described above) it can be generated with the scintilla notes suggest it shouldn't be.

comment:3 follow-up: Changed 4 years ago by awi

  • Status changed from new to infoneeded_new

Generally wxSTC seems to work fine after the upgrade, but I have some remarks:

  1. Ref. "because of the new lexer file, I had to edit \build\bakefiles\scintilla.bkl and so the make files will need to be regenerated."

In the previous upgrades also makefile.in, build/msw/make.*, build/msw/wx_vc*_wxscintilla.vcproj, etc. files were explicitly regenerated.
See: 4ad886ca8666124d39b775041184784e4f52b353/git-wxWidgets, c03ce59b86fc3dbb89f56bab62b0b969327d3942/git-wxWidgets

  1. Ref. "the signature of UsePopUp was changed from taking a bool parameter to an int". UsePopUp(bool) exists in wxSTC for years from at least Scintilla 1.45, so it shouldn't be just replaced by the function with new signature. I think both should be implemented, but UsePopUp(bool) should be marked as deprecated. Maybe something like this:
    Declaration:
    wxDEPRECATED_MSG("Don't call UsePopUp(bool)") void wxStyledTextCtrl::UsePopUp(bool allowPopUp);
    
    Definition:
    void wxStyledTextCtrl::UsePopUp(bool allowPopUp)
    {
        SendMsg(SCI_USEPOPUP, allowPopUp ? SC_POPUP_ALL : SC_POPUP_NEVER, 0);
    }
    
    void wxStyledTextCtrl::UsePopUp(int popUpMode)
    {
        SendMsg(SCI_USEPOPUP, popUpMode, 0);
    }
    
    
  1. I couldn't find wxSTC's literals representing UsePopUp modes (like wxSTC_POPUP_NEVER, etc.).
  1. In wxStyledTextCtrl::StartStyling(int start, int unused), "The unused parameter is no longer used and should be set to 0", so maybe in the body of StartStyling there should be checked an assertion unused == 0 to warn the caller that meaning of 2nd has been changed?

comment:4 in reply to: ↑ 3 Changed 4 years ago by NewPagodi

  • Status changed from infoneeded_new to new

Replying to awi:

Generally wxSTC seems to work fine after the upgrade, but I have some remarks:

  1. Ref. "because of the new lexer file, I had to edit \build\bakefiles\scintilla.bkl and so the make files will need to be regenerated."

In the previous upgrades also makefile.in, build/msw/make.*, build/msw/wx_vc*_wxscintilla.vcproj, etc. files were explicitly regenerated.
See: 4ad886ca8666124d39b775041184784e4f52b353/git-wxWidgets, c03ce59b86fc3dbb89f56bab62b0b969327d3942/git-wxWidgets

Aren't all those files generated by bakefile? The directions for submitting patches say not to include generated files, so in the patch I only included the change to the bkl file needed to generate the build files.


  1. Ref. "the signature of UsePopUp was changed from taking a bool parameter to an int". UsePopUp(bool) exists in wxSTC for years from at least Scintilla 1.45, so it shouldn't be just replaced by the function with new signature. I think both should be implemented, but UsePopUp(bool) should be marked as deprecated. Maybe something like this:
    Declaration:
    wxDEPRECATED_MSG("Don't call UsePopUp(bool)") void wxStyledTextCtrl::UsePopUp(bool allowPopUp);
    
    Definition:
    void wxStyledTextCtrl::UsePopUp(bool allowPopUp)
    {
        SendMsg(SCI_USEPOPUP, allowPopUp ? SC_POPUP_ALL : SC_POPUP_NEVER, 0);
    }
    
    void wxStyledTextCtrl::UsePopUp(int popUpMode)
    {
        SendMsg(SCI_USEPOPUP, popUpMode, 0);
    }
    
    

Because of how bools can be implicitly converted to ints in c++, in any existing code which calls 'UsePopUp(false)', the false should be converted to 0 (ie wxSTC_POPUP_NEVER) which is what would be expected. Likewise, in any existing code which calls 'UsePopUp(true)', the true will be coverted to 1 (ie wxSTC_POPUP_ALL) which is also what would be expected.

So any existing code should continue to work as expected even though the type of the parameter of the UsePopUp method has technically changed.


  1. I couldn't find wxSTC's literals representing UsePopUp modes (like wxSTC_POPUP_NEVER, etc.).

As above, I didn't include the generated stc.h and stc.cpp since the directions say not to include generated files. When the new gen_iface.py is run, those constants will be at lines 301-303 in stc.h.


  1. In wxStyledTextCtrl::StartStyling(int start, int unused), "The unused parameter is no longer used and should be set to 0", so maybe in the body of StartStyling there should be checked an assertion unused == 0 to warn the caller that meaning of 2nd has been changed?

I'll add that and update the patch tomorrow.

In the meantime, should I include the generated files? If so, how should I do that - should I have a separate patch for the generated files or include everything in the same patch?

comment:5 follow-up: Changed 4 years ago by awi

  1. Project file for VS 2010+ are not generated automatically so far. I am not sure about policy with regards to legacy project files and makefiles but it seems that in practice they are stored as "ready to use" in the repository.
  1. ASFAICS in the Scintilla documentation, all SC_POPUP_* constants have non-zero values:
    SC_POPUP_NEVER 	0x01 	Never show default editing menu.
    SC_POPUP_ALL 	0x02 	Show default editing menu if clicking on scintilla.
    SC_POPUP_TEXT 	0x04 	Show default editing menu only if clicking on text area.
    

So what would mean e.g. call to UsePopUp(false)?

Another reason for overloading UsePopUp and for deprecating its legacy implementation is to warn the caller that something has changed in API. I faced this problem recently with style bits API and I think that every warning about change is priceless.

  1. All changes in one patch would be fine.

comment:6 in reply to: ↑ 5 Changed 4 years ago by NewPagodi

Replying to awi:

  1. Project file for VS 2010+ are not generated automatically so far. I am not sure about policy with regards to legacy project files and makefiles but it seems that in practice they are stored as "ready to use" in the repository.

I've attached a new patch with the generated make files. For some reason bakefile also generated new wx_vc{7,8,9}_base.vcproj files, but the only difference was adding one header file to the project. I don't know why that happened; I'm pretty sure it's not due to any of the changes I made.

I also manually edited wx_wxscintilla.vcxproj and wx_wxscintilla.vcxproj.filters to add the new lexer file. Is that how that is supposed to be done.


  1. ASFAICS in the Scintilla documentation, all SC_POPUP_* constants have non-zero values:
    SC_POPUP_NEVER 	0x01 	Never show default editing menu.
    SC_POPUP_ALL 	0x02 	Show default editing menu if clicking on scintilla.
    SC_POPUP_TEXT 	0x04 	Show default editing menu only if clicking on text area.
    

So what would mean e.g. call to UsePopUp(false)?

Another reason for overloading UsePopUp and for deprecating its legacy implementation is to warn the caller that something has changed in API. I faced this problem recently with style bits API and I think that every warning about change is priceless.

That's a mistake in the scintilla documentation. In Scintilla.iface, those values are introduced as:

enu PopUp=SC_POPUP_
val SC_POPUP_NEVER=0
val SC_POPUP_ALL=1
val SC_POPUP_TEXT=2

which gets translated in stc.h as:

#define wxSTC_POPUP_NEVER 0
#define wxSTC_POPUP_ALL 1
#define wxSTC_POPUP_TEXT 2

With those definitions, the method remains backwards compatible with all old code, but I've added the version taking the bool parameter as a (deprecated) manually declared method in stc.h.in and defined it stc.cpp.in.


  1. All changes in one patch would be fine.

Apparently including all the generated stuff made the patch over the size limit that trac would accept. So I had to split it into 2 files.

Last edited 4 years ago by NewPagodi (previous) (diff)

Changed 4 years ago by NewPagodi

update to scintilla 3.7.2. This supercedes to previous patch

Changed 4 years ago by NewPagodi

comment:7 Changed 4 years ago by awi

  • Owner set to awi
  • Status changed from new to accepted

It looks fine to me now. No problems with stc sample on MSW and GTK (Debian).
I modified the patch a little bit by adding one new assertion in BraceMatch() and by updating changelog.
I also created PR on GitHub PR409 to trigger automatic builds on Travis and AppVeyor to make sure that everything is fine.

comment:8 Changed 4 years ago by NewPagodi

Thanks. I was looking over the pull request and noticed that bakefile made this change in makefile.vc

@@ -5707,7 +5708,7 @@ __THREADSFLAG = L
 __THREADSFLAG = T
 !endif
 !if "$(USE_RTTI)" == "0"
-__RTTIFLAG = /GR-
+__RTTIFLAG = 
 !endif
 !if "$(USE_RTTI)" == "1"
 __RTTIFLAG = /GR


I'm pretty sure it wasn't supposed to do that. Can I ask that you make 1 more change and put that flag back before committing. Or should I resubmit the patch. Sorry about not catching that before.

comment:9 follow-up: Changed 4 years ago by awi

No problem. I've fixed this makefile and pushed everything again.
I also noticed that in vc_[7, 8, 9]_base.vcproj files, there were added missing references to the winver.h file. This fix itself is fine but it doesn't seem to be related to the Scintilla upgrade, so I move it to the separate commit in this PR. Let me know if you have objections. And shouldn't be also VS2010+ project files (wx_base.vcxproj, wx_base.vcxproj.filters) updated?

comment:10 in reply to: ↑ 9 Changed 4 years ago by NewPagodi

Replying to awi:

No problem. I've fixed this makefile and pushed everything again.
I also noticed that in vc_[7, 8, 9]_base.vcproj files, there were added missing references to the winver.h file. This fix itself is fine but it doesn't seem to be related to the Scintilla upgrade, so I move it to the separate commit in this PR. Let me know if you have objections.

I wasn't sure why that happened, but I think I see now. The files.bkl file was changed about a month ago and I guess I was the first person to try to regenerate the windows make files since then.

And shouldn't be also VS2010+ project files (wx_base.vcxproj, wx_base.vcxproj.filters) updated?

I assume so. I'll add a small patch that should do that. But as you said it's probably best for a separate commit.

Changed 4 years ago by NewPagodi

comment:11 Changed 4 years ago by Artur Wieczorek <artwik@…>

  • Resolution set to fixed
  • Status changed from accepted to closed
Note: See TracTickets for help on using tickets.