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

ClickSound and ClickDisabledSound and ChatLine are optional ui sounds. #15390

Merged
merged 1 commit into from Sep 29, 2018

Conversation

Projects
None yet
4 participants
@IceReaper
Copy link
Contributor

IceReaper commented Jul 26, 2018

Splitted out of #15357 as pchote requested.

@@ -39,6 +39,8 @@ public class ButtonWidget : Widget
public bool Shadow = ChromeMetrics.Get<bool>("ButtonTextShadow");
public Color ContrastColorDark = ChromeMetrics.Get<Color>("ButtonTextContrastColorDark");
public Color ContrastColorLight = ChromeMetrics.Get<Color>("ButtonTextContrastColorLight");
private string clickSound = ChromeMetrics.Get<string>("ClickSound");

This comment has been minimized.

@pchote

pchote Jul 26, 2018

Member

Please make these public so that individual buttons can choose to override them. This applies to the other widget types too.

@@ -25,6 +25,8 @@ public class CycleProductionActorsHotkeyLogic : SingleHotkeyBaseLogic
readonly Selection selection;
readonly World world;

private string clickSound = ChromeMetrics.Get<string>("ClickSound");

This comment has been minimized.

@pchote

pchote Jul 26, 2018

Member

Please also expose this to yaml via the logicArgs. Take a look at how the logic hotkeys are defined for an example to work from. This applies to the other logic types too.

This comment has been minimized.

@pchote

pchote Jul 26, 2018

Member

Also note that private should not be explicitly declared - it is the default for fields.

This comment has been minimized.

@IceReaper

IceReaper Aug 15, 2018

Contributor

Need further explanation why this wrong here, but correct in all the other files.

This comment has been minimized.

@pchote

pchote Aug 15, 2018

Member

The other files are Widgets, which the engine automatically expose to yaml overrides.

@IceReaper IceReaper force-pushed the IceReaper:optional_uisounds branch 2 times, most recently from 462db92 to 1728c79 Jul 26, 2018

@IceReaper IceReaper force-pushed the IceReaper:optional_uisounds branch from 1728c79 to ca1c5a6 Aug 15, 2018

@IceReaper

This comment has been minimized.

Copy link
Contributor

IceReaper commented Aug 15, 2018

Rebased and updated.

@@ -40,6 +40,8 @@ public class ButtonWidget : Widget
public bool Shadow = ChromeMetrics.Get<bool>("ButtonTextShadow");
public Color ContrastColorDark = ChromeMetrics.Get<Color>("ButtonTextContrastColorDark");
public Color ContrastColorLight = ChromeMetrics.Get<Color>("ButtonTextContrastColorLight");
public string clickSound = ChromeMetrics.Get<string>("ClickSound");

This comment has been minimized.

@pchote

pchote Aug 15, 2018

Member

These will need to be ClickSound etc (public members are PascalCase)

@@ -29,6 +29,8 @@ public class DropDownButtonWidget : ButtonWidget
public string PanelRoot;
public string SelectedItem;

public string clickSound = ChromeMetrics.Get<string>("ClickSound");

This comment has been minimized.

@pchote

pchote Aug 15, 2018

Member

This is already inherited from ButtonWidget so can be omitted

@@ -25,6 +25,8 @@ public class CycleProductionActorsHotkeyLogic : SingleHotkeyBaseLogic
readonly Selection selection;
readonly World world;

public string clickSound = ChromeMetrics.Get<string>("ClickSound");

This comment has been minimized.

@pchote

pchote Aug 15, 2018

Member

This and the other *Logic files should allow override via logicArgs too.

@IceReaper IceReaper force-pushed the IceReaper:optional_uisounds branch 2 times, most recently from aab2579 to 06d51e6 Aug 15, 2018

@@ -39,6 +39,8 @@ public class IngameChatLogic : ChromeLogic
bool disableTeamChat;
bool teamChat;

public string ChatLine = ChromeMetrics.Get<string>("ChatLine");

This comment has been minimized.

@reaperrr

reaperrr Aug 18, 2018

Contributor

Could you rename this to ChatLineSound?
Of course the value in metrics.yaml then still needs to be ChatLine for backwards compatibility, but the field name would otherwise be a bit ambigous to modders.

@@ -63,6 +63,8 @@ enum PanelType { Players, Options, Music, Servers, Kick, ForceStart }
bool addBotOnMapLoad;
bool teamChat;

public string ChatLine = ChromeMetrics.Get<string>("ChatLine");

This comment has been minimized.

@reaperrr

reaperrr Aug 18, 2018

Contributor

Same here.

@IceReaper IceReaper force-pushed the IceReaper:optional_uisounds branch from 06d51e6 to 00a440a Sep 26, 2018

@IceReaper

This comment has been minimized.

Copy link
Contributor

IceReaper commented Sep 26, 2018

Renamed and rebased :)

