Repository navigation
Feature 62: Automatic game end - #80
tommy33790 wants to merge 16 commits into
Conversation
DivineThreepwood
left a comment
DivineThreepwood
left a comment
There was a problem hiding this comment.
Thanks for putting this together! I have a few concerns that I’d like to address before we move forward.
From a code perspective, there is currently quite a bit of boilerplate, and some parts feel more complex than necessary. The implementation also follows a rather Java-oriented style in several places. Since this is Kotlin, I’d prefer to simplify and make the code more idiomatic where possible.
I also think we could improve the separation of concerns. At the moment, the Game Manager is taking on quite a lot of responsibility, and the game start/stop flows are becoming rather large as a result. It might be worth restructuring this so that these responsibilities are separated more clearly.
From a UX perspective, I’m also not fully convinced by the current placement of the settings. I would prefer to have them in the top menu. In addition, the current alignment and positioning of the visual components feels a little inconsistent and could benefit from some cleanup.
That said, considering our current timeline and the announced feature freeze, I would personally prioritize more critical topics at this point. Since this functionality can still be handled manually for now, I don’t consider it essential for the upcoming release.
So my preference would be to postpone this feature and revisit it afterwards, when we have enough time to properly refine both the implementation and the UX.
| private var secondsRemaining = durationInMs / 1000 | ||
| private var fired: Boolean = false | ||
|
|
||
| init { // This is a blatant copy of LevelMenuPanel, thus knowingly violating single source of truth |
| import java.awt.event.ActionListener | ||
| import javax.swing.Timer | ||
|
|
||
| // This still de-syncs slightly from the timer in LevelMenuPanel |
There was a problem hiding this comment.
is this an open todo / issue? Or what is the intent of this comment? :)
| private var gameEndDurationInMs: Long = 0 | ||
| private var gameEndCalcDurationInMs: Long = 0 |
There was a problem hiding this comment.
why not using Duration for that?
| fun setGameEndDuration(durationInMs: Long) { | ||
| gameEndDurationInMs = durationInMs | ||
| } | ||
|
|
||
| fun setGameEndCalcDuration(durationInMs: Long) { | ||
| gameEndCalcDurationInMs = durationInMs | ||
| } |
There was a problem hiding this comment.
thats java not kotlin, just make the var public accessable.
| private var gameEndTimeout: GameTimeout? = null | ||
| private var gameEndCalcTimeout: GameTimeout? = null |
There was a problem hiding this comment.
make non nullable and directly initialize it please.
There was a problem hiding this comment.
The calculation is always at the end of the game, thus this names are quite confusing. Furthermore its not a timeout of the game end but a game timeout right ;)
| if (state == GameState.Break) { | ||
| isPause = true | ||
| gameEndCalcTimeout?.stopTimer() | ||
| gameEndTimeout?.stopTimer() | ||
| } else if (isPause && gameState == GameState.Running) { | ||
| isPause = false | ||
| gameEndCalcTimeout?.startTimer() | ||
| gameEndTimeout?.startTimer() | ||
| } | ||
|
|
There was a problem hiding this comment.
why not letting the timeout consume the GameState? by that you could save lots of boilerplate code.
| private var finished = false | ||
|
|
||
| private fun finalizeGame() { | ||
| fun finalizeGame() { |
DivineThreepwood
added
changes requested
low-priority
and removed
ready-to-test
ready-to-review
labels
This branch has not been deployed
2 participants
📜 Description
Changes proposed in this pull request:
SpeedcontrolFrame) to prevent issues with saving values or changing them after starting a levelLevelMenuPanel(after changing game speed the initial delay was still 1000ms)LevelMenuPanelwhen changing it while in configuration screenfinalizeGame)🔨 Breaking Changes
✅ Checklist:
📸 Screenshots
Screenshots to review the UX/UI Design:
