yairm210 / Unciv

Open-source Android/Desktop remake of Civ V
Mozilla Public License 2.0
8.51k stars 1.57k forks source link

Feature request: New game screen overloaded / columns scrollable #9419

Open Lakoja opened 1 year ago

Lakoja commented 1 year ago

Problem When tapping "New Game" on a Tablet the screen looks like this. (A "Pixel C" from the Android Studio Emulator.) Game and Map options are horizontally scrollable. Most content is not fully readable (one must scroll rather far horizontally).

grafik

Describe the solution you'd like Not quite sure if tablet is a target audience. If it is this should probably be made more directly accessible: at most vertically scrollable anywhere. For the "Base ruleset" I did exemplatory what I would do for a reactive web page: the label has its own line.

From a consistency perspective it could (should?) maybe be done like the options screen: with selectable tabs for each section. So that each section has the whole screen available.

Related The lower section could probably be reduced (in height). I. e. with removing "Reset to defaults" - as that is likely the same as "close and open the screen again".

SomeTroglodyte commented 1 year ago

"Reset to defaults" - as that is likely the same as "close and open the screen again".

Nope. This screen uses either the exact same settings as a running game did (when you open it from the WorldScreen menu), or the settings from the last successful new-game (when you open it from main menu). Reset was added later "by popular demand" and it resets to Unciv virgin defaults. What's strange is that I've seen a screenshot of that button to the left of the Start one somewhere but can't remember what context. As it is, it's been added late in #5141 to an existing VerticalGroup - my my, why did I not use VerticalGroup.space for that ugly padding?

tabs

Not a bad idea. Why didn't nobody not think of that before? Would also likely make the separate layout for portrait redundant. What do other issue readers think? The header buttons would easily sport icons too!

SomeTroglodyte commented 1 year ago

image

