Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Notes for updating N++'s Scintilla beyond 5.x #10504

Closed
ArkadiuszMichalski opened this issue Sep 4, 2021 · 25 comments
Closed

Notes for updating N++'s Scintilla beyond 5.x #10504

ArkadiuszMichalski opened this issue Sep 4, 2021 · 25 comments
Assignees
Labels
scintilla dependent Can't be considered for N++ implementation unless/until Scintilla changes

Comments

@ArkadiuszMichalski
Copy link
Contributor

ArkadiuszMichalski commented Sep 4, 2021

Notepad++ 8.1.4 (current version as of this writing) uses Scintilla version 4.4.6. Starting from Scintilla 5.0.0 we have:

First version that separates Lexilla from Scintilla. Each of the 3 projects now has a separate history page but history before 5.0.0 remains combined.

https://github.com/ScintillaOrg/lexilla
https://www.scintilla.org/Lexilla.html
https://www.scintilla.org/LexillaDoc.html
https://www.scintilla.org/ScintillaDoc.html#alpha

Issues fixed after the update:
#10925
#10651
#10908
#10706
#10065
#10423
#3858
#9229
#8359
#8087
#63
#5264
#5805, #5750

Issues that can be fixed after the update:
#11413
#10171
#10991
#11014
#10301
#1260

Even some early version (Notepad++ with new Scintilla/Lexilla) for testing would be advisable to catch possible problems.

@ivan-u7n
Copy link
Contributor

ivan-u7n commented Sep 4, 2021

Notepad++ 8.4 (current version as of this writing) uses Scintilla version 4.4.6. Starting from Scintilla 5.0.0 we have:

A time traveller! :) In my time it's still 8.1.4.

Even some early version (Notepad++ with new Scintilla/Lexilla) for testing would be advisable to catch possible problems.

I wanted to wait for a next release of Lexilla (which will include my latest patch) and then to try the update. However I have no idea whether it will succeed, for I'm not that good at GUI coding.

@ozone10
Copy link
Contributor

ozone10 commented Sep 4, 2021

