Max-Github / FireFoxInvertColors

A simple addon that will invert a web page colors.
GNU General Public License v3.0
56 stars 15 forks source link

Synchronize changes between options and toolbar #8

Closed kidhanis closed 7 years ago

kidhanis commented 7 years ago

Pressing the options button now triggers an update to the toolbar icon. Furthermore, a change of state triggered by the toolbar button or the context menu item also updates the state the options button.

Max-Github commented 7 years ago

Apologies for not talking a look at these earlier.

One thing that I did notice was that sometimes the storage was not updating immediately or at all (very rare), and this was the reason that I was applying the CSS in parallel with saving the state.

kidhanis commented 7 years ago

These changes should allow the CSS to update only after the storage variable is updated. At least if storage does not update, then the CSS stays the same and doesn't cause a discrepancy between the visible state and the stored state.

Do you recall if there were some specific actions that failed to update the storage?

Max-Github commented 7 years ago

I was running tests using "web-ext run", and often I would see that the storage would not update immediately (Button is clicked, or short-cut triggered, and nothing was happening). And later the storage would update, at time it would not.

This was the reason I changed the logic to apply the CSS regardless if the storage was set.

I prefer for now to keep it this way, as having the CSS apply on request is much more important than updating the state in storage.

kidhanis commented 7 years ago

I haven't had problems with storage so far; nevertheless, I'm fine with closing this pull request if you believe that these changes might cause some problems to come back.

storage would not update immediately (Button is clicked, or short-cut triggered, and nothing was happening)

When you say that nothing was happening, is it that not even the colors inverted? If so, did you notice if the page you were on had finished loading? browser.tabs.insertCSS() defaults to injecting the CSS after the page has finished loading all its resources, so in this scenario it might take a while before colors get inverted.

Max-Github commented 7 years ago

Initially, the CSS was applied or removed based on the state of the storage. And pressing the button would just flip the storage.

When testing, I was noticing that sometimes the storage would either not update or update after a delay.

I did make sure that the page was fully loaded. This was also noticed with a simple local html file.

After that I changed the logic, due to the reasons explained earlier.