Make setXXXBadge(undefined) set flag instead of clearing

Question

Make setXXXBadge(undefined) set flag instead of clearing

Closed this issue 5 years ago · 12 comments

Maybe this is just reopening #19 but I think it's a slightly different issue.

We noticed it was quite difficult to write a wrapper around the Badge set methods, like this:

function wrapBadgeSet(contents) {
  return navigator.setAppBadge(contents);  // BAD
}

This fails because if contents is omitted in the call to wrapBadgeSet, it will pass undefined to setAppBadge, which has different semantics to passing no argument (specifically: passing no argument will set the badge state to flag, while passing undefined will coerce to 0 which happens to clear the badge).

Instead, to wrap correctly, you must do this:

function wrapBadgeSet(...args) {
  return navigator.setAppBadge(...args);
}

Which is non-obvious and a bit hard to read.

As a general rule, I think passing undefined to an argument should be the same as omitting it. So I propose that we change the IDL to include a "?" which would make both undefined (and null, an unintended but unavoidable side effect) set it to flag, the same as omitting an argument altogether.

Answer 1 · 2019-11-01T02:17:52.000Z

Note: You can also write it like this, which is mildly cleaner:

function wrapBadgeSet(...args) {
    return navigator.setAppBadge(...args);
}

But in general, I agree.

Answer 2 · 2019-11-01T02:34:21.000Z

I think we may be over complicating it. Calling setBadge(requiredArgument) should always set the badge to something (as the name states), and the only way to clear it should be to call clearBadge(). That makes the separation of concerns clear, and it then it becomes an authoring concern to deal with: if the developer wants to treat null, undefined, or any value they choose as "clear the badge", then can just add some logic to do that.

Answer 3 · 2019-11-01T05:50:43.000Z

Updated the bug report with @fallaciousreasoning's simpler method.

Answer 4 · 2019-11-01T06:01:28.000Z

I think we may be over complicating it. Calling setBadge(requiredArgument) should always set the badge to something (as the name states), and the only way to clear it should be to call clearBadge(). That makes the separation of concerns clear, and it then it becomes an authoring concern to deal with: if the developer wants to treat null, undefined, or any value they choose as "clear the badge", then can just add some logic to do that.

You seem to be suggesting a fundamentally different API surface than the one we have specced and implemented.

Currently:

setBadge's argument is optional.
Passing 0, undefined, null or false clears the badge.
Passing nothing sets the badge to "flag".
Passing a positive number sets the badge to that number.

You're suggesting that passing 0 sets it to something else? (Possibly the number value 0 which is currently not allowed?) And that there be a different method to set the badge to "flag"?

