-
Notifications
You must be signed in to change notification settings - Fork 1.2k
launch: fix fullscreen and showClocks setting #3788
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
Conversation
...by now clearing cache when changing those settings.
Thanks! This looks great! For the flicker, I guess there are some options:
|
apps/launch/app.js
Outdated
@@ -81,8 +84,14 @@ | |||
// cleanup the timeout to not leave anything behind after being removed from ram | |||
if (lockTimeout) clearTimeout(lockTimeout); | |||
Bangle.removeListener("lock", lockHandler); | |||
// Restore widgets if they were hidden by fullscreen setting | |||
if (global.WIDGETS) require("widget_utils").show(); |
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.
Do we also need:
if (global.WIDGETS) require("widget_utils").show(); | |
if (global.WIDGETS || settings.fullscreen) require("widget_utils").show(); |
or perhaps:
if (global.WIDGETS) require("widget_utils").show(); | |
if (hiddenWidgets) require("widget_utils").show(); |
(with that bool set when we call widget utils' hide()
)
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 think we might be ok: https://github.com/espruino/BangleApps/blob/master/modules/widget_utils.js#L21
But it feels like a good idea to check anyway as otherwise it may force a redraw? Did you mean if (global.WIDGETS && settings.fullscreen)
instead?
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 no - we're looking at it from different sides. Your side is whether it's safe to call (so I can see why you're thinking &&
and the widget._draw
check), mine is if we need to undo our changes, which are either if we've done a .hide()
from having widgets:
Lines 27 to 28 in 5dd73bb
} else if (global.WIDGETS) { | |
require("widget_utils").hide(); |
or if we've had the fullscreen setting:
Lines 91 to 92 in 5dd73bb
if (settings.fullscreen) { | |
require("widget_utils").hide(); |
If either of those are true, we need to .show()
, hence global.WIDGETS || settings.fullscreen
, right?
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.
But if there are no widgets, there's no need to update their properties with the .show()
call - right? There's nothing for widget-utils
to act on.
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.
Yes, but then the for loop wouldn't be entered in that case so we're ok, still. And we're balancing our above call to .hide()
, which I think is as far as this code needs to go, and it can then trust that widget_utils
will do the right thing
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.
Ahh, right - I see what you mean - but actually we only call require("widget_utils").hide();
when we're supposed to be in fullscreen mode (line 27 is actually if (!fullscreen) {..} else if (...WIDGETS) ...
.
If we're not in fullscreen mode we almost certainly do have widgets but they're not hidden.
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 hadn't spotted the fullscreen check, yeah I'm with you
apps/launch/app.js
Outdated
} | ||
}); | ||
if (settings.fullscreen) { | ||
require("widget_utils").hide(); |
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.
@bobrippling this makes me think you may be right regarding L88
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.
But we also have this consideration about maybe changing the back-function behaviour: #3788 (comment)
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.
That's true, it is a bit of a special case I suppose. Perhaps we try out a few different approaches?
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.
Ok - so in this case the widgets would have been hidden before (if there were any), but because we're using back
a widget is being added anyway and so we're having to hide them again?
I feel we should probably just fix the back
issue rather than having these lines in there?
@thyttan @bobrippling I think there's a question about whether So right now WIDGETS is defined if you call So are you ok with me fixing that? Supplying |
Without thinking too much about it - yes I like that solution! |
I'm debating in my head if the touch in top left corner should still work though even when the widget is not displayed. Could be neat. But maybe not? |
Yeah, although I wonder whether someone might tap in the launcher and expect to launch the app? I was considering making setUI use an overlay to display the back button in that case, but it just feels like overkill! |
It's such a small area that it shouldn't happen very often. But I'm not sure which way is best.
Agreed. It could be cool - but probably not worth the overhead? I'd like it in the context of my messagegui changes - but not if it impacts scrolling drawing performance/input latency. |
(For messagegui it's E.showScroller and not Bangle.setUI though) |
I suppose one question would be, for apps without widgets do users regularly use the back button anyway? Or are they more likely to soft-reset to escape? Or are we talking menus? |
Ahh - yes, I looked at that. It seems showScroller calls setUI under the hood :) Having the overlay definitely would hurt scroll performance, so maybe we forget that for now.
Yes, I think they'd almost certainly use the button. I reckon if the whole screen is being used there's a high likelihood that UI elements might end up being over that top left area anyway - imagine if you have a menu with no title - the top menu item would then be forever half obscured by the back button. I'll go fix setUI then... |
Huh, well I did that - and I guess as expected it wasn't that easy! Because when widgets are there but hidden WIDGETS is still defined - so I had to check if the widget bar was there. Fixed now though 453def81645c08e46195ad0627c1b77c8a59ee2a I've updated the launcher code - does this look ok now? Obv it'll need new firmware but I should probably do a release soon anyway |
Nice! I believe so - didn't test now though |
Cool, yeah LGTM too |
Just FYI I added another change which allows the item height to be adjusted independently of the font. |
0.24: Fix fullscreen when fastloading the launcher. (TODO:fix back btn flicker)
Fix respecting the showClocks setting.
Try it via my app loader: https://thyttan.github.io/BangleApps/?id=launch
closes #3786 and #3785
@gfwilliams because it is a core app.