added front end support for auto update preference to fix issue #1633

[timmoy]

I added some code to handle issue #1633 to the bramble-ui-bridge.js file. The back end for this is mozilla/brackets#595.

This code just does the front end part, checking on load to see the preference stored for autoUpdate and changing the .refreshwrapper element to reflect that (by removing the checkmark if needed).

Final testing has yet to be done and will be done once the back end has been finalized as well.

[timmoy]

I found what was causing the back end part of the solution to not work, and have pushed a new commit to that pull request.

This one doesn't require any further changes to support the functionality of keeping the same auto update setting when refreshing the browser.

If there are any problems with the code or styling that I might have missed, please let me know and I will fix them as soon as I can.

[flukeout]

Okay, here's what I'm finding

• The editor correctly remembers the setting for the Auto-refresh feature
  • If I turn it off, and reload, it comes back turned off
• However, even though the UI remembers the state, it still auto-refreshes

Check out the GIF below. I have a javascript file that fires an alert every time the page is reloaded. Initially, auto-refresh is disabled, so my changes to the file don't trigger the alert. Then when I reload the browser you can see the 'auto refresh' is turned off - BUT - it's still refreshing the preview when I make changes to the doc.

Let me know if you are finding something similar @timmoy - recommend just linking a JS file that includes an alert("hello!") to your index.html for testing.

[humphd]

@timmoy grab me tomorrow if you want to debug this together.

[timmoy]

Good catch @flukeout, looks like the setting wasn't saved for the editor although it was for the UI.
I managed to reproduce it and added the proper function calls and it works as intended for me now.
Let me know if you still get the bug.

[gideonthomas]

Functionally works well! Nice job! Make the one small change and we can land this.

[gideonthomas]

Let's simplify this logic:

if(!bramble.getAutoUpdate()) {
  ...
}

so that we remove the need for if...else

[flukeout]

Double checking functionality now.

[flukeout]

Hey great work, just confirmed that the setting is remembered and works properly now. Once you make the changes requested by @gideonthomas in this PR and the brackets one we can go ahead and merge!

[timmoy]

The code has been optimized to remove the else branch and not do the comparison.
I've tested it and functionality is working on my machine.

[flukeout]

Running this and the brackets patch together produces this error in the console for me...

...do you see this @timmoy

[timmoy]

@flukeout Indeed I got the same errors when I checked the console. I managed to debug the commandmanager error by taking out this line:
https://github.com/timmoy/brackets/blob/a88f84681b4c708c8a1d60fed990e1024c50b3c7/src/editor/EditorOptionHandlers.js#L107
and functionality still seems intact.

I still get the other error

I suspect there might be other bits of code that aren't needed and with that in mind I will continue to try and debug it.

A side note, do you also get this error?

[humphd]

@timmoy don't remove that line, we need that. The errors are likely due to your dist/bramble.js being out of date. Try running npm run build in Brackets before you run npm start.

The second set of errors you noticed are a known issue related to localization and running Brackets from src/ (not localized) vs. dist/ (localized) and missing a string. You can ignore it.

[gideonthomas]

See mozilla/brackets#595 (comment) for further fixes.

[gideonthomas]

Thanks for the contribution @timmoy!