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

Refactor the save/load file list "Widget" #13124

Closed

Conversation

SomeTroglodyte
Copy link
Collaborator

... to prepare to remove the code duplication in MapEditorFilesTable. Should be entirely transparent - I hope.

Can't test Android though, somehow this setup is hosed, all knowledge about modules is gone (and thus the run config invalid) and stays so no matter how I mix Invalidate Caches, Repair IDE and Gradle Sync...

Mainly

  • Separate the Table and FileHandle/Buttons part out of VerticalFileListScrollPane
  • DoubleClick handling inside instead of on the Container
  • More consistent callback scheme up in and into the actual PickerScreen layer
  • I opted for a different callback scheme for the new Widget, though - as overridable methods - felt cleaner at the time. A few more source lines, maye a tad more efficient bytecode, but IMO slightly clearer to see where things from the hosting class are pulled in. Can do the lambda form instead if you insist.

@yairm210
Copy link
Owner

yairm210 commented Apr 1, 2025

I don't see anything out of the ordinary, I wonder if the abstraction is worth it - clarity > deduplication in my opinion 🤔

@SomeTroglodyte
Copy link
Collaborator Author

wonder if the abstraction is worth it

Maybe, we'll see. Map Editor additionally carries mods around, so I'll see how well that meshes. This step seemed necessary because the map editor is a TabbedPager which already handles scrolling of the pages, so it was either lock one of the ScrollPanes - often leading to hard to track minor UI glitches - or this.

That is to say, I could set this to draft until I know how that story unfolds - or it looks cleaner enough to you as is.

@SomeTroglodyte
Copy link
Collaborator Author

I was wrong. MapEditorFilesTable isn't hosted in the TabbedPager's ScrollPane after all. Will need to see where the actual deduplication takes me.

@SomeTroglodyte SomeTroglodyte marked this pull request as draft April 2, 2025 05:36
@SomeTroglodyte
Copy link
Collaborator Author

No, ends up as no advantage. Still, I'll want to cherry-pick bullet points 2 and 3 to move on...

@SomeTroglodyte SomeTroglodyte deleted the RefactorFilesListTable branch April 3, 2025 11:56
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