1.0.1: tabs not carouselling when higher Flip Wait values set about tabcarousel HOT 9 CLOSED

Hi @benjaminoakes

Given the amount of technical knowledge you have (I noticed Jimadine/chromeextension-digitalsignage-autoreloadtabs on your profile 😃), I wonder: would you be interested in attempting a fix on TabCarousel? We would be very grateful for any contributions you could make.

I can certainly give it a go! My Javascript skills are a bit rusty, as I'm no longer doing much web development, but I am willing to try. I'll report back soon 😄

from tabcarousel.

Hi @Jimadine!

Thank you for reporting this. The amount of technical detail you have included is especially helpful!

This does seem related to the recent required changes for Manifest v3, and your intuition about setTimeout makes sense to me.

Given the amount of technical knowledge you have (I noticed Jimadine/chromeextension-digitalsignage-autoreloadtabs on your profile 😃), I wonder: would you be interested in attempting a fix on TabCarousel? We would be very grateful for any contributions you could make.

Other timer APIs would be a good option to consider. We could also consider using setTimeout with a shorter time in combination with manual bookkeeping (e.g., decrement a counter in localStorage). The approach is ultimately the decision of whoever implements the fix. 🙂

Thanks again for this bug report and take care. 👋

from tabcarousel.

I have moved CONTRIBUTING.md to the "Contributing" wiki page to facilitate updates.

from tabcarousel.

I have updated the "Contributing" wiki page to better reflect the state of things in 2024. (This is the first substantial update to "Contributing" since 2011!) Please feel free to update it if you'd like. 😄

Hope this helps!

from tabcarousel.

from tabcarousel.

I've had a look and it's definitely the service worker becoming inactive that's triggering the problem. Googling around, there are various references to the service worker becoming inactive after 30 seconds, which fits with the problem behaviour.

A very simple fix I've tested is to add setInterval(()=>{self.serviceWorker.postMessage('test')},20000) to the top of background.js to persist the service worker. There are other options on that SO page, and using the offscreen API also works, though this requires extra files and the offscreen permission.

I also had a look at the Alarms API and the minimum alarm interval is 30 seconds, so I don't think we can straight replace setTimeout due to this constraint. I guess one could add some logic to use setTimeout for flipWait values <= 30, else chrome.alarms, but things could easily become messy.

As an aside: I followed the instructions in CONTRIBUTING.md but there was a bit more to do before TabCarousel "unpacked" would work without errors. There are several files that contain file path references which are valid for the minified package but not for the repo's src folder. One example: manifest.json - I had to change the service_worker property value from background.min.js to javascripts/background.js. I'm thinking the source code should reference the latter, and changes to file paths should be made as part of the release process? i.e. so one can hit the ground running after cloning the repo.

from tabcarousel.

I've had a look and it's definitely the service worker becoming inactive that's triggering the problem. Googling around, there are various references to the service worker becoming inactive after 30 seconds, which fits with the problem behaviour.

Thanks for taking a look! You definitely had good intuition! 😄

A very simple fix I've tested is to add setInterval(()=>{self.serviceWorker.postMessage('test')},20000) to the top of background.js to persist the service worker. There are other options on that SO page, and using the offscreen API also works, though this requires extra files and the offscreen permission.

It may be best to avoid requesting (and having to justify) more permissions.

I also had a look at the Alarms API and the minimum alarm interval is 30 seconds, so I don't think we can straight replace setTimeout due to this constraint. I guess one could add some logic to use setTimeout for flipWait values <= 30, else chrome.alarms, but things could easily become messy.

That makes sense to me. I defer to your judgment as to what option is best for our use case. Thanks for doing so much research!

As an aside: I followed the instructions in CONTRIBUTING.md but there was a bit more to do before TabCarousel "unpacked" would work without errors. There are several files that contain file path references which are valid for the minified package but not for the repo's src folder. One example: manifest.json - I had to change the service_worker property value from background.min.js to javascripts/background.js. I'm thinking the source code should reference the latter, and changes to file paths should be made as part of the release process? i.e. so one can hit the ground running after cloning the repo.

It seems CONTRIBUTING.md needs some TLC. It may make sense to move it to the wiki.

Recently, I haven't been contributing much code, but I have been releasing this extension since @madhur rewrote it for Manifest v3. I have run into some of the same issues you noted. I'm glad you found my notes on how to release helpful. It may be simpler if there was no build step, but I defer to @madhur as to what's best. (Thank you again for your contributions @madhur!)

Is any of this aside a blocker for fixing the "> 30s" bug? Should we discuss this more here or move it to a new issue?

from tabcarousel.

It may be best to avoid requesting (and having to justify) more permissions.

Yeah, the setInterval(()=>{self.serviceWorker.postMessage('test')},20000) way doesn't need an extra permission, so it avoids having to provide justification. It's probably the most convenient quick fix option at this point.

That makes sense to me. I defer to your judgment as to what option is best for our use case. Thanks for doing so much research!

You're welcome! I've had a look at the code but haven't wrapped my head around a good way to implement a proper, long-term fix. Hopefully I'll have some inspiration soon.

It seems CONTRIBUTING.md needs some TLC. It may make sense to move it to the wiki.

Recently, I haven't been contributing much code, but I have been releasing this extension since @madhur rewrote it for Manifest v3. I have run into some of the same issues you noted. I'm glad you found my notes on how to release helpful. It may be simpler if there was no build step, but I defer to @madhur as to what's best. (Thank you again for your contributions @madhur!)

Is any of this aside a blocker for fixing the "> 30s" bug? Should we discuss this more here or move it to a new issue?

No, none of this is a blocker. A new issue is best, and I'll submit one soon. I'll also submit a PR for the quick fix of the "> 30s" bug.

from tabcarousel.

This has been fixed in the newest release (v1.0.2). Confirmation from other users is welcome. Please comment here if you have an opportunity to test the fix. (Thank you for your help!)

We can reopen this issue if needed, of course. 🙂

from tabcarousel.