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

Explore removing clear() and possibly change set method? #19

Closed
markcellus opened this issue Dec 6, 2018 · 6 comments
Closed

Explore removing clear() and possibly change set method? #19

markcellus opened this issue Dec 6, 2018 · 6 comments

Comments

@markcellus
Copy link

markcellus commented Dec 6, 2018

The proposal states:

The Badge interface [...] contains two methods:

  • void set(optional USVString or long): Sets the associated app's badge to the given data, or just "flag" if the argument is not given.
  • void clear(): Sets the associated app's badge to nothing.

The proposal around the clear() method seems a bit unclear here 😄 . Is there any particular reason to have a clear() method? Wouldn't set(undefined), set(null), or set() all clear the badge count and hide it? Or is clear() meant to serve some other purpose? Maybe clear the count, but still show the badge (without a number)?

@mgiuca
Copy link
Collaborator

mgiuca commented Dec 6, 2018

I typed up a doc yesterday here. (This doc is not meant to circumvent the issue tracker discussions, but summarize them — thanks for filing a separate issue.)

To paste what I said there: as I see it there are a number of options:

  • set(number) to set a number (0 clears), set() to set a flag. clear() clears. (incumbent)
  • set(number) to set a number (0 clears), set(bool) to set a flag (false clears). No "clear" method.
  • setNumber(number) to set a number (0 clears), setFlag(bool) to set a flag (false clears). No "clear" method.
  • setNumber(number) to set a number (0 clears), show() to set a flag, hide() clears.

This isn't just about removing clear(), it also suggests redesigning the way set works.

@markcellus
Copy link
Author

Thanks. I vote for the second one! Based on your latest change in #20. It would be great if we could just do:

function showPlayerTurn(playerTurnId) {
      Badge.set(playerTurnId === localPlayerId);
}

instead of

function showPlayerTurn(playerTurnId) {
    if (playerTurnId === localPlayerId) {
        Badge.set();
    } else {
        Badge.clear();
}

@domenic
Copy link

domenic commented Dec 9, 2018

I'd prefer avoiding overloads; in an untyped language they hurt more than they help. For example, with an integer-boolean overload, you could have Badge.set(foo) and Badge.set(!!foo) causing different behaviors. A more realistic example would be Badge.set(isPlaying || playerHealth), where the intent is to flip the flag or not, but because of how the || operator works it will either clear the badge (if isPlaying is false) or set the badge number to the amount of health the player has (if isPlaying is true). Or do something else entirely, if isPlaying contains a truthy value that is not true.

I'd propose something like:

  • Badge.toggle(boolean): sets a flag/clears a flag. You could maybe allow omitting the argument to toggle it from its previous state, like DOMTokenList's toggle(). Or, just name it set() or setFlag() instead.
  • Badge.clear(): equivalent to Badge.toggle(false), but a bit more say-what-I-mean. Optional.
  • Badge.setNumber([EnforceRange] unsigned long long): sets a number. Probably 0 should clear, although at that point we're kind of getting in to UI territory (i.e., does the UA/OS have a notion of "zero items" in the badge, or does that always clear).

@markcellus
Copy link
Author

markcellus commented Dec 9, 2018

Yeah I agree with your sentiments @domenic about overloading the set with a boolean, but it seemed that others favored simple overloads. I do like having toggle() as the flag setter. I would think it would be pretty rare that a consumer would need both boolean setter and integer setter in the same logic.

Your !!foo example is interesting and that could get confusing, but I'd argue that such a coercion has always been a confusing thing in JS and tends to cause unexpected behaviors. We dont need this API example to illustrate that. 😛

@markcellus markcellus changed the title Explore removing clear()? Explore removing clear() and possibly change set method? Dec 9, 2018
@fallaciousreasoning
Copy link
Collaborator

I'm kind of partial to option one, where we have a set(number) to set a number and set() to set a flag.

It avoids the overloading concern @domenic raised, and the result of:

Badge.set(7); // 7
Badge.clear(); // Clear
Badge.set(): // A bit weird, but Flag

is more obvious to me than

Badge.setNumber(7); // 7
Badge.toggle(); // Clear? Flag?
Badge.toggle(); // 7? Flag? Clear?

// Explicitly setting flag is better
Badge.setNumber(7); // 7
Badge.toggle(false); // Clear
Badge.toggle(true); // Not sure if toggling restores the last value? 7? Flag?

I also think that using set() for both flags and numerical values makes it more obvious that this is an either or case (you can't have a flag and a numerical badge at the same time).

That said, I don't feel that strongly about this. I'm sympathetic to @mkay581's argument for overloading set, and agree that coercion has always been a confusing part of JS, so I could be convinced for option 1), 2) or 3) (not 4)).

In the interests of deciding something, I'm thinking we pick 1), unless anyone has any significant objections?

@domenic
Copy link

domenic commented Jun 19, 2019

The only issue with that approach is that it leads to code such as

Badge[hasMessages ? "set" : "clear"]();

instead of

Badge.toggle(hasMessages);

But I can understand

I also think that using set() for both flags and numerical values makes it more obvious that this is an either or case (you can't have a flag and a numerical badge at the same time).

and, if we want to avoid overloading, I think this is probably the better path. So I am OK with 1).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants