Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API: scopes for commands #1546

Closed
alwx opened this issue Aug 2, 2017 · 2 comments
Closed

API: scopes for commands #1546

alwx opened this issue Aug 2, 2017 · 2 comments
Assignees
Milestone

Comments

@alwx
Copy link
Contributor

alwx commented Aug 2, 2017

Some commands should be available only for group chats, some — only for 1-to-1 chats. Some commands can be used when all participants in group chat are bots, and others cannot.
We can solve all these issues by introducing visibility scopes.
More importantly — there should be an ability to define two commands with the same name and different visibility scopes. This improvement will allow us to remove some parameters like registeredOnly, get rid of some hardcoded parts and also improve /send and /request commands (for instance, get rid of contact chooser in 1-to-1 chats).

@alwx alwx added this to the Beta milestone 1 of 6 milestone Aug 2, 2017
@alwx alwx self-assigned this Aug 2, 2017
@janherich
Copy link
Contributor

janherich commented Aug 2, 2017

Hi @alwx when you are already working on this, there is one thing I need to share. The current way how commands are loaded in Status is very poor - when any command function taking message params (validator, preview, shortPreview) is registered through API, Status app loads them from Jail.VM not as a functions ready to be applied to message params, but already as results of function application to message params (which is happening in Jail.VM i guess).
As a result of this, the logic of loading commands is super complicated (you can't just load command, you need to load command (function) results for every contact/chat/message) and app-db is bloated with command-results for every message.
Since the very short time I have been working on the project, I already encountered 2 bugs directly caused by this (the need to have command fn results loaded for every message).
When you introduce command scoping, without addressing this issue, it will be even much worse.

What I propose is to not fetch command results from jail and only use them as needed in Status (for example preview function will be used by subscription for messages ready to be rendered).
We will need to solve the issue of having to call Jail each time command fn is being called, I will try to investigate if this is viable option and if it wouldn't slow the application too much.

@sla-shi sla-shi modified the milestones: 0.9.12, Beta milestone 2 of 6 Sep 11, 2017
@flexsurfer flexsurfer modified the milestones: 0.9.12, 0.9.13 Oct 19, 2017
@oskarth
Copy link
Contributor

oskarth commented Oct 23, 2017

@flexsurfer @alwx How come this is in 0.9.13 milestone? Hasn't this been fixed for 0.9.12 and can thus be closed? If there are enhancements I suggests these are captured in additional issues.

@oskarth oskarth closed this as completed Nov 13, 2017
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

No branches or pull requests

5 participants