Fix Replace All crash & performance issue#14685
Fix Replace All crash & performance issue#14685donho wants to merge 1 commit intonotepad-plus-plus:masterfrom
Conversation
|
@donho |
Well, plugins that act on "modified" notifies to do their "whatever" thing obviously aren't going to be doing it. :-) But I'd say that's negligible in trade for substantial performance improvement. |
What @alankilborn has said is pertinent... let's see if there any complaint from plugin authors, then we will see what we do about it. |
Big, loud complaint! I didn’t discover this until I read Ekopalypse’s comment today; I hadn’t happened to do a Replace All in a file where my plugin’s Elastic tabstops function was affected since installing a version of Notepad++ with this change. 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:
|
|
|
||
| // Turn ON the notifications after operations | ||
| (*_ppEditView)->execute(SCI_SETMODEVENTMASK, notifFlag); | ||
| if (op == ProcessReplaceAll && nbProcessed > 0) // All the notification of modification (SCN_MODIFIED) were removed during the operations, so we set modified status true here |
There was a problem hiding this comment.
NPPN_MODIFIEDREPLACEALL could be added here for notifying the plugins "the current document has been changed after Replace All action".
Could you create an issue with a step by step instructions for reproduce the bug?
You're right about it. But I neglected the part of plugins being able to get Scintilla's notification. Sorry for neglecting it.
What do you think? |
Issue #14767.
I can think of two options, depending on whether you prioritize not breaking existing plugins, or being as “honest” as possible with plugins. The notification you described would tell plugins exactly what happened and allow them to choose how to respond. However, existing plugins would be broken until and unless their authors update them. Alternatively, you could “forge” the SCN_MODIFIED notifications that would occur if the entire document were deleted and replaced. (Save the old document length before you start; then after the replace is complete, send SC_MOD_BEFOREDELETE, SC_MOD_DELETETEXT, SC_MOD_BEFOREINSERT and SC_MOD_INSERTTEXT. A potential complication is that these should occur before Scintilla sends an SCN_UPDATEUI notification, and I don’t know if that is how it would happen.) That would avoid breaking existing plugins, at the expense of “lying” to plugins about what actually happened. Much as I hate! hate! hate! the idea of breaking existing plugins (especially when — unlike the NPPM_MODELESSDIALOG fiasco –— they are already following the rules), if I had to choose, I guess I’d go for the NPPN message rather than forging Scintilla messages... there are too many unforeseeable ways the latter could come back to bite us. I think I would not make the notification specific to Replace All, though. Suppose this sort of thing happens again, with a different operation? Better to have a generic notification that there was a non-specific, global modification to the document while Scintilla notifications were suppressed (NPPN_GLOBALMODIFIED?) and a secondary field that specifies what operation produced it (GLOBALMODIFIED_REPLACEALL, other values reserved for future use?). NPPN_READONLYCHANGED is precedent for putting the BufferID in the hwndFrom field and using the idFrom field for details. |
Add NPPN_DOCMODIFIEDBYREPLACEALL to notify plugins that the current document is just modified by Replace All action. For solving the performance issue (from v8.6.4), Notepad++ doesn't trigger SCN_MODIFIED & other Scitilla notifications during Replace All action anymore. Plugin devs should monitor NPPN_DOCMODIFIEDBYREPLACEALL instead. This notification is implemented in Notepad++ v8.6.5. Ref: notepad-plus-plus#14685 (comment) Fix notepad-plus-plus#14767
Even if a kludgy workaround could avoid it now, the next breaking change is only around the corner. Scintilla is an application interface, not a plugin interface. SciTE's mainatiner had the wisdom to natively support Lua scripting like many other extensible editors, NotepadNext included. |
Amen! |
Add NPPN_GLOBALMODIFIED to notify plugins that the current document is just modified by Replace All action. //scnNotification->nmhdr.code = NPPN_GLOBALMODIFIED; //scnNotification->nmhdr.hwndFrom = BufferID; //scnNotification->nmhdr.idFrom = 0; // preserved for the future use, must be zero For solving the performance issue (from v8.6.4), Notepad++ doesn't trigger SCN_MODIFIED & other Scitilla notifications during Replace All action anymore. Plugin devs should monitor NPPN_GLOBALMODIFIED instead. This notification is implemented in Notepad++ v8.6.5. Ref: #14685 (comment) Fix #14767, close #14768
Fix #14630