-
Notifications
You must be signed in to change notification settings - Fork 291
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
ReplacePlayerDialog can now edit existing bot configs #4605
Conversation
|
||
Objects.requireNonNull(game); | ||
|
||
Optional<Player> possible = game.getPlayersVector().stream() |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation
|
||
Objects.requireNonNull(game); | ||
|
||
Optional<Player> possible = game.getPlayersVector().stream() |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code largely looks good, just some cleanups first
Looks cool. Should the menu item be renamed "Edit Player" to reflect the expanded functionality? |
Or maybe more appropriately, "Edit Bots..." What say you all? |
Edit Bots makes sense to me |
Should I go through and refactor property names, class names and method names to match? Or is that too disruptive? |
H
Oof. I was just making the suggestion for clarity. That sounds like way too much work. |
Just changing the label is easy, just checking with the group if they think it is worth having the code match. |
I think anything that improves code readability and doesn't create a ton of work is always a win. |
OK, refactoring to rename ReplacePlayersDialog to EditBotsDialog and rename related symbols in code done. |
Add ability to edit existing Princess bots via the Replace Player... menu item.