@@ -16,3 +16,6 @@ Metrics:
IncompatibleProtectedGameColor: B22222
IncompatibleVersionColor: D3D3D3
TextfieldColorHighlight: 562020
ChatLine: ChatLine

This comment has been minimized.

@abcdefg30

abcdefg30 Sep 27, 2018

Member

Can we rename this to ChatLineSound? (ChatLineSound: ChatLine)
Imho it is not clear what this does from just looking at it.

This comment has been minimized.

@pchote

pchote Sep 27, 2018

Member

I agree. This makes it consistent with the sounds defined below.

@pchote
Copy link
Member

pchote left a comment

LGTM - just @abcdefg30's comment, and then one extra change to make in the logic classes - but not in the widgets!

@@ -25,6 +25,8 @@ public class CycleProductionActorsHotkeyLogic : SingleHotkeyBaseLogic
readonly Selection selection;
readonly World world;

public string ClickSound = ChromeMetrics.Get<string>("ClickSound");

This comment has been minimized.

@pchote

pchote Sep 27, 2018

Member

Widget logic can't be automatically overridden as widgets can.

This should go to a readonly string clickSound = ChromeMetrics.Get<string>("ClickSound"); and then support for per-instance overrides done in the constructor by addings

MiniYaml yaml;
if (logicArgs.TryGetValue("ClickSounds", out yaml))
	ClickSound = yaml.Value;
@@ -39,6 +39,8 @@ public class IngameChatLogic : ChromeLogic
bool disableTeamChat;
bool teamChat;

public string ChatLineSound = ChromeMetrics.Get<string>("ChatLine");

This comment has been minimized.

@pchote

pchote Sep 27, 2018

Member

Same comment as CycleProductionActorsHotkeyLogic.

@@ -27,6 +27,8 @@ public class MenuButtonsChromeLogic : ChromeLogic
bool disableSystemButtons;
Widget currentWidget;

public string ClickSound = ChromeMetrics.Get<string>("ClickSound");

This comment has been minimized.

@pchote

pchote Sep 27, 2018

Member

Same comment as CycleProductionActorsHotkeyLogic.

@@ -43,6 +43,8 @@ public class ObserverStatsLogic : ChromeLogic
readonly World world;
readonly WorldRenderer worldRenderer;

public string ClickSound = ChromeMetrics.Get<string>("ClickSound");

This comment has been minimized.

@pchote

pchote Sep 27, 2018

Member

Same comment as CycleProductionActorsHotkeyLogic.

@@ -63,6 +63,8 @@ enum PanelType { Players, Options, Music, Servers, Kick, ForceStart }
bool addBotOnMapLoad;
bool teamChat;

public string ChatLineSound = ChromeMetrics.Get<string>("ChatLine");

This comment has been minimized.

@pchote

pchote Sep 27, 2018

Member

Same comment as CycleProductionActorsHotkeyLogic.

@@ -16,3 +16,6 @@ Metrics:
IncompatibleProtectedGameColor: B22222
IncompatibleVersionColor: D3D3D3
TextfieldColorHighlight: 562020
ChatLine: ChatLine

This comment has been minimized.

@pchote

pchote Sep 27, 2018

Member

I agree. This makes it consistent with the sounds defined below.

@IceReaper IceReaper force-pushed the IceReaper:optional_uisounds branch 2 times, most recently from c758a4b to fd1de3a Sep 28, 2018

@IceReaper IceReaper force-pushed the IceReaper:optional_uisounds branch from fd1de3a to ace5276 Sep 28, 2018

@pchote
Copy link
Member

pchote left a comment

LGTM. Just a couple of very minor style nits:

@@ -87,6 +87,9 @@ public class ProductionTabsWidget : Widget
Lazy<ProductionPaletteWidget> paletteWidget;
string queueGroup;

public string ClickSound = ChromeMetrics.Get<string>("ClickSound");

This comment has been minimized.

@pchote

pchote Sep 29, 2018

Member

Please make these readonly and put them up top with the other fields that are exposed to yaml.

@@ -64,6 +64,8 @@ public class ScrollPanelWidget : Widget
// The current value is the actual list offset at the moment
float currentListOffset;

public string ClickSound = ChromeMetrics.Get<string>("ClickSound");

This comment has been minimized.

@pchote

pchote Sep 29, 2018

Member

Please put this up top with the other fields that are exposed to yaml. Probably best to keep consistency with the other fields and omit the readonly (a future PR can fix all the widgets properly).

@IceReaper IceReaper force-pushed the IceReaper:optional_uisounds branch from 1922fdb to 996596f Sep 29, 2018

@pchote

pchote approved these changes Sep 29, 2018

@reaperrr reaperrr merged commit 4824826 into OpenRA:bleed Sep 29, 2018

2 checks passed

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

@IceReaper IceReaper deleted the IceReaper:optional_uisounds branch Sep 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment