godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
89.92k stars 21.05k forks source link

Previously opened script's horizontal scrollbars are shifted after application restart #33865

Open p10tr3k opened 4 years ago

p10tr3k commented 4 years ago

Godot version: v3.2.beta.custom_build.28613ab8c

OS/device including version: Windows 10

Issue description: Godot remembers for each script position of a text cursor. And after restart tries to set position of the horizontal scrollbar so that the text cursor is located at the beginning of the editor space. Scrollbar should always be at position 0 (or as it was before)

scroll_offset_bug

Calinou commented 4 years ago

I think it makes sense to remember vertical scrolling in the script editor, but not horizontal scrolling as it's rarely used.

KoBeWi commented 4 years ago

The scrolling isn't restored at all. It's correctly saved and loaded, but then TextEdit does some inexplicable magic and the scroll actually focuses on the cursor. With wide scripts it results in horizontal scroll being shifted.

EDIT: This is the line that causes the issue. Not sure what to do about it. https://github.com/godotengine/godot/blob/71d372a8ab1ee972326f8bd333f510330b7b7204/scene/gui/text_edit.cpp#L431

CC @Paulb23 >_> The issue is really annoying.

Calinou commented 4 years ago

Maybe horizontal scrolling should always be discarded? It's likely not commonly used on most displays anyway. Vertical scrolling is much more important to keep in comparison :slightly_smiling_face:

KoBeWi commented 4 years ago

The problem is that after restoring scrolls there's a call to TextEdit::_update_scrollbars() which moves the scrollbars to some weird initial position near the cursor. "Discarding" the stored scroll doesn't help. Also restoring it using set_deferred doesn't help either.

TheSecurityDev commented 4 years ago

I hope this is fixed soon because it's really annoying.

Calinou commented 4 years ago

@TheSecurityDev Please don't bump issues without contributing significant new information. Use the :+1: reaction button on the first post instead.

ice-blaze commented 4 years ago

Maybe horizontal scrolling should always be discarded? It's likely not commonly used on most displays anyway. Vertical scrolling is much more important to keep in comparison slightly_smiling_face

@Calinou I believe it's a good thing to keep the state of the scroll bar of where the user left it. Because the code already allow that I think it would be sad to remove a nice feature. Of course the feature should work and the current issue seems to be a regression instead of a bad feature.

@KoBeWi I made a messed up fix on my fork, it works but I guess I removed too many things. Might help you to understand a bit more. I believe one issue is that when we change the value of the scroll bar the events are fired, and there are many, many events that starts to be fired, and one seems to have the wrongs values. I'm quite bad for debugging this project since I'm new here. https://github.com/ice-blaze/godot/commit/a71b6e5c6387c3d753e6f93e5c4182f2ab3cdf69 I'll still continue to do some more research.

Also I found that the function _scroll_moved(arg) do not use it's argument at all... So it's confusing. It looks like at start of the function the argument was useful and then after refactoring the argument became useless and more logic where added into the function (I prefer keeping function simple and doing only one job, anyway I'm new here so my favorite code style are not important).

In the future it would be nice to have tests that check regression like this issue. (I would like to add them if there doesn't exist yet)

Calinou commented 4 years ago

In the future it would be nice to have tests that check regression like this issue. (I would like to add them if there doesn't exist yet)

See #30743.

Leleat commented 4 years ago

So from what I've understood the scrollbar should be on the far left on startup, right?

A simple solution would be to just move the cursor to that position on the start. Would that be an acceptable solution or is it too dumb since it ignores the actual problem?

KoBeWi commented 4 years ago

IMO it would be acceptable. The cursor position is much less relevant than current line.

prominentdetail commented 1 year ago

Any updates on this? Why was it removed from 4.0 milestone? Been 3 years since original post, and seems like this would be a simple fix.

KoBeWi commented 1 year ago

It was removed from the milestone, because it's not a critical issue. Also the fix is not that simple. I gave it a shot once, but the logic that causes the issue is convoluted.

dataCobra commented 1 year ago

How do we get more attention for that issue?

This is super annoying and should be fixed.

lostminds commented 7 months ago

Based on the now closed https://github.com/godotengine/godot/pull/42033 it looks like the issue might be that when setting the script cursor column position the script editor isn't properly sized yet, so it changes the h scroll position because it thinks it's needed to fit the cursor position on screen.

However, since the logic behind when and how this sizing and cursor setting happens seems difficult to fix, perhaps it would be easier to bypass this in line with @Leleat's original suggestion by simply disregarding the column field for each script resource (setting it to 0) when loading the script_editor_cache.cfg file at startup (not sure where this is?). That way the cursor will just be placed at the start of the row instead of the last position, but this seems like a small sacrifice to fix this annoying issue.

prominentdetail commented 7 months ago

If it isn't working as it should, and is counter-intuitive, the best thing would be to just remove the horizontal positioning completely until someone can implement it appropriately. Just have it set to 0. It's annoying having it set to anything else.

Calinou commented 7 months ago

I think it makes sense to salvage what https://github.com/godotengine/godot/pull/42033 did (even if that means discarding the column when reloading), but TextEdit has been refactored into multiple classes including CodeEdit so it can't be cherry-picked as-is.

amarraff commented 5 months ago

Just had this issue today, glad it's being talked about. I would like to see horizontal scroll position be set to the far left/0 by default as well. Perhaps an editor option could be used to override it, and make it work how it currently does. But, the default being leftmost would make sense. The only thing I usually have off-screen to the right are comments for code context, and long math functions. The left most side takes priority for me.

akien-mga commented 2 months ago

Not fully fixed by #94439.

This doesn't fix https://github.com/godotengine/godot/issues/33865 in all situations, such as when the editor is opened. It may depend on resolution.