New API to fix eventual regression regarding SCN_MODIFIED for some plugins

Michael Vincent

@Coises said in New API to fix eventual regression for some plugins:

I know what you wrote isn’t what you meant, but if taken literally that statement will cause many plugin authors to panic and/or give up hope.

I was there momentarily, but your post explains it perfectly and the only time in PythonScript scripts or my own C++ plugins I use SCN_MODIFIED is when I’m looking for SC_MOD_INSERT/DELETETEXT. Indeed a quick check of functionality in N++ 8.7.6 shows I’m still working fine.

Also, your SCI_GETMODEVENTMASK hint is useful. I used NppExec to get it:

SCI_SENDMSG  SCI_GETMODEVENTMASK
echo $(MSG_RESULT)

and in N++ pre 8.7.6 it is = 8388607(which I didn’t bother “decoding”),
while in N++ 8.7.6 it is = 16483 (which from your link equates to the 5 messages N++ still forwards by default).

Cheers.

donho

@Coises
You’re absolutely right about it - it was not accurate at all.
I’ve reworded the announcement title & content to make it more consistent and clearer.

Thank you everyone for providing your valuable opinions.
Please let me know if there are any other points that could be improved.

Coises

@donho said in New API to fix eventual regression regarding SCN_MODIFIED for some plugins:

Please let me know if there are any other points that could be improved.

One observation and one technical question.

Observation:

A small point, but the comments here still use adding SC_MOD_DELETETEXT and SC_MOD_INSERTTEXT as an example, when they are already part of the default. I think you meant to use SC_MOD_BEFOREDELETE and SC_MOD_BEFOREINSERT as examples. Also — and I understand you might not agree — would it not be better, as a matter of “future-proofing,” to illustrate best practice as explicitly enabling all SCN_MODIFIED events the plugin plans to use, even if they are (currently) part of the default set?

Technical question:

My plugin might or might not need SCN_MODIFIED events (including SC_MOD_BEFOREDELETE). Must NPPM_ADDSCNMODIFIEDFLAGS be sent in NPPN_READY, or can I wait and send it only if and when the user does something which enables the feature (elastic tabstops) that makes use of these events? I understand that once any plugin enables them, they will remain enabled for the entire session. I’m just wondering if I can avoid imposing the performance loss you documented on users who have Columns++ installed but don’t always (or perhaps never) use the elastic tabstops feature.

I’m a lot slower at getting things changed than you are… but my eventual goal now will be to eliminate using SC_MOD_BEFOREDELETE. (In my implementation of elastic tabstops, it has always been a work-around for a common case which exposes an underlying weakness in my design.) In the meantime I’d like to avoid making users pay a performance penalty, relative to unaugmented Notepad++, just for having the plugin installed.

donho

@Coises said in New API to fix eventual regression regarding SCN_MODIFIED for some plugins:

A small point, but the comments here still use adding SC_MOD_DELETETEXT and SC_MOD_INSERTTEXT as an example, when they are already part of the default. I think you meant to use SC_MOD_BEFOREDELETE and SC_MOD_BEFOREINSERT as examples.

Fixed. Thank you for pointing out this issue.

Also — and I understand you might not agree — would it not be better, as a matter of “future-proofing,” to illustrate best practice as explicitly enabling all SCN_MODIFIED events the plugin plans to use, even if they are (currently) part of the default set?

What do you mean exactely “to illustrate best practice as explicitly enabling all SCN_MODIFIED events the plugin plans to use”?

My plugin might or might not need SCN_MODIFIED events (including SC_MOD_BEFOREDELETE). Must NPPM_ADDSCNMODIFIEDFLAGS be sent in NPPN_READY, or can I wait and send it only if and when the user does something which enables the feature (elastic tabstops) that makes use of these events? I understand that once any plugin enables them, they will remain enabled for the entire session. I’m just wondering if I can avoid imposing the performance loss you documented on users who have Columns++ installed but don’t always (or perhaps never) use the elastic tabstops feature.

I just modified the code for your specific requirement (and it’s a good one):
https://github.com/notepad-plus-plus/notepad-plus-plus/commit/d888fb5f1263f5ea036c610b6980e5c4381ce7eb