As patch - anyone willing to thoroughly test? ```patch Index: core/src/com/unciv/ui/screens/pickerscreens/PickerScreen.kt IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/core/src/com/unciv/ui/screens/pickerscreens/PickerScreen.kt b/core/src/com/unciv/ui/screens/pickerscreens/PickerScreen.kt --- a/core/src/com/unciv/ui/screens/pickerscreens/PickerScreen.kt (revision d13763ea00d1ac127721842e43abc477fe759ee0) +++ b/core/src/com/unciv/ui/screens/pickerscreens/PickerScreen.kt (date 1684771899073) @@ -32,14 +32,14 @@ } /** Make sure that anyone relying on sizes of the tables within this class during construction gets correct size readings. - * (see [com.unciv.ui.pickerscreens.PolicyPickerScreen]) */ + * (see [com.unciv.ui.screens.pickerscreens.PolicyPickerScreen]) */ private fun ensureLayout() { pickerPane.validate() } /** * Initializes the [Close button][closeButton]'s action (and the Back/ESC handler) - * to return to the [previousScreen] if specified, or else to the world screen. + * to return to the previous screen (see [popScreen][com.unciv.UncivGame.popScreen]) */ fun setDefaultCloseAction() { pickerPane.closeButton.onActivation { Index: core/src/com/unciv/ui/screens/pickerscreens/PickerPane.kt IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/core/src/com/unciv/ui/screens/pickerscreens/PickerPane.kt b/core/src/com/unciv/ui/screens/pickerscreens/PickerPane.kt --- a/core/src/com/unciv/ui/screens/pickerscreens/PickerPane.kt (revision d13763ea00d1ac127721842e43abc477fe759ee0) +++ b/core/src/com/unciv/ui/screens/pickerscreens/PickerPane.kt (date 1684772222210) @@ -2,34 +2,33 @@ import com.badlogic.gdx.scenes.scene2d.Actor import com.badlogic.gdx.scenes.scene2d.ui.Button +import com.badlogic.gdx.scenes.scene2d.ui.HorizontalGroup import com.badlogic.gdx.scenes.scene2d.ui.SplitPane import com.badlogic.gdx.scenes.scene2d.ui.Table -import com.badlogic.gdx.scenes.scene2d.ui.VerticalGroup import com.unciv.Constants import com.unciv.GUI -import com.unciv.UncivGame -import com.unciv.ui.images.IconTextButton import com.unciv.ui.components.AutoScrollPane -import com.unciv.ui.screens.basescreen.BaseScreen import com.unciv.ui.components.extensions.disable import com.unciv.ui.components.extensions.enable import com.unciv.ui.components.extensions.isEnabled import com.unciv.ui.components.extensions.toLabel import com.unciv.ui.components.extensions.toTextButton +import com.unciv.ui.images.IconTextButton +import com.unciv.ui.screens.basescreen.BaseScreen class PickerPane( disableScroll: Boolean = false, ) : Table() { - /** The close button on the lower left of [bottomTable], see [setDefaultCloseAction] */ + /** The close button on the lower left of [bottomTable], see [PickerScreen.setDefaultCloseAction] */ val closeButton = Constants.close.toTextButton() /** A scrollable wrapped Label you can use to show descriptions in the [bottomTable], starts empty */ val descriptionLabel = "".toLabel() /** A wrapper containing [rightSideButton]. You can add buttons, they will be arranged vertically */ - val rightSideGroup = VerticalGroup() + val rightSideGroup = HorizontalGroup() /** A button on the lower right of [bottomTable] you can use for a "OK"-type action, starts disabled */ val rightSideButton = "".toTextButton() - private val screenSplit = 0.85f + private val minBottomTableHeight = 70f // Standard button with paddding private val maxBottomTableHeight = 150f // about 7 lines of normal text /** @@ -59,12 +58,15 @@ scrollPane.setScrollingDisabled(disableScroll, disableScroll) // lock scrollPane if (disableScroll) scrollPane.clearListeners() // remove focus capture of AutoScrollPane too add(splitPane).expand().fill() + splitPane.splitAmount = 1f // Start the bottomTable at minimum height } override fun layout() { super.layout() - bottomTable.height = bottomTable.height.coerceAtMost(maxBottomTableHeight) - splitPane.splitAmount = (scrollPane.height / (scrollPane.height + bottomTable.height)).coerceAtLeast(screenSplit) + val bottomHeight = bottomTable.height.coerceIn(minBottomTableHeight, maxBottomTableHeight) + if (bottomTable.height == bottomHeight) return + bottomTable.height = bottomHeight + splitPane.splitAmount = (stage.height - bottomHeight - splitPane.style.handle.minHeight) / stage.height } /** Enables the [rightSideButton]. See [pick] for a way to set the text. */ Index: core/src/com/unciv/ui/screens/newgamescreen/NewGameScreen.kt IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/core/src/com/unciv/ui/screens/newgamescreen/NewGameScreen.kt b/core/src/com/unciv/ui/screens/newgamescreen/NewGameScreen.kt --- a/core/src/com/unciv/ui/screens/newgamescreen/NewGameScreen.kt (revision d13763ea00d1ac127721842e43abc477fe759ee0) +++ b/core/src/com/unciv/ui/screens/newgamescreen/NewGameScreen.kt (date 1684773843004) @@ -4,7 +4,6 @@ import com.badlogic.gdx.graphics.Color import com.badlogic.gdx.scenes.scene2d.ui.SelectBox import com.badlogic.gdx.scenes.scene2d.ui.Skin -import com.badlogic.gdx.scenes.scene2d.ui.VerticalGroup import com.badlogic.gdx.utils.Array import com.unciv.Constants import com.unciv.UncivGame @@ -19,17 +18,13 @@ import com.unciv.models.metadata.GameSetupInfo import com.unciv.models.ruleset.RulesetCache import com.unciv.models.translations.tr -import com.unciv.ui.components.ExpanderTab import com.unciv.ui.components.KeyCharAndCode -import com.unciv.ui.components.extensions.addSeparator -import com.unciv.ui.components.extensions.addSeparatorVertical +import com.unciv.ui.components.TabbedPager import com.unciv.ui.components.extensions.disable import com.unciv.ui.components.extensions.enable import com.unciv.ui.components.extensions.keyShortcuts import com.unciv.ui.components.extensions.onActivation import com.unciv.ui.components.extensions.onClick -import com.unciv.ui.components.extensions.pad -import com.unciv.ui.components.extensions.toLabel import com.unciv.ui.components.extensions.toTextButton import com.unciv.ui.images.ImageGetter import com.unciv.ui.popups.ConfirmPopup @@ -38,13 +33,12 @@ import com.unciv.ui.screens.basescreen.BaseScreen import com.unciv.ui.screens.basescreen.RecreateOnResize import com.unciv.ui.screens.pickerscreens.PickerScreen -import com.unciv.utils.Log import com.unciv.utils.Concurrency +import com.unciv.utils.Log import com.unciv.utils.launchOnGLThread import kotlinx.coroutines.coroutineScope import java.net.URL import java.util.UUID -import com.unciv.ui.components.AutoScrollPane as ScrollPane class NewGameScreen( _gameSetupInfo: GameSetupInfo? = null @@ -52,9 +46,13 @@ override val gameSetupInfo = _gameSetupInfo ?: GameSetupInfo.fromSettings() override var ruleset = RulesetCache.getComplexRuleset(gameSetupInfo.gameParameters) // needs to be set because the GameOptionsTable etc. depend on this + + /** First tab contents: Rulesets and game options */ private val newGameOptionsTable: GameOptionsTable - private val playerPickerTable: PlayerPickerTable + /** Second tab contents: Map control */ private val mapOptionsTable: MapOptionsTable + /** Third tab contents: Players / Nations */ + private val playerPickerTable: PlayerPickerTable init { val isPortrait = isNarrowerThan4to3() @@ -85,16 +83,15 @@ } pickerPane.closeButton.keyShortcuts.add(KeyCharAndCode.BACK) - if (isPortrait) initPortrait() - else initLandscape() + initTabbed() pickerPane.bottomTable.background = skinStrings.getUiBackground("NewGameScreen/BottomTable", tintColor = skinStrings.skinConfig.clearColor) pickerPane.topTable.background = skinStrings.getUiBackground("NewGameScreen/TopTable", tintColor = skinStrings.skinConfig.clearColor) if (UncivGame.Current.settings.lastGameSetup != null) { - rightSideGroup.addActorAt(0, VerticalGroup().padBottom(5f)) val resetToDefaultsButton = "Reset to defaults".toTextButton() rightSideGroup.addActorAt(0, resetToDefaultsButton) + rightSideGroup.space(10f) resetToDefaultsButton.onClick { ConfirmPopup( this, @@ -219,48 +216,29 @@ // In sync with isPortrait in init, here so UI details need not know about 3-column vs 1-column layout internal fun getColumnWidth() = stage.width / (if (isNarrowerThan4to3()) 1 else 3) - private fun initLandscape() { - scrollPane.setScrollingDisabled(true,true) - - topTable.add("Game Options".toLabel(fontSize = Constants.headingFontSize)).pad(20f, 0f) - topTable.addSeparatorVertical(Color.BLACK, 1f) - topTable.add("Map Options".toLabel(fontSize = Constants.headingFontSize)).pad(20f,0f) - topTable.addSeparatorVertical(Color.BLACK, 1f) - topTable.add("Civilizations".toLabel(fontSize = Constants.headingFontSize)).pad(20f,0f) - topTable.addSeparator(Color.CLEAR, height = 1f) + private fun initTabbed() { + topTable.remove() + scrollPane.remove() - topTable.add(ScrollPane(newGameOptionsTable) - .apply { setOverscroll(false, false) }) - .width(stage.width / 3).top() - topTable.addSeparatorVertical(Color.CLEAR, 1f) - topTable.add(ScrollPane(mapOptionsTable) - .apply { setOverscroll(false, false) }) - .width(stage.width / 3).top() - topTable.addSeparatorVertical(Color.CLEAR, 1f) - topTable.add(playerPickerTable) // No ScrollPane, PlayerPickerTable has its own - .width(stage.width / 3).top() - } + val tabs = TabbedPager(separatorColor = Color.DARK_GRAY, shortcutScreen = this, capacity = 3) + tabs.addPage("Game Options", newGameOptionsTable.top(), + ImageGetter.getImage("OtherIcons/Mods").apply { color = Color.GOLDENROD }, + 24f, + shortcutKey = KeyCharAndCode.ctrl('G') + ) + tabs.addPage("Map Options", mapOptionsTable.top(), + ImageGetter.getImage("OtherIcons/Terrains").apply { color = Color.GREEN }, + 24f, + shortcutKey = KeyCharAndCode.ctrl('M') + ) + tabs.addPage("Civilizations", playerPickerTable.top(), + ImageGetter.getImage("OtherIcons/Nations").apply { color = Color.CORAL }, + 24f, + shortcutKey = KeyCharAndCode.ctrl('C') + ) + tabs.selectPage(0) - private fun initPortrait() { - scrollPane.setScrollingDisabled(false,false) - - topTable.add(ExpanderTab("Game Options") { - it.add(newGameOptionsTable).row() - }).expandX().fillX().row() - topTable.addSeparator(Color.DARK_GRAY, height = 1f) - - topTable.add(newGameOptionsTable.modCheckboxes).expandX().fillX().row() - topTable.addSeparator(Color.DARK_GRAY, height = 1f) - - topTable.add(ExpanderTab("Map Options") { - it.add(mapOptionsTable).row() - }).expandX().fillX().row() - topTable.addSeparator(Color.DARK_GRAY, height = 1f) - - (playerPickerTable.playerListTable.parent as ScrollPane).setScrollingDisabled(true,true) - topTable.add(ExpanderTab("Civilizations") { - it.add(playerPickerTable).row() - }).expandX().fillX().row() + splitPane.setFirstWidget(tabs) } private fun checkConnectionToMultiplayerServer(): Boolean { ```
Lakoja commented 1 year ago

