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

Leaving game should bring you back to respective lobby #15361

Merged
merged 2 commits into from Jul 23, 2018

Conversation

Projects
None yet
6 participants
@BGluth
Copy link
Contributor

BGluth commented Jul 20, 2018

Leaving a game now brings you back to the respective lobby (#15325)

I'm very new to the code base so please let me know if I could have done this in a better way!

Roughly what I did was whenever the player leaves the main menu and goes into a "game" (missions, skirmish, multiplayer, map editor, and replays), it stores what type of thing they started and when the main menu is loaded later it reopens back up the appropriate menu again.

I'm not sure if this last game state is already available somewhere else but if it is I can just update my PR to use that instead.

Also tested on Dune TD RA and TS.

@BGluth BGluth changed the title 15325 leaving game back to lobby Leaving game should bring you back to respective lobby Jul 20, 2018

@BGluth BGluth force-pushed the BGluth:15325_Leaving_Game_Back_To_Lobby branch from 4ffc6fc to d302840 Jul 20, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Jul 20, 2018

First of all, thanks for taking this on!

Seems there's still a style issue remaining (or rather, it was introduced with the other style fixes).

In my opinion it would be great if we could get this into the playtest, so for now I'm moving it to the milestone.

@reaperrr reaperrr added this to the Next release milestone Jul 20, 2018

@@ -59,6 +59,11 @@ public static class Game
static Task discoverNat;
static bool takeScreenshot = false;

public enum GameState { NONE, MISSIONS, SKIRMISH, MULTIPLAYER, MAP_EDITOR, REPLAYS }

This comment has been minimized.

@pchote

pchote Jul 20, 2018

Member

The engine doesn't impose any specific main menu layout (this is up to the individual mods to defined), so these can't be defined here. Moving them to MainMenuLogic in Mods.Common, making LastGameState static, is an option but this may cause other unwanted sideeffects.

This comment has been minimized.

@BGluth

BGluth Jul 20, 2018

Author Contributor

I wasn't really sure where to put LastGameState since it needed to outlive the MainMenuLogic instance. Could it maybe live in ModData somewhere in a string value dictionary? Like maybe something like this could be used in MainMenuLogic:

var lastGameState = modData.getModSpecificValue("LastGameState")

Then this way mods that have a different layout can handle this on their own.

This comment has been minimized.

@pchote

pchote Jul 20, 2018

Member

Making it static will share it across all MainMenuLogic instances.

@BGluth

This comment has been minimized.

Copy link
Contributor Author

BGluth commented Jul 20, 2018

@reaperrr

Sorry about that. I fixed the initial StyleCop issues and then I must have created some more when I amended my last commit to re-add the end of file new line that I took out by accident.

Edit: I didn't see on the wiki if I should be squashing/amending additional pull request changes. Is there a preference for OpenRA?

@Smittytron

This comment has been minimized.

Copy link
Contributor

Smittytron commented Jul 20, 2018

I didn't see on the wiki if I should be squashing/amending additional pull request changes. Is there a preference for OpenRA?

Squash fixup commits

@BGluth BGluth force-pushed the BGluth:15325_Leaving_Game_Back_To_Lobby branch from d302840 to c7396d4 Jul 20, 2018

@BGluth

This comment has been minimized.

Copy link
Contributor Author

BGluth commented Jul 21, 2018

Applied the requested changes to the PR. Squashed them into one.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Jul 21, 2018

You should also fixup c7396d4 into d7e0d3c. You can leave the author commit as a separate one, but the other two should be merged into one.

@BGluth BGluth force-pushed the BGluth:15325_Leaving_Game_Back_To_Lobby branch from c7396d4 to 235c10e Jul 21, 2018

@BGluth

This comment has been minimized.

Copy link
Contributor Author

BGluth commented Jul 21, 2018

@GraionDilach

Squashed the commits.

@@ -59,6 +59,8 @@ public static class Game
static Task discoverNat;
static bool takeScreenshot = false;

public static event Action OnShellLoaded;

This comment has been minimized.

@pchote

pchote Jul 22, 2018

Member

Style nit: Please call this OnShellmapLoaded for consistency.

@@ -28,6 +28,8 @@ public class MainMenuLogic : ChromeLogic
{
protected enum MenuType { Main, Singleplayer, Extras, MapEditor, SystemInfoPrompt, None }

protected enum GameState { NONE, MISSIONS, SKIRMISH, MULTIPLAYER, MAP_EDITOR, REPLAYS }

This comment has been minimized.

@pchote

pchote Jul 22, 2018

Member

Style nit: Please use TitleCase for enum definitions, and rename this from GameState to MenuPanel.

{ "directConnectPort", 0 },
});
};
mainMenu.Get<ButtonWidget>("MULTIPLAYER_BUTTON").OnClick = () => OpenMultiplayerPanel();

This comment has been minimized.

@pchote

pchote Jul 22, 2018

Member

This can simplify to mainMenu.Get<ButtonWidget>("MULTIPLAYER_BUTTON").OnClick = OpenMultiplayerPanel;

base.Dispose(disposing);
}

public void OpenMenuBasedOnLastGame()

This comment has been minimized.

@pchote

pchote Jul 22, 2018

Member

Style nit: use the lowest level of visibility required to get the job done - remove the public here.

@BGluth BGluth force-pushed the BGluth:15325_Leaving_Game_Back_To_Lobby branch from 235c10e to d7554ea Jul 22, 2018

@BGluth

This comment has been minimized.

Copy link
Contributor Author

BGluth commented Jul 22, 2018

@pchote
Made requested changes.

@pchote
Copy link
Member

pchote left a comment

One last minor nit, then LGTM.

Thanks for working on this! It's a nice usability improvement to include in the next release.

@@ -59,6 +59,8 @@ public static class Game
static Task discoverNat;
static bool takeScreenshot = false;

public static event Action OnShellMapLoaded;

This comment has been minimized.

@pchote

pchote Jul 22, 2018

Member

Can you please rename this to OnShellmapLoaded (little m)? We use "Shellmap" as a single word everywhere except the LoadShellMap method.

@BGluth BGluth force-pushed the BGluth:15325_Leaving_Game_Back_To_Lobby branch from d7554ea to 2990f9a Jul 22, 2018

BGluth added some commits Jul 20, 2018

Implemented #15325
- Leaving a game now returns you to the respective menu.
- I think that I covered all of the possibilities (mission, skirmish, multiplayer, map editor, replay).

@BGluth BGluth force-pushed the BGluth:15325_Leaving_Game_Back_To_Lobby branch from 2990f9a to 3ef17aa Jul 22, 2018

@BGluth

This comment has been minimized.

Copy link
Contributor Author

BGluth commented Jul 22, 2018

Made the changes.

Haha I think this is the 5th time I changed this commit? I feel like I should have caught a lot of these before I pushed so sorry about going back and forth like this.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Jul 23, 2018

Such stuff are quite common within all the PRs, no surprises there, really. Don't be too serious about that.

@Smittytron
Copy link
Contributor

Smittytron left a comment

Works as advertised.

@pchote

pchote approved these changes Jul 23, 2018

@pchote pchote merged commit e5bbe70 into OpenRA:bleed Jul 23, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@pchote pchote referenced this pull request Jul 23, 2018

Merged

Add a newspost for the first mid-2018 playtest #391

4 of 4 tasks complete

@BGluth BGluth deleted the BGluth:15325_Leaving_Game_Back_To_Lobby branch Jul 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.