Feature: Show a desktop notification when extension is toggled with key shortcut

[tuurep]

I prefer to not have any extension icons on my firefox toolbar, so I have no feedback whether I successfully toggled on using the key shortcut (default: Ctrl+Alt+D)

The extension already can show a notification when using the "Need to quickly check something? You get X minutes" thing.

So I tried extending that for toggling on key shortcut in the fork here:

https://github.com/tuurep/NoDistractions-fork/commit/a8e7b8987673eb8f366a8cde95064f7eae748d15

This is how it looks on my setup:

Literally beautiful.

I think it should be only enabled through a setting, because if you have the extension icon on your toolbar, that's already a nice indication of enabled status.

Note:

The way I added the 'correct color' of the NoDistractions icon feels a bit hacky in the code. Although maybe it's not too bad. Thoughts?

Also since I'm adding meaning based on the color of the icon in the notifiation, think about whether the "You get X minutes" -notification's icon color should be green instead of red.

[AsciiJakob]

Will have a look today. We are also gonna be changing the you get x minutes icon to green like you proposed, it makes more sense.

[AsciiJakob]

Oh another thought: Since it's pretty essential for this extension to be accessible in the extension bar (for adding sites, enabling disabling when you don't know the keybind etc), I wonder if there is a way to tell firefox to put it there by default?

Obviously some people wouldn't want this, but considering that this extension is meant to be super simple to use, some more computer-nooby users might find it easier if it is there by default. People can always unpin extensions

[tuurep]

Yeah, gotta look into that. It's been so long since I had extensions on the toolbar I'm not sure where they go by default and things like that :)

Also, I used to have them in the "overflow menu", but then they went into the "unified extension menu" (puzzle piece icon) a while back.

[AsciiJakob]

Yeah, gotta look into that. It's been so long since I had extensions on the toolbar I'm not sure where they go by default and things like that :)

Also, I used to have them in the "overflow menu", but then they went into the "unified extension menu" (puzzle piece icon) a while back.

They seem to go into the puzzle piece icon thing by default and you have to click the cogwheel and then pin extension

[AsciiJakob]

I found a manifest option for it and the action bar is now placed by the navigation bar by default as of 63879df

[AsciiJakob]

Code feedback:

The iconEnabled argument for createNotification should be renamed to iconStatusEnabled to avoid confusion, otherwise it migh seem like it toggles whether an icon should be displayed or not. Maybe iconStatusGreen would be even less confusing.

The way I added the 'correct color' of the NoDistractions icon feels a bit hacky in the code. Although maybe it's not too bad. Thoughts?

I wouldn't necesscary call it bad, it's readable and makes sense. Though a more concise and elegant way of deciding the icon could also be done with a ternary operator:

let icon = iconEnabled ? "/static/assets/icon-low.png" : "/static/assets/icon-enabled-low.png";

Something similar could also be done for the following code:

browser.commands.onCommand.addListener(name => {
    if (name == "toggle-enabled") {
        const newStatus = !enabled.status;
        enabled.setStatus(newStatus);

        browser.storage.local.get("settings").then(res => {
            if(res.settings.notifyOnKeyShortcut) {
                if (newStatus === true) {
                    Utilities.createNotification("toggle-status-notification", "Enabled", true);
                } else {
                    Utilities.createNotification("toggle-status-notification", "Disabled", false);
                }
            }
        });
    }
});

Where it instead becomes:

browser.commands.onCommand.addListener(async name => {
    if (name == "toggle-enabled") {
        const newStatus = !enabled.status;
        enabled.setStatus(newStatus);

        await checkMissingSettings(await getActiveSettings());
        browser.storage.local.get("settings").then(res => {
            if (res.settings.notifyOnKeyShortcut) {
                const msg = newStatus ? "Enabled" : "Disabled";
                Utilities.createNotification("toggle-status-notification", msg, newStatus);
            }
        });
    }
});

I think it is a little cleaner as there is less indentation, but overall your approach was not bad. Note though that I added a check for missing settings. There is always after the extension has been updated, but this particular piece of code may run before that, so it's best that this also runs a check before using any settings, it never hurts. Note also that I had to make the code block async in order to use the await keyword.

This is more of a nitpick, but I think you should move notifyOnKeyShortcut up once in defaultSettings, just so we could have bool and ints seperated a little bit 😄

Overall it looks good though and I can't see any problems. Submit a PR when ready and I'll check again and merge if good 👍