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.

Sign up for GitHub

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

Jump to bottom

Update MultiplayerGameLobby controls visibility logic. #318

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Update MultiplayerGameLobby controls visibility logic. #318

wants to merge 1 commit into from

Conversation

frg2089
Copy link
Contributor

@frg2089 frg2089 commented Mar 26, 2022

This will provide more flexible control visibility support.
Applying different styles depending on the suffix will no longer be limited to "lbChatMessages" and "tbChatInput" controls.

Now we can write INI like this.

[MultiplayerGameLobby]
$CC-ChatMessages=ChatMessages:XNAPanel

[ChatMessages]
$X=1
$Y=1
$CC-lbChatMessages=lbChatMessages:ChatListBox

[ChatMessages_Host]
$BaseSection=ChatMessages
Size=100,200
BackgroundTexture=ChatMessagesBorder_Host.png

[ChatMessages_Player]
$BaseSection=ChatMessages
Size=100,100
BackgroundTexture=ChatMessagesBorder_Player.png

On INItializableWindow Initialized.
ParseExtraControls() method will create an XNAPanel named ChatMessages.

Then the Refresh() method will be called when creating a game or joining a game.
The ChatMessages control will be renamed to ChatMessages_Host or ChatMessages_Player via the IsHost field. and reload its properties.

@github-actions
Copy link

github-actions bot commented May 12, 2022 •
edited
Loading

Nightly build for this pull request:

@frg2089 frg2089 force-pushed the feature/new-controls-loader branch from 1b28b1a to 58f8aa0 Compare June 11, 2022 02:49
@frg2089 frg2089 force-pushed the feature/new-controls-loader branch from 58f8aa0 to c6a54ac Compare June 17, 2022 08:34
Metadorius
Metadorius requested changes Nov 29, 2022
View reviewed changes
Copy link
Member

@Metadorius Metadorius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of nitpicks. I am not quite sure this is the direction we want to take (with hardcoded suffixes and handling only the very special case of map list visibility), I would prefer a more generalized solution. But the execution of that idea is more or less fine.

@Rampastring what's your opinion? Basically the code is very specific to this case and introduces INI behavior based on section names, which is only tied to map box being shown or not.

DXMainClient/DXGUI/Multiplayer/GameLobby/MultiplayerGameLobby.cs Outdated
Comment on lines 643 to 694
ddGameModeMapFilter.Enable();
lblGameModeSelect.Enable();
lbGameModeMapList.Enable();
tbMapSearch.Enable();
btnPickRandomMap.Enable();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty sure you forgot btnMapSortAlphabetically. Also wouldn't tunnel selection button need to be handled too at CnCNet game lobby? Maybe would be good to have this function as protected virtual?

DXMainClient/DXGUI/Multiplayer/GameLobby/MultiplayerGameLobby.cs Outdated Show resolved Hide resolved
DXMainClient/DXGUI/Multiplayer/GameLobby/MultiplayerGameLobby.cs Outdated
Comment on lines 640 to 641
sourceSuffix = "_Player";
targetSuffix = "_Host";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should make those consts at class level.

DXMainClient/DXGUI/Multiplayer/GameLobby/MultiplayerGameLobby.cs Outdated Show resolved Hide resolved
DXMainClient/DXGUI/Multiplayer/GameLobby/MultiplayerGameLobby.cs Outdated
/// Change MapList and other controls visibility.
/// </summary>
/// <param name="isHost">Visibility</param>
private void ChangeMapListAndOtherControlsVisibility(bool isHost)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not fond of the name, needs something more concise.

DXMainClient/DXGUI/Multiplayer/GameLobby/MultiplayerGameLobby.cs Outdated
Comment on lines 38 to 40
s => ChangeMapListAndOtherControlsVisibility(false)),
new ChatBoxCommand("SHOWMAPS", "Show map list (game host only)".L10N("UI:Main:ChatboxCommandShowMapsHelp"), true,
s => ShowMapList()),
s => ChangeMapListAndOtherControlsVisibility(true)),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, you're mixing two things. The prefixes you have and the bool you use as a parameter suggest differentiation between host and player, yet you're mixing this with map list hidden or shown.

@frg2089 frg2089 force-pushed the feature/new-controls-loader branch 2 times, most recently from f9862e5 to 3bb9360 Compare November 30, 2022 04:04
@frg2089 frg2089 requested review from Metadorius and removed request for Starkku and Rampastring November 30, 2022 04:04
@frg2089 frg2089 closed this Feb 22, 2024
@frg2089 frg2089 deleted the feature/new-controls-loader branch February 22, 2024 00:16
@frg2089 frg2089 restored the feature/new-controls-loader branch February 22, 2024 00:17
@frg2089 frg2089 reopened this Feb 26, 2024
@frg2089 frg2089 force-pushed the feature/new-controls-loader branch 2 times, most recently from f577488 to a2f047d Compare February 26, 2024 12:48
@frg2089 frg2089 force-pushed the feature/new-controls-loader branch from a2f047d to 5866048 Compare June 2, 2024 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devo1929 devo1929 devo1929 approved these changes

@Metadorius Metadorius Awaiting requested review from Metadorius

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@frg2089 @devo1929 @Metadorius