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

#368 show colors in color drop down #369

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

Conversation

devo1929
Copy link
Contributor

@devo1929 devo1929 commented Sep 12, 2022

Decreased width of color drop down allows for increase of width of player name and side. Color drop down is now the same width as team and start drop downs.

Moved the "Random" label to an INI key.

image

@github-actions
Copy link

github-actions bot commented Sep 12, 2022

Nightly build for this pull request:

  • artifacts.zip
    This comment is automatic and is meant to allow guests to get latest automatic builds without registering. It is updated on every successful build.

@@ -17,6 +18,10 @@ public class MultiplayerColor

private static List<MultiplayerColor> colorList;

private static string randomColorLabel;

private static readonly string RandomColorDefaultLabel = "Random".L10N("UI:Main:RandomSide");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private static readonly string RandomColorDefaultLabel = "Random".L10N("UI:Main:RandomSide");
private static readonly string RandomColorDefaultLabel = "Random".L10N("UI:Main:RandomColor");

And also considering making this a readonly property to prevent L10N being called before loading the translation table (although it should works without being a property as long as the class is initialized after L10N initialized but it is not straightforward enough).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I understand. It is currently readonly. Are you saying it shouldn't be readonly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch on the incorrect string.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand. It is currently readonly. Are you saying it shouldn't be readonly?

No. I mean changing = to =>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahhhhh, I got it. Will do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

@SadPencil SadPencil left a comment

Choose a reason for hiding this comment

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

LGTM

@Rampastring
Copy link
Member

Seeing the gap between the dropdown border and the color, it might be best if either a custom dropdown control was added that got rid of the gap, or I made the gap customizable in XNAUI.

Copy link
Contributor

@pzhlkj6612 pzhlkj6612 left a comment

Choose a reason for hiding this comment

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

The design of this PR has side-effect on COOP maps. I'm now testing the YR client btw.

Symptom

disallowed-colors-cannot-be-recognized-in-new-color-drop-down

We cannot see which colors are disallowed.

Test

  • Make a map. Maps\Custom\yuri.map for example.
  • Modify INI\MPMaps.ini and add these lines:
[MultiMaps]
; ...
test=Maps\Custom\yuri

[Maps\Custom\yuri]
IsCoopMission=yes
GameModes=Battle
DisallowedPlayerSides=5,6,7,8 ; World Socialist Alliance
DisallowedPlayerColors=0,2,4,6 ; Yellow, Blue, Orange, Purple

.

Comment on lines +790 to +791
if(mpColor.Name.StartsWith("$"))
ddPlayerColor.AddItem(string.Empty, AssetLoader.CreateTexture(mpColor.XnaColor, ddPlayerColor.Width - 2, ddPlayerColor.ItemHeight + 2));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if(mpColor.Name.StartsWith("$"))
ddPlayerColor.AddItem(string.Empty, AssetLoader.CreateTexture(mpColor.XnaColor, ddPlayerColor.Width - 2, ddPlayerColor.ItemHeight + 2));
if (mpColor.Name.StartsWith("$"))
ddPlayerColor.AddItem(string.Empty, AssetLoader.CreateTexture(mpColor.XnaColor, ddPlayerColor.Width - 2, ddPlayerColor.ItemHeight));

Otherwise, some items will be partially outside the box.

A screenshot. The width of the color has been decreased to show which color is being highlighted. This is the client (bae3855) with built-in resources:

items-in-color-drop-down-are-partially-outside-the-box

Seeing the gap between the dropdown border and the color, ...

Yes, the additional 2 pixels look like a workaround for the gap, but we didn't fully succeed.

@@ -17,6 +18,10 @@ public class MultiplayerColor

private static List<MultiplayerColor> colorList;

private static string randomColorLabel;

private static string RandomColorDefaultLabel => "Random".L10N("UI:Main:RandomColor");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private static string RandomColorDefaultLabel => "Random".L10N("UI:Main:RandomColor");
private static string RandomColorDefaultLabel => "Random".L10N("Client:Main:RandomColor");

Before we merge the code into the current "develop" branch.

@pzhlkj6612
Copy link
Contributor

If we don't have to decrease the width of color drop down, I think we can try the design like I said in #368 (comment) to eliminate the side-effect on COOP maps with disallowed colors. Screenshot:

both-color-block-and-name-in-color-drop-down

Code:

                 foreach (MultiplayerColor mpColor in MPColors)
                 {
                     if(mpColor.Name.StartsWith("$"))
-                        ddPlayerColor.AddItem(string.Empty, AssetLoader.CreateTexture(mpColor.XnaColor, ddPlayerColor.Width - 2, ddPlayerColor.ItemHeight + 2));
+                        ddPlayerColor.AddItem(new XNADropDownItem()
+                        {
+                            Text = mpColor.Name.TrimStart('$'),
+                            Texture = AssetLoader.CreateTexture(mpColor.XnaColor, ddPlayerColor.ItemHeight - 2, ddPlayerColor.ItemHeight - 2),
+                        });
                     else
                         ddPlayerColor.AddItem(mpColor.Name, mpColor.XnaColor);
                 }

And of course, we can decrease the width of color drop down by reducing the width of color blocks. For example:

Texture = AssetLoader.CreateTexture(..., (ddPlayerColor.ItemHeight - 2) / 3, ...),

.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants