Need to coordinate scroll and font size preferences

[redmunds]

My new lenovo laptop is high DPI, so I have Brackets editor font-size zoomed at +2 notches larger than default, and Word Wrap is off.

Recipe:

1. Open Brackets
2. View > Increase Font Size (2 times)
3. View > Word Wrap (click to turn off)
4. Open a file with 500 lines or so and put selection at end of file
5. Shutdown and restart Brackets

Results:
Same file is re-opened, cursor position is restored, zoom setting is restored to +2, and page is scrolled down, but it's not scrolled all the way to where cursor is. Seems that the combination of zoom + font-size needs to be coordinated better.

Expected:
Cursor is scrolled into view

This was split off of #7054

[peterflynn]

@redmunds @dangoor Should this be required for Sprint 37 since it's a newly-injected regression?

[redmunds]

It does not repro in Sprint 33.

Interesting that I can see this slightly happens in Sprint 35/36 (scrolling is only off by 12 lines in a 1378 line file). I don't have Sprint 34 installed because new installer makes it more difficult :)

It's much worse in Sprint 37 (scrolling is off by 213 lines in 1344 line file).

I think it's worth a look for Sprint 37, but not critical.

[peterflynn]

Makes sense, I'm assuming it's due to the view-state prefs migration

[dangoor]

Medium priority to @RaymondLim as he likely would be quickest to spot the issue given his work on the view state migration.

@RaymondLim this is a recent regression, so if you can check this out soon that would be helpful. If you don't think you can get to it soon, I can try taking a look.

[RaymondLim]

@redmunds How do you change your Lenovo Win 7 laptop to zoom +2. I think I have the same laptop as you, but don't know what zoom level it is using.

Are you using Control panel Display setting to "Larger - 150%" or "Medium - 125%"? Mine is "Smaller - 100%".

[redmunds]

@RaymondLim This is not OS Zoom, this is Brackets font-size zoom: Ctrl-+ (2 times)

[RaymondLim]

This issue has to do with the scroll positions we saved for each document in the working set. I tried with two different files in the working set by setting cursor on the same line. Once I set cursor on the same line in the second file, then I increase font two times and then reload Brackets. Then I see the issue of incorrect scroll position in the current file. If I switch to the other file in the working set, the scroll position is off by more than the one we had increased font size in the previous session. So I see two issues with my testing. First, the scroll position that we calculate and store for each document is not quite precise (regardless of font size). Second, all inactive files in the working set do not have the correct scroll position remembered (or recalculated in the current session) if we have changed the font size in previous session.

[TomMalbran]

The first issue might be because of #3115 which is still happening.

[redmunds]

@TomMalbran Thanks for adding that link -- I wasn't aware of that issue. As I stated above, part of the problem was introduced in Sprint 34 or 35, and then got much worse since Sprint 36 was released.

[TomMalbran]

@redmunds peter just made me realize that #3115 has been fixed in CM, and the fix was merged to Brackets after sprint 36. This made the hack that I used to calculate how to adjust the scroll after a font resize work really bad.

There is also another issue with not recalculating the saved scroll positions after a font-size change, but I don't think we ever did that before. Maybe a combination of both things makes this seem to be a lot worst in master?

@RaymondLim Maybe instead of recalculating the scroll positions when changing the font size, we should save the font size with the scroll position and adjust the scroll when switching documents and or opening them.

I submitted PR #7118 to fix the scroll adjustment issue when increasing the font size.

[RaymondLim]

@TomMalbran We don't need to do anything else. Your removal of viewport hack indeed fixes this issue.

Update: Actually, the second issue that I described in the above comment still exists. And you're right that we never handle that case. I'll be logging a new issue for that, but we don't need to fix it for sprint 37.

[TomMalbran]

There is still the issue of restoring the scroll position of multiple open editors. If you open 2 or more editors and then increase the font in 1, the others have their scroll position messed up. Same happens when restoring the scroll position from editors that were saved with another font size. But I don't those issues are recent regressions.

Those issues can be fixed when switching documents.

[RaymondLim]

Actually, scroll position issue has nothing to do with view states migration as described. You can see the issue as soon as you increase font size by watching the start line number of the visible editor area.

[redmunds]

This is related to #7125, but the original recipe is not fixed, so leaving open.

[RaymondLim]

Sorry, I still can reproduce it with your exact steps and it is definitely caused by the new view states. Reopening to myself.

[RaymondLim]

I figure out that the real culprit is still the recent CodeMirror update, specifically the fix for #3115 also introduced this issue. The fix for #3115 is only good for the top portion of the page, but not for the bottom portion of the page starting from a specific line number for each page. You can try the steps in this issue and remember the incorrect line number that Brackets scroll to. Then repeat the same steps with a different line number and you will see it still scrolls to the same incorrect line number again. For example, if you scroll to any line number larger than 837 in EditorManager.js in step 4, you will see Brackets always scrolls to 837 after reload. If you try with any line up to line 837, then you will see the correct scroll position after reload.

To see where it goes wrong in the code, you can set a breakpoint on line 116 editor.refreshAll(); in _setSizeAndRestoreScroll function in ViewCommandHandler.js. When you try with a line larger than 837 in EditorManager.js, you will see Brackets already scrolls to the correct position when you hit the breakpoint. Then if you execute the editor.refreshAll(); call, CM then scrolls to line 837 in my example.

[TomMalbran]

That it what similar to what I posted in #7165 (comment). The issue is that we are scrolling before increasing the font-size. So when we scroll, the total height of the editor is less than the height after the font-size is increased, so in some cases, the saved scroll is higher than the total height of the editor, and it gets clipped.

We see this issue now, because #3115 was fixed, which means that now all the lines have the same height. Before that, only the rendered lines have the new height after increasing the font-size. If you can see from line 810, CM, might just render from line 805 more or less, which means that before from line 0-805 the height was 15px and from 805 it could be 17px. So at max, the scroll would be 20px off, which isn't much.

[RaymondLim]

FBNC to @redmunds

[redmunds]

Confirmed. Closing.