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

ClockFace: set Bangle.CLOCK=1 before loading widgets #1864

Merged
merged 1 commit into from May 23, 2022

Conversation

rigrig
Copy link
Contributor

@rigrig rigrig commented May 22, 2022

No description provided.

@alessandrococco
Copy link
Contributor

alessandrococco commented May 22, 2022

😄 I just opened a similar PR (#1867), didn't notice this.

I thought CLOCK = 1 was set in the setUI method - Am I doing it wrong?

edit: Found this discussion in the forum.

[about setUI] For clocks, Bangle.CLOCK gets defined, so you can do stuff only when in a clock app (eg the clock widget now only appears if you're not on a clock app)

@rigrig
Copy link
Contributor Author

rigrig commented May 22, 2022

Yes, CLOCK = 1 is set by setUI, but there's a bit of a chicken and egg problem:

  • Bangle.appRect checks if WIDGETS is loaded, so we want to call loadWidgets before init, in case the clock uses the Layout library. (or does other clever things with the widgets)
  • We need to call setUI("clock") after init, because if the clock does use the Layout library, that calls Bangle.setUI(), clearing the clock event handlers.

@gfwilliams
Copy link
Member

Ok, thanks. This definitely doesn't feel ideal and I'd prefer folks just used setUI of possible, but I guess this is in a library so it's not too bad. I'll add a comment to the lib explaining this though...

@rigrig
Copy link
Contributor Author

rigrig commented May 23, 2022

I'd prefer folks just used setUI of possible

Well, maybe we could get rid of this line (Bangle.setUI(); // remove all existing input handlers) from the Layout library? So it only calls setUI if it needs to for Bangle.js 1 buttons or a back option.
Then we could just move setUI to the top in ClockFace.

But that would mean hunting down all code that now relies on Layout calling setUI :-(

@alessandrococco
Copy link
Contributor

alessandrococco commented May 23, 2022

Out of curiosity (I'm still learning the Layout module) : why the module needs to remove the handlers?

But that would mean hunting down all code that now relies on Layout calling setUI :-(

Instead of removing it we can add a new param (with a "as is now" default):

if (resetHandlers == undefined || !!resetHandlers) {
  Bangle.setUI();
}

@rigrig
Copy link
Contributor Author

rigrig commented May 23, 2022

why the module needs to remove the handlers?

If you define btn elements, the Layout library sets up event listeners to make them just work. The Bangle.js 1 doesn't have a full touch screen, so it uses setUI({mode:"updown"... to scroll through them with the physical buttons.
And for users of the library it's easier to just remember "Layout calls setUI", than "Layout might sometimes call setUI". Especially if it works differently between the Bangle.js 1 and 2.

So I guess simply removing that line won't really do, maybe make it conditional on there being any btn elements?

@gfwilliams
Copy link
Member

I've just had to refactor Layout a bit to fix the run app, so we could have an options.noSetUI quite easily. It feels like the one CLOCK=1 hack in a library is better than the few tiny hacks we'd have to implement to do this nicely though ;)

@rigrig rigrig deleted the clockface-clock=1 branch July 29, 2022 18:53
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.

None yet

3 participants