Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CLOSED] Need to coordinate scroll and font size preferences #6421

Open
core-ai-bot opened this issue Aug 30, 2021 · 19 comments
Open

[CLOSED] Need to coordinate scroll and font size preferences #6421

core-ai-bot opened this issue Aug 30, 2021 · 19 comments

Comments

@core-ai-bot
Copy link
Member

Issue by redmunds
Wednesday Mar 05, 2014 at 15:55 GMT
Originally opened as adobe/brackets#7093


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

@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Wednesday Mar 05, 2014 at 19:15 GMT


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

@core-ai-bot
Copy link
Member Author

Comment by redmunds
Wednesday Mar 05, 2014 at 19:28 GMT


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.

@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Wednesday Mar 05, 2014 at 20:08 GMT


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

@core-ai-bot
Copy link
Member Author

Comment by dangoor
Thursday Mar 06, 2014 at 15:31 GMT


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.

@core-ai-bot
Copy link
Member Author

Comment by RaymondLim
Thursday Mar 06, 2014 at 17:59 GMT


@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%".

@core-ai-bot
Copy link
Member Author

Comment by redmunds
Thursday Mar 06, 2014 at 18:20 GMT


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

@core-ai-bot
Copy link
Member Author

Comment by RaymondLim
Thursday Mar 06, 2014 at 19:26 GMT


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.

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Thursday Mar 06, 2014 at 19:55 GMT


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

@core-ai-bot
Copy link
Member Author

Comment by redmunds
Thursday Mar 06, 2014 at 20:03 GMT


@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.

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Friday Mar 07, 2014 at 04:14 GMT


@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.

@core-ai-bot
Copy link
Member Author

Comment by RaymondLim
Friday Mar 07, 2014 at 04:56 GMT


@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.

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Friday Mar 07, 2014 at 05:02 GMT


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.

@core-ai-bot
Copy link
Member Author

Comment by RaymondLim
Friday Mar 07, 2014 at 06:48 GMT


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.

@core-ai-bot
Copy link
Member Author

Comment by redmunds
Monday Mar 10, 2014 at 22:47 GMT


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

@core-ai-bot
Copy link
Member Author

Comment by RaymondLim
Tuesday Mar 11, 2014 at 17:07 GMT


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

@core-ai-bot
Copy link
Member Author

Comment by RaymondLim
Wednesday Mar 12, 2014 at 17:00 GMT


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.

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Wednesday Mar 12, 2014 at 19:31 GMT


That it what similar to what I posted in adobe/brackets#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.

@core-ai-bot
Copy link
Member Author

Comment by RaymondLim
Friday Mar 14, 2014 at 23:32 GMT


FBNC to@redmunds

@core-ai-bot
Copy link
Member Author

Comment by redmunds
Saturday Mar 15, 2014 at 00:02 GMT


Confirmed. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant