Save note scroll position and restore when returning to note

[sandymcfadden]

Fix

This will help fix #2951 and help fix #2861

Currently, if you have a longer note that needs to be scrolled when you leave the note and come back the scroll position is set back to the top. This includes if you are returning from a Markdown preview, or even if you change a setting that causes the editor to re-render.

This saves the line and scroll position of a note when you leave and then restores it when you come back. This is only saved for the current session.

Test

1. Open a note which needs to be scrolled
2. Scroll to a point in the note
3. Leave the note
4. Open the note again and ensure you are taken to the same spot in the note

Release

• Updated to save the scroll position of a note so you can be restored when the note is viewed again

[codebykat]

Code looks good and mostly tests well. I noticed a few oddities that may or may not be fixable...

• When you switch back to a note, it loads at the top and then jumps to the scroll position. I'm wondering if there's any way to get that to happen when we pass new content to the editor? Does it need the timeout? Can we move it higher up in the componentDidMount function (maybe even inside the bootTimer where we set the editor content)?

• Switching into Markdown preview obviously doesn't know to scroll to the same part of the note, which is unfortunate. I am not sure there's any easy way to do that but we might want to consider it in future.

• Left an in-line comment about revealLineInCenter which I think has the potential to cause additional jumpiness.

[codebykat]

• I'm curious why we're calling both revealLineInCenter and setScrollPosition? revealLineInCenter will scroll the note to the position requested. Similarly, do we really need to save both the position and the scrollTop value?
• Did you test this without the timeout? It seems odd that we need it for setScrollPosition but not for revealLineInCenter.
• See also line 1106 where we use a promise that immediately resolves to update the cursor position; perhaps that would be faster than the 200ms timeout?

[sandymcfadden]

You're right, there really isn't a need to keep both and call both. In this case I think keeping and setting the scroll position is the best way. The reason for this is the selected line may not match where you are scrolled to. Also if the line is long and wraps it will really only bring you to the top of that line and not where you were scrolled to.

The downside with scroll position and something I haven't accounted for yet is if the window size changes from the time when the position is saved and when you open the note again. There is a good chance it doesn't take you to the same spot in the note in this case.

[sandymcfadden]

I've updated to use only scroll position now and clear the saved positions of things are resized as I think it is likely better to go to the top of a note rather than a random place if the size changes.

[sandymcfadden]

Thanks @codebykat!

When you switch back to a note, it loads at the top and then jumps to the scroll position. I'm wondering if there's any way to get that to happen when we pass new content to the editor? Does it need the timeout? Can we move it higher up in the componentDidMount function (maybe even inside the bootTimer where we set the editor content)?

I've moved it inside the bootTimer as you suggested and it seems there we don't need the timeout at all. You still get a bit of a jump, but I think the content has to load into the editor before you scroll so I'm not sure there is a way to eliminate it completely.

Switching into Markdown preview obviously doesn't know to scroll to the same part of the note, which is unfortunate. I am not sure there's any easy way to do that but we might want to consider it in future.

Yes, this would require more work and I think a different PR. The same scroll position wouldn't work in preview especially if you have things like markdown images or something that are going to take up a lot of space.

Saving the line I don't think it will really work because of what I mentioned in another comment about long lines that wrap. So I think to do that well would require a fair bit more work.

[codebykat]

Looks good, I think this is working better without the timeout (or it's a placebo effect).

Nice thinking on the resize. I suppose if we wanted to try to make it more bulletproof, we could store both the scroll position and the line, and use revealLineInCenter instead of the scroll position after a resize... but it's probably an edge case and I'm not sure it would work that well anyhow.