-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[widclkinfo] Ensure we cancel focus when widget_utils.swipeOn() pushes widget bar off screen #3680
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
base: master
Are you sure you want to change the base?
Conversation
Nice find, and I like your fix too.
I like your event approach - one way to avoid having a dependency between the modules is to copy what's done elsewhere and emit (and listen to) the events on -exports.emit("shown");
+Bangle.emit("widgets-shown"); For the issue with "refcounting" blur, were there some problems with this initial approach you had? -clockInfoMenu.force_blur(); // needs to be here so it doesn't stay the focused color
if (clockInfoMenu.focus) {
- //clockInfoMenu.force_blur();
+ clockInfoMenu.blur();
console.log("Forced blur bc hidden");
} |
I was having a problem with the widclkinfo sliding back in still in the redish "focused" color. Actually, in the current state with always calling Not sure if we should make events for the animation steps. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, yes this looks good!
apps/widclkinfo/widget.js
Outdated
clockInfoMenu.force_blur(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we've checked focus
above, I think it's safe to just have the blur
method and avoid the cost of an extra jsvar, wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think clockInfoMenu.blur()
is exposed. I chose the name force_blur
to avoid internal name confusion inside clock_info. Also, clock_info exposes focus
as a boolean but internally uses focus
as a function, so I wasn't sure how to keep names consistent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see your problem - I think that's ok to have the internal focus and the exposed one, we could rename the internal one so we present an API that doesn't give away internal details if you like?
...so an external action like hiding widgets with widclkinfo can unfocus the clock info. This always decrements Bangle.CLKINFO_FOCUS, so in the future we should only call force_blur if we know the Clock Info is focused. We could provide a different function ensure_blur.
This can help widgets, such as widclkinfo, know when they are on screen.
TODO: disable these changes when widget_utils are not being used. Use new (prototype) widget_utils events to blur and set offscreen the clock info when the widgets are hidden. This prevents activating or further interacting with the widclkinfo.
Key change to function: not setting `clockInfoMenu.y = e.y`
I think it's okay to not update the clock_info in this case because they are designed to be on screen.
a673ea4
to
dd5fcd4
Compare
Let me know when you're happy for a re-review :) |
1) Run exports.offset+=dir only in animation steps using the existing stop variable. We check if near animation boundary instead of doing the addition and seeing if we overstepped the animation boundary. The logic table now has absolute values that mean "nearly done with animation". This should mean that, eg, if the user swipes up in the last frame of the show animation, the widgets-start-hide event will still be triggered. 2) Also, standardize event name to widgets-*
@bobrippling I think I'm ready for a look now! Identified and hopefully fixed two pernicious bugs that I thought were due to my new code in widclkinfo, but were actually:
Before I mark as officially "ready for review" I'm:
|
e.x>(options.x+options.w-1) || e.y>(options.y+options.h-1)) { | ||
// touch at y=0 focuses when widclkinfo is off screen | ||
// may have off by one error here | ||
// options.y is -24 | ||
// options.h is 24 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Want to be sure this is consistent with how we define rectangles for widgets and app area etc
var wi = WIDGETS["clkinfo"]; | ||
wi.clockInfoInfo = info; | ||
wi.clockInfoMenu.y = options.y; | ||
if (WIDGETS["clkinfo"] && wi.clockInfoMenu.y > -24) { | ||
WIDGETS["clkinfo"].draw(WIDGETS["clkinfo"]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any way to not refer to the global values here?
// exports.offset < -23 + |dir| > -|dir| otherwise | ||
// dir >0 start showing shown this step showing | ||
// <0 hidden this step start hiding hiding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is probably a more efficient way to do these nested conditions
Great! I'll take an in-depth look soon :) |
Fixes bug with widclkinfo interacting with widget_utils.
Expose a ensure_blur in clock_info that widclkinfo can call. Handles animation with events.
The relevant versions are in my app loader: https://cbkerr.github.io/BangleApps/
I've been testing with the watchface Andark.
Symptoms
Load widget_utils and widclkinfo.
widclkinfo
stays focused after the widgets autohide, so the selected clock info (options.menuA
,options.menuB
) changes when swiping the screen (like when using menus).widclkinfo
is focused, swiping down to show widgets also changes the selected clock info.Minimal reproduction of bug
With useful debug info printed
Expected behavior
When widgets autohide, the Clock Info in
widclkinfo
blurs itself just like tapping outside of its region.Tapping only selects the Clock Info in
widclkinfo
if it is visible. (whenwidget_utils.offset == 0
)To test these fixes
clock_info
Code to upload to RAM from Web IDE that writes custom widget_utils to storage