-
Notifications
You must be signed in to change notification settings - Fork 677
Description
The Issue
I just found out about the SelectCommand and honestly it's kinda fantastic when it's needed. I noticed that there's a few commands that internally run their own commands or handle schedule management of those commands. The ProxyCommand sets its name to "Proxy (<name>)" for whatever <name> is passed in. Same thing with RepeatCommand with "Repeat" instead of "Proxy". WrapperCommand just sets its name to the name of the wrapped command.
I realized it might make a lot of sense for the default implementations of these to also set their name based on the currently running command, maybe in the format of "Selected(<name>)". I think that would help students understand which command was running compared to just "SelectCommand" which is what is shown now.
There's one aspect to this that I wanted to get thoughts on. All the commands that set their name based on a passed in command do it in the constructor. This is because a user can later change the name to something else. For the SelectCommand, it would need to be done when we determine which command we want to run which happens in intialize. That means if someone did name the SelectCommand itself, it would be overwritten with the name of the running command (which may just be the name of the selected class).
I can implement this, but wanted to run the details by y'all first before I submit a PR.
Where I would like others' thoughts
The SelectCommand could store a private String of its class name (By calling getName in the constructor) and would check if getName returned that string before calling setName. If it didn't match, someone else changed the name and it wouldn't be safe to call setName. The two aspects I don't like about this are that it's possible to hit state bugs (that are luckily VERY unit testable) and the fact that now we're storing a variable for the internally set name.
Frankly, I think for the SelectCommand, having a private m_lastInternallySetName variable isn't that bad.
We would be adding this code to the end of initialize (as well as setting it in the constructor and declaring the field, of course):
if(m_lastInternallySetName == getName()) {
String newName = "Selected(" + m_selectedCommand + ")";
setName(newName);
m_lastInternallySetName = newName;
}