So yes, you can send NPPM_ADDSCNMODIFIEDFLAGS anytime you want now - even if you want to set the different events separately at the different time, according users’ need.

Coises

@donho said in New API to fix eventual regression regarding SCN_MODIFIED for some plugins:

What do you mean exactely “to illustrate best practice as explicitly enabling all SCN_MODIFIED events the plugin plans to use”?

I just mean, should examples and documentation suggest that if a plugin uses SC_MOD_INSERT (which is part of the default) and SC_MOD_BEFOREDELETE (which isn’t), it should send:
SC_MOD_INSERT | SC_MOD_BEFOREDELETE
and not just:
SC_MOD_BEFOREDELETE
so as not to assume SC_MOD_INSERT will always be part of the default? I understand that you might be so certain that the default will not change in the future that there is no point to this.

Should a plugin care what the default mask is? I mention it because I personally don’t see any advantage to plugins being dependent on the default; it doesn’t hurt anything to specify every event that will be used. Once we give plugins the responsibility to declare which events they need enabled, why not routinely declare all of them instead of relying on a particular set of defaults?

So yes, you can send NPPM_ADDSCNMODIFIEDFLAGS anytime you want now - even if you want to set the different events separately at the different time, according users’ need.

Thank you. That makes me feel better about using this message (though I still hope to find time to re-design my way around it eventually).

donho

@Coises said in New API to fix eventual regression regarding SCN_MODIFIED for some plugins:

Should a plugin care what the default mask is? I mention it because I personally don’t see any advantage to plugins being dependent on the default; it doesn’t hurt anything to specify every event that will be used. Once we give plugins the responsibility to declare which events they need enabled, why not routinely declare all of them instead of relying on a particular set of defaults?

you’re not wrong about this point.

However, the authors of concerning plugins won’t understand why SCN_MODIFIED works with their plugins work under v8.7.5 & previous versions but not under v8.7.6 & later version.
So the 5 events used by Notepad++ are listed to address to the regression for the historic reason,

With the example you provided, plugins use SC_MOD_INSERT | SC_MOD_BEFOREDELETE or only SC_MOD_BEFOREDELETE will get the same effect. I don’t see any inconvenience about it.

Of course, you’re encouraged to improve the document of this message to make your point clearer, if it’s not too redundant.

Eric Pendergrass

I noticed that after I make 1 change to a file, it starts an endless loop of auto-backup every 7 seconds which triggers my onedrive sync and keeps me steadily using high CPU as a result of constantly writing backup files even though I’ve made no changes.

I don’t know what SCN_MODIFIED is or how it works in NP++, but I’m willing to bet I have a bug with a plugin or something that is constantly triggering false changes after my 1st actual change.

I’m going to have to work around and write the file somewhere else as well as decrease how often the files get backed up, but sharing in case this helps anyone else realize the impact of this.

Reach out to me if anyone needs help reproducing this bug or wants to collect any logs from me. Thanks ya’ll!

Mark Olson

@Eric-Pendergrass

I’m willing to bet I have a bug with a plugin or something that is constantly triggering false changes after my 1st actual change.

Have you tried, for each plugin you have installed, temporarily disabling it and seeing if that solves the problem? That usually helps isolate the issue.

I don’t know what SCN_MODIFIED is or how it works in NP++

It’s a notification from Scintilla (the component that actually handles how text is stored and rendered in Notepad++) to plugins telling them “when the text or styling of the document changes or is about to change” (quote from official docs).

Lycan Thrope

@Eric-Pendergrass,
What it seems you’re experiencing is mostly likely the Session Snapshot and periodic backup feature working as designed. This screenshot shows you the default setup:

You can change the time the auto backup occurs or just unselect it, but I suggest you point it somewhere other than your OneDrive directory, because that will include network traffic issues to complete the updates, and that many times is ludicrous for this situation. Better you point it to your local drive as shown in my setup. Hopefully you don’t have a 2 billion size file, or multiples, as that also would explain the horrendous delay. Less is more.

mkupper

@Eric-Pendergrass said in New API to fix eventual regression regarding SCN_MODIFIED for some plugins:

Reach out to me if anyone needs help reproducing this bug or wants to collect any logs from me

