Add Customizability to SystemCommandMap #84

delvh · 2020-10-06T22:23:40+02:00

delvh commented

2020-10-06 22:23:40 +02:00

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.

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.

delvh added this to the v0.3-beta milestone 2020-10-06 22:23:40 +02:00

delvh added the

M

client

labels 2020-10-06 22:23:40 +02:00

delvh self-assigned this 2020-10-06 22:23:40 +02:00

delvh requested review from mpk 2020-10-06 22:23:49 +02:00

delvh requested review from kske 2020-10-06 22:23:50 +02:00

delvh commented

2020-10-06 22:25:44 +02:00

A little hint for reviewers: In this case, the old side of the diff is really uninteresting as half the class has been remodeled.

kske requested changes 2020-10-07 07:41:40 +02:00

kske left a comment

This is basically what I was looking for in this branch, however I have one question / suggestion:

The defaultActivator and 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 is basically what I was looking for in this branch, however I have one question / suggestion: The `defaultActivator` and `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.

delvh commented

2020-10-07 11:14:43 +02:00

@kske better?

kske approved these changes 2020-10-07 12:06:39 +02:00

kske left a comment

Better.

mpk approved these changes 2020-10-07 21:40:22 +02:00

mpk left a comment

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. 👍

client/src/main/java/envoy/client/data/commands/SystemCommandMap.java Outdated

						
				@ -64,3 +90,3 @@

					 * It returns the command as (most likely) entered as key in the map for the

					 * first word of the text.<br>

					 * It should only be called on strings that contain a "/" at position 0/-1.

					 * Activators in the middle of the wod will be disregarded.

mpk commented

2020-10-07 21:24:58 +02:00

there is a typo

delvh commented

2020-10-07 21:46:33 +02:00

@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.

@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.

mpk commented

2020-10-07 22:00:40 +02:00

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.