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

Add proper window state handling #6

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

pbaykalov
Copy link

@pbaykalov pbaykalov commented Sep 24, 2022

Right now window handling is incomplete: the state of toggles is global but the shortcuts and toolbar acts on the window state and does not change other windows. This manifests itself in the bug: if you try to toggle a switch in two windows, it will take 2 button presses to activate the style in second window.

I do not know about others but it seems to me that per-window handling is desired way of operation. Therefore this pull request makes the behaviour consistent with my idea.

  1. Toggles are now explicitly stored with respect to window IDs.
  2. Toggle states are reset to "off" upon browser start
  3. Toggle states for the new windows are inherited from the last window active.

Please test it yourself, I did not (I have no desire to setup anything to debug it).

@pbaykalov
Copy link
Author

In what order are onFocusChanged and onCreated fired when new window is created?

@Joolee
Copy link
Owner

Joolee commented Sep 26, 2022

I don't know about your last question but that shouldn't be too hard to find out. Is it important for the pull-request, e.g. is it compete in the current state?

I'm not using multiple windows myself so I haven't actually thought about this functionality to be honest. I'm willing to merge and publish your code if it works because it looks fine to me (could use some more spaces though 😉) but I don't know when I'll get a chance to test it myself.

@pbaykalov
Copy link
Author

Okay I will then try packing it and testing myself.

is it compete in the current state?

If I use getLastFocused() then it won't be a preoblem anymore.

@Joolee
Copy link
Owner

Joolee commented Sep 27, 2022

Packaging is quite easy. Compress the whole thing into a zipfile: https://extensionworkshop.com/documentation/publish/package-your-extension/
Then load it in about:debugging#/runtime/this-firefox

@pbaykalov
Copy link
Author

There are multiple problems. 🙃

@pbaykalov
Copy link
Author

Seems to be working well now.

@pbaykalov
Copy link
Author

pbaykalov commented Oct 10, 2022

The only important thing left is that there is no sense in storing runtime state in local storage, I should just use global variable for that.
There is no reason to store runtime state in local storage, right?

@pbaykalov
Copy link
Author

fixed it now

@pbaykalov
Copy link
Author

pbaykalov commented Oct 10, 2022

Is setting storage is supposed to be serializing the objects?

изображение

So two windows were getting entangled somehow until I used structuredClone.

I tried reproducing this using different structures but this did not happen. So, settings storage is preserving relations in some cases but not others.

изображение

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.

2 participants