GH-1081 Auto-dismiss notification. #1355

lmcbout · 2018-02-22T16:27:15Z

Auto-dismiss the notification
Increase the timer when the user has choices to decide

Signed-off-by: Jacques Bouthillier [email protected]

marcdumais-work · 2018-02-22T17:57:02Z

I think the original idea was to have the auto-dismiss be optional (with configurable delay). See #1081

We can continue the discussion in that issue?

epatpol · 2018-02-22T20:22:10Z

packages/messages/src/browser/notifications.ts

+            let adjustedCloseTimer = this.defaultAutoCloseTimeout;
+            // Give more time before activating the autoClose Notification when user has choices
+            if (properties.actions !== undefined && properties.actions.length > 1) {
+                adjustedCloseTimer = adjustedCloseTimer * properties.actions.length;


Just for reference, we discussed this offline and the conclusion is that it's better to make it auto-dismissable for certain severity level i.e info/warning and make it must-be-dismissed-manually for things like errors.

Also I don't think using the number of possible actions in the message is a good factor to use for the duration of the message, as there's probably no correlation between the number of actions a user can do on that message and the severity of it.

I overlooked your comment, as already collapsed.

That's the thing, all kind of use cases require different options. I think, it's up to the MessageService clients to decide about timeout and other properties of messages. OTOH we can have default options for most common cases.

AlexTugarev

Let's start with adding an optional timeout property to Message

https://github.com/theia-ide/theia/blob/issue_1081/packages/core/src/common/message-service-protocol.ts#L20-L24

AlexTugarev · 2018-02-22T20:58:47Z

packages/messages/src/browser/notifications.ts

@@ -29,6 +29,8 @@ export interface Notification {

 export class Notifications {

+    private defaultAutoCloseTimeout = 5000;
+    private closeTimer: NodeJS.Timer;


please do not use node types in browser/*.

if multiple messages are pushed, Notifications.show will be called that many times, but this closeTimer will be reassigned with latest only. actually, closing the first message will clear last timer.

AlexTugarev · 2018-02-22T20:59:45Z

packages/messages/src/browser/notifications.ts

@@ -29,6 +29,8 @@ export interface Notification {

 export class Notifications {

+    private defaultAutoCloseTimeout = 5000;


I think, it's better to use preferences for this kind of defaults.

AlexTugarev · 2018-02-22T21:03:26Z

packages/messages/src/browser/notifications.ts

                });
            }
+
+            // Do not auto-close the "error" notification
+            if (properties.icon.toLowerCase() !== 'error') {


it should be configurable via properties of a Message as required in #1081 (comment), because the client needs to decide, if this message should be sticky or not.

lmcbout · 2018-02-28T18:31:43Z

I Have a question:
I put the auto-Dismiss value optional in message-service.ts.
When I call the service in window-impl.ts or in cpp-client-contribution, I needed to add an extra parameter even if it was optional. I put "undefined" for the auto-dismiss value even if there was other optionals value defined. When I tested with task, I did not need to define the optional value since there was nothing else than the message text. I was able to add an auto-dismiss value and it was working fine.
Is there a way to have a second optional fields and not put "undefined" if we don't plan to use a timer for the auto-dismiss notification ?

AlexTugarev · 2018-03-01T06:17:41Z

packages/languages/src/common/window-impl.ts

@@ -28,16 +28,16 @@ export class WindowImpl implements Window {
            return undefined;
        };
        if (type === MessageType.Error) {
-            return this.messageService.error(message, ...actionTitles).then(mapActionType);
+            return this.messageService.error(message, undefined, ...actionTitles).then(mapActionType);


we should introduce function overload to keep the less verbose API for most common use cases.

Overload method added, so no need to use undefined anymore

AlexTugarev · 2018-03-01T06:21:36Z

packages/core/src/common/message-service.ts

+     * If we set autoDismiss: 0, then the notification preference will be ignored and notification remains until user intervention.
+     */
+
+    log(message: string, autoDismiss?: number | undefined, ...actions: string[]): Promise<string | undefined> {


function overload would help to add more optional message option while keeping simple API for most common use cases. for example:

log(message: string, ...actions: string[]): Promise<string | undefined>; log(message: string, options?: MessageOptions, ...actions: string[]): Promise<string | undefined>; log(message: string, ...args: any[]): Promise<string | undefined> { // TODO decompose optionaly MessageOptions from args[0] // return ... }

clients would see these two overloads in content assist. we would need to unwrap the arguments.

also we should add MessageOptions to encapsulate, as we might add more of them.

@AlexTugarev what about also putting actions in that MessageOptions structure?

Good question. Should the options describe the message, or it's rendering/behavior.
I'd also like to improve actions of messages, as they currently are label and value at the same time. Have a look at the resolution of resulting promises, that's what you selected.
OTOH the very first and simplest signature from above is quiet handy, and should be kept.
But do you think, we need to tackle that point now?

Oh sorry I didn't see the message above where you mentionned function overloads. Yes that could work also, I just thought that if they were also options they could go in the same structure, but indeed it looks like the structure might not be appropriate for them.

AlexTugarev · 2018-03-01T06:26:40Z

packages/messages/src/browser/notification-preferences.ts

+export const NotificationConfigSchema: PreferenceSchema = {
+    "type": "object",
+    "properties": {
+        "notification.autoDismiss": {


please rename it to something like notification.timeout. I'm planing to revise more parts of user notifications in #1360, and I think we should distinct between dismissing and hiding a message. a timeout is more generics, which leaves more room for interpretation.

just to mention it, hiding message and keeping them in kind of an inbox are ideas from #1360. I'm aware of this PR to target the closing of messages after the timeout period elapsed.

auto-dismiss rename to timeout

AlexTugarev · 2018-03-01T06:28:43Z

packages/cpp/src/browser/cpp-client-contribution.ts

@@ -45,7 +45,7 @@ export class CppClientContribution extends BaseLanguageClientContribution {
            const ERROR_MESSAGE = "Error starting C/C++ language server. " +
                "Please make sure 'clangd' is installed on your system. " +
                "You can refer to the clangd page for instructions.";
-            this.messageService.error(ERROR_MESSAGE, READ_INSTRUCTIONS_ACTION).then(selected => {
+            this.messageService.error(ERROR_MESSAGE, undefined, READ_INSTRUCTIONS_ACTION).then(selected => {


you see, it's inconvenient to add this undefined parameter all over the places.

agree, no more needed to use undefined with the overload method

AlexTugarev · 2018-03-01T06:37:28Z

packages/messages/src/browser/notifications-message-client.ts

@@ -35,14 +41,17 @@ export class NotificationsMessageClient extends MessageClient {
            label: action,
            fn: element => onCloseFn(action)
        });
+
+        const timeout = (message.timeout || 0);


if message.actions are undefined or empty, timeout should be undefined, right?

Timeout should be undefined when there is some actions defined. If no actions defined, then the user can still set a timeout to 0 (keep notification until user interaction) or any timeout set in milisec or the user preferences.

you're right! the existence of actions should be the first criteria.

but I think instead of patching the timeout in NotificationsMessageClient.showMessage and evaluating everything in Notifications.contructor, we should compute the effective timeout here onve. considering the requirement could we calculate the timeout like this?

const timeout = (message.action) ? undefined : (message.timeout || this.preferences['notification.timeout'])

could you update the PR, please?

AlexTugarev · 2018-03-01T06:39:26Z

packages/messages/src/browser/notifications.ts

                });
            }
+            // Auto-dismiss is only allowed when there is only one action available.


please move the the check to showToast, it will simplify things here.

AlexTugarev · 2018-03-01T06:40:28Z

packages/messages/src/browser/notifications.ts

@@ -29,6 +30,7 @@ export interface Notification {

 export class Notifications {

+    private closeTimer: any;


it's number | undefined if you use window.setTimeout below.

I think this should go into the scope of createNotificationElement

we can avoid use any here, as already commented

Move the closeTimer to createNotificationElement

lmcbout · 2018-03-06T20:05:02Z

ready to be reviewed

AlexTugarev · 2018-03-07T14:23:49Z

packages/messages/src/browser/notifications.ts

@@ -72,8 +74,16 @@ export class Notifications {
                button.addEventListener('click', () => {
                    action.fn(handler);
                    close();
+                    clearTimeout(this.closeTimer);


@lmcbout here you're referencing the closeTimer property of the Notifications instance. this is not, what you want to say it should do, right? you want to clear the timeout of the single message, but this way each click handler will clear the timeout of the latest shown message. I think I already commented on this, but GH comments were rooted out.

Craete a clearTimeout() and setTimeout, so I can use the window.setTimeout and window.cearTimeout() method

No need to create the set and clear timeout method, using wndow.set instead

AlexTugarev · 2018-03-07T14:24:50Z

packages/messages/src/browser/notifications.ts

                });
            }
+            if (properties.timeout !== undefined && properties.timeout > 0) {


minor: !!properties.timeout && ... is shorter

Done this minor modification

AlexTugarev · 2018-03-07T14:30:10Z

packages/messages/src/browser/notifications.ts

@@ -72,8 +74,16 @@ export class Notifications {
                button.addEventListener('click', () => {


shouldn't we clear the timeout before fn call?

Should not matter, but I will mve the clearTimeout before the close() call

AlexTugarev · 2018-03-07T14:36:33Z

packages/messages/src/browser/notifications.ts

                });
            }
+            if (properties.timeout !== undefined && properties.timeout > 0) {


you can move this if statement up before L70 to make a local closeTimer.
something like const closeTimer = !!properties.timeout ? window.setTimeout() : undefined should work.
then you can use it in click handler.

Move the closeTimer before and use it as a const

AlexTugarev · 2018-03-07T14:36:52Z

packages/messages/src/browser/notifications.ts

                });
            }
+            if (properties.timeout !== undefined && properties.timeout > 0) {
+                this.closeTimer = setTimeout(() => {
+                    if (this.closeTimer !== undefined) {


if won't be necessary then.

AlexTugarev · 2018-03-07T14:38:05Z

packages/messages/src/browser/notifications-message-client.ts

@@ -35,14 +37,24 @@ export class NotificationsMessageClient extends MessageClient {
            label: action,
            fn: element => onCloseFn(action)
        });
+
+        // If some actions are defined, we don't set the timeout


convert to JSDoc, please.

Using macro does not need anymore the comment

AlexTugarev · 2018-03-07T14:39:19Z

packages/core/src/common/message-service-protocol.ts

@@ -21,12 +21,17 @@ export interface Message {
    type: MessageType;
    text: string;
    actions?: string[];
+    timeout?: number;


shouldn't that be a options?: MessageOptions property?

Will be name options: MessageOptions

AlexTugarev · 2018-03-07T14:40:48Z

packages/core/src/common/message-service.ts


 @injectable()
 export class MessageService {

    constructor(
        @inject(MessageClient) protected readonly client: MessageClient
    ) { }
+    /**
+     *
+     * MessageSevice uses some methods to report information either log, info, warn and error


please, convert to JSDoc.

AlexTugarev · 2018-03-07T14:41:41Z

packages/core/src/common/message-service.ts

+    /**
+     *
+     * MessageSevice uses some methods to report information either log, info, warn and error
+     * One parameter is optional:  @param options.timeout


this doc should go to MessageOptions, right?

AlexTugarev · 2018-03-07T14:50:18Z

packages/core/src/common/message-service.ts

+        let actionsValue: string[] | undefined;
+
+        if (args && args.length > 0) {
+            if (args[0]['timeout'] !== undefined) {


please use type guards if possible. also, I think we can assume args[0] to be message options if type is object.

would be nice, if you add the type guard in the namespace of MessageOptions.

In the next version, I remove theoverloa method by addibg the actions and timeout within the same API, so the typeguard are not require, the code is simplier here

AlexTugarev · 2018-03-07T14:51:13Z

packages/core/src/common/message-service.ts

+            } else {
+                actionsValue = args;
+            }
+            return this.client.showMessage({ type: messageType, timeout: timeoutvalue, text: messageText, actions: actionsValue });


there is no need to explode the properties of options here.

AlexTugarev · 2018-03-07T14:52:17Z

packages/core/src/common/message-service.ts

-    log(message: string, ...actions: string[]): Promise<string | undefined> {
-        return this.client.showMessage({ type: MessageType.Log, text: message, actions: actions || [] });
+    log(message: string, ...actions: string[]): Promise<string | undefined>;
+    log(message: string, options?: MessageOptions): Promise<string | undefined>;


is this one actually needed? please check, if it's same as the next line.

lmcbout · 2018-03-08T19:16:03Z

With the commit : fd4b468
In file message-service.ts, I added a new API structure for the message(MessageArgs). I tested with the error method only.
With this API structure, it allow us to add any number of "optional " fields or structure. It prevent us to create overload method. Long term, it would be better. Short term, the drawback we need to adjust all messageService being used. Also, when using the messageService like :
OLD: this.messageService.error(Error launching task '${taskName}': ${error} );
instead of:
NEW: this.messageService.error({ message: Error launching task '${taskName}': ${error}, options: { timeout: 1357 } });

we need to use the API data structure ({message:...}) and we can add optional data as well

AlexTugarev · 2018-03-09T05:32:26Z

@lmcbout, we discussed that kind of change before, and decided to have simple parameters to be less verbose. just compare the two examples, and consider that a lot of use cases won't require anythings but the message.

also, this is not about us avoid overload methods for MessageService, but to provide a decent set of methods for API clients to cover different use cases and avoid unnecessary structures.

I don't see the need to break the MessageService API. MessageOptions are optional and give us enough space to add more options in future. please have a look at the usages of the MessageService, what kind of issues would that change solve?

Add notifications preferences to allow auto-dismiss Auto-Dismiss can be used/disabled for each notification since it is optional When the user has more than one action available, the auto-Dismiss timeout will not be applied Signed-off-by: Jacques Bouthillier <[email protected]>

lmcbout · 2018-03-09T19:28:33Z

Adjusted the MessageOption API to include the optional timeout and actions within the same API. This allowed me to simplify the code and allow the end-user to enter just a message string if no actions or timeout were added. Refering to some link on the web, it seems the way I implemented the optional parameter is the right one. Also, all web sites were in favor not to use to much the overload in Typescript.
Code ready to be reviewed

AlexTugarev · 2018-03-10T08:24:16Z

@lmcbout

Refering to some link on the web, it seems the way I implemented the optional parameter is the right one.

Which sources?

Also, all web sites were in favor not to use to much the overload in Typescript.

Sources? That sounds a bit arbitrary for an explanation. Are there technical reasons?
To emphasize this use case, let's focus on adding new options without breaking API, when there are no advantages gained.

AlexTugarev · 2018-03-10T08:28:16Z

packages/core/src/common/message-service-protocol.ts

+    options?: MessageOptions;
+}
+
+export interface MessageOptions {


the options were meant to add more hints, so that MessageClients could decide on how to handle them.

please, move the actions back to Message.

should be message.actions, not message.options!.actions.

AlexTugarev · 2018-03-10T08:41:33Z

@lmcbout please, reintroduce the overload methods for MessageService.

https://github.com/theia-ide/theia/blob/dd4bc3f35cb81702c96eb34b95c9661acb6494c9/packages/languages/src/common/window-impl.ts#L30-L32

such a change does require to adapt all caller sites, but there is no noticeable advantage. I think we should keep this convenience API for actions at the moment.

also, as stated before, we can consider to revise the actions. but this should be in a different scope then #1081. let's focus on the original request.

lmcbout · 2018-03-12T18:54:10Z

@AlexTugarev
I will bring back the overload, and look at your latest comment.
But just as an example, here a link why we should be using less overload, just read the first paragraph of microsoft/TypeScript#12041 . It says:
"Today's function overloading in TypeScript is a compromise which is the worst of both worlds of dynamic, non-overloaded language like JS, and static, overloaded language such as TS."

There are also other links talking on the subject, but stop here.
I personnally think regrouping all optional parameters within the same API would reduce the complexity to maintain the code in a long run.

lmcbout · 2018-03-12T21:23:53Z

@AlexTugarev I just created a new version for the auto-dismiss with the function overload. I think I covered all your previous comments. If there is something missing, I would be happy to get your version for the fix, I will be happy to merge it if ever you find something missing form previous version.

AlexTugarev · 2018-03-13T11:38:01Z

@lmcbout, could you please reopen this?

AlexTugarev · 2018-03-13T13:00:31Z

@lmcbout

But just as an example, here a link why we should be using less overload, just read the first paragraph of microsoft/TypeScript#12041 . It says:
"Today's function overloading in TypeScript is a compromise which is the worst of both worlds of dynamic, non-overloaded language like JS, and static, overloaded language such as TS."

That sounds like Shlomibo tried to make a case to change overloads in TS, which was declined. I do not see a reason to stop using it, just because people wish it would be extended.

I personnally think regrouping all optional parameters within the same API would reduce the complexity to maintain the code in a long run.

Couldn't agree more. It's the point about making the breaking change right now.

akosyakov requested a review from AlexTugarev February 22, 2018 16:33

epatpol reviewed Feb 22, 2018

View reviewed changes

lmcbout force-pushed the issue_1081 branch from 812cc28 to d9d0434 Compare February 22, 2018 20:22

AlexTugarev reviewed Feb 22, 2018

View reviewed changes

lmcbout force-pushed the issue_1081 branch 3 times, most recently from 4ee02bc to e0e62e4 Compare February 28, 2018 18:23

AlexTugarev reviewed Mar 1, 2018

View reviewed changes

lmcbout force-pushed the issue_1081 branch from e0e62e4 to 3ba43a7 Compare March 6, 2018 19:49

AlexTugarev reviewed Mar 7, 2018

View reviewed changes

lmcbout force-pushed the issue_1081 branch from 3ba43a7 to fd4b468 Compare March 8, 2018 19:14

lmcbout force-pushed the issue_1081 branch from fd4b468 to dd4bc3f Compare March 9, 2018 19:24

AlexTugarev reviewed Mar 10, 2018

View reviewed changes

lmcbout closed this Mar 12, 2018

lmcbout mentioned this pull request Mar 13, 2018

GH-1081 Auto-dismiss notification. #1491

Merged

		@@ -29,6 +29,8 @@ export interface Notification {

		export class Notifications {

		private defaultAutoCloseTimeout = 5000;

		@@ -29,6 +30,7 @@ export interface Notification {

		export class Notifications {

		private closeTimer: any;

		@@ -72,8 +74,16 @@ export class Notifications {
		button.addEventListener('click', () => {

GH-1081 Auto-dismiss notification. #1355

GH-1081 Auto-dismiss notification. #1355

Conversation

lmcbout commented Feb 22, 2018

marcdumais-work commented Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexTugarev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmcbout commented Feb 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmcbout commented Mar 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmcbout commented Mar 8, 2018

AlexTugarev commented Mar 9, 2018

lmcbout commented Mar 9, 2018

AlexTugarev commented Mar 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexTugarev commented Mar 10, 2018

lmcbout commented Mar 12, 2018

lmcbout commented Mar 12, 2018

AlexTugarev commented Mar 13, 2018

AlexTugarev commented Mar 13, 2018

marcdumais-work commented Feb 22, 2018 •

edited

Loading