Add Customizability to SystemCommandMap #84

Merged
delvh merged 2 commits from f/enhanced-system-commands into develop 2 years ago
delvh commented 2 years ago
Owner

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 2 years ago
delvh added the
M
client
labels 2 years ago
delvh self-assigned this 2 years ago
delvh requested review from DieGurke 2 years ago
delvh requested review from kske 2 years ago
Poster
Owner

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

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 2 years ago
kske left a comment
Owner

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.
Poster
Owner

@kske better?

@kske better?
kske approved these changes 2 years ago
kske left a comment
Owner

Better.

Better.
DieGurke approved these changes 2 years ago
DieGurke left a comment
Owner

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

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

there is a typo

there is a typo
Poster
Owner

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

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

I created an extra issue for this now. (just to avoid duplication) #87

I created an extra issue for this now. (just to avoid duplication) #87
delvh merged commit 6f9982bbc3 into develop 2 years ago
delvh deleted branch f/enhanced-system-commands 2 years ago
This repo is archived. You cannot comment on pull requests.
Loading…
There is no content yet.