Skip to content

Conversation

@goaaats
Copy link
Member

@goaaats goaaats commented Apr 15, 2025

No description provided.

@goaaats goaaats requested a review from a team as a code owner April 15, 2025 19:10
Copy link
Member

@KazWolfe KazWolfe left a comment

Choose a reason for hiding this comment

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

We're moving from InternalName to GUID in a bunch of places. Is there a reason for this vs just passing in a reference to that loaded plugin?


/// <inheritdoc/>
public ReadOnlyDictionary<string, IReadOnlyCommandInfo> Commands => new(this.commandMap);
[Api13ToDo("Make this sensible. Don't use exposed API for internal structures.")]
Copy link
Member

Choose a reason for hiding this comment

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

API note: some people can/do use this, we may need to create an interface to expose commands. (See: SimpleTweaks)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we definitely still want this. The intent isn't to remove it, but to replace it with something nicer.

/// <inheritdoc/>
public bool RemoveHandler(string command)
[Experimental("Dalamud001")]
public bool AddCommand(string commandName, string helpMessage, Delegate func, bool showInHelp = true, int displayOrder = -1)
Copy link
Member

Choose a reason for hiding this comment

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

Honestly not sure here re a bare Delegate. Maybe a new class like a CommandExecutionContext would be better? Would let us pass args as a bare string, "parsed" args, and other context info (e.g. if we're in a macro).

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.

2 participants