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

Added a player action dropdown for handling kick, promote to admin and move to spectators. #15307

Merged
merged 4 commits into from Jul 5, 2018

Conversation

Projects
None yet
4 participants
@teinarss
Copy link
Contributor

teinarss commented Jun 29, 2018

Added a dropdown to the lobby for handlig kick, move to spectators and promote to Admin for the admin.

server.SendOrderTo(conn, "Message", "Malformed state command");
return true;
}
{ "state", State(server, conn, client) },

This comment has been minimized.

@pchote

pchote Jun 29, 2018

Member

Can you please split this (much needed!) refactoring change into its own commit before the commit adding the new behaviour? (this is now done)

() => panel = PanelType.Kick, () => panel = PanelType.Players);
}
else
{

This comment has been minimized.

@pchote

pchote Jun 29, 2018

Member

Style nit: omit braces around single line blocks. This may apply to other places in this PR too.

@CatGirls420-NerevarII
Copy link

CatGirls420-NerevarII left a comment

Yes, purrfect. This would be extremely useful.

A button for kick/ban would be more useful, as some players tend to join and leave quick enough repeatedly, avoiding a kick or ban, because you can't click fast enough on the current button, then have to click again on the option to kick or ban.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Jun 30, 2018

some players tend to join and leave quick enough repeatedly,

That would be better solved in other ways, such as disabling the leave server button for the first five seconds after you join a server if the game detects you are doing this too often.

@CatGirls420-NerevarII

This comment has been minimized.

Copy link

CatGirls420-NerevarII commented Jun 30, 2018

That would be better solved in other ways, such as disabling the leave server button for the first five seconds after you join a server if the game detects you are doing this too often.

100% agree, good idea.

@pchote
Copy link
Member

pchote left a comment

The implementation here looks good, and i'd ideally like to get this merged for the playtest. I found a couple of obvious bugs that we'll need to fix in addition to my style fix requests above:


var position = new int2(rb.X + LeftMargin, rb.Y - BaseLine + (Bounds.Height - textSize.Y) / 2);

if (Align == TextAlign.Center)

This comment has been minimized.

@pchote

pchote Jun 30, 2018

Member

This appears to have regressed normal buttons, which now have text all offset to the right.

Y: 7
Font: Regular
Visible: false
Align: Right

This comment has been minimized.

@pchote

pchote Jun 30, 2018

Member

This doesn't look right:
screen shot 2018-06-30 at 16 58 28

{
new ShowPlayerActionDropDownOption("Promote to Admin", () => { orderManager.IssueOrder(Order.Command("promote {0}".F(c.Index))); }),
new ShowPlayerActionDropDownOption("Kick", onClick),
new ShowPlayerActionDropDownOption("Move to spectator", () => { orderManager.IssueOrder(Order.Command("move_to_spectators {0}".F(c.Index))); })

This comment has been minimized.

@pchote

pchote Jun 30, 2018

Member

IMO we should capitalize Spectator like we do Admin.

new ShowPlayerActionDropDownOption("Kick", onClick),
new ShowPlayerActionDropDownOption("Move to spectator", () => { orderManager.IssueOrder(Order.Command("move_to_spectators {0}".F(c.Index))); })
{
IsVisible = () => !c.IsObserver && orderManager.LobbyInfo.GlobalSettings.AllowSpectators

This comment has been minimized.

@pchote

pchote Jun 30, 2018

Member

This leaves a hole when opening the menu on a spectator:
screen shot 2018-06-30 at 17 00 56

Define the list with just the Kick option, then add this and the admin option to options only if the appropriate conditions are true.


var options = new List<ShowPlayerActionDropDownOption>
{
new ShowPlayerActionDropDownOption("Promote to Admin", () => { orderManager.IssueOrder(Order.Command("promote {0}".F(c.Index))); }),

This comment has been minimized.

@pchote

pchote Jun 30, 2018

Member

This allows players who are hosting their own local servers to hand over admin to another player. We only want to enable this on dedicated servers – this can be fixed similarly to the spectator comment above.

This comment has been minimized.

@teinarss

teinarss Jul 1, 2018

Author Contributor

What would be the best way to detect this?

This comment has been minimized.

@pchote

pchote Jul 1, 2018

Member

Good question. If this was server-side code then it would be easy: we have a Dedicated flag on the Server class. This could perhaps be moved to the Global class where it would be automatically synced to clients, and then accessed from here as orderManager.LobbyInfo.GlobalSettings.Dedicated.

@teinarss teinarss force-pushed the teinarss:promote-to-admin branch from a2141ad to f52d61f Jul 1, 2018

@teinarss

This comment has been minimized.

Copy link
Contributor Author

teinarss commented Jul 1, 2018

Fixed the issues that were raised.

Having problem reproducing the button text alignment issue, for me all the buttons have the text alignment set as center.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Jul 1, 2018

for me all the buttons have the text alignment set as center.

Compare the TD main menu before/after:
screen shot 2018-07-01 at 14 54 12
screen shot 2018-07-01 at 14 51 16

@teinarss

This comment has been minimized.

Copy link
Contributor Author

teinarss commented Jul 1, 2018

Good catch! Should be fixes now.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Jul 1, 2018

Can you please do a couple of quality of life changes to simplify commit-by-commit reviewing ?

  • The first commit has 7 newlines in a row between the InterpretCommand and the SyncLobby method definitions. This really confuses the diff view and i'm surprised it doesn't trigger a StyleCop error.
  • Can you please squash the alignment fix into the second commit, where the regression was introduced?

@teinarss teinarss force-pushed the teinarss:promote-to-admin branch from b905b1e to 37d8b76 Jul 1, 2018

@teinarss

This comment has been minimized.

Copy link
Contributor Author

teinarss commented Jul 1, 2018

Fixed it!

@pchote
Copy link
Member

pchote left a comment

A few review comments. All straightforward I hope!

@@ -87,6 +90,7 @@ protected ButtonWidget(ButtonWidget other)
ModRules = other.ModRules;

Text = other.Text;
Align = other.Align;

This comment has been minimized.

@pchote

pchote Jul 1, 2018

Member

LeftMargin and RightMargin need to be cloned too!

var stateOffset = Depressed ? new int2(VisualHeight, VisualHeight) : new int2(0, 0);
var position = new int2(rb.X + (UsableWidth - s.X) / 2, rb.Y - BaseLine + (Bounds.Height - s.Y) / 2);

var position = new int2(rb.X + LeftMargin, rb.Y - BaseLine + (Bounds.Height - textSize.Y) / 2);

This comment has been minimized.

@pchote

pchote Jul 1, 2018

Member

Style nit: It would be cleaner to keep the LeftMargin out of this line and then have a switch(Align) where you add the offsets for all cases (including left align). This improves the symmetry between the three cases, and you then don't need to subtract off the left margin in the others.

@@ -95,6 +95,8 @@ public bool InterpretCommand(S server, Connection conn, Session.Client client, s
{ "option", Option(server, conn, client) },
{ "assignteams", AssignTeams(server, conn, client) },
{ "kick", Kick(server, conn, client) },
{ "promote", Promote(server, conn, client) },

This comment has been minimized.

@pchote

pchote Jul 1, 2018

Member

IMO these names could be made a bit more consistent. How about something like "make_admin" / MakeAdmin and "make_spectator" / MakeSpectator?

server.SendMessage("{0} is now the admin.".F(newAdminClient.Name));
Log.Write("server", "{0} is now the admin.".F(newAdminClient.Name));
server.SyncLobbyClients();
server.SyncLobbySlots();

This comment has been minimized.

@pchote

pchote Jul 1, 2018

Member

Is syncing the slots necessary here?

}
else
{
LobbyUtils.SetupSlotWidget(template, slot, client);

This comment has been minimized.

@pchote

pchote Jul 1, 2018

Member

Was this intentional?


CheckAutoStart(server);
static Func<string, bool> SyncLobby(S server, Connection conn, Session.Client client)

This comment has been minimized.

@pchote

pchote Jul 1, 2018

Member

Can you please reorder these to match the original switch (i.e. the order from the dict definition above). This will help git identify the real changes in this file and preserve the git history, instead of overriding it with a bunch of misattributed additions/deletions.

return true;
}
{ "state", State(server, conn, client) },
{ "startgame", StartGame(server, conn, client) },

This comment has been minimized.

@pchote

pchote Jul 1, 2018

Member

These all take the same arguments, so we can simplify the definitions and remove a level of abstraction by referring to the static methods directly:

public bool InterpretCommand(S server, Connection conn, Session.Client client, string cmd)
{
	if (server == null || conn == null || client == null || !ValidateCommand(server, conn, client, cmd))
		return false;

	var dict = new Dictionary<string, Func<S, Connection, Session.Client, string, bool>>
	{
		{ "state", State },
		// Other command definitions
	};

	var cmdName = cmd.Split(' ').First();
	var cmdValue = cmd.Split(' ').Skip(1).JoinWith(" ");

	Func<S, Connection, Session.Client, string, bool> a;
	if (!dict.TryGetValue(cmdName, out a))
		return false;

	return a(server, conn, client, cmdValue);
}

static bool State(S server, Connection conn, Session.Client client, string s)
{
	var state = Session.ClientState.Invalid;
	// .. the rest of the body of State, removing the s => { } wrapper
	return true;
}

// Other command definitions

This comment has been minimized.

@pchote

pchote Jul 1, 2018

Member

The dict definition could then be pulled out to a field on the object, which avoids unnecessary allocations each time the InterpretCommand method is called.

@pchote pchote added this to the Next release milestone Jul 1, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Jul 1, 2018

I'm going to go ahead and put this on the milestone. This aligns nicely with one of the big themes of the next release (improving the lobby: we have the new color picker, selection in text fields, and hopefully player profiles in addition to this) and I don't forsee any major gotchas that could delay reviewing longer than the other milestone PRs.

@teinarss

This comment has been minimized.

Copy link
Contributor Author

teinarss commented Jul 2, 2018

Update: Fixes the issues that pchote mentioned and did some cleaning in LobbyCommands.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Jul 2, 2018

Can you please squash the fixups into their parent commits again before I re-review?

@teinarss teinarss force-pushed the teinarss:promote-to-admin branch from 4db569d to cfdceb5 Jul 3, 2018

@teinarss

This comment has been minimized.

Copy link
Contributor Author

teinarss commented Jul 3, 2018

Update: Squashed!

@teinarss teinarss force-pushed the teinarss:promote-to-admin branch 2 times, most recently from 633e949 to c9cdcee Jul 4, 2018

Code updated.

@abcdefg30
Copy link
Member

abcdefg30 left a comment

Haven't tested this thoroughly, but it looks good to me. (At least :+.5:)

teinarss added some commits Jul 5, 2018

Added text aligment to button
Cloning LeftMargin and RightMargin

Refactored the calculations for Text position in ButtonWidget

@pchote pchote force-pushed the teinarss:promote-to-admin branch from c9cdcee to 338e5cb Jul 5, 2018

@pchote pchote force-pushed the teinarss:promote-to-admin branch from 338e5cb to 1d58063 Jul 5, 2018

Added a player action dropdown.
Adds options for:
 - handling kick
 - transferring admin
 - move to spectator

@pchote pchote force-pushed the teinarss:promote-to-admin branch from 1d58063 to 977c4ea Jul 5, 2018

@pchote

pchote approved these changes Jul 5, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Jul 5, 2018

I made two minor tweaks to this PR before approving:

  • Moved a couple of functions in LobbyCommands to minimize changes in the (whitespace-suppressed) diff.
  • Renamed "Promote to Admin" to "Transfer Admin" to make it clear that this affects both the giver and receiver.
  • Added missing newlines between dropdown definitions in the yaml.
@pchote

This comment has been minimized.

Copy link
Member

pchote commented Jul 5, 2018

23:11 <+pchote> shall I merge or do you want to check my tweaks?
23:11 <+abcdefg30> what else other than the transfer change was made?
23:12 <+pchote> I added newlines between the new dropdown definitions in the yaml
23:12 <+abcdefg30> ah, so that was the first push
23:12 <+abcdefg30> should be alright then +1

@pchote pchote added the PR: Needs +2 label Jul 5, 2018

@pchote pchote merged commit 1c0aa24 into OpenRA:bleed Jul 5, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.