This will also allow autocomplete to be dark (#10171)

@dail8859
Copy link
Contributor

dail8859 commented Sep 7, 2021

In one of my projects I moved from Scintilla v4.x to v5.x and found that overall no major changes are needed, but alot of code will need touched.

Setting lexers is different now. You have to use Lexilla to instantiate the lexer by name (not by a SCI_xxx constant). Not sure if the names in the Notepad++ config files have the proper lexer names or not...I think they are close if not completely accurate.

The interface to Scintilla has changed (not sure if the old interface is still available in Win32). From the release notes for v5.0.3:

A more type-safe binding of Scintilla's API that can be used from C++ is implemented in the ScintillaTypes.h, ScintillaMessages.h, and ScintillaStructures.h headers.

And v5.1:

Add more type-safe wrappers to the API. These are defined in include/ScintillaCall.h and implemented in call/ScintillaCall.cxx. ScintillaCall throws Scintilla::Failure exceptions when a call fails.

This adds a strongly typed interface which makes the code safer and more readable, but potentially a bit more verbose (not always a bad thing).

My biggest concerns for Notepad++ would be plugin compatibility and external lexers.

@Kered13
Copy link
Contributor

Kered13 commented Oct 16, 2021

The interface to Scintilla has changed (not sure if the old interface is still available in Win32).

It is:
https://sourceforge.net/p/scintilla/code/ci/default/tree/include/Scintilla.h
https://sourceforge.net/p/scintilla/code/ci/default/tree/win32/ScintillaWin.cxx

This means that plugins should mostly still be compatible.

@ArkadiuszMichalski ArkadiuszMichalski added the scintilla dependent Can't be considered for N++ implementation unless/until Scintilla changes label Oct 20, 2021
@ArkadiuszMichalski
Copy link
Contributor Author

@donho Any approximate time when it will be updated?

@donho donho self-assigned this Nov 3, 2021
@donho
Copy link
Member

donho commented Nov 3, 2021

@ArkadiuszMichalski
I cannot estimate the approximate time, but it's for sure that updating Scintilla to v5.x will be in v8.2 (next release) if there's no critical regression/bug in v8.1.9 (after auto-update being triggered for v8.1.9).

@alankilborn
Copy link
Contributor

alankilborn commented Jan 3, 2022

@donho said:

but it's for sure that updating Scintilla to v5.x will be in v8.2

I must have missed the change.log entry for this. :-(

A long-time user (me!) would like to see a N++ release that integrates Scintilla 5.1.2 or later. :-)

@chcg
Copy link
Contributor

chcg commented Jan 6, 2022

@donho Do you have an updated timeframe for the move to scintilla 5?
I created a test branch for the integration at https://github.com/chcg/notepad-plus-plus/tree/scintilla5 with the current versions

Testbinaries could be found at https://ci.appveyor.com/project/chcg/notepad-plus-plus/builds/42099721.

Seems it is mostly working.
Known issues:

  • More rework is necessary for usage of CreateLexer with lexer names without deprecated ID interface
  • Some code is still commented out as there seems to be no direct replacement for SCI_LOADLEXERLIBRARY.
  • Searchresult window seems to have a problem with highlighting the search phrase.
  • makefile changes needs to be checked
  • ...

@ArkadiuszMichalski
Copy link
Contributor Author

Maybe Lexilla itself could be updated more often, because now we have to wait a long time for small corrections in lexers.

@donho
Copy link
Member

donho commented Jan 6, 2022

@chcg

I created a test branch for the integration at https://github.com/chcg/notepad-plus-plus/tree/scintilla5 with the current versions

Nice job! Though you have already built a branch for that. I still want to do it myself - not because of the lack of trust for your work, but for learning the new Scintilla component - by benefiting from your experience of course.

Do you have an updated timeframe for the move to scintilla 5?

It was planned to be done earlier (and as I said in v8.2). Due to the several regressions from v8.1.6 to v8.1.9.3, I have had no time to do it.
It "seems" v8.2 is stabilized (knocking on wood very hard), I would like take this chance to do one more release in order to extend its stability and to include some bug-fixes and enhancements from the long time old issue tickets.
OTOH, I will move in January; I am surely less available in this month. As a result I guess I will be able to start this task on the beginning of February.
Do you think this Scintilla updated time frame is reasonable?

@chcg
Copy link
Contributor

chcg commented Jan 6, 2022

@donho As stated above there a still some obstacles in the way. So it seems fine for me to have a stabilisation phase for the current version. In parallel the problems with scintilla 5 could be analysed and hopefully fixed. The time frame is fine for me.

@ArkadiuszMichalski
Copy link
Contributor Author

This was a problem at Geany: SCI_GETTEXT & friends API change in Scintilla 5.1.5.

@rdipardo
Copy link
Contributor

rdipardo commented Jan 11, 2022

This was a problem at Geany: SCI_GETTEXT & friends API change in Scintilla 5.1.5.

Update

A patch for the affected API calls was pushed to Geany's trunk: geany/geany@142b8ff

Some additional context

Scintilla 5.1.5 changed the return value of the SCI_GETTEXT,1 SCI_GETSELTEXT and SCI_GETCURLINE APIs to not count the terminating NUL in the total length of the string. [ Corrected by @kugel- ]

Without refactoring 2 (or wrapping their return values 3), functions that call the new APIs can result in fatal allocation errors.

Footnotes

  1. The string data is not affected: "SCI_GETTEXT always terminates with a NUL.": https://sourceforge.net/p/scintilla/bugs/2308/#ab6e

  2. The Scintilla mailing list offers some guidance on how to refactor: "The normal technique is to call the Scintilla API with NULL as the string argument which returns the length. Then allocate a string of length+1 bytes (the +1 may be set to NUL). Then call the API with this allocated string. Use the resulting string in a way that ignores the trailing NUL. If using the C++ std::string type, an extra NUL byte is automatically allocated at the end which is not counted in std::string::length() so works without problems."

  3. https://sourceforge.net/p/scintilla/bugs/2308/#6e13

@kugel-
Copy link

kugel- commented Jan 11, 2022

@rdipardo Wrong! The new return values do not include the NUL terminator.

When calling SCI_GETTEXT, SCI_GETSELTEXT, and SCI_GETCURLINE with a NULL buffer argument to discover the length that should be allocated, do not include the terminating NUL in the returned value

@rdipardo
Copy link
Contributor

rdipardo commented Jan 11, 2022

@kugel-,
thanks; I see now that the mailing list thread is actually about the problems with the former API.
Bullet 3 of the v5.1.5 change log is the correct point of reference.

@alankilborn
Copy link
Contributor

alankilborn commented Jan 11, 2022

SCI_GETTEXT & friends API change in Scintilla 5.1.5.

Avoid any pain from that for the near term, and target something earlier than 5.1.5 for Notepad++ inclusion?

Actually, looking at current N++ codebase, there doesn't seem to be an overwhelming number of calls to SCI_GETTEXT, SCI_GETSELTEXT, and SCI_GETCURLINE ... so maybe it isn't a big concern.

@rdipardo
Copy link
Contributor

. . . looking at current N++ codebase . . .

Don't forget the plugins 😱!

@alankilborn
Copy link
Contributor

Don't forget the plugins

LOL. Well, yes, my main concern (already thought about) is the PythonScript plugin. Quite frankly, I would pick another text editor if I didn't have this. But its interface to the new APIs is probably quite "wrapped" and thus necessary changes would be tightly localized.

But, yes, a concern for all plugins that might use these Scintilla APIs.

Perhaps not a ton of usage in the plugins either?
Maybe I'm grossly underestimating... :)

@chcg
Copy link
Contributor

chcg commented Jan 12, 2022

From a first look at least N++ itself is not affected by this change as N++ already allocated a buffer +1 in case of the changed apis, but at e.g.

notifyView->execute(SCI_GETTEXT, length + 1, reinterpret_cast<LPARAM>(buf));
in the understanding of version 5.1.5 we are trying to request one byte to much. In this case the requested length is not available and at
https://github.com/chcg/notepad-plus-plus/blob/scintilla5/scintilla/src/Editor.cxx#L5893
restricted to the document length (=requested one via wparam -1). So this will likely cause no direct issue of writing data beyond the buffer, but anyhow should be checked and adapted.

@rdipardo
Copy link
Contributor

rdipardo commented Feb 3, 2022

Big HUGE news:

New versions of Lexilla (5.1.5), Scintilla (5.2.0), and SciTE (5.2.0) will be released in a few days.

[ . . .]

• Scintilla 5.2.0
Add multithreaded layout to significantly improve performance for very wide lines. Feature #1427.

https://groups.google.com/g/scintilla-interest/c/JUvvJttiHfs

@donho
Copy link
Member

donho commented Mar 19, 2022

@chcg
I just checked your branch https://github.com/chcg/notepad-plus-plus/tree/scintilla5 and compared the interface between v4.66 & v5.2.1. The change you've brought looks good and binary seems work fine - bravo!

In order to refine what you have done so far, could you please create a PR based on the branch, so the community could work on it together?

@DietmarSchwertberger
Copy link

DietmarSchwertberger commented Mar 19, 2022

Recent Scintilla versions have support for displaying e.g. CR/LF in a different colour.

Search https://www.scintilla.org/ScintillaDoc.html for SC_REPRESENTATION_COLOUR and SCI_SETREPRESENTATIONCOLOUR

It would be great if this feature makes it into N++ as well.

The now closed feature request for Scintilla: https://sourceforge.net/p/scintilla/feature-requests/1129/

And some messages on the mailing list:
https://groups.google.com/g/scintilla-interest/c/-zLNwzJ18Q4/m/EZfyqGEaAQAJ
Not sure whether this is related. The search engine finds it if one searches for the SC_/SCI_ constants, but I don't see why:
https://groups.google.com/g/scintilla-interest/c/g1Ypo8qijJU/m/o9zZCGSSAAAJ

@donho
Copy link
Member

donho commented Mar 19, 2022

@DietmarSchwertberger
Thank you for the information.
Could you open an issue for this feature request in Notepad++?
I will consider it.

@DietmarSchwertberger
Copy link

@donho : Submitted as #11413
Thanks a lot.

@chcg chcg mentioned this issue Mar 21, 2022
@donho donho closed this as completed in a61b03e Mar 27, 2022
@DietmarSchwertberger
Copy link

Thanks a lot. Looking forward to the release.
I followed your donation recommendations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scintilla dependent Can't be considered for N++ implementation unless/until Scintilla changes
Projects
None yet
Development

No branches or pull requests