-
-
Notifications
You must be signed in to change notification settings - Fork 225
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
action command prompts #1661
action command prompts #1661
Conversation
the socket service shouldn't be aware of the promptService. This should rather be handled by the event service and should be pushed through the I didn't had time to check out the notification service error today unfortunately, will do this tomorrow. Edit: If the prompt-service is needed than this should subscribe directly to the event-subject. |
fullscreen error sound correct as a base for the prompts, i can add the buttons in there. Did you have a chance to take a look? that is our last blocker, the rest is improvement |
I can't see any reference to fullscreen errors in the code, is it something you're working on? Maybe in that case it would be better named fullscreen messsage that can be an error, printer notification or prompt. |
Ah no that was more of a question on how stuff from the actions_command_prompt should be displayed. If it completely seizes printer operation in all cases it should be a fullscreen thing (like the sleep screen), otherwise it should be similar to notifications. In the second case it would also make more sense to extend the notification component, instead of creating a new one. |
I think actually it makes sense to extend the notification component in both cases, i'll try it now |
@UnchartedBull I don't see how i can pass the prompt through the event service without heavy modification, as it needs at least a title and a list of choices. I will keep the notification service in the socket service until a better way to do it is described. |
Create another interface in
Check what type the input is in |
3e930a1
to
ba2a3a5
Compare
i've started from scratch, can you check the new architecture ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this looks really good! Definitely the right way to go here I think. There are some minor things, most of them are just me being curious though :D. It also would be great if you could attach a screenshot on how the choices thing looks like :)
Once the last few kinks are ironed out this is ready to be merged. I originally planed on releasing v3 on Friday, could easily postpone to Monday or Tuesday though, if this PR should make it into v3.
// https://stackoverflow.com/questions/14425568/interface-type-check-with-typescript | ||
// https://stackoverflow.com/questions/8511281/check-if-a-value-is-an-object-in-javascript/8511350#8511350 | ||
private isPrinterNotification(object: any): object is PrinterNotification { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe another way of doing this would be to check whether the received message is NOT of type int
(which would indicate that it's an enum, thus PrinterEvent). It should work (haven't tested it though), but might be a bit cleaner.
I wouldn't switch the logic around though, as the default should be handle PrinterEvent. Alternatively you could check whether the received message is of type object
.
// event is action:notification | ||
// this.notificationService.setInfo( | ||
// $localize`:@@printer-information:Printer information`, | ||
// messages[event.message] || event.message | ||
// ); | ||
// TODO: annoying as a notification | ||
// should be put with an autoclear timeout in the bottom-right statusline |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it still should be a notification, just to keep everything consistent. If this isn't too important it might be a good idea to introduce a fourth notification type with a white strip and just set a timeout here to clear the notification after 5s (is that enough time?).
What do you think of that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to retest what exactly was being sent, but AFAIR it was every heat enable/disable message (not always coherent with the user actions) and all changes of % being broadcasted, really cluttering information. We have been using it for a month without these and it seems on-point.
|
||
<div | ||
class="notification-prompt" | ||
*ngIf="notification.choices?.length > 0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be enough to check whether notification.choices
exist here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is just in case an empty list is sent, we would not want to display a prompt without choices
this.http | ||
.post( | ||
this.configService.getApiURL('plugin/action_command_prompt'), | ||
{ command: 'select', choice: index }, | ||
this.configService.getHTTPHeaders() | ||
) | ||
.pipe( | ||
catchError(error => | ||
this.notificationService.setError($localize`:@@error-answer-prompt:Can't answer prompt!`, error.message), | ||
), | ||
) | ||
.subscribe(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be in system-service
(or another service if there's one that's better fitting) since it won't be compatible with Klipper / other interfaces.
From now on all communication with OctoPrint should be behind an abstract service with a generic interface to enable easy integration of other printer software.
]; | ||
|
||
plugins.forEach(plugin => | ||
plugin.check(pluginMessage.plugin.plugin) && plugin.handler(pluginMessage.plugin.data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That definitely looks complicated (which isn't bad). Just curious: Will call this both check methods from the plugins array? Seems like a nice way to do this, since it's easily extendable 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I think I understood it. Correct me if I'm wrong:
This will go through top to bottom and call the check method with the plugin string. If the check message returns true it will call the handler in the same object, thus handle the message. Otherwise it will just fall-through until it either matches or reaches the end of the array. Really elegant way of handling this 👍
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
I am sorry to announce that I do not have the work power to maintain all our changes in compliance with upstream requirements. This feature has been published to our repo CosmoDash. |
The code on this branch was in working state last time I checked so it should be pretty easy to finish up and merge ! |
Implements support for action:prompt and action:notification
Fixes #1624