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

Split settings panels logic and add support for custom panels #18948

Merged
merged 1 commit into from
Feb 8, 2021
Merged

Split settings panels logic and add support for custom panels #18948

merged 1 commit into from
Feb 8, 2021

Conversation

dragunoff
Copy link
Contributor

This PR:

  • Splits SettingsLogic into chrome logic classes for each panel
  • Splits settings.yaml into a wrapper for the settings window and containers for each panel
  • Adds the ability to add custom settings panels

Why:

  • SettingsLogic had become very large and difficult to maintain
  • Mods could not add their own settings tabs

@pchote
Copy link
Member

pchote commented Dec 22, 2020

Good idea. How about putting these in a Settings subdir so they can stay grouped, like we have done for Installation, Lobby, etc?

@dragunoff
Copy link
Contributor Author

Update: moved classes to a Settings subdir.

@dragunoff
Copy link
Contributor Author

Update: rebased after #18973 and #18959

@pchote
Copy link
Member

pchote commented Jan 10, 2021 •
edited
Loading

Can you please start bugging me on Discord every couple of days until I review this? We should try and merge this before anyone else wants to touch the settings menu.

dragunoff
dragunoff commented Jan 13, 2021
View reviewed changes
Copy link
Contributor Author

@dragunoff dragunoff left a comment

Choose a reason for hiding this comment

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

@pchote I added some comments over the code to guide the review. The diff is quite large but that's mostly because of splitting and moving things around.

OpenRA.Mods.Common/Widgets/Logic/Settings/SettingsLogic.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Widgets/Logic/Settings/SettingsLogic.cs Outdated
};
}

public void RegisterSettingsPanel(string panelID, string label, Func<Widget, Action> init, Func<Widget, Action> reset)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method keeps the same logic as before only now each separate chrome logic class registers itself.

mods/cnc/chrome/settings.yaml
Comment on lines +34 to 26
Button@BUTTON_TEMPLATE:
Width: 110
Height: 35
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tab buttons are created from a template.

mods/cnc/chrome/settings.yaml
ImageName: enabled
IgnoreMouseOver: True
Container@ADVANCED_PANEL:
Container@PANEL_TEMPLATE:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tab panes are created from a template.

OpenRA.Mods.Common/Widgets/Logic/Settings/SettingsUtils.cs

namespace OpenRA.Mods.Common.Widgets.Logic
{
public static class SettingsUtils
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 moved the common methods for binding inputs to a static class.

@dragunoff
Copy link
Contributor Author

Update: Cleaned up unnecessary usings and constructor arguments and addressed #18948 (comment)

@dragunoff dragunoff mentioned this pull request Jan 21, 2021
Merged
pchote
pchote reviewed Jan 24, 2021
View reviewed changes
Copy link
Member

@pchote pchote left a comment

Choose a reason for hiding this comment

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

Looks good overall. I diffed the split files versus the original and have confirmed that nothing has been unexpectedly dropped, or changed during rebases etc.

IMO having the panels reach into SettingsLogic to modify NeedsRestart isn't great - what do you think about doing this instead: ccef396 ?

pchote
pchote reviewed Jan 24, 2021
View reviewed changes
mods/cnc/chrome/settings.yaml Outdated
Types: Music
ButtonStride: 120, 0
Panels:
Display:
Copy link
Member

Choose a reason for hiding this comment

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

These keys don't seem to be used at all?

How about using a straight dictionary?

Panels:
	DISPLAY_PANEL: Display
	AUDIO_PANEL: Audio
	INPUT_PANEL: Input
	HOTKEYS_PANEL: Hotkeys
	ADVANCED_PANEL: Advanced

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is more concise and straight-forward 👍

pchote
pchote reviewed Jan 24, 2021
View reviewed changes
OpenRA.Mods.Common/Widgets/Logic/Settings/SettingsLogic.cs Outdated
readonly int2 buttonStride;
readonly List<ButtonWidget> buttons = new List<ButtonWidget>();
readonly List<Tuple<string, string>> panels = new List<Tuple<string, string>>();
string activePanel = "";
Copy link
Member

Choose a reason for hiding this comment

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

Cleaner if this defaulted to null?

@dragunoff
Copy link
Contributor Author

Update:

  • rebased
  • cherry-picked ccef396
  • applied other fixups (see commit messages)

I will squash the commits once this is approved.

pchote
pchote previously approved these changes Jan 31, 2021
View reviewed changes
Copy link
Member

@pchote pchote left a comment

Choose a reason for hiding this comment

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

LGTM once the fixups are squashed.

@dragunoff
Copy link
Contributor Author

Update: squashed fixups.

@pchote
Copy link
Member

pchote commented Feb 7, 2021

pinging for a second review.

abcdefg30
abcdefg30 approved these changes Feb 8, 2021
View reviewed changes
Copy link
Member

@abcdefg30 abcdefg30 left a comment

Choose a reason for hiding this comment

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

Rebased and updated the dates to 2021.

@abcdefg30 abcdefg30 merged commit 641b05e into OpenRA:bleed Feb 8, 2021
@abcdefg30
Copy link
Member

Changelog

@dragunoff dragunoff deleted the feature/settings-logic-split branch June 3, 2021 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abcdefg30 abcdefg30 abcdefg30 approved these changes

@pchote pchote pchote left review comments

Assignees
No one assigned
Labels
PR: Needs +2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@dragunoff @pchote @abcdefg30