Fix auto scroll to last line after update not working

[jofon]

When using the "Scroll to the last line after update" setting with Word wrap, the screen was not scrolling down to the last line. It would simply flicker down to the end, and come back to the previous scroll location.

Before version 7.8.2 (including), it would scroll to what would be the last line if Word Wrap weren't active (example: 50 lines, with 1 added line from word wrap, would result in a scroll to line 49).

This pull request fixes the issue by saving the location after the scroll to the end is done, which prevents further operations from restoring a previous position.

Fixes #8477 and fixes #8214

[jofon]

On Notepad_plus::notifyBufferChanged DOC_NEEDRELOAD also suffers from the same problem. The fix is the same as the one in this PR, but I was unsure if that was the behaviour you wanted (although it is called from NPPM_INTERNAL_RELOADSCROLLTOEND). I can add the same fix to DOC_NEEDRELOAD too, if you agree.

[Yaron10]

@jofon,

The cause of the problem is _positionRestoreNeeded.

_mainEditView._positionRestoreNeeded = false;
_mainEditView.execute(SCI_GOTOLINE, _mainEditView.execute(SCI_GETLINECOUNT) -1);

Would have solved the issue but it's inaccessible here.

Thank you for working on it.

[donho]

@Yaron10

Would have solved the issue but it's inaccessible here.

What do you mean by "it's inaccessible here" ? The modified place in the code of this PR?

[Yaron10]

@donho,

_positionRestoreNeeded is protected and therefore it's inaccessible in Notepad_plus:: functions.

class ScintillaEditView : public Window
{
    friend class Notepad_plus;

Would make _positionRestoreNeeded accessible in the modified place in this PR.
And also in Notepad_plus::notifyBufferChanged (@jofon's second comment. I haven't tested the problem there myself).

[donho]

@jofon

Tested your PR with langs.model.xml (line wrap enabled).
It doesn't fix the bug at all.

[jofon]

@donho

With word wrap and "Scroll to the last line after update" enabled, I edited the langs.model.xml on Notepad, then switched to Notepad++ and it did scroll to the end of the file. Compare to the behaviour without the PR, which keeps the scroll where it was before switching back to Notepad++.

If you are referring to it not completely scrolling to the last line, I can confirm that it happens, and it's not due to the part of the code that scrolls to the end, but to the word wrap itself not scrolling correctly. I've been looking into it (on and off), but have yet to find a solution. The behaviour is the following: if there is one extra line (due to wrapping) on the visible text after scrolling to the end, then the scroll will be wrong by one line. If there's two extra lines, then it'll be wrong by two lines, etc.

[Yaron10]

If you are referring to it not completely scrolling to the last line...

I have noticed that too but assumed it was not related to _positionRestoreNeeded introduced in v7.8.3.
I did not want to add an "Off Topic" comment.

Another point:
Even in files where the caret does go to the last line (and visible), there's a problem in Double-View mode.

[jofon]

@Yaron10
Do you mean cloning the file to another view? Yes, the second view will not scroll down. I was going to create a new issue and PR for that, as I thought it could be considered a different problem from this one. The fix is fairly simple, as it's already done on DOC_NEEDRELOAD. We just need to change the else on performPostReload to an if, and do the necessary changes to check if the buffers are cloned at that point.

[Yaron10]

@jofon,

Do you mean cloning the file to another view?

Open two different files and move one to the other view.

---

I haven't tested it but I guess those problems (including @donho's langs.model.xml) existed in earlier versions too.

[jofon]

@Yaron10
I can't reproduce the problem with double view with this PR applied. Could you give more details as to what happened and the steps you took?

---

Yes, the word wrap scroll not working correctly was present in at least version 7.7. I haven't tested any version before that.

[Yaron10]

@jofon,

I can't reproduce the problem with double view with this PR applied. Could you give more details as to what happened and the steps you took?

I'll do that later today.

Yes, the word wrap scroll not working correctly was present in at least version 7.7. I haven't tested any version before that.

Thanks for testing.

[Yaron10]

@jofon,

I can't reproduce the problem with double view with this PR applied. Could you give more details as to what happened and the steps you took?

Please edit config.xml with another editor and replace your <GUIConfig name="AppPosition" line with
<GUIConfig name="AppPosition" x="276" y="4" width="1008" height="732" isMaximized="no" />.

Open any file.
Open this file and move it to the other view.
Modify it in another editor and Save.
Activate NPP.

---

EDIT:
The problem is probably caused by the editing window size and not by the Double-View mode.

[jofon]

@Yaron10

Strange, still can't replicate the problem.

With this PR, word wrap enabled, File Status Auto-Detection -> Enable for all opened files ( or just Enable, but keep the focus on the file that will be edited), Scroll to the last line after update enabled, and varying the sizes of the second view. The two behaviours I see are: scroll to the last line of the .js file (which is an empty line), or, if there are wrapped lines, the problem I spoke about in a previous comment occurs.

EDIT: the scroll doesn't happen if the NPP window was minimized, even though the line is selected and the code runs. Probably something else that prevents scrolling when the window appears after being minimized.

[Yaron10]

@jofon,

Did you try it with a not-maximized window?
Did you replace the <GUIConfig name="AppPosition" line?

[jofon]

@Yaron10

Yes, I followed the steps you described.

[Yaron10]

@jofon,

I've downloaded a fresh portable NPP (x32).
I've replaced the original EXE with your build (https://ci.appveyor.com/project/donho/notepad-plus-plus/builds/34933632/job/kuhqnstqxqxlw94v/artifacts).
I've changed only 3 settings: Word-wrap: ON, Update silently: checked and Scroll to the last line after update: checked.

