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

Don't show again notification management #114105

Closed
wants to merge 4 commits into from
Closed

Conversation

meganrogge
Copy link
Contributor

@meganrogge meganrogge commented Jan 10, 2021

Click the notification icon (bell) and then the settings gear to see the template for the notification management.

@meganrogge meganrogge added this to the January 2021 milestone Jan 10, 2021
@meganrogge meganrogge requested a review from bpasero January 10, 2021 17:55
@@ -363,6 +370,8 @@ export interface INotificationService {
* @param filter the filter to use
*/
setFilter(filter: NotificationsFilter): void;

getNotifications(): INotificationItem[];
Copy link
Contributor Author

@meganrogge meganrogge Jan 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I plan to remove this from the notificationService and will instead use the registry we talked about to get the storage keys

@@ -120,6 +131,25 @@ class KeybindingsEditorInputFactory implements IEditorInputFactory {
}
}

class NotificationsEditorInputFactory implements IEditorInputFactory {

canSerialize(editorInput: EditorInput): boolean {
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 don't think I need this yet

@@ -569,19 +569,17 @@ export class TerminalInstance extends Disposable implements ITerminalInstance {
if (this._storageService.getBoolean(SHOW_TERMINAL_CONFIG_PROMPT, StorageScope.GLOBAL, true) &&
this.hasHadInput &&
!TERMINAL_CREATION_COMMANDS.includes(resolveResult.commandId)) {
this._notificationService.prompt(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adopting neverShowAgain for my terminal notification

@@ -24,6 +25,10 @@ export class NotificationService extends Disposable implements INotificationServ
super();
}

getNotifications(): INotificationItem[] {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

planning to remove this and will use registry to directly get the keys instead from storage

run: () => {
this.storageService.store(id, true, scope, StorageTarget.USER);

this.getNotifications().push(<INotificationItem>{ neverShowAgain: true, id: id, label: 'label', when: 'whenever' });
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as we discussed, this won't work because we can't get all the info we need from the notification here. will have to register it with an id/key, label, and when description of when the notification gets fired


async openNotifications(): Promise<void> {
const options = { pinned: true, revealIfOpened: true };
Copy link
Contributor Author

@meganrogge meganrogge Jan 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here if I try to instantiate the editor directly (rather than do that via the input), it doesn't work

@meganrogge meganrogge removed the request for review from bpasero January 10, 2021 20:12
@meganrogge meganrogge closed this Jan 10, 2021
@meganrogge meganrogge removed this from the January 2021 milestone Jan 10, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Feb 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant