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

typescript: permit Bangle.setUI({ ... }) with no mode #2389

Closed
wants to merge 1 commit into from

Conversation

bobrippling
Copy link
Contributor

A small typing tweak from espruino/BangleApps#2871

@gfwilliams
Copy link
Member

Do you mean literally just Bangle.setUI({})? I'm not sure we should allow that.

Normally, you should do Bangle.setUI() to completely clear everything out, or Bangle.setUI({mode:"...", ...}) - but I think if you're supplying an object you should supply a mode.

@bobrippling
Copy link
Contributor Author

Ah sorry - I think the title may have confused the situation. What I should've said was that we want to allow mode to be omitted, while other entries can be passed in, like in messagelist's use (I suppose this is effectively a setUI() no-argument call, just that it's in the same codepath as a call that might include mode too).

My intention here is to allow development of wrappers around setUI in typescript, where typescript will guide a developer into handling the case where mode is missing in the argument object. Or even better, if we do manage to get typescript to check the javascript apps, it would help catch bugs like espruino/BangleApps#2869

@bobrippling bobrippling changed the title typescript: permit Bangle.setUI({}) typescript: permit Bangle.setUI({ ... }) with no mode Jul 13, 2023
@gfwilliams
Copy link
Member

So just to be clear, this is meant to allow this:

Bangle.setUI({remove : ... });

If so, I don't believe we should be allowing that. It was never intended behaviour, and isn't documented. I think if you made that call you might rightly assume that the remove function would be called when setUI was next called, but as far as I can see it's not - remove is just ignored:

https://github.com/espruino/Espruino/blob/master/libs/js/banglejs/Bangle_setUI_Q3.js

I think probably the intended use in that case is Bangle.setUI({mode:"custom",remove : ... });

@bobrippling
Copy link
Contributor Author

Yes that makes sense and I agree, an object with no mode doesn't make sense.

My concern was that if someone writes their own setUI, they would assume that if there's an object, it'll have a valid mode, but we saw in espruino/BangleApps#2871 that this isn't necessarily the case - we might get passed an object without mode, and I wanted to guard against this kind of bug, if you see where I'm coming from?

(so less "documented" behaviour and more reflecting what existing apps already do)

@gfwilliams
Copy link
Member

we might get passed an object without mode

Yes, I see what you mean (if you're implementing setUI). However adding an exception would encourage users (of which there are far more) to think omitting mode is ok. I think we should just fix messagelist since I believe it is not just mis-using setUI, but is broken. I did a quick check and as far as I can see messagelist is the only app doing it that way too.

And maybe even make the default .setUI throw an exception to stop people miss-using it.

I've just fixed messagelist so hopefully we can close this now?

@gfwilliams gfwilliams closed this Jul 14, 2023
@bobrippling
Copy link
Contributor Author

Sounds good to me, and yeah that's a fair point about not encouraging that invalid use - thanks again.

(if you like, I'm happy to submit a patch to setUI and have it throw an exception if we get an object without mode)

@bobrippling bobrippling deleted the ts-setui branch July 14, 2023 17:15
@gfwilliams
Copy link
Member

if you like, I'm happy to submit a patch to setUI and have it throw an exception if we get an object without mode

That would be great, thanks!

bobrippling added a commit to bobrippling/Espruino that referenced this pull request Jul 17, 2023
Following on from espruino#2389, this makes it clear that while `setUI()`
can be used to reset the UI, `setUI({})` cannot, and must always
specify `mode` (amongst other options).
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

Successfully merging this pull request may close these issues.

2 participants