Color picker update - built-in mod colors and a user palette #13072

drogoganor · 2017-04-07T03:05:06Z

Hi,

I had a go at updating the color picker. Now it has:

A set of preset color swatches for RA1 and TD, (defined in mod.yaml: TeamColorPresets: C4B060, 4494E4, ....)
A user-defined palette defined in settings.yaml (Player: CustomColors: 3B99CA, A33BCA, ....)

This is my first time contributing to this project so hopefully I have done it correctly and haven't broken any rules.

I'd also like to know what preset colors were available in d2k, TS, and RA2 so I can add them into the mod.yaml for each of these mods. Does anyone happen to know?

Cheers

…ned palette

atlimit8

You should not have separate commits for fixing the commits you want merged. You can squash commits and amend (to amend to previous commits, use git rebase -i). Every commit is added when a PR is merged and the fix-ups would just be noise.

Aside from tabs/spaces, StyleCop actually complains about a number of things in the Travis continuous integration test.

Judging from all the StyleCop errors and the StyleCop fix-up commit, it appears that you may not have StyleCop/your code editor configured correctly.

You can run make check from the command line in the top directory of your OpenRA clone to find StyleCop format errors.

StyleCop errors from travis-ci log:

OpenRA.Mods.Common/Widgets/ColorPaletteSwatchWidget.cs:L27: [SA1512] A single-line comment must not be followed by a blank line. To ignore this error when commenting out a line of code, begin the comment with '////' rather than '//'.

OpenRA.Mods.Common/Widgets/ColorPaletteSwatchWidget.cs:L38: [SA1512] A single-line comment must not be followed by a blank line. To ignore this error when commenting out a line of code, begin the comment with '////' rather than '//'.

OpenRA.Mods.Common/Widgets/ColorPaletteSwatchWidget.cs:L80: [SA1515] A single-line comment must be preceded by a blank line or another single-line comment, or must be the first item in its scope. To ignore this error when commenting out a line of code, begin the comment with '////' rather than '//'.

OpenRA.Mods.Common/Widgets/ColorPaletteSwatchWidget.cs:L88: [SA1508] A closing curly bracket must not be preceded by a blank line.

OpenRA.Mods.Common/Widgets/ColorPaletteSwatchWidget.cs:L27: [SA1005] The comment must start with a single space. To ignore this error when commenting out a line of code, begin the comment with '////' rather than '//'.

OpenRA.Mods.Common/Widgets/ColorPaletteSwatchWidget.cs:L37: [SA1005] The comment must start with a single space. To ignore this error when commenting out a line of code, begin the comment with '////' rather than '//'.

OpenRA.Mods.Common/Widgets/ColorPaletteSwatchWidget.cs:L38: [SA1005] The comment must start with a single space. To ignore this error when commenting out a line of code, begin the comment with '////' rather than '//'.

OpenRA.Mods.Common/Widgets/ColorPaletteSwatchWidget.cs:L80: [SA1005] The comment must start with a single space. To ignore this error when commenting out a line of code, begin the comment with '////' rather than '//'.

OpenRA.Mods.Common/Widgets/ColorPaletteSwatchWidget.cs:L81: [SA1005] The comment must start with a single space. To ignore this error when commenting out a line of code, begin the comment with '////' rather than '//'.

OpenRA.Mods.Common/Widgets/ColorPaletteSwatchWidget.cs:L84: [SA1005] The comment must start with a single space. To ignore this error when commenting out a line of code, begin the comment with '////' rather than '//'.

OpenRA.Mods.Common/Widgets/ColorPaletteSwatchWidget.cs:L67: [SP2000] Invalid spacing at the end of the line.

OpenRA.Mods.Common/Widgets/Logic/ColorPickerLogic.cs:L97: [SA1001] Invalid spacing around the comma.

OpenRA.Mods.Common/Widgets/Logic/ColorPickerLogic.cs:L97: [SA1001] Invalid spacing around the comma.

