add support for backend to fix #1633 #595

timmoy · 2017-02-17T07:06:39Z

This is a work in progress for adobe#1633 for the bramble side of the fix.
The fix will create a preference for the auto update setting so that if a user refreshes their browser, the setting will persist.

timmoy · 2017-02-17T23:35:24Z

I have updated this request with a new commit fixing the typo that prevented the new Preference "autoUpdate" from initializing properly.
The back end functionality of this fix is now working as intended: the auto update preference is saved, changed and read from properly, allowing the editor to remember the last known auto update setting in the event of a browser refresh.

If there are any code or styling changes I have missed, please let me know and I will change them as soon as possible.

gideonthomas

This works well! Add an entry for getAutoUpdate to the README and this should be good to go.

timmoy · 2017-02-22T06:47:57Z

The README has been updated with getAutoUpdate.
I also added autoUpdateChange since I saw there was a section for that too.

humphd · 2017-02-23T17:38:00Z

@gideonthomas is this ready to merge?

gideonthomas · 2017-02-23T19:15:23Z

@timmoy doesn't look like it's remembering the setting when you refresh the page. I'm gonna try to find the cause of that because I don't see an issue in your code.

gideonthomas · 2017-02-23T22:04:47Z

@timmoy, I made a pull request against your branch with the changes that need to be made to make this patch work fully: timmoy#1. Feel free to merge it in or make the changes yourself.

Here's an explanation of the changes:

Although we set the autoUpdate preference based on the start up state passed into Bramble, we don't actually do anything with that preference, i.e. if the startup preference was set to false, we don't actually turn auto update off in response. The change to PostMessageTransport.js makes it actually act on the startup state and turn auto update on/off.
The change to Commands.js will fix the error that was mentioned in the Thimble part of this patch. You basically need to create the command before you register it.

Once you have made the above changes, there are also two other small changes needed: you are missing a closing ` in your README docs and https://github.com/mozilla/brackets/pull/595/files#r102832444. Once you make those changes, I think we're good to merge this in.

timmoy · 2017-02-24T04:10:11Z

Requested changes have been made and errors are no longer occurring for registering the internal command related to auto update.

Note that I kept this line since it is used in the code we want to keep here.

Minor text changes have been made to fix the closing ` in the README, as well as spacing in StateManager for consistency purposes.

Pomax · 2017-02-24T18:56:17Z

src/extensions/default/bramble/lib/PostMessageTransport.js

