Notepad++ release 8.6.4

Coises

Notepad++ release 8.6.3 change log:
[…]
4. Fix “Replace All” crash & performance issue. (Fix #14630)

Unfortunately, as @Ekopalypse noted in this comment, the fix for this is a breaking change for plugins.

This potentially breaks any plugin that needs to know when the text in the file has changed, should the user use Replace All while that plugin is active.

This is a major, breaking change to the “contract” with plugins. If it’s absolutely necessary (and I hope it isn’t, because this makes a mess), it should be:

advertised well before it’s implemented, to give plugin authors a chance to fix the damage it could cause.
complemented with an NPPN notification indicating that multiple changes may have been made while SCN_MODIFIED notifications were suppressed (or with some similar mechanism) so plugins know they need to rescan the entire file if they are sensitive to changes in the file content.

donho

@Coises

advertised well before it’s implemented, to give plugin authors a chance to fix the damage it could cause.

If I hadn’t neglected the fact that plugins can monitor Scintilla’s notifications, I would do it surely.

complemented with an NPPN notification indicating that multiple changes may have been made while SCN_MODIFIED notifications were suppressed (or with some similar mechanism) so plugins know they need to rescan the entire file if they are sensitive to changes in the file content.

A fix proposal has been provided here:
https://github.com/notepad-plus-plus/notepad-plus-plus/pull/14768

Any plugin dev is welcome to confirm the result of adding new notification message in your plugin and checking with the binary of PR
If it fixes the issue, I will merge it and announce this new notification ASAP to the plugin devs.

This is a major, breaking change to the “contract” with plugins. If it’s absolutely necessary (and I hope it isn’t, because this makes a mess), it should be:

The fix of Replace All performance issue breaks the “contract” with plugins indeed.
Nevertheless, this fix remains necessary. The broken aspect is, unfortunately, unavoidable.

In theory, maintaining retro-compatibility should always be our goal. However, in practice, during an application’s evaluation, certain compatibility compromises become inevitable. This situation as a prime example.

Coises

@donho said in Notepad++ release 8.6.4:

A fix proposal has been provided here:
https://github.com/notepad-plus-plus/notepad-plus-plus/pull/14768

Thank you for getting this together so quickly.

In my tests so far, it makes it possible for my plugin to know when Replace All and Replace All in All Opened Files have made modifications to files while Scintilla notifications were disabled — that lets me either re-analyze the entire file (if it’s currently visible) or flag it for full re-analysis (if it’s not visible).

Mark Olson

I think it would be a good idea to only disable Scintilla notifications for large files, because it appears to me that the performance issue this change was meant to solve was not really a problem for small files. That said, I think that the current solution makes sense, and I appreciate the speed of implementation.

Of course, there’s the separate issue of what “large” means; currently NPP has one “large file” threshold at 200MB, and the issue as I understand is that the message-driven poor performance was happening for smaller files, say around 20MB.

rdipardo

@Mark-Olson said in Notepad++ release 8.6.4:

Of course, there’s the separate issue of what “large” means […].

In practice, a file’s content is more impactful than sheer byte count. In the case that prompted notification filtering, the bottleneck was blamed on the super high frequency of commas, which numbered in the millions, each one triggering several events.

There are too many potential forms of repetitive content to generalize them all into a single reliable test, and trying to guess a file’s content is extremely bug prone. One of the most publicized CVEs ever reported against Notepad++ was caused by a function that peeks at file headers in search of XML doctype declarations.

donho

@donho said in Notepad++ release 8.6.4:

A fix proposal has been provided here:
https://github.com/notepad-plus-plus/notepad-plus-plus/pull/14768

The PR has merged into master and an announce has been done in Plugin devs’ forum:
https://community.notepad-plus-plus.org/topic/25504/new-nppn_globalmodified-notification

Ekopalypse

@donho

It seems that SC_MOD_INSERTCHECK, SC_MOD_BEFOREINSERT and SC_MOD_BEFOREDELETE are disabled by default with 8.6.4.
Is this also the intention?
Are there any other notifications that are also suppressed or where it is planned to do so?

rdipardo

@Ekopalypse said in Notepad++ release 8.6.4:

It seems that SC_MOD_INSERTCHECK, SC_MOD_BEFOREINSERT and SC_MOD_BEFOREDELETE are disabled by default with 8.6.4.

Both v8.6.3/4 send SCI_SETMODEVENTMASK with the same select list of events to monitor. If the event type’s bit mask is not included, then it’s not monitored.

donho

@Ekopalypse said in Notepad++ release 8.6.4:

It seems that SC_MOD_INSERTCHECK, SC_MOD_BEFOREINSERT and SC_MOD_BEFOREDELETE are disabled by default with 8.6.4.
Is this also the intention?

In v8.6.3 / v8.6.4 only the following Scintilla notification types are activated:
SC_MOD_DELETETEXT, SC_MOD_INSERTTEXT, SC_PERFORMED_UNDO, SC_PERFORMED_REDO and SC_MOD_CHANGEINDICATOR.

Do you need the 3 types of notification you mentioned above for your plugin(s)?

Are there any other notifications that are also suppressed or where it is planned to do so?

The restriction is for the sake of global performance.
If other notification types are needed by plugins, I can reopen them.

Ekopalypse

Yes, I use SC_MOD_BEFOREINSERT and SC_MOD_BEFOREDELETE to report changes to the LSP server.
I need to convert the start and end position of the changes to the LSP specific format.
When I receive SC_MOD_DELETETEXT, it is already too late to find out where the end is/was.
But I can always send the whole document in case of a deletion, but this could slow down the server’s responses for large source code files.
The SC_MOD_INSERTCHECK is used by a PythonScript of mine that can be easily rewritten.

If the goal of Npp is to provide the best possible performance in general, and yes, I know that is the goal, then it makes less sense to process and forward notifications that may not even be needed. On the other hand, I can also set the event mask in the plugin, but if one starts doing that, then there needs to be a tacit agreement that plugins always set this to 0x1FFFFF, because otherwise one plugin could disable functions of another plugin … or cause even worse.
Or, maybe better, plugins have to register with Npp which Scintilla notifications are needed, and Npp then sets the appropriate mask at the end of the plugins loading.

donho

@Ekopalypse said in Notepad++ release 8.6.4:

Or, maybe better, plugins have to register with Npp which Scintilla notifications are needed, and Npp then sets the appropriate mask at the end of the plugins loading.

Your suggestion is good, but consider that if your LSP server plugin needs 3 types of notification, there are surly other plugins need other notification types.
I prefer to remove all the restrictions - the goal is fixing the performance issue for Replace All - I do believe that the rest codes do the job.

The fix has been already merged in master. Please check it and confirm me that the problem of you plugin is fixed.

Ekopalypse

@donho said in Notepad++ release 8.6.4:

Please check it and confirm me that the problem of you plugin is fixed.

Thank you and I will be able to let you know in about 2-3 hours.

Coises

@donho said in Notepad++ release 8.6.4:

Your suggestion is good, but consider that if your LSP server plugin needs 3 types of notification, there are surly other plugins need other notification types.
I prefer to remove all the restrictions - the goal is fixing the performance issue for Replace All - I do believe that the rest codes do the job.

The fix has been already merged in master. Please check it and confirm me that the problem of you plugin is fixed.

Ah… so this explains the mysterious regression I observed while testing upcoming changes to my plugin! I make use of SC_MOD_BEFOREDELETE.

Replacing with a binary from CI_build #475 appears to restore expected operation in Columns++.

Ekopalypse

@donho

I can also confirm that my lsp client is working as expected again after a build with the latest changes.

Coises

@donho said in Notepad++ release 8.6.4:

The fix has been already merged in master. Please check it and confirm me that the problem of you plugin is fixed.

When you know yourself, would it be possible to give us notice of the approximate cut-off time to get new plugin versions created and pull requests added for the plugin list in order for them to be included in the version of Notepad++ that will implement NPPN_GLOBALMODIFIED?

I’m assuming auto-update for 8.6.3 and 8.6.4 will not be enabled, and we can ignore compatibility issues for those versions?

donho

@Coises said in Notepad++ release 8.6.4:

When you know yourself, would it be possible to give us notice of the approximate cut-off time to get new plugin versions created and pull requests added for the plugin list in order for them to be included in the version of Notepad++ that will implement NPPN_GLOBALMODIFIED?

OK, I’ll do an independent announce in Announcement (in about 3 weeks) to notify plugin authors the cut off time (about 1 week) for Notepad++ Plugin List.

I’m assuming auto-update for 8.6.3 and 8.6.4 will not be enabled, and we can ignore compatibility issues for those versions?

It’s correct.

pnedev

Hello @donho , all,

In Compare plugins I am monitoring SCN_MODIFIED with the following flags:
SC_MOD_BEFOREDELETE,
SC_PERFORMED_USER,
SC_PERFORMED_UNDO,
SC_PERFORMED_REDO,
SC_MOD_INSERTTEXT,
SC_MOD_DELETETEXT

Thanks for keeping them intact.

Some time ago I tried also using SC_MOD_CHANGEFOLD but I had never received such notification even when I explicitly enabled it with SCI_SETMODEVENTMASK.
I did a workaround so I don’t rely on it but I was curious if someone has managed to use it successfully.

pnedev

@donho ,

I have one question regarding the new NPPN_GLOBALMODIFIED.
It is written in the comment for that notification the following:

scnNotification->nmhdr.hwndFrom = BufferID;
scnNotification->nmhdr.idFrom = 0; // preserved for the future use, must be zero

Is scnNotification->nmhdr.hwndFrom trully the buffer ID or it is the window handle?

Thanks.

Coises

@pnedev said in Notepad++ release 8.6.4:

Is scnNotification->nmhdr.hwndFrom trully the buffer ID or it is the window handle?

It is the BufferID — I can verify because my plugin code uses it.

For Replace All, one NPPN_GLOBALMODIFIED message is sent after the replace is complete.

For Replace All in All Open Files, one NPPN_GLOBALMODIFIED is sent for each modified file. Since I have to process visible files (not just the active tab, but files visible in either view) differently than files that are open but not currently visible, I use this code to tell the difference:

UINT_PTR bufferID = reinterpret_cast<UINT_PTR>(nmhdr->hwndFrom);
intptr_t cdi1 = SendMessage(nppData._nppHandle, NPPM_GETCURRENTDOCINDEX, 0, 0);
intptr_t cdi2 = SendMessage(nppData._nppHandle, NPPM_GETCURRENTDOCINDEX, 0, 1);
bool visible1 = cdi1 < 0 ? false : bufferID == static_cast<UINT_PTR>(SendMessage(nppData._nppHandle, NPPM_GETBUFFERIDFROMPOS, cdi1, 0));
bool visible2 = cdi2 < 0 ? false : bufferID == static_cast<UINT_PTR>(SendMessage(nppData._nppHandle, NPPM_GETBUFFERIDFROMPOS, cdi2, 1));
if (visible1 || visible2) {
    // code for visible files
}
else {
    // code for tabs that are not visible
}

Edit: There was a problem with the code I posted originally; when a cloned tab was visible only in the second view, it was not detected as visible. The above code might not be perfect yet, but it does fix that mistake.

donho

@pnedev

Is scnNotification->nmhdr.hwndFrom trully the buffer ID or it is the window handle?

It is the BufferID — I can verify because my plugin code uses it.

I confirm the reply of @Coises .
NPPN_GLOBALMODIFIED could be triggered from different buffers while Replace All in all opened documents. So we use nmhdr.hwndFrom for containing BufferID. And nmhdr.idFrom value 0 for Relace All operation.