It definitely looks like something that we should attempt to reproduce. Let’s start with your debug info. From Notepad++ click on the ? that is at the top-right of the menu bar at the top of Notepad++ and then select Debug Info.... Click the [Copy debug info into clipboard] button and then paste the results into this forum thread. Next, select the text you just added to the forum’s message box and click the </> button from the forum toolbar that’s just above the message box. This will bracket the selected text with ``` lines.

I believe the main value will be the list of plugins you are using but it’s best to include the full debug info dump.

rdipardo

@mkupper said in New API to fix eventual regression regarding SCN_MODIFIED for some plugins:

It definitely looks like something that we should attempt to reproduce.

I doubt that. The only way a plugin could misuse SCN_MODIFIED would be to program a callback that caused another SCN_MODIFIED event to fire, recursively. Like, say the modificationType field of the scn notification header contained the SC_MOD_DELETETEXT flag, and the callback took the header’s text field and put it back into the document with ::SendMessage(scn->nmhdr.idFrom, SCI_INSERTTEXT, scn->position, scn->text), or something equally stupid.

In other words, a plugin would have to deliberately behave like malware to create such a performance-degrading feedback loop; changing the default event mask as happened in v8.7.7 would not have that effect.

mkupper

@rdipardo I have a bit more than casual interest in what @Eric-Pendergrass reported as I too have seen some unexpected behavior. A couple of weeks ago I noticed that Npp seemed to be writing to disk every 7 seconds despite Notepad++ being idle/unused at the time. However, by the time I had a chance to dig into what was happening the problem went away.

Luckily, it’s happening again to me right now and so I was able to track down what is happening but don’t understand why. I have also confirmed that the issue is not a recent regression and is not related to the SCN_MODIFIED changes. We likely should shift this topic drift to a new forum thread.

However, Notepad++'s behavior still bothers me or at least it is unexpected. What I am seeing is that if a file/tab has been dirtied since I started Notepad++.exe that Notepad++ starts writing that file to the backup folder every 7 seconds when it is the active tab. This occurs with npp versions v8.7.5, v8.7.6, and v8.7.7. This still occurs when Notepad++.exe is started with -noPlugin

@Lycan-Thrope says this is “by design” but it still surprises me. Why does Notepad++ need to flush the current tab’s file to disk every 7 seconds even when Notepad++ is sitting idle in the background? I can stop the continuous flushing either by exiting and restarting Notepad++, by switching to a tab/file that has not been modified since I started Notepad++.exe, or by saving the current tab to disk via Ctrl+S.

If a file was already using the npp/backup copy (red tab) when I started Notepad++.exe then Notepad++ does not flush the file(s) to disk every 7 seconds. The continuous every-7-seconds flushing starts as soon as I type a single letter into a file. It does does not stop when I use Ctrl+Z undo to restore the file back to the state it was in when I started Notepad++.exe.

@Eric-Pendergrass likely has his Notepad++\backup folder included the things monitored by OneDrive as he wants his off-machine backup to stay up to date. Normally we’d give a person an all-thumbs-up for doing that.

@Eric-Pendergrass - you can work around the issue by switching to another file/tab in Notepad++ that has not had any changes made to it since you started Notepad++.exe. If the current active tab has had any changes made to it since you started Notepad++.exe then Notepad++ writes it to disk every 7 seconds meaning your OneDrive updates the Internet based copy of the file even though you are not making any changes.

Alan Kilborn

@mkupper said:

What I am seeing is that if a file/tab has been dirtied since I started Notepad++.exe that Notepad++ starts writing that file to the backup folder every 7 seconds when it is the active tab

I can confirm this behavior with 8.7.6 and 8.7.7; I didn’t try any other versions
I would add to @mkupper’s description: “…and there is no need to write the file because the data has not changed”.
@mkupper Are you going to put in an “issue” on github about this behavior?

Lycan Thrope

@Alan-Kilborn ,
I’m just not sure if it really is an issue. It seems, according to @mkupper solutions, it would seem that on selection and change of a tab, the session 7 second back up kicks in…happening every 7 seconds. Once you change tabs, or save the document, I’m guessing, it flushes the signal to back up that particular document, so…If I use logic on this scenario, it appears it’s working as designed, unless you change the value of back up frequency, or you turn if off, at which point, you now don’t have snapshot backup. Double-edged sword.

Alan Kilborn

@Lycan-Thrope

If the data hasn’t changed, there’s no reason to bother the file system. Period, end of story.

Lycan Thrope

@Alan-Kilborn ,
Perhaps. But maybe making the first change, and not saving it, causes it to continually back it up regardless, until an event happens that stops it from focusing on that document/tab, and turns its attention to other tabs/documents that aren’t changed yet. It’s just a thought, since I’m not sure how the developer implemented that situation. Could it be that it doesn’t check to see if there was a change, after the first one signaled a change occurred that started it?
Maybe before putting in a report of the bug, try shutting off the Session Snapshot and periodic backup or varying it’s frequency, and see if the behavior stops or changes, first. Then we’ve isolated it to just that particular routine/implementation.

mkupper

Thank you for the feedback @Alan-Kilborn and @Lycan-Thrope. Hopefully I was able to integrate that into the github ticket.

I have created an issue at https://github.com/notepad-plus-plus/notepad-plus-plus/issues/16186
https://community.notepad-plus-plus.org/user/

I recall some changes a while back to prevent spurious backups of the non-active tabs. I now suspect that those changes resulted in Notepad++ never clearing its internal “file dirty” flag when it saves a file to the backup system. This flag is not same as the user-visible file-modified flag that causes the tabs to turn red. The issue started with v8.7.1 though the release notes for that version don’t mention changes to the backup system.

PeterJones

@Lycan-Thrope,

Perhaps. But maybe making the first change, and not saving it, causes it to continually back it up regardless, until an event happens that stops it from focusing on that document/tab, and turns its attention to other tabs/documents that aren’t changed yet.

The point is, that’s not how it used to be, and that’s not how it should be.

Using the definitions

make snapshot = automated system writes its temporary copy to disk in the backups\ folder
to save = the user does File > Save or equivalent, to write a permanent copy to the user’s specified directory

The old, ~~intended~~ desired behavior:

user edits active file
7 seconds later, N++ makes snapshot of the file
user doesn’t edit the file for 70 seconds, so no new snapshots are made
user edits active file
7 seconds later, N++ makes snapshot #2 of the file

The new, bad user experience behavior:

user edits active file
7 seconds later, N++ makes snapshot of the file
user doesn’t edit the file for 70 seconds, but N++ snapshots the exact same contents 10 more times unnecessarily
user edits active file
7 seconds later, N++ makes snapshot #12 of the file

~~The old behavior is what happened for all the years of Notepad++ “session snapshot and periodic backup” existence, until v8.7.1 changed the behavior.~~
update: per Don’s comments in the linked issue, and the issue it linked back to, apparently, the always-write was always the intention, and implied that it’s always been the behavior, but I didn’t remember it doing that; either my memory is wrong, or …

Alan Kilborn

@PeterJones said :

the always-write was always the intention

Isn’t that the “lazy coder’s” way of doing it? And what’s the advantage?
Say it’s a huge file/buffer; isn’t that possibly going to take appreciable time to do a redundant save? (annoying the user as he can’t do other things while that is going on – I’d think).
I stand by my original stance that there’s no point in rewriting the same thing every so many seconds…
EDIT: Think of how many problems and discussion points simply wouldn’t exist if the “snapshot” capability itself never existed!

PeterJones

@Alan-Kilborn said in New API to fix eventual regression regarding SCN_MODIFIED for some plugins:

Think of how many problems and discussion points simply wouldn’t exist if the “snapshot” capability itself never existed!

Interestingly, the “app” version of MS Notepad recently informed me that it’s started to perpetually save – I don’t know whether that’s implemented like snapshot or the MS Sticky Notes (where it saves to a hidden file instead of your main file), or whether it’s done like N++'s AutoSave plugin.

… A quick web search tells me that MS has gone down the “session / snapshot” path, not saving over the original unless you hit SAVE, so MS is copying Don at this point. Next thing you know, they are going to implement Lexilla syntax highlighting instead of hooking it to an LSP. ;-)