Add mechanism for rulesets to add their own menu items to the editor … #35699
Conversation
| /// <summary> | ||
| /// Allows for providing custom menu items to be appended to the <see cref="Editor"/>'s menu bar. | ||
| /// </summary> | ||
| public abstract partial class RulesetEditorMenuBarItems : Component |
There was a problem hiding this comment.
why is this a Component and not a plain old class?
| public enum EditorMenuBarItemType | ||
| { | ||
| File, | ||
| Edit, | ||
| View, | ||
| Timing | ||
| } |
There was a problem hiding this comment.
I'm not sure we want to be allowing rulesets to arbitrarily enter items into any submenu. and even supposing we did, then why not give the ruleset full control over the menu bar at that point? just make it return all possible menu items, which would simplify all of this massively.
There was a problem hiding this comment.
The idea was that while I think there's valid reasons to add to each of the 4 main menus, I didn't think there was gonna be much of a need for removing/altering the existing ones. I'm not quite sure what you mean by making it return all possible menu items. Do you mean moving the menu items that already exist from the editor into the base class?
There was a problem hiding this comment.
The idea was that while I think there's valid reasons to add to each of the 4 main menus, I didn't think there was gonna be much of a need for removing/altering the existing ones.
disagree, File and Timing do not feel like they should require adjusting from rulesets
I'm not quite sure what you mean by making it return all possible menu items. Do you mean moving the menu items that already exist from the editor into the base class?
if we're going the "everything is modifiable by rulesets" route, then just have a method in ruleset API that returns all menu items including the "built in" ones, and default to a sane default if the ruleset doesn't implement it, instead of trying this half-and-half thing that's in here
There was a problem hiding this comment.
disagree, File and Timing do not feel like they should require adjusting from rulesets
For the File menu it can be useful for rulesets based on existing games to allow importing maps from said games. For the Timing menu I can't think of something off the top of my head but it would be weird to exclude only that one.
if we're going the "everything is modifiable by rulesets" route, then just have a method in ruleset API that returns all menu items including the "built in" ones, and default to a sane default if the ruleset doesn't implement it, instead of trying this half-and-half thing that's in here
Sure, I can do that.
There was a problem hiding this comment.
@bdach would it be fine if I just turn the entire menubar into it's own Drawable and have the Rulesets provide a class that extends from it? If rulesets are gonna get full control over it may as well give it the whole drawable too. Would also get rid of the awkward inheritance from Component you pointed out.
There was a problem hiding this comment.
Alright, I'll try to cook something up 👍
There was a problem hiding this comment.
would be best if it was just possible to specify a different set of items / do ruleset-local adjustments to the items without having to subclass. see how you go I guess.
There was a problem hiding this comment.
Gotcha. Is it fine to keep using a class for this that extends from Component? You pointed it out in the other review comment so I wanted to double-check. If your answer is "I dunno, I'd have to see it first" that's also fine for me.
There was a problem hiding this comment.
I mean if you're going to be using an actual drawable menu then that's a bit of a moot point isn't it? a drawable is a component already, conceptually
There was a problem hiding this comment.
Oh, I guess I misunderstood you then. I thought I was supposed to do this without giving the ruleset access to the menu drawable.
Regardless, trying to divorce the editor class from all the menu logic turned out to be quite tricky so far. Lots of stuff relies on internals of the editor class which look like they should probably remain there. Gonna need a while to figure this stuff out.
|
This was split it out from the linked pr so I guess it's a prerequisite, updated the pr description. |
2 participants
Prerequisite to #35700
Adds an API for rulesets to add their own menu items to the editor menubar.
Since at the creation of the menu items access to DI is needed (i.e. to get ahold of a ConfigManager), the
RulesetEditorMenuBarItemsclass extends fromComponentand is added as a child to theEditor.