Can we not redesign it? (This was litigated in #19 and we came to the soft consensus of the current design.)

What I'm proposing here is a very small change to make undefined and null set the badge to "flag" rather than clearing the badge.

Answer 5 · 2019-11-04T05:36:51.000Z

You're suggesting that passing 0 sets it to something else?

I'm suggesting it throws.

"If type of value is unsigned long long, and value is 0, throw TypeError."

...

And that there be a different method to set the badge to "flag"?

Yes... flag is setBadge() no arg.

Can we not redesign it? (This was litigated in #19 and we came to the soft consensus of the current design.)

What I'm proposing here is a very small change to make undefined and null set the badge to "flag" rather than clearing the badge.

This feels like we are trying to change type coercion in WebIDL. I don't think we should do that. We could support it explicitly for /null/, by making it nullable... but again, i'm worried about fiddling with coercion WebIDL/JS coercion rules.

Answer 6 · 2019-11-18T04:14:35.000Z

Sorry for no reply on here. I'm still not really getting what you're proposing (if anything) that we change, unless you just mean that 0 should throw (which is a separate issue to what undefined should do).

This feels like we are trying to change type coercion in WebIDL. I don't think we should do that. We could support it explicitly for /null/, by making it nullable... but again, i'm worried about fiddling with coercion WebIDL/JS coercion rules.

I understand the concern. We don't want to "go against the grain" of WebIDL / how the web usually works. But we also don't want a "surprising" API, so we have to be careful. Firstly:

§2.5.7.1:

Specification authors are encouraged to treat missing argument and undefined argument the same way in the ECMAScript language binding.

So what I am trying to do here (make undefined equivalent to a missing argument) is the right thing to do, according to this sentence.

Having said that, I'm reading WebIDL now and I think actually this should already be working the way I want (no changes to the spec required). That means we possibly we have an implementation bug in Chrome.

The §3.5 Overload resolution algorithm is complex and hard to digest, but the summary box below says:

When converting an optional argument’s ECMAScript value to its equivalent IDL value, undefined will be converted into the optional argument’s default value, if it has one, or a special value “missing” otherwise.

That suggests that our prose algorithm should see a user-supplied undefined as "missing", not 0, which our algorithm (which calls it "omitted" but I'll change it to "missing" for compat with WebIDL) translates to "flag". On the other hand, a user-supplied null should come through as 0, which our algorithm translates to "nothing". That's exactly what I want (but it isn't what Chrome currently does).

Edit: Wait a minute, was I simply wrong the whole time? Chrome's test case shows that setBadge(undefined) gives "flag". And that corresponds to WebIDL algorithm §3.5, which I've now read through, and says that undefined is indistinguishable from a missing argument. So probably nothing needs to be done.

I've been thinking of adding this table to the spec (regardless of what we settle on) so it's clear at a glance what the various values set the badge to. This is my expectation of what the current behaviour should be (based on the current Badging spec and WebIDL):

API call	Badge value or error
`setBadge()`	flag
`setBadge(0)`	nothing
`setBadge(5)` (pos int)	5
`setBadge(-5)` (neg int)	`TypeError`
`setBadge(undefined)`	flag
`setBadge(null)`	nothing
`clearBadge()`	nothing

The setBadge(undefined) currently produces "nothing" in Chrome. I'll investigate why.

So with reference to the above table, @marcoscaceres what are you proposing we change?

You're suggesting that passing 0 sets it to something else?

I'm suggesting it throws.

So you would just change this one row?:

API call	Badge value or error
`setBadge(0)`	`TypeError`

Why do you want to make this change? The reason we specified it as 0 = "nothing" is so that you can do this:

function pollForMail() {
  // ... Fetch unread mail from server. ...

  // Set the document badge. If getUnreadCount() returns 0, this is
  // equivalent to navigator.clearClientBadge().
  navigator.setClientBadge(getUnreadCount());
}

No need to write an if statement for the special case where you have 0 mail messages. In my view, this is fully general, because the badge number is supposed to represent "number of things you have to deal with". The reason we're disallowing 0 as a value is because if you have 0 things to deal with, then the user shouldn't be bothered at all. Therefore, it seems perfectly logical to allow the client to pass 0 as a value (without throwing), having the same semantics as clearing the badge.

Answer 7 · 2019-11-18T04:25:27.000Z

So with reference to the above table, @marcoscaceres what are you proposing we change?

I guess I don't want two ways of doing the same thing. If set*Badge(0) clear the badge, then clear*Badge() doesn't seem useful.

The reason we're disallowing 0 as a value is because if you have 0 things to deal with, then the user shouldn't be bothered at all. Therefore, it seems perfectly logical to allow the client to pass 0 as a value (without throwing), having the same semantics as clearing the badge.

I agree... so is there any use for clear*Badge() if it's just an alias for set*Badge(0)?

Answer 8 · 2019-11-18T04:53:17.000Z

Note: See my edit above. It turns out that Chrome is actually doing the right thing and this whole issue is already working. Sorry for the noise.

I agree... so is there any use for clear*Badge() if it's just an alias for set*Badge(0)?

Good question! The reason for clearBadge() is that serves as a nice counterpart to setBadge() (no args), for the other use case:

function showPlayerTurn(playerTurnId) {
  if (playerTurnId === localPlayerId)
    navigator.setClientBadge();
  else
    navigator.clearClientBadge();
}

It would feel "silly" if we had to write that as "if (...) { setBadge(); } else { setBadge(0); }", which I would imagine a casual reader unfamiliar with the Badging API would flag as a bug. Whereas the above code is extremely readable for anyone who isn't familiar with the API.

Perhaps even more obnoxious, you might see this:

function showPlayerTurn(playerTurnId) {
  navigator.setClientBadge(playerTurnId === localPlayerId ? undefined : 0);
}

Which I don't want to encourage.

Yes, clearBadge() technically does the same thing as setBadge(0), but in my mind, our developers are in two different "mind sets": integer mode or Boolean mode.

In integer mode, any non-negative integer is valid, with 0 being a special case that clears the badge. (Captured by Examples 1 & 3.)
In Boolean mode, you don't pass arguments. setBadge() sets and clearBadge() clears. (Captured by Examples 2 & 4.)

I believe this is why in #19 there were some proposals to have a separate "set integer" and "set Boolean" method, but those still have the same issue where "set integer 0" and "set Boolean false" produce the same outcome.

So I would prefer to keep clearBadge. Having said that, I think I'd be more inclined to remove clearBadge than setBadge(0), since there's a nice logical reason for setBadge(0) to exist. But my preference is to keep both.

Answer 9 · 2019-11-18T05:04:55.000Z

Since the original issue is resolved (WAI), closing this.

Feel free to continue discussion about clearBadge vs setBadge(0) on here or a new issue.

Answer 10 · 2019-11-18T05:49:09.000Z

Ok, so yes... navigator.setClientBadge(playerTurnId === localPlayerId ? undefined : 0); is ugly. With the non-interger mode, should we ever need it, clear*Badge() makes sense. Thanks for reminding me of that.

Answer 11 · 2019-11-18T22:29:17.000Z

Note: See my edit above. It turns out that Chrome is actually doing the right thing and this whole issue is already working. Sorry for the noise.

Wow, after all my angsting about this it already works 😆

Answer 12 · 2019-12-13T03:25:29.000Z

Note: This still shows up in the spec. We should remove it @mgiuca