@@ -105,6 +106,11 @@ define(function (require, exports, module) {
    function start(){
        window.addEventListener("message", _listener);

+        var autoUpdate = BrambleStartupState.ui("autoUpdate");
+        if(typeof autoUpdate === "boolean") {


is there a possibility that this is not a boolean? What are the values that it can take?

literally anything I believe. BrambleStartupState is just a simple map currently that doesn't do any type checking.

Does that mean we should type check, or do we need to coerce with a !!?

Probably just type check? I think for all the other properties, we don't do any coercion so maybe keep it like that for now?

it feels odd to have to type check a value that we supposedly control. Would we at least be able to enforce that whetever it's set it is a boolean? Then we don't have to bother with the check here, and we can let the handler function do whatever it needs to do when it gets "not a bool" passed into it.

We actually don't really control that, it sets it to whatever the hosting app sends us. But, I agree that we should probably enforce type checks as soon as the values are being set. We'll have to add that logic to BrambleStartupState which currently is pretty generic to store any kind of value. Filed #615 to fix that.

gideonthomas · 2017-02-24T19:56:43Z

@timmoy would you be able to rebase your code onto master? I see some extra commits in here.

timmoy · 2017-02-25T05:45:09Z

@gideonthomas So, in my local brackets repo I did:

git checkout master
git pull upstream master
git checkout issue1633
git rebase -i master
//fixed any merge conflicts
git push origin issue1633

This seems to have resulted in adding empty commits with original commit names and made even more extra commits.
Perhaps I incorrectly interpreted what you meant?

Pomax · 2017-02-25T19:49:39Z

locales/zh-CN/editor.properties

@@ -125,7 +125,7 @@ CMD_TOGGLE_QUICK_DOCS=快速文档

 # Drag and Drop

-DND_MAX_FILE_SIZE_EXCEEDED=文件超过支持的最大大小: 3MB
+DND_MAX_FILE_SIZE_EXCEEDED=文件超过支持的最大大小：3MB


it looks like you accidentally changed this file. Can you revert it so it doesn't show up as part of this commit?

NOTE: if you find that there are files that got accidentally changed in your commit, and you need to fix them to be what they were on master:

git checkout master locales/zh-CN/editor.properties git commit --amend --no-edit

Pomax · 2017-02-25T19:55:32Z

@timmoy when you git checkout master, and then you do git log, does it show the top commit as c63d.... ?

If so, when you are on your branch and use git rebase -i master you should be able to remove all the commit entries that are not part of this issue (literally just remove each line from the list of commits). After the rebase, you should then be able to say git log and only see the c36d.... commit with above it only the commits with this issue's changes.

If not, the easiest solution is to bypass git entirely: copy the files you intentionally touched as part of this issue's work to a temp dir, do a git reset --hard while on the master branch, then do a git checkout -B issue1633 which will reset your issue1633 branch to be the same as master (so make sure you copied all the files you touched =), and then you can copy your changed files back in. You now have a branch that consists of the master branch commit and only your updated files, without any commits. Form a commit with git add and git commit, and then push it up with git push origin issue1633 -f

timmoy · 2017-02-26T05:27:30Z

@Pomax for some reason the top commit was showing correctly (c63d...), but when I tried to rebase it, only my commits were shown. Not entirely sure how the other commits got mixed up in here, but might have been when I updated thimble and did a rebase of this branch onto master?

Anyways, I did end up using the second solution and it seems to have worked. The commits are all cleaned up and the fix is still working.
Also, thanks for the guidance of how to use reset --hard properly; I know it is a powerful tool, but it can mess things up if not properly used.

rebased

gideonthomas

Very nice work @timmoy! Thanks for sticking this one out.

gideonthomas · 2017-02-28T04:32:31Z

src/extensions/default/bramble/lib/PostMessageTransport.js

@@ -105,6 +106,11 @@ define(function (require, exports, module) {
    function start(){
        window.addEventListener("message", _listener);

+        var autoUpdate = BrambleStartupState.ui("autoUpdate");
+        if(typeof autoUpdate === "boolean") {


We actually don't really control that, it sets it to whatever the hosting app sends us. But, I agree that we should probably enforce type checks as soon as the values are being set. We'll have to add that logic to BrambleStartupState which currently is pretty generic to store any kind of value. Filed #615 to fix that.

timmoy · 2017-03-01T19:11:25Z

@gideonthomas I'm glad to have participated in resolving this bug, it was a great learning experience of both how the code works between the two repos, expectations and the flow of work in the community! :)

timmoy mentioned this pull request Feb 17, 2017

added front end support for auto update preference to fix issue #1633 mozilla/thimble.mozilla.org#1751

Merged

timmoy changed the title ~~add support for backend to fix #1633 [WIP]~~ add support for backend to fix #1633 Feb 19, 2017

gideonthomas self-requested a review February 21, 2017 17:53

gideonthomas previously requested changes Feb 21, 2017

View reviewed changes

humphd mentioned this pull request Feb 23, 2017

Simplify code paths related to Bramble API methods, events and preferences #605

Open

Pomax reviewed Feb 24, 2017

View reviewed changes

Pomax previously requested changes Feb 25, 2017

View reviewed changes

add support for backend to fix adobe#1633

bca7f6f

timmoy force-pushed the issue1633 branch from f7640e6 to bca7f6f Compare February 26, 2017 05:11

Pomax requested review from humphd, gideonthomas and Pomax February 26, 2017 18:57

gideonthomas approved these changes Feb 28, 2017

View reviewed changes

gideonthomas merged commit 71970bd into mozilla:master Feb 28, 2017

snyk-bot mentioned this pull request Jan 22, 2020

[Snyk] Fix for 1 vulnerabilities mukuljainx/brackets#3

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for backend to fix #1633 #595

add support for backend to fix #1633 #595

timmoy commented Feb 17, 2017

timmoy commented Feb 17, 2017

gideonthomas left a comment

timmoy commented Feb 22, 2017

humphd commented Feb 23, 2017

gideonthomas commented Feb 23, 2017

gideonthomas commented Feb 23, 2017 •

edited

Loading

timmoy commented Feb 24, 2017

Pomax Feb 24, 2017

gideonthomas Feb 24, 2017

Pomax Feb 24, 2017

gideonthomas Feb 24, 2017

Pomax Feb 24, 2017 •

edited

Loading

gideonthomas Feb 28, 2017

gideonthomas commented Feb 24, 2017

timmoy commented Feb 25, 2017

Pomax Feb 25, 2017

humphd Feb 25, 2017

Pomax commented Feb 25, 2017 •

edited

Loading

timmoy commented Feb 26, 2017 •

edited

Loading

gideonthomas left a comment

gideonthomas Feb 28, 2017

timmoy commented Mar 1, 2017

add support for backend to fix #1633 #595

add support for backend to fix #1633 #595

Conversation

timmoy commented Feb 17, 2017

timmoy commented Feb 17, 2017

gideonthomas left a comment

Choose a reason for hiding this comment

timmoy commented Feb 22, 2017

humphd commented Feb 23, 2017

gideonthomas commented Feb 23, 2017

gideonthomas commented Feb 23, 2017 • edited Loading

timmoy commented Feb 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pomax Feb 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gideonthomas commented Feb 24, 2017

timmoy commented Feb 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pomax commented Feb 25, 2017 • edited Loading

timmoy commented Feb 26, 2017 • edited Loading

gideonthomas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timmoy commented Mar 1, 2017

gideonthomas commented Feb 23, 2017 •

edited

Loading

Pomax Feb 24, 2017 •

edited

Loading

Pomax commented Feb 25, 2017 •

edited

Loading

timmoy commented Feb 26, 2017 •

edited

Loading