Skip to content

Add proper window state handling#6

Open
pbaykalov wants to merge 9 commits into
Joolee:mainfrom
pbaykalov:add_proper_window_state
Open

Add proper window state handling#6
pbaykalov wants to merge 9 commits into
Joolee:mainfrom
pbaykalov:add_proper_window_state

Conversation

@pbaykalov

@pbaykalov pbaykalov commented Sep 24, 2022

Copy link
Copy Markdown

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
Copy Markdown
Author

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

@Joolee

Joolee commented Sep 26, 2022

Copy link
Copy Markdown
Owner

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
Copy Markdown
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

Joolee commented Sep 27, 2022

Copy link
Copy Markdown
Owner

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
Copy Markdown
Author

There are multiple problems. 🙃

@pbaykalov

Copy link
Copy Markdown
Author

Seems to be working well now.

@pbaykalov

pbaykalov commented Oct 10, 2022

Copy link
Copy Markdown
Author

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
Copy Markdown
Author

fixed it now

@pbaykalov

pbaykalov commented Oct 10, 2022

Copy link
Copy Markdown
Author

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