Skip to content

edgeclk: Reset graphics before initial clearing of the screen. #3813

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

thyttan
Copy link
Collaborator

@thyttan thyttan commented Apr 20, 2025

Helps in some situations if using fastload utils where the graphics from the previous app would not be cleared.

What triggered the issue for me was:

  1. Use fasload utils
  2. have a message notification pending showing with widmsggrid
  3. exit spotrem by clicking the HW button on the main ui.
  4. edgeclk is displayed but the spotrem graphics are showing behind the clock graphics.

I didn't figure out what exactly caused it, but maybe somehow the theme bg color ended up transparent or not set for the g.clear() call. This change seems to fix it. But don't know if we should be looking to fix something somewhere else.

Helps in some situations if using fastload utils where the graphics from
the previous app would not be cleared.
@bobrippling
Copy link
Collaborator

Yeah very odd, I can't think how the graphics would remain - like you say, I'd expect (at worst) a different BG colour to be used instead. LGTM anyway, happy to merge if you are

@thyttan
Copy link
Collaborator Author

thyttan commented Apr 22, 2025

@gfwilliams I think we probably don't have to look into why g.clear() sometimes fails to clear the the screen withot a g.reset() first? (Especially since this problem mostly shows with fastload utils present)

If so I'll just merge this.

@gfwilliams
Copy link
Member

You can always just do g.clear(1) which has the same effect as g.reset().clear() - it's probably good practice to do reset anyway as the colors/etc could have been changed anyway.

but maybe somehow the theme bg color ended up transparent or not set for the g.clear() call.

I don't believe you can set a transparent color, but about the only thing I can think of is g.setClipRect could have been called. If so it'd be really good to figure out why though as anything that sets clipRect should also reset it.

Maybe try g.setClipRect = () => { throw new Error("Called here"); }; and see if you can figure out what's doing it and not resetting it after?

@thyttan
Copy link
Collaborator Author

thyttan commented Apr 22, 2025

Maybe try g.setClipRect = () => { throw new Error("Called here"); }; and see if you can figure out what's doing it and not resetting it after?

I'll try this 🙏👍

@thyttan
Copy link
Collaborator Author

thyttan commented Apr 23, 2025

I think we found the culprit then at

.setClipRect(w.x, w.y, w.x + w.width - 1, w.y + 24); // guard against oversized icons
As that's the only setClipRect I find between leaving spotrem for edgeclk. So it's not a bug with the firmware which is nice :)

So I can look at adding a g.reset() there somewhere.

But what is the policy really? That:

  1. One should only reset graphics state before setting up new draws?
  2. One should only reset graphics state after done drawing?
  3. One should reset graphics state before setting up new draws, but it's also good to reset after drawing?
  1. Should be best for reliability + performance.
  2. Doesn't make much sense to me.
  3. May be nice so stuff like this issue with edgeclk is less likely to surface. On the other hand it feels a bit like a devil in disguise 😛

So, I prefer to just fix edgeclk to properly reset state before clearing the screen. And not add a g.reset() to widmsggrid (but am open to doing it anyway).

Writing this out it feels a bit bikeshedding-y 😂

@gfwilliams
Copy link
Member

gfwilliams commented Apr 24, 2025

But what is the policy really?

One should only reset graphics state before setting up new draws?

Yes, this one - assume anyone could have been messing with Graphics color/font/etc when your execution starts - and if that calls other functions you don't want to add .reset to all of those.

But an exception really is setClipRect should always be reset to point to the whole screen, because otherwise you end up with really annoying hard to track down bugs (and calling setClipRect should be pretty rare anyway) - so widmsggrid should really be fixed as well. It seems if (w.total > 1) ... g.reset() so I guess this only really happens if you have less than 2 messages showing.

Also, there are cases like g.setColor(..).fillRect(...) where you're overwriting all the state that you're using (hopefully!) so g.reset() doesn't need to be called though. And sometimes (like E.showScroller) we don't want to call reset because the clipRect will have already been set up to the rect that we're supposed to draw and we don't want to get rid of it.

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.

3 participants