Skip to content

Map editor reuses the files widget for load/save #13173

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

Merged
merged 5 commits into from
Apr 11, 2025

Conversation

SomeTroglodyte
Copy link
Collaborator

... plus a few other things, see commit names.

User-visible:

  • Changes PageUp/PageDown behaviour for all clients of the widget. Both are intuitive so most won't notice, but I like the new one marginally better plus it's less code.
  • Map Editor load/save gains PageUp/PageDown which it didn't have - and here it's more useful as you can easily get tons of maps with a few mods.
  • Stops arrow keys from still panning the open map while map editor's load/save is open - minor, true, but felt right.

And the surprise:

  • Fixes a borderline leak: Previously, triggering a MapHolder reload or WorldScreen go-to-options or MapEditor-load-another-map while a WASD or ⬅️⬆️⬇️➡️ key is being held will remove the listener and orphan it to be GC'ed but - its reference will still live on the stage in the forever action which is not stopped in that case, also keeping the MapHolder instance alive... Haven't actually proven that in the debugger though, too lazy.

Copy link
Owner

@yairm210 yairm210 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly there's so much change that for this one I'm basically blind approving and trusting your judgement and prior history of good changes
I can't find anything obviously wrong

if (it < 0) existingSavesTable.rows - 1
else if (it >= existingSavesTable.rows) 0
else it
private fun getButtonAt(y: Float): FileHandleButton {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is unclear to me

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PageUp / PageDown keys go by virtual screen coordinates, unlike Up/Down/Home/End. Since in Map Load the actual buttons are NOT linearly distributed, I go and find the closest button to a "one screen up from current" coordinate. The function existed before, only limited to a rigid one-file - one-cell and all cells same height structure. That meant it's not as simple as asking Gdx what's there, because you would have to treat the decoration rows too, and the complexity wouldn't have been better. That it's a binary search is just my mean-ness.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, that reminds me - I dreamed of another nice kind of Easter-Egg idea image, but forgot waking up.

@SomeTroglodyte
Copy link
Collaborator Author

so much change

Yeah! I was so confident I had prepared the load/save stuff well enough to make this a simple plug-it-it, and then it worked perfectly except sometimes it crashed on navigating... Dim light bulb igniting, and bang some approaches were ditched and rewritten differently.

@yairm210 yairm210 merged commit ff896fd into yairm210:master Apr 11, 2025
3 checks passed
@SomeTroglodyte SomeTroglodyte deleted the MapEditorFilesScroll branch April 11, 2025 23:19
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

Successfully merging this pull request may close these issues.

2 participants