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

dtlaunch: remember page between dtlaunch instances #3642

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

thyttan
Copy link
Collaborator

@thyttan thyttan commented Nov 5, 2024

@bobrippling interested what you think about this!

Available on https://thyttan.github.io/BangleApps/?id=dtlaunch

  • test for ram leaks

@thyttan
Copy link
Collaborator Author

thyttan commented Nov 5, 2024

hm, the linter doesn't seem to handle ?? well.

@@ -33,7 +33,8 @@
s.writeJSON("launch.cache.json", launchCache);
}
let apps = launchCache.apps;
for (let i = 0; i < 4; i++) { // Initially only load icons for the current page.
let page = (Bangle.dtHandlePagePersist&&Bangle.dtHandlePagePersist()) ?? (parseInt(s.read("dtlaunch.page")) ?? 0);
Copy link
Collaborator Author

@thyttan thyttan Nov 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bot complains about ?? here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think the linter doesn't know how to handle ??, I'm sure @gfwilliams pointed out why/what changes would need to be made previously but I can't find the comment

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'll be the acorn parse in sanitycheck - it would be a nice one to fix I guess

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, see #3653

@thyttan
Copy link
Collaborator Author

thyttan commented Nov 5, 2024

It feels like this adds just enough milliseconds to be noticable when switching back and forth to the clock. Feels a little less snappy.

Might do some timings to compare.

Copy link
Collaborator

@bobrippling bobrippling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea! Just a few questions about where we store variables

apps/dtlaunch/app-b2.js Show resolved Hide resolved

this.page = page;
};
Bangle.dtHandlePagePersist(page);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly to the this thing above, do we want to chuck this onto Bangle, or perhaps we just have an unrelated global object we use instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe that's better!

Should I do it by e.g. global.dtlaunch = {}; global.dtlaunch.handlePagePersist = ()=>{...}?

Copy link
Collaborator

@bobrippling bobrippling Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, yeah I think that'll work great - and if you make it

global.dtlaunch.handlePagePersist = function() {...}
//                                  ^~~~~~~~

then when you call it as dtlaunch.handlePagePersist(), then you can use this exactly as you intended (as this == dtlaunch)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice - perhaps we want to camelCase the function to keep with the existing pattern? (handlePagePersist instead of HandlePagePersist)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - just noticed that as well 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, yeah I think that'll work great - and if you make it

global.dtlaunch.handlePagePersist = function() {...}
//                                  ^~~~~~~~

then when you call it as dtlaunch.handlePagePersist(), then you can use this exactly as you intended (as this == dtlaunch)

Interesting! So that's a difference between function() {...} and () => {...} ?

Copy link
Collaborator Author

@thyttan thyttan Nov 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... also is the global at the beginning in your suggestion required as well?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes exactly - () => ... won't have its own this, it just uses one from up a level, whereas functions always have their own this. Also no, the global at the start of my suggestion isn't needed, nice spot

What you have now looks great btw!

thyttan added 4 commits November 11, 2024 20:23
The global dtlaunch object contains function and information to restore
the page between instances of dtlaunch.
@thyttan
Copy link
Collaborator Author

thyttan commented Nov 18, 2024

Measuring performance on non-pretokenized code:

Time to first draw: 438 ms -> 441 ms (shouldn't be noticable, I guess I placeboed myself)

Time until the whole file has run: 600 ms -> 615 ms

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