Re: Scintilla 3.21.1 (#1331)
VZ
AFAICS we should be able to use this version of Scintilla with C++14 only, but latest versions require C++17. We need to decide if support for the latest version is important enough for us and, if so, whether we can use 2 Scintilla versions in parallel and select the one compatible with the current compiler, or just require C++17 compiler for building wx itself too.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
The Scintilla long-term branch doesn't seem to get updates anymore. So If we want to include a more recent version, it requires c++17. But seeing how Scintilla adopts new versions, this might become c++20 before wx3.3.
A while back I looked at having two versions in parallel, and I had it working with CMake. But with bakefile or MSVC projects it was not possible (for me), so I never finished it.
I had the current version and the one from this PR in parallel, and based on CMAKE_CXX_STANDARD, or an explicit option, a version was selected.
The same can be done in wx3.3 to choose between this version (or the last version before c++17 was required), and a version with the latest release (c++17 or maybe even c++20).
Another options is to require c++14 for wxWidgets, but require c++17 when wxUSE_STC is enabled.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
A while back I looked at having two versions in parallel, and I had it working with CMake. But with bakefile or MSVC projects it was not possible (for me), so I never finished it. I had the current version and the one from this PR in parallel, and based on
CMAKE_CXX_STANDARD, or an explicit option, a version was selected. The same can be done in wx3.3 to choose between this version (or the last version before c++17 was required), and a version with the latest release (c++17 or maybe even c++20).
We probably should do this, I should be able to implement the same thing with configure. For MSVS projects we just have to make the Scintilla version conditional on MSVS version, so it should be doable too. And we won't be using bakefile anyhow soon -- even though it's not clear what will replace it.
Another options is to require c++14 for wxWidgets, but require c++17 when
wxUSE_STCis enabled.
If we keep makefile.gcc, we ought to be able to add some STC_VERSION option and tell people to set it to 4 or 5 depending on whether they use new enough compiler. Or we could simply rely on people using makefile.gcc using recent enough gcc because this seems to be the case in practice: old gcc are only used on old (typically RHEL) Linux systems.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent pushed 10 commits.
- ba60834 Update scintilla to 3.21.1
- 094010e Add new scintilla sources to build systems
- 79ff9e1 Update scintilla version, remove patch instructions
- add4238 Update and run wxSTC python scripts
- 2c28ead Fix compiling wxSTC
- aa6c0d4 Use native scintilla selection event
- c35b2df Use all modifiers with mouse events
- a0898dc Add GradientRectangle implementations
- 585952a Exclude Scintilla from whitespace check
- 316cf3b Fix build warnings in Scintilla code
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent pushed 2 commits.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent pushed 2 commits.
You are receiving this because you are subscribed to this thread.
Maarten
This can be merged now.
I'll start investigating updating to 4 and/or 5.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Great, thanks!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz approved this pull request.
Thanks for all your work on this, I obviously didn't really look at all the changes, but what I saw looks good and, in any case, I think we need to merge it to get some testing as any problems almost surely won't be discovered by just reading the code. Of course, if more people can test this before the merge, it would be very great too, but otherwise I'll merge it -- unless you decide to change the explicit Scintilla additions as I think this adds a lot of "noise" to the patch and might be unnecessary.
I also have another question: should we move src/stc/scintilla contents in a submodule? I think it might make it easier to update it in the future and it definitely makes it simpler to avoid making global changes to the files there.
Thanks again!
In build/bakefiles/scintilla.bkl:
> @@ -55,6 +54,7 @@
<define>$(WXUNIV_DEFINE)</define>
<define>$(DEBUG_DEFINE)</define>
<define>$(UNICODE_DEFINE)</define>
+ <define>LPEG_LEXER=0</define>
Out of curiosity, why do we disable this one?
In build/osx/wxiphone.xcodeproj/xcshareddata/xcschemes/static.xcscheme:
> @@ -1,6 +1,6 @@ <?xml version="1.0" encoding="UTF-8"?> <Scheme - LastUpgradeVersion = "0940" + LastUpgradeVersion = "0900"
Is this intentional? Important? @csomor It it ok to merge this?
> @@ -3459,86 +3450,44 @@ DynamicLibrary *DynamicLibrary::Load(const char *modulePath) {
//----------------------------------------------------------------------
-ColourDesired Platform::Chrome() {
+ColourDesired Scintilla::Platform::Chrome() {
Could we perhaps add a using declaration to avoid inserting the explicit Scintilla:: scope in many places? It seems to be already done elsewhere...
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent commented on this pull request.
In build/osx/wxiphone.xcodeproj/xcshareddata/xcschemes/static.xcscheme:
> @@ -1,6 +1,6 @@ <?xml version="1.0" encoding="UTF-8"?> <Scheme - LastUpgradeVersion = "0940" + LastUpgradeVersion = "0900"
I just ran the python script. Maybe different Xcode version? Mine is 13.2.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent commented on this pull request.
> @@ -3459,86 +3450,44 @@ DynamicLibrary *DynamicLibrary::Load(const char *modulePath) {
//----------------------------------------------------------------------
-ColourDesired Platform::Chrome() {
+ColourDesired Scintilla::Platform::Chrome() {
I'll see if I can add that.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent commented on this pull request.
In build/bakefiles/scintilla.bkl:
> @@ -55,6 +54,7 @@
<define>$(WXUNIV_DEFINE)</define>
<define>$(DEBUG_DEFINE)</define>
<define>$(UNICODE_DEFINE)</define>
+ <define>LPEG_LEXER=0</define>
When enabled, it needs external lua files that are not part of the scintilla package:
https://github.com/wxWidgets/wxWidgets/blob/06dbd32194b2978b6162d61cbe6237eea7b1b661/src/stc/scintilla/lexers/LexLPeg.cxx#L39-L44
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
A submodule would be nice and should eventually be used, but it has some problems. I don't think there is any up-to-date git upstream we can use. And with Scintilla 5, the project has split into two parts: Scintilla (using mercurial on sourceforge) and Lexilla on github.
And then we have to decide if and how we can support multiple versions of Scintilla for different c++ versions. Put them all in one submodule in different folders, or different submodules for each.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent commented on this pull request.
In build/osx/wxiphone.xcodeproj/xcshareddata/xcschemes/static.xcscheme:
> @@ -1,6 +1,6 @@ <?xml version="1.0" encoding="UTF-8"?> <Scheme - LastUpgradeVersion = "0940" + LastUpgradeVersion = "0900"
It also changed file order and many identifiers in the pbxproj files. I don't know if this is expected or if it is possible to only add/remove the changed scintilla files.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Paul Kulchenko
@pkulchenko commented on this pull request.
In build/bakefiles/scintilla.bkl:
> @@ -55,6 +54,7 @@
<define>$(WXUNIV_DEFINE)</define>
<define>$(DEBUG_DEFINE)</define>
<define>$(UNICODE_DEFINE)</define>
+ <define>LPEG_LEXER=0</define>
FYI, lpeg lexer can be built and loaded separately, so no functionality is really lost if it's not included by default (although it's not going to be statically linked). It uses LoadLexerLibrary and it does require a bit of a setup (for example, lexer.lpeg.home property may need to be set). I've been using this lexer/approach for many years in my wxiwdgets/wxlua-based project and can provide some additional details if needed.
Another option is to bundle those Lua files, but unfortunately they are (Lua) version dependent and will load a Lua library at run-time, so not going to work without it being present anyway.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent pushed 1 commit.
- 6cb37f4 Add 'using namespace Scintilla'
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent pushed 11 commits.
- 6c3b0f6 Update scintilla to 3.21.1
- 2af3719 Add new scintilla sources to build systems
- 27ddafc Update scintilla version, remove patch instructions
- 3cb8d55 Update and run wxSTC python scripts
- 50449f9 Fix compiling wxSTC
- 447fff9 Use native scintilla selection event
- e2133aa Use all modifiers with mouse events
- 8038860 Add GradientRectangle implementations
- 1322c54 Exclude Scintilla from whitespace check
- 63e66a5 Fix build warnings and errors in Scintilla code
- a4d0a75 Regenerate Xcode projects
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent pushed 9 commits.
- 314fe7e Update and run wxSTC python scripts
- f02a430 Fix compiling wxSTC
- 5d0d599 Use native scintilla selection event
- 7b823ac Use all modifiers with mouse events
- 530275b Add GradientRectangle implementations
- 289cd2e Exclude Scintilla from whitespace check
- fb6d816 Fix build warnings and errors in Scintilla code
- 875e127 Regenerate Xcode projects
- 75d698f Fix NULL usage
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent pushed 1 commit.
- 619071c Fix NULL usage
You are receiving this because you are subscribed to this thread.
VZ
Thanks again for your work on this, I'm merging and pushing it and we can discuss switching submodules later if you think it's worth it.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Merged #1331 into master.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.