Add Customizability to SystemCommandMap #84
Now, the activator of such a map can be freely chosen.
Additionally cleaned up SystemCommandMap a lot, added a new system command and
informed the user in case of an error.
This branch will serve as the base of Envoy CLI as now also "no activator"-detection is possible.
A little hint for reviewers: In this case, the old side of the diff is really uninteresting as half the class has been remodeled.
This is basically what I was looking for in this branch, however I have one question / suggestion:
noActivator constants seem quite unnecessary to me. The value
/ can be assigned to the
activator variable in the default constructor.
Then, both the default activator and the special value for no activator can be documented inside the respective constructors.
The reason being, that someone who isn't well acquainted with the class would first look at the Javadoc of the constructors, and not search for some obscure constants, one of which (
defaultActivator) isn't even intended to be used, as you would just call the default constructor instead.
This looks good to me. But please make sure you add usage to the recommendation functionallity and an activator selector in the settings screen because otherwise this code just lies around unused and as you probably know by now: @kske (and I as well) doesn't like that. But besides that excellent work. 👍
@DieGurke It was never intended for the reason you just stated.
This action rather needed to be taken so that we can somewhen start Envoy CLI without always having to prepend a slash when starting commands. Just think of the Bash and how annoyed you'd be if you'd always have to start with a '/' (i. e. like in postgres).
However, I do see your point.
Still, I'd say that implementing this feature is a branch of its own. Open an issue.
Another reason why this had to be implemented, is to ensure reusability. The SystemCommands are one of those components in Envoy that can be easily ported into another project. (Well, you maybe have to delete three or four logger statements and delete the lines showing the alerts if you don't use JavaFX), but that's a really easy fix.
I totally agree with your point and I see your intial thoughts of why you implemented the recommondation mechanism (no activatior commands), but I really think that we should use this good looking mechanism as a recommondation feature (just like code assist) as well. But I totally agree creating a new Issue and a seperate PR for this.
I created an extra issue for this now. (just to avoid duplication) #87
No due date set.
No dependencies set.
Deleting a branch is permanent. Although the deleted branch may exist for a short time before cleaning up, in most cases it CANNOT be undone. Continue?