As patch - anyone willing to thoroughly test?

Thank's. That was quick. It works in (my) the aforementioned test setting.

Some minor observations:

And - probably not to be solved in this issue:

SomeTroglodyte commented 1 year ago

That was quick

Not a "solution" yet, just a <1h toy. So, I haven't really taken the old code out except to silence warnings. Getting that button to the left would require even more hacking of the "PickerPane" superclass it's using, so, for this express route, no. Vertical scrollbars - yes I noticed what you mean yesterday, but - quick&dirty and I don't like the scrollbars we currently have. Too ugly (or maybe too plain - or maybe I'm prejudiced because another Widget, SelectBox, makes them really ugly. Just try actually dragging one with a mouse, let alone a finger). What I'd like isn't really easily done - have it alpha-fade over ~1.5 lines of text, both top and botton if there's stuff out of view, which would be indication enough... No, activate scrollbars is better. Should go in the TabbedPager API, however, which could be painful - I'd do that Widget totally different today, and resisting a total rewrite may be hard.

map options have their own "Reset to defaults"?

Yup, that's code grown over time for you. The entire Widget combo is reused elsewhere, so moving the button would be no fun, and no confirmation isn't bad as the "loss" on an accidental click isn't really much. If you tweaked those meaningfully, then you'll likely remember why. But there's another ticket floating around asking for a copy/paste for those settings, so maybe, someday, someone, could do a luxury editor for that simple set of numbers (history w/renaming entries??).

thoroughly test

I also meant:

github-actions[bot] commented 7 months ago

This issue is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 15 days.

SeventhM commented 7 months ago

👀I may take a look at that patch because that menu style looks nice, at least for portrait mode (though, I forgot how to turn on portrait mode). Leaving open anyways as a general "this doesn't work on small screens"