OpenRA.Mods.Common/Widgets/Logic/ColorPickerLogic.cs:L98: [SA1001] Invalid spacing around the comma.

OpenRA.Mods.Common/Widgets/Logic/ColorPickerLogic.cs:L98: [SA1001] Invalid spacing around the comma.

OpenRA.Mods.Common/Widgets/Logic/ColorPickerLogic.cs:L99: [SA1001] Invalid spacing around the comma.

OpenRA.Mods.Common/Widgets/Logic/ColorPickerLogic.cs:L99: [SA1001] Invalid spacing around the comma.

OpenRA.Mods.Common/Widgets/Logic/ColorPickerLogic.cs:L100: [SA1001] Invalid spacing around the comma.

OpenRA.Mods.Common/Widgets/Logic/ColorPickerLogic.cs:L100: [SA1001] Invalid spacing around the comma.

OpenRA.Mods.Common/Widgets/Logic/ColorPickerLogic.cs:L101: [SA1001] Invalid spacing around the comma.

OpenRA.Mods.Common/Widgets/Logic/ColorPickerLogic.cs:L101: [SA1001] Invalid spacing around the comma.

OpenRA.Mods.Common/Widgets/Logic/ColorPickerLogic.cs:L102: [SA1001] Invalid spacing around the comma.

OpenRA.Mods.Common/Widgets/Logic/ColorPickerLogic.cs:L102: [SA1001] Invalid spacing around the comma.

OpenRA.Mods.Common/Widgets/Logic/ColorPickerLogic.cs:L103: [SA1001] Invalid spacing around the comma.

OpenRA.Mods.Common/Widgets/Logic/ColorPickerLogic.cs:L103: [SA1001] Invalid spacing around the comma.

OpenRA.Mods.Common/Widgets/Logic/ColorPickerLogic.cs:L104: [SA1001] Invalid spacing around the comma.

OpenRA.Mods.Common/Widgets/Logic/ColorPickerLogic.cs:L104: [SA1001] Invalid spacing around the comma.

OpenRA.Mods.Common/Widgets/Logic/ColorPickerLogic.cs:L69: [SP2000] Invalid spacing at the end of the line.

OpenRA.Mods.Common/Widgets/Logic/ColorPickerLogic.cs:L93: [SP2000] Invalid spacing at the end of the line.

OpenRA.Mods.Common/Widgets/Logic/ColorPickerLogic.cs:L123: [SP2000] Invalid spacing at the end of the line.

OpenRA.Mods.Common/Widgets/Logic/ColorPickerLogic.cs:L191: [SP2000] Invalid spacing at the end of the line.

OpenRA.Mods.Common/Widgets/Logic/ColorPickerLogic.cs:L205: [SP2000] Invalid spacing at the end of the line.

OpenRA.Mods.Common/Widgets/Logic/ColorPickerLogic.cs:L225: [SP2000] Invalid spacing at the end of the line.

Thanks for contributing.

atlimit8 · 2017-04-07T03:23:33Z

OpenRA.Game/Manifest.cs


-		readonly string[] reservedModuleNames = { "Metadata", "Folders", "MapFolders", "Packages", "Rules",
+        readonly string[] reservedModuleNames = { "Metadata", "Folders", "MapFolders", "Packages", "Rules",


Please don't include tab to space changes. They are not even style fixes, but are comparison noise (extra changes to review).

atlimit8 · 2017-04-07T03:24:45Z

OpenRA.Game/Manifest.cs

+
+            if (yaml.ContainsKey("TeamColorPresets"))
+                TeamColorPresets = FieldLoader.GetValue<string[]>("TeamColorPresets", yaml["TeamColorPresets"].Value);
+        }


Same here for tab -> spaces.

atlimit8 · 2017-04-07T03:24:53Z

OpenRA.Game/Settings.cs

@@ -149,7 +149,8 @@ public class PlayerSettings
 		public string Name = "Newbie";
 		public HSLColor Color = new HSLColor(75, 255, 180);
 		public string LastServer = "localhost:1234";
-	}
+        public string[] CustomColors;
+    }


