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

sync changelogger settings across devices #6

Closed
kinghat opened this issue Oct 2, 2020 · 10 comments
Closed

sync changelogger settings across devices #6

kinghat opened this issue Oct 2, 2020 · 10 comments

Comments

@kinghat
Copy link
Contributor

kinghat commented Oct 2, 2020

would like to sync the settings across devices if you would accept of pr when i get around to it?

@Rayquaza01
Copy link
Owner

Sure, I'll accept the PR if you make one

@kinghat
Copy link
Contributor Author

kinghat commented Mar 13, 2021

i forgot to update the readme > options > theme documentation in the PRs

@Rayquaza01
Copy link
Owner

That's fine, I can add that myself. The extension/docs/help.html also needs to be updated.

@kinghat
Copy link
Contributor Author

kinghat commented Mar 13, 2021

The extension/docs/help.html also needs to be updated.

if you remove the theme section from this ill apply the styling/theme media queries to this as well, to bring it all together, later this afternoon or tomorrow.

@Rayquaza01
Copy link
Owner

I ended up removing the help page and moving most of the unique points it had into the README. It mainly said the same things as the README anyway.

I also did a decent bit of work cleaning up the code (adding comments, fixing the race condition bug, updating the AMO API version, converting to TypeScript, etc.) on the feature branch. I think it's pretty much ready to go, but I'd be happy to hear if you would like to make any changes/suggestions before I submit it.

@kinghat
Copy link
Contributor Author

kinghat commented Mar 17, 2021

oh neat, ive been learning the basics of TS recently. i will give it a look over and see what you've done. i appreciate you valuing my opinion on the project 🙏

@kinghat
Copy link
Contributor Author

kinghat commented Mar 21, 2021

looked over the feature branch rewrite, it looks really clean and i learned some things. first time seeing a semaphore. is that the fix for the race condition bug? i tested it a bit and didnt find any issues. theres no easy way to see the sync storage area or test the sync api though 🤷‍♂️ thanks for your work on refactoring the project!

superficially "updated" here could be "shown":

<p>Should the icon badge be updated when an extension is updated?</p>
and the corresponding interface comment:
/** Should the icon badge be updated when an extension is updated? */

@Rayquaza01
Copy link
Owner

Yes, the semaphore is the fix for the race condition bug. Saving the changelog to storage is actually 3 smaller steps, reading the current list, updating the list, and writing the new list. Because the WebExtension storage API is async, if 2+ extensions were installed/updated at basically the same time, the steps might execute out of order, causing some of the changelogs to not actually be saved. The semaphore "locks" storage so that it's guaranteed that only one changelog can be written at once, and that any others trying to access storage will have to wait for the lock to be released.

Good catch on that comment, it makes more sense as "shown".

And thanks again for your help on this version! I really appreciate it!

@Rayquaza01
Copy link
Owner

The new version should be up on AMO now. That was faster than expected! It only took 4 minutes for the submission to be approved.

@kinghat
Copy link
Contributor Author

kinghat commented Mar 22, 2021

very nice. just came through for me and updated nicely. enjoy your week!

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

No branches or pull requests

2 participants