4ian / GDevelop

🎮 Open-source, cross-platform 2D/3D/multiplayer game engine designed for everyone.
https://gdevelop.io
Other
10.91k stars 854 forks source link

The event sheet doesn't remember the scroll position #614

Closed blurymind closed 6 years ago

blurymind commented 6 years ago

To Reproduce

Simply go to the event sheet, scroll, go to the scene tab, then back to the event sheet

KinkGD commented 6 years ago

+1, quite confusing on a looong event sheet.

4ian commented 6 years ago

So this is because any component that is in a not active tab is not rendered (for performance reasons), and so the whole tab is re-rendered after going back to the events sheet.

Technically, this would need to spy on the scrolling position of EventsTree and have it set back to the last value stored when mounted (surely using scrollToPosition of the react-virtualized List that is used by react-sortable-tree: https://github.com/bvaughn/react-virtualized/blob/master/docs/List.md#scrolltoposition-scrolltop-number).

The storage of the scrolling position could maybe be done in the preferences or in some kind of persistant UI layout values - so that the position is also remembered after GDevelop or the game is closed and opened again.

Wend1go commented 6 years ago

This would really be helpful. I also noticed the issue in the objects list. You often switch between event sheet and the scene where you add a new property to an object. Every time you have to scroll through the list of objects again in order to find it.

blurymind commented 6 years ago

@4ian I notice that scroll position does get stored for the scene tab viewport camera. I was wondering if there is already a global mechanism in the newIde to store gui state

4ian commented 6 years ago

@blurymind That's a very good remark, I don't remember right now what is the difference that could allow scene viewport position not to be changed - but as a wild guess I would tell that components are not unmounted so nothing changes, will lists that are built with react-virtualized (i.e: Objects List and the the Events Sheet) will get reset.

So the solution might be simpler than expected if there is a way to "freeze" these lists while not on screen (i.e: when there tab is not active), but not sure exactly how.

blurymind commented 6 years ago

@4ian not absolutely sure if related, but while googling for a solution, I stumbled on this twitter post https://twitter.com/brian_d_vaughn/status/959600888242307072 then https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html

but maybe https://reacttraining.com/react-router/web/guides/scroll-restoration ?

4ian commented 6 years ago

Mmm in fact I was totally wrong about where the problem was coming from 😄 I've fixed this in https://github.com/4ian/GD/commit/3075ff963978a92f8e37f7ee287aa9637a5f25e6

The tabs implementation used from material-ui was, for some reason, hiding tabs by setting their height to 0, collapsing everything including lists or scrolling components that then went back to a scroll position of 0. I've switched to simply hiding the not active tabs (display: none). Will have to double check if it makes any difference, but in theory it should be a bit more performant (not active tabs are not part of the DOM rendering at all with this style).

Closing as this should be in the next version!

blurymind commented 6 years ago

@4ian thank you for the quick fix, also for taking the time to explain the issue