Same here for tab -> spaces.

atlimit8 · 2017-04-07T04:27:29Z

OpenRA.Mods.Common/Widgets/Logic/ColorPickerLogic.cs

+            int maxCustomColors = 10;
+
+            var clearCustomPaletteButton = widget.GetOrNull<ButtonWidget>("CLEAR_CUSTOM_BUTTON");
+		    if (clearCustomPaletteButton != null)


Here is a reason to use tabs; the indentation for if here does not lined up with the rest.

atlimit8 · 2017-04-07T04:33:53Z

OpenRA.Mods.Common/Widgets/ColorPaletteSwatchWidget.cs

+
+        public Action OnClick = () => { };
+
+        //protected bool CtrlPressed = false;


Why do you have commented-out code?
If you are unsure about adding code then ask.
If you want to include the commented-out stuff, you can put it in a separate commit here or in a separate branch.

atlimit8 · 2017-04-07T04:40:25Z

OpenRA.Mods.Common/Widgets/Logic/ColorPickerLogic.cs

+            {
+                presetColors = defaultColorPresets.ToList();
+            }
+


The spaces on empty line 123 should be removed.

atlimit8 · 2017-04-07T04:40:43Z

OpenRA.Mods.Common/Widgets/Logic/ColorPickerLogic.cs

+                    };
+
+                    widget.AddChild(newSwatch);
+


The spaces on empty line 191 should be removed.

atlimit8 · 2017-04-07T04:41:04Z

OpenRA.Mods.Common/Widgets/Logic/ColorPickerLogic.cs

+            int startY = 158;
+
+            addSwatchesAction(presetColors, startX, startY, "COLORPRESET", false);
+


The spaces on empty line 205 should be removed.

atlimit8 · 2017-04-07T04:41:18Z

OpenRA.Mods.Common/Widgets/Logic/ColorPickerLogic.cs

+                    i++;
+                }
+            }
+


The spaces on empty line 225 should be removed.

atlimit8 · 2017-04-07T04:42:10Z

OpenRA.Mods.Common/Widgets/ColorPaletteSwatchWidget.cs

+
+            if (mi.Event == MouseInputEvent.Down && !TakeMouseFocus(mi))
+                return false;
+


The spaces on empty line 67 should be removed.

Thanks for the comprehensive comments. I'll sort these out and give it another go.

atlimit8 · 2017-04-07T05:23:02Z

OpenRA.Mods.Common/Widgets/ColorPaletteSwatchWidget.cs

+            return false;
+        }
+
+    }


I missed OpenRA.Mods.Common/Widgets/ColorPaletteSwatchWidget.cs:L88: [SA1508] A closing curly bracket must not be preceded by a blank line. Line 87 should be deleted.

… palette

…penRA into color-picker-update

pchote · 2017-04-17T10:37:21Z

Any progress here @drogoganor?

It looks like you're having some issues with rebasing, feel free to drop in to the IRC channel if you need some help.

I will have some comments and requests on the UI, but lets get the current set of issues out of the way first.

drogoganor · 2017-04-17T22:57:16Z

Hi, Yes I am actually struggling with rebasing, I am not skilled enough with git. I wasn't sure if my current situation could be fixed? In any case I might drop into IRC and ask about it. Cheers, David

…

On 17/04/2017 8:37 PM, Paul Chote wrote: Any progress here @drogoganor <https://github.com/drogoganor>? It looks like you're having some issues with rebasing, feel free to drop in to the IRC channel if you need some help. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#13072 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABkFAfIC3hnwecFbQNtVvb5iR1mWAgMNks5rw0DmgaJpZM4M2Z8O>.

pchote · 2017-04-26T18:01:57Z

Yeah, it's actually quite simple to fix if you know how.
Drop by IRC and someone should be able to help. IRC is mainly active during european evenings, so if nobody is around just try again later.

GraionDilach · 2017-05-01T13:55:39Z

This will fix #10591.

pchote · 2017-05-13T12:59:03Z

@drogoganor are you still interested in working on this feature?
If not, we can move it to the future milestone for someone else to pick up when they have time and interest.

drogoganor · 2017-05-15T22:52:24Z

Hi, I'm afraid I'll have to ask you to put it on the future milestone. I've been really busy lately and it seems it won't let up. Sorry.

…

On 13/05/2017 10:59 PM, Paul Chote wrote: @drogoganor <https://github.com/drogoganor> are you still interested in working on this feature? If not, we can move it to the future milestone for someone else to pick up when they have time and interest. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#13072 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABkFAVIOz6MhYWtshcO2t7aaYnWN2MQNks5r5akbgaJpZM4M2Z8O>.

abcdefg30 · 2017-05-15T23:52:25Z

No problem. Just let us know if you want to work on this again.

drogoganor added 2 commits April 7, 2017 12:46

Proposed color picker with mod-specific preset colors and a user-defi…

28da368

…ned palette

- Color picker update: changes to pass stylecop validation

2f182b4

atlimit8 requested changes Apr 7, 2017

View reviewed changes

atlimit8 reviewed Apr 7, 2017

View reviewed changes

drogoganor added 2 commits April 7, 2017 19:35

Color picker update with mod-defined player colors and a user-defined…

7a41d64

… palette

Merge branch 'color-picker-update' of https://github.com/drogoganor/O…

b64b20d

…penRA into color-picker-update

pchote added the PR: Changes requested label Apr 17, 2017

abcdefg30 closed this May 15, 2017

abcdefg30 added this to the Future milestone May 15, 2017

drogoganor mentioned this pull request Feb 25, 2018

Color picker update with team color presets #14853

Merged

pchote removed this from the Future milestone May 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Color picker update - built-in mod colors and a user palette #13072

Color picker update - built-in mod colors and a user palette #13072

drogoganor commented Apr 7, 2017

atlimit8 left a comment

atlimit8 Apr 7, 2017

atlimit8 Apr 7, 2017

atlimit8 Apr 7, 2017

atlimit8 Apr 7, 2017

atlimit8 Apr 7, 2017

atlimit8 Apr 7, 2017

atlimit8 Apr 7, 2017

atlimit8 Apr 7, 2017

atlimit8 Apr 7, 2017

atlimit8 Apr 7, 2017

drogoganor Apr 7, 2017

atlimit8 Apr 7, 2017

pchote commented Apr 17, 2017 •

edited

drogoganor commented Apr 17, 2017 via email

pchote commented Apr 26, 2017

GraionDilach commented May 1, 2017

pchote commented May 13, 2017

drogoganor commented May 15, 2017 via email

abcdefg30 commented May 15, 2017


		readonly string[] reservedModuleNames = { "Metadata", "Folders", "MapFolders", "Packages", "Rules",
		readonly string[] reservedModuleNames = { "Metadata", "Folders", "MapFolders", "Packages", "Rules",


		public Action OnClick = () => { };

		//protected bool CtrlPressed = false;

		int startY = 158;

		addSwatchesAction(presetColors, startX, startY, "COLORPRESET", false);


		if (mi.Event == MouseInputEvent.Down && !TakeMouseFocus(mi))
		return false;

Color picker update - built-in mod colors and a user palette #13072

Color picker update - built-in mod colors and a user palette #13072

Conversation

drogoganor commented Apr 7, 2017

atlimit8 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pchote commented Apr 17, 2017 • edited

drogoganor commented Apr 17, 2017 via email

pchote commented Apr 26, 2017

GraionDilach commented May 1, 2017

pchote commented May 13, 2017

drogoganor commented May 15, 2017 via email

abcdefg30 commented May 15, 2017

pchote commented Apr 17, 2017 •

edited