Reuse never show again logic - work in progress by YisraelV · Pull Request #73968 · microsoft/vscode

YisraelV · 2019-05-19T17:00:48Z

@bpasero this is a work in progress, too early for a thorough code review, but the main api is there. Please let me know if you like the way this is going.

The main 2 points to emphasize:

The caller of "prompt" doesn't even know "never show again" was selected. They just call prompt and leave this logic to the service.
The "never show again" selections are persisted once when the program shuts down, and under a single key.

YisraelV · 2019-05-28T11:53:42Z

@bpasero I'll appreciate your thought on the general approach, so I know if to polish it further and make it ready for code review. Thanks in advance.

bpasero · 2019-05-28T12:35:28Z

@YisraelV yeah keep on going

YisraelV · 2019-06-03T16:45:09Z

@bpasero this is ready for code review. Thanks!

bpasero · 2019-06-04T13:09:59Z

@YisraelV your solution is not preserving existing storage keys if I see correctly? We cannot break users that already made a choice by inventing new storage keys, we have to respect the current ones.

If we introduce options to control this behaviour, I think we should make them available to both prompt and notify methods because we may have some intent to also never show notifications again (not just prompts). In that case I would think the "Do not shot again" button should be a secondary actions (under the gear icon). Does that make sense?

YisraelV · 2019-06-05T16:50:16Z

@bpasero totally missed breaking the current keys thanks!

I see two ways around this:

simple way: just store each key provided directly to storage, without wrapping it inside an object.

What worries me about this solution is that it could cause collisions if client code provides too generic keys. If I was using the "never show again" functionallity in my code, and didn't know my key was saved directly to stoeage, I would probably think "fileTooBig" was a specific enough key.

Check if the provided key is stored as is (without the wrapping object) The check can be removed after a few versions.

Please tell me what you think.

Regarding the notify method - will do.

bpasero · 2019-06-06T07:18:46Z

I think storing the key "as is" is OK, I mean we have been doing this today as well right? With the chance of collisions too.

YisraelV · 2019-06-06T07:27:14Z

@bpasero that's right but until now the prompt caller was dealing with the storage directly, so there might have been more awareness to the need of very specific keys, whereas now the client code doesn't know the key is stored in storage, and therefore might provide too generic keys.

Anyway if you don't think it's something to worry about I'll carry on.

bpasero · 2019-06-06T07:30:58Z

Understood, but I think we can document this and probably call this property "somethingId" to make this clear it is an identifier that should be unique.

YisraelV · 2019-06-11T18:55:52Z

@bpasero I addressed the issues you pointed out.

Thought it was better to add a second parameter to notify method since the first one is supposed to be passed forward to the model.

Let me know if anything needs changing.

Maybe it's best to use the new option of notify so we can make sure it works correctly?

bpasero

@YisraelV I think we need to preserve today's behaviour where the "Do not show again" action can either show as primary or secondary action, right? Your solution seems to only put it as secondary option.

Also, for the notify() case I would have expected the neverShowOptions as part of INotificationProperties

YisraelV · 2019-06-16T20:20:31Z

@bpasero fixed let me know of any other changes.

bpasero

@YisraelV some feedback provided. On top of that can we rethink if this "Do not show again" action appears as first or last choice? I feel like we should preserve todays behaviour where this action seems to appear as first entry and not last, simply because the most important buttons are typically more towards the right, but maybe you could check with our current usage and see what fits best.

bpasero · 2019-06-17T07:51:58Z

src/vs/platform/notification/common/notification.ts

Move this into INotificationProperties you do not need it both here and in IPromptOptions

Where is best to document the different default behaviors?

What are the differences?

The default in the prompt method is to show the "never show again" as primary action, while in the notify method the default is secondary (I thought that's what you suggested. Or maybe you meant secondary in both?)

the different behavior is documented separately in each interface.

I think it is OK that the option is implemented differently depending on where it is being used.

bpasero · 2019-06-17T07:52:20Z

src/vs/platform/notification/common/notification.ts

Suggest to just call this id

bpasero · 2019-06-17T07:54:03Z

src/vs/workbench/services/notification/common/notificationService.ts

I prefer to not use underscores unless there is a public get access.

bpasero · 2019-06-17T07:54:30Z

src/vs/workbench/services/notification/common/notificationService.ts

I prefer to inline messages, not have them as variables.

It's for the sake of reuse in "prompt" and "notify".

Yeah don't worry about that, we are good as long as the key used for NLS is the same.

Do you mean to make the key a member or just remove the member variable completely?

No simply repeat the same nls.localize call twice, i prefer to read the english label in code where it is being used.

bpasero · 2019-06-17T07:55:15Z

src/vs/workbench/services/notification/common/notificationService.ts

Remove all these empty lines

YisraelV · 2019-06-18T19:50:35Z

@bpasero thank you very much for the feedback. Please see my last commit.

bpasero

Some more feedback provided.

bpasero · 2019-06-28T05:29:46Z

src/vs/platform/notification/common/notification.ts

Suggested change

neverShowOptions?: INeverShowOptions;

neverShowAgainOptions?: INeverShowAgainOptions;

bpasero · 2019-06-28T05:30:12Z

src/vs/platform/notification/common/notification.ts

Please correct the formatting and be a bit more descriptive

bpasero · 2019-06-28T05:30:24Z

src/vs/platform/notification/common/notification.ts

Suggested change

export interface INeverShowOptions {

export interface INeverShowAgainOptions {

bpasero · 2019-06-28T05:30:43Z

src/vs/platform/notification/common/notification.ts

Suggest to add a comment explaining the purpose.

bpasero · 2019-06-28T05:32:03Z

src/vs/workbench/services/notification/common/notificationService.ts

I suggest to inline this method since it is very simple.

bpasero · 2019-06-28T05:32:08Z

src/vs/workbench/services/notification/common/notificationService.ts

I suggest to inline this method since it is very simple.

bpasero · 2019-06-28T05:34:04Z

src/vs/workbench/services/notification/common/notificationService.ts

The default cannot be secondary if the option is called isSecondary and is optional. Also it would change the way we used to do it for the large fie optimization case.

How about:
appearance: 'primary' | 'secondary';
?

@YisraelV I think the name isSecondary?: boolean; is fine, we use it for IPromptChoice as well but if not provided, it needs to default to being primary.

@bpasero you suggested before the default should be "secondary" in the notify method (here) or did I misunderstand you?

@YisraelV I would make it consistent with the prompt choice, not sure what I thought back then.

bpasero · 2019-06-28T05:35:25Z

src/vs/workbench/services/notification/common/notificationService.ts

I would suggest to add the "Don't show again" as last entry to the secondary actions.

I am sorry I forgot what you said about it in your comment. Will fix.

bpasero · 2019-06-28T05:35:44Z

src/vs/workbench/services/notification/common/notificationService.ts

I would suggest to add the "Don't show again" as first entry to the primary actions so that it appears first and the most important button is to the right.

YisraelV · 2019-07-16T12:25:16Z

@bpasero please see changes. Thank you.

… or primary

bpasero · 2019-08-08T13:47:11Z

@YisraelV thanks, I will go ahead and merge it in with a few cosmetic changes. Since this is only the first half of the desired changes, how about you continue to adopt the new support around the places where today we still manually add a "Don't show again" button?

I am not sure if we persist this choice in global storage scope everywhere, so we may need to beef up the options a little bit to allow for persisting the choice on the workspace scope too.

YisraelV · 2019-08-08T15:56:30Z

@bpasero thanks for your guidance! I'll see if someone wants to take this up and if not I will.

sbatten assigned bpasero May 20, 2019

bpasero added this to the On Deck milestone May 23, 2019

YisraelV force-pushed the master branch from 65e9fd6 to 4383925 Compare June 2, 2019 18:39

YisraelV force-pushed the master branch from c49d330 to 962db66 Compare June 11, 2019 18:46

bpasero requested changes Jun 16, 2019

View reviewed changes

YisraelV force-pushed the master branch from e780b76 to bad4d5e Compare June 16, 2019 19:49

bpasero requested changes Jun 17, 2019

View reviewed changes

bpasero requested changes Jun 28, 2019

View reviewed changes

YisraelV force-pushed the master branch 2 times, most recently from 4f32f7c to 7b8051b Compare July 15, 2019 15:49

YisraelV added 6 commits August 1, 2019 12:29

Support never show again option in notification service

6791b11

in notify put never show again as secondary

da51f2c

Allow customization of whether never show again is shown as secondary…

02cdfa3

… or primary

put never show again inside notification object

6c32a32

feedback

5f020c6

feedback

ba7b6d8

YisraelV force-pushed the master branch from 7b8051b to ba7b6d8 Compare August 1, 2019 09:29

bpasero removed this from the On Deck milestone Aug 5, 2019

bpasero added this to the August 2019 milestone Aug 5, 2019

bpasero added 2 commits August 7, 2019 11:23

Merge branch 'master' into master

7677e19

Merge branch 'master' into master

fb003e7

bpasero merged commit f084f2c into microsoft:master Aug 8, 2019

bpasero mentioned this pull request Aug 8, 2019

Notification service should help with 'Don't show again' #72417

Closed

github-actions bot locked and limited conversation to collaborators Mar 27, 2020

	neverShowOptions?: INeverShowOptions;
	neverShowAgainOptions?: INeverShowAgainOptions;

	export interface INeverShowOptions {
	export interface INeverShowAgainOptions {

Conversation

YisraelV commented May 19, 2019

Uh oh!

YisraelV commented May 28, 2019

Uh oh!

bpasero commented May 28, 2019

Uh oh!

YisraelV commented Jun 3, 2019

Uh oh!

bpasero commented Jun 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

YisraelV commented Jun 5, 2019

Uh oh!

bpasero commented Jun 6, 2019

Uh oh!

YisraelV commented Jun 6, 2019

Uh oh!

bpasero commented Jun 6, 2019

Uh oh!

YisraelV commented Jun 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bpasero left a comment

Choose a reason for hiding this comment

Uh oh!

YisraelV commented Jun 16, 2019

Uh oh!

bpasero left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YisraelV commented Jun 18, 2019

Uh oh!

bpasero left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bpasero commented Jun 4, 2019 •

edited

Loading

YisraelV commented Jun 11, 2019 •

edited

Loading