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

feat: automatic re-isolation after n seconds (#315) #429

Merged
merged 5 commits into from Jun 29, 2020

Conversation

rkodey
Copy link
Contributor

@rkodey rkodey commented Jun 21, 2020

Add handler logic in isolation.ts
Add a new function setIsolation, to consolidate some overlapping code
Replace code in commands.ts with setIsolation function
Update addIsolationInactiveBadge in browseraction.ts to accept an optional counter
Closes #315

Add handler logic in isolation.ts
Add a new function setIsolation, to consolidate some overlapping code
Replace code in commands.ts with setIsolation function
Update addIsolationInactiveBadge in browseraction.ts to accept an optional counter
Copy link
Owner

@stoically stoically left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for working on this! Only got minor variable wording nits.

Something that came to mind which might make this slightly more complicated - how should automatically re-enabling isolation behave in case of Firefox shutdown/restart? Like, if I set the delay to 60seconds and just happen to close Firefox while it ticks down, it'll stay disabled after Firefox starts again. Maybe it might make sense to store the re-enable target time in storage (not preferences) and check that while initialization as well?

src/background/preferences.ts Outdated Show resolved Hide resolved
src/background/isolation.ts Outdated Show resolved Hide resolved
Adds isolation helpers setActiveState and handleActiveState to consolidate isolation toggle code
Consolidates pageaction.showOrHide into isolation.handleActiveState
Renames members in isolation.ts with a common root autoEnable*
Adds isolation local storage so the auto re-isolation can resume if Firefox closed
Adds tests for isolation active, auto re-isolation, and the toggle_isolation keyboardShortcut
@rkodey
Copy link
Contributor Author

rkodey commented Jun 28, 2020

Something that came to mind which might make this slightly more complicated - how should automatically re-enabling isolation behave in case of Firefox shutdown/restart? Like, if I set the delay to 60seconds and just happen to close Firefox while it ticks down, it'll stay disabled after Firefox starts again. Maybe it might make sense to store the re-enable target time in storage (not preferences) and check that while initialization as well?

New commit should resolve all conversations above, and addresses the question of an interrupted countdown if the browser is closed. Using the local storage was rather seamless, thanks to your storage class!

I think there's a good argument to move the isolation.active out of preferences. My thought is that it's not really a preference as it cannot be toggled from the Preferences / Options page. It's more of a pseudo-pref that's only in the popup. I think I can cleanly pull it out of prefs and into the new local storage I added to resume the auto-re-isolate. That might also resolve #433.

Copy link
Owner

@stoically stoically left a comment

Choose a reason for hiding this comment

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

Thanks for adding tests and the initialization logic, that's really thorough work!

src/background/isolation.ts Outdated Show resolved Hide resolved
src/background/isolation.ts Outdated Show resolved Hide resolved
src/background/preferences.ts Outdated Show resolved Hide resolved
test/background.isolation.test.ts Outdated Show resolved Hide resolved
test/background.isolation.test.ts Outdated Show resolved Hide resolved
src/background/isolation.ts Outdated Show resolved Hide resolved
src/background/pageaction.ts Outdated Show resolved Hide resolved
Renames autoEnable naming prefix to automaticReactivate to better associate with main "active" var
Removes unneeded comments and debug code
@rkodey
Copy link
Contributor Author

rkodey commented Jun 28, 2020

New commit.
NOTE: Somehow something changed on my setup, and a build error popped up in test/helper.ts. It feels like some library got updated, but even after fully rebuilding my local node_packages, I'm still getting it. I fixed the error to allow my local build to proceed - but I did not commit the change it since I'm not sure where the underlying change came from.

Updates isolation tests to avoid internal method calls
Clarifies background test descriptions
@stoically
Copy link
Owner

I fixed the error to allow my local build to proceed

What kind of error did you encounter?

Copy link
Owner

@stoically stoically left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@stoically stoically merged commit fbef802 into stoically:main Jun 29, 2020
@rkodey
Copy link
Contributor Author

rkodey commented Jun 29, 2020

I fixed the error to allow my local build to proceed

What kind of error did you encounter?

You can disregard. Now that I'm caught up on the main branch, the head is compiling clean.

@rkodey
Copy link
Contributor Author

rkodey commented Jun 29, 2020

Thanks for the merge!

I will work on refactoring the isolation active flag in a new PR related to #433

@rkodey rkodey deleted the rk-re-enable-isolation-315 branch June 29, 2020 22:38
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.

Automatically re-enable Isolation after a delay
2 participants