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.

Sign up for GitHub

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

Closed
wants to merge 6 commits into from
Closed

action command prompts #1661

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ffee138
change setNotification to setInfo conforming to standard logging leve…
pciavald Apr 13, 2021
1ad7051
implement printernotification socket event emitter
pciavald Apr 13, 2021
ba2a3a5
implement printernotification prompt
pciavald Apr 13, 2021
2c69c5b
handle prompts correctly
pciavald Apr 13, 2021
242e395
Merge branch 'upstream' into action-commands
pciavald Apr 13, 2021
b9eb29c
Merge branch 'main' into action-commands
pciavald May 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/app/config/setup/octoprint-authentication/octoprint-authentication.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ export class OctoprintAuthenticationComponent {
private sendLoginRequest(): void {
this.authService.startAuthProcess(this.octoprintURL).subscribe(
token => {
this.notificationService.setNotification(
this.notificationService.setInfo(
$localize`:@@login-request-sent:Login request send!`,
$localize`:@@login-request-sent-message:Please confirm the request via the popup in the OctoPrint WebUI.`,
);
Expand Down
97 changes: 76 additions & 21 deletions src/app/event.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,9 @@ import { Router } from '@angular/router';
import { Subscription } from 'rxjs';

import { ConfigService } from './config/config.service';
import { PrinterEvent } from './model/event.model';
import { PrinterEvent, PrinterNotification } from './model/event.model';
import { SocketService } from './services/socket/socket.service';
import { NotificationService } from './notification/notification.service';

@Injectable()
export class EventService implements OnDestroy {
Expand All @@ -15,35 +16,89 @@ export class EventService implements OnDestroy {
public constructor(
private socketService: SocketService,
private configService: ConfigService,
private notificationService: NotificationService,
private router: Router,
) {
this.subscriptions.add(
this.socketService.getEventSubscribable().subscribe((event: PrinterEvent) => {
if (event === PrinterEvent.PRINTING || event === PrinterEvent.PAUSED) {
setTimeout(() => {
this.printing = true;
}, 500);
this.socketService.getEventSubscribable().subscribe((event: PrinterEvent | PrinterNotification) => {
if (this.isPrinterNotification(event)) {
this.handlePrinterNotification(event);
} else {
setTimeout(() => {
this.printing = false;
}, 1000);
}

if (event === PrinterEvent.CLOSED) {
this.router.navigate(['/standby']);
} else if (event === PrinterEvent.CONNECTED) {
setTimeout(() => {
if (this.configService.isTouchscreen()) {
this.router.navigate(['/main-screen']);
} else {
this.router.navigate(['/main-screen-no-touch']);
}
}, 1000);
this.handlePrinterEvent(event);
}
}),
);
}

// 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 {
Comment on lines +33 to +35
Copy link
Owner

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.

return typeof object === 'object'
&& object !== null
&& ('text' in object || 'message' in object || 'action' in object);
}

private handlePrinterNotification(event: PrinterNotification): void {
const messages = {
'FilamentRunout T0': $localize`:@@prompt-filament-runout-t0:Filament runout detected. Ejecting filament, please wait...`,
'Nozzle Parked': $localize`:@@prompt-filament-runout:A filament runout has been detected. Please remove the ejected filament, insert filament from a new spool and press Continue.`,
'Continue': $localize`:@@prompt-continue:Continue`,
'Paused': $localize`:@@prompt-filament-runout-resume:The filament has been primed. Do you want to continue printing?`,
'PurgeMore': $localize`:@@prompt-filament-runout-purge:Purge more filament`,
'Heater Timeout': $localize`:@@prompt-heater-timeout:The hotend has been disabled due to inactivity, to avoid burning the filament. Press Reheat when ready to resume.`,
'Reheat': $localize`:@@prompt-reheat:Reheat`,
'Reheat Done': $localize`:@@prompt-reheat-done:The hotend is now ready.`,
};
if (event.action === 'close') {
this.notificationService.closeNotification();
} else if (event.choices?.length > 0) {
// event is action:prompt
this.notificationService.setPrompt(
$localize`:@@action-required:Action required`,
messages[event.text] || event.text,
event.choices.map(c => messages[c] || c)
);
} else if (event.choices?.length == 0) {
// event is action:prompt without choices
this.notificationService.setWarning(
$localize`:@@printer-information:Printer information`,
messages[event.text] || event.text
);
} else {
// 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
Comment on lines +68 to +74
Copy link
Owner

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?

Copy link
Contributor Author

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.

}
}

private handlePrinterEvent(event: PrinterEvent): void {
if (event === PrinterEvent.PRINTING || event === PrinterEvent.PAUSED) {
setTimeout(() => {
this.printing = true;
}, 500);
} else {
setTimeout(() => {
this.printing = false;
}, 1000);
}

if (event === PrinterEvent.CLOSED) {
this.router.navigate(['/standby']);
} else if (event === PrinterEvent.CONNECTED) {
setTimeout(() => {
if (this.configService.isTouchscreen()) {
this.router.navigate(['/main-screen']);
} else {
this.router.navigate(['/main-screen-no-touch']);
}
}, 1000);
}
}

ngOnDestroy(): void {
this.subscriptions.unsubscribe();
}
Expand Down
8 changes: 8 additions & 0 deletions src/app/model/event.model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,11 @@ export enum PrinterEvent {
IDLE,
UNKNOWN,
}

// either notification (message) or prompt (action, text and choices)
export interface PrinterNotification {
message?: string,
action?: string,
text?: string,
choices?: string[],
}
1 change: 1 addition & 0 deletions src/app/model/system.model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ export interface Notification {
heading: string;
text: string;
type: string;
choices?: string[];
closed: () => void;
}

Expand Down
20 changes: 20 additions & 0 deletions src/app/notification/notification.component.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,29 @@
<div
class="notification"
*ngIf="!notification.choices"
[ngClass]="['notification__border-' + notification.type, show ? 'notification__show' : '']"
(click)="hideNotification()"
>
<span class="notification__heading">{{ notification.heading }}</span>
<span class="notification__text">{{ notification.text }}</span>
<span class="notification__close" i18n="@@tap-close">tap this card to close it</span>
</div>

<div
class="notification-prompt"
*ngIf="notification.choices?.length > 0"
Copy link
Owner

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?

Copy link
Contributor Author

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

[ngClass]="[show ? 'notification-prompt__show' : '']"
>
<div class="notification__heading">{{ notification.heading }}</div>
<div class="notification__text">{{ notification.text }}</div>
<div class="notification-prompt__choices">
<div
class="notification-prompt__choice"
*ngFor="let choice of notification.choices; index as i; first as isFirst"
[ngClass]="[isFirst ? 'notification-prompt__choice-first' : '']"
(click)="answerPrompt(i)"
>
{{ choice }}
</div>
</div>
</div>
148 changes: 94 additions & 54 deletions src/app/notification/notification.component.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,54 +1,94 @@
.notification {
display: block;
position: fixed;
width: 80vw;
background-color: #5a6675;
border-radius: 1.3vw;
left: 10vw;
top: -75vh;
transition: top 0.7s ease-in-out;
box-shadow: 0 4px 10px -3px rgba(0, 0, 0, 0.75);
z-index: 100;
border-right: 1vw solid #5a6675;

&__show {
top: 5vh;
}

&__heading {
display: block;
text-align: center;
font-size: 3.5vw;
font-weight: 500;
margin-top: 4vh;
}

&__text {
font-size: 2.7vw;
padding: 5vh 3vw 1vh;
display: block;
text-align: center;
}

&__close {
font-size: 2vw;
display: block;
text-align: center;
padding-bottom: 2vh;
opacity: 0.4;
}

&__border {
&-error {
border-left: 1vw solid #c23616;
}

&-warn {
border-left: 1vw solid #fbc531;
}

&-notification {
border-left: 1vw solid #4bae50;
}
}
}
.notification {
display: block;
position: fixed;
width: 80vw;
background-color: #5a6675;
border-radius: 1.3vw;
left: 10vw;
top: -75vh;
transition: top 0.7s ease-in-out;
box-shadow: 0 4px 10px -3px rgba(0, 0, 0, 0.75);
z-index: 100;
border-right: 1vw solid #5a6675;

&-prompt {
position: fixed;
width: 100vw;
height: 100vh;
left: 0vw;
top: -100vh;
transition: top 0.7s ease-in-out;
background-color: #5a6675;
z-index: 100;
display: flex;
flex-direction: column;
justify-content: space-around;

&__show {
top: 0vh;
}

&__choices {
display: flex;
flex-direction: column;
flex-wrap: wrap;
justify-content: center;
padding: 0vw 5vw;
}

&__choice {
display: block;
font-size: 3vw;
background-color: #2196f3;
text-align: center;
padding: 3vh;
border-radius: 0.8vw;
margin: 1vw;

&-first {
background-color: #44bd32;
}
}
}

&__show {
top: 5vh;
}

&__heading {
display: block;
text-align: center;
font-size: 3.5vw;
font-weight: 500;
margin-top: 4vh;
}

&__text {
font-size: 2.7vw;
padding: 5vh 3vw 1vh;
display: block;
text-align: center;
}

&__close {
font-size: 2vw;
display: block;
text-align: center;
padding-bottom: 2vh;
opacity: 0.4;
}

&__border {
&-error {
border-left: 1vw solid #c23616;
}

&-warn {
border-left: 1vw solid #fbc531;
}

&-info {
border-left: 1vw solid #4bae50;
}
}
}
26 changes: 25 additions & 1 deletion src/app/notification/notification.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import { Component, NgZone, OnDestroy } from '@angular/core';
import { HttpClient } from '@angular/common/http';
import { Subscription } from 'rxjs';
import { catchError } from 'rxjs/operators';

import { Notification } from '../model';
import { ConfigService } from '../config/config.service';
import { NotificationService } from './notification.service';

@Component({
Expand All @@ -16,11 +19,17 @@ export class NotificationComponent implements OnDestroy {
heading: '',
text: '',
type: '',
choices: null,
closed: null,
};
public show = false;

public constructor(private notificationService: NotificationService, private zone: NgZone) {
public constructor(
private notificationService: NotificationService,
private zone: NgZone,
private http: HttpClient,
private configService: ConfigService,
) {
this.subscriptions.add(
this.notificationService
.getObservable()
Expand All @@ -35,6 +44,21 @@ export class NotificationComponent implements OnDestroy {
}
}

public answerPrompt(index: number): void {
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();
}
Comment on lines +48 to +60
Copy link
Owner

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.


private setNotification(notification: Notification | 'close'): void {
this.zone.run(() => {
if (notification === 'close') {
Expand Down
Loading