And this is the position I get after externally modifying the file:

Windows 7.

[jofon]

@Yaron10

Ah, that seems to be the issue with wrapping that I commented on before when replying to donho.

The behaviour is the following: if there is one extra line (due to wrapping) on the visible text after scrolling to the end, then the scroll will be wrong by one line. If there's two extra lines, then it'll be wrong by two lines, etc.

In your case, you have more or less 20 lines added by the wrap in your visible window, right? 223 total lines minus 20 wrapped, and you get line 203 at the bottom. It's also probably flickering from your position, then to the end, and then to the final scroll position (203). That last scroll is the word wrap scroll, which as I said, is still wrong and I haven't been able to find out why yet.

Right now this PR only fixes the scroll to the end never happening, and it'll also work correctly if in the final view there are no wrapped lines.

[Yaron10]

@jofon,

OK. :)
And I just mentioned that there might be a case where in Single-View it scrolls to the last line and in Double-View it won't.
IOW: it may depend on the text editing window size.

It should be interesting to test it with "goToPos(getDocumentLength())" (obviously this isn't the actual code).

[donho]

@jofon Here is how I test:

[Yaron10]

@donho, @jofon,

Replacing _mainEditView.execute(SCI_GOTOLINE, _mainEditView.execute(SCI_GETLINECOUNT) -1); with _mainEditView.execute(SCI_DOCUMENTEND); fixes the issue in the cases I've tested (except for minimized NPP).

Can you try it?

If it's important that the caret should be at the beginning of the last line, you can add _mainEditView.execute(SCI_HOME);.

[jofon]

@Yaron10

And here I was playing around with the wrapcount of the visible lines...
SCI_DOCUMENTEND seems to be working nicely. Thanks! I'll run some more tests after work and then if it all checks out, I'll commit the necessary changes.

[Yaron10]

@jofon,

Great. Thanks for testing.

The following code also fixes the problem.

		int lastLine = _mainEditView.execute(SCI_GETLINECOUNT) - 1;
		_mainEditView.execute(SCI_ENSUREVISIBLEENFORCEPOLICY, lastLine);
		_mainEditView.execute(SCI_GOTOLINE, lastLine);
		_mainEditView.saveCurrentPos();

I'd suggest to let @donho decide which should be used before committing any further changes.

Another question for @donho: should we make _positionRestoreNeeded accessible here and use _mainEditView._positionRestoreNeeded = false; or saveCurrentPos() would be better?

[donho]

@Yaron10

The following code also fixes the problem.

Could you indicate where I should insert the code you provided?

Another question for @donho: should we make _positionRestoreNeeded accessible here and use

If the code fix the bug and with no regression, than we will make it accessible.

[Yaron10]

@donho,

If the code fix the bug and with no regression, than we will make it accessible.

The cause of the main problem (the visible lines do not change at all) is _positionRestoreNeeded.
I think that _positionRestoreNeeded = false; should not cause any other regressions.

In ScintillaEditView.h:

class ScintillaEditView : public Window
{
	friend class Notepad_plus;
	friend class Finder;

In Notepad_plus.cpp:

void Notepad_plus::performPostReload(int whichOne)
{
	NppParameters& nppParam = NppParameters::getInstance();
	const NppGUI & nppGUI = nppParam.getNppGUI();
	bool toEnd = (nppGUI._fileAutoDetection & cdGo2end) ? true : false;
	if (!toEnd)
		return;
	if (whichOne == MAIN_VIEW)
	{
		_mainEditView._positionRestoreNeeded = false;
		_mainEditView.execute(SCI_DOCUMENTEND);
	}
	else
	{
		_subEditView._positionRestoreNeeded = false;
		_subEditView.execute(SCI_DOCUMENTEND);
	}
}

or:

void Notepad_plus::performPostReload(int whichOne)
{
	NppParameters& nppParam = NppParameters::getInstance();
	const NppGUI & nppGUI = nppParam.getNppGUI();
	bool toEnd = (nppGUI._fileAutoDetection & cdGo2end) ? true : false;
	if (!toEnd)
		return;
	if (whichOne == MAIN_VIEW)
	{
		_mainEditView._positionRestoreNeeded = false;
		int lastLine = _mainEditView.execute(SCI_GETLINECOUNT) - 1;
		_mainEditView.execute(SCI_ENSUREVISIBLEENFORCEPOLICY, lastLine);
		_mainEditView.execute(SCI_GOTOLINE, lastLine);
	}
	else
	{
		_subEditView._positionRestoreNeeded = false;
		int lastLine = _subEditView.execute(SCI_GETLINECOUNT) - 1;
		_subEditView.execute(SCI_ENSUREVISIBLEENFORCEPOLICY, lastLine);
		_subEditView.execute(SCI_GOTOLINE, lastLine);
	}
}

[donho]

@Yaron10 Thank you for the suggestion. It's committed now: c607e42

@jofon Thank you for opening this PR

[Yaron10]

@donho,

Thank you for committing it.

@jofon,

I'd also like to thank you for opening this PR.

On Notepad_plus::notifyBufferChanged DOC_NEEDRELOAD also suffers from the same problem. The fix is the same as the one in this PR, but I was unsure if that was the behaviour you wanted (although it is called from NPPM_INTERNAL_RELOADSCROLLTOEND).

I didn't look at that.
Can you please open an issue and a PR?