Skip to content

RoundChangeLifecycleStageEvent to implement the cancellable interface#21

Open
KieronWiltshire wants to merge 5 commits intocaseif:developfrom
KieronWiltshire:develop
Open

RoundChangeLifecycleStageEvent to implement the cancellable interface#21
KieronWiltshire wants to merge 5 commits intocaseif:developfrom
KieronWiltshire:develop

Conversation

@KieronWiltshire
Copy link
Copy Markdown

@KieronWiltshire KieronWiltshire commented Jul 24, 2017

Proposal

The RoundChangeLifecycleStageEvent event should implement the cancellable interface in order to prevent the round from crossing over to the next stage in the life cycle. The current API restricts a stage to a single state of completion, this being the specified duration of time. It may be a need for some to perform other checks in order to determine whether or not the life cycle is completed.

Consider this scenario

You have a stage in your life cycle called "Lobby". During this stage, you may want to perform a check to determine the number of players that have currently joined the round. If the number of players are less than 6, you wish to reset the lobby timer, and wait for more players.

Conclusion

Making this simple adjustment to the API would allow these types of scenarios to be carried out with very little effort.

@caseif
Copy link
Copy Markdown
Owner

caseif commented Jul 25, 2017

I'm thinking it may be better to have a method such as RoundChangeLifecycleStageEvent#getAdvanceCause which returns enum value AdvanceCause. This way, the API method is a little more extensible and its exact purpose is clearer. Of course, I'm totally open to discussion about that.

@KieronWiltshire
Copy link
Copy Markdown
Author

Well the purpose of this would be to halt the change event, thus resetting that lifecycle stage. What would be the purpose of AdvanceCause?

@caseif
Copy link
Copy Markdown
Owner

caseif commented Jul 28, 2017

It would expose the same functionality as the current isDone method in a more intuitive way (IMO).

Also, the semantics of cancelling a stage change would need to be considered and documented - should the timer be stopped, or simply keep ticking? I'm personally leaning more towards the latter, but again. that's open to discussion.

@KieronWiltshire
Copy link
Copy Markdown
Author

KieronWiltshire commented Jul 28, 2017

Im not sure you fully understand what I propose? The AdvanceCause is something to be considered but has no difference to what I propose.

Right now the current implementation only allows a set timer to exist, and once the countdown has completed, the stage is forcibly changed which may be the unwanted action, as previously mentioned, imagine you have a LOBBY stage with 4 players, but let's say the minigame requires 6 players minimum to begin, then the LOBBY change event would be cancelled thus resetting the LOBBY stage in the lifecycle, couting down again until the requirements are met, which in this case is a minimum of 6 players.

@caseif
Copy link
Copy Markdown
Owner

caseif commented Jul 28, 2017

I understand what you're proposing; I'm just pointing out that AdvanceCause may be a better way to expose the new event semantics as opposed to a boolean getter.

That being said, I disagree that the timer should be reset to its initial state, as that would equate to only partial cancellation of the event, which I feel is unintuitive. My question is whether the timer should be stopped, or keep ticking once the event has been cancelled.

Also, @jamierocks - any input you could provide on this would be much appreciated.

@KieronWiltshire
Copy link
Copy Markdown
Author

keep ticking? the minigame isn't over just because a stage in the lifecycle wants to be reset?
If resetting the timer is unintuitive then allow the user to specify exactly what happens when the event is cancelled.

@caseif
Copy link
Copy Markdown
Owner

caseif commented Jul 28, 2017

Halting the timer doesn't end the minigame; it does just that: halt the timer.

Explicit specification of behavior isn't technically possible while still implementing the Cancellable interface.

@KieronWiltshire
Copy link
Copy Markdown
Author

hmm, halt the timer then.

@caseif
Copy link
Copy Markdown
Owner

caseif commented Jul 30, 2017

The Javadoc needs to be updated to describe the semantics before I can consider a merge. Also, what's your opinion on a boolean flag versus a separate AdvanceCause enum?

@KieronWiltshire
Copy link
Copy Markdown
Author

Lets do it! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants