Added logic to control autocomplete #678

hkirat · 2017-03-31T12:27:25Z

In reference to #1920.
The P.R. adds logic to handle toggling of auto complete

…ocorrect

Pomax · 2017-04-03T19:56:35Z

@hkirat your comment points to an issue in the Adobe/brackets repository, did you mean to point to a Thimble repository? If so, note that if you want to point to an issue in a different repo, you can't use the #... formatting, you need to use the full URL for that issue

hkirat · 2017-04-03T20:00:46Z

Thanks for pointing that out @Pomax. It points to an issue in Thimble. Updated the same.

flukeout · 2017-04-05T18:31:48Z

@hkirat thanks for the update testing now.

humphd · 2017-04-05T20:28:48Z

src/bramble/client/main.js

@@ -909,6 +920,14 @@ define([
        this._executeRemoteCommand({commandCategory: "bramble", command: "BRAMBLE_DISABLE_SCRIPTS"}, callback);
    };

+    BrambleProxy.prototype.startAutoComplete = function(callback) {


Please change this to enable vs. start

humphd · 2017-04-05T20:29:00Z

src/bramble/client/main.js

+        this._executeRemoteCommand({commandCategory: "bramble", command: "START_AUTO_COMPLETE"}, callback);
+    };
+
+    BrambleProxy.prototype.stopAutoComplete = function(callback) {


Please change this to disable vs. start

humphd · 2017-04-05T20:29:49Z

src/bramble/client/main.js

@@ -204,6 +204,7 @@ define([
        self.getSidebarVisible = function() { return _state.sidebarVisible; };
        self.getRootDir = function() { return _root; };
        self.getWordWrap = function() { return _state.wordWrap; };
+        self.getAutoComplete = function() { return _state.allowAutoComplete; };


Because you're changing the public API, please update the README as well, see https://github.com/mozilla/brackets#bramble-instance-getters

humphd · 2017-04-05T20:33:20Z

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

@@ -157,6 +157,40 @@ define(function (require, exports, module) {
            });
        });

+        // Listen for changes to TagHints
+        PreferencesManager.on("change", "codehint.TagHints", function () {


I don't like the idea of sending 4 separate events for state changes on a variable that we're tracking in the StateManager as a single boolean. Can you harmonize these so they all send the same type: "bramble:autocompleteChange" with a single boolean? It will mean we fire this change event 4 times, but that shouldn't break anything, since Thimble will just set it true or false in all cases.

humphd · 2017-04-05T20:37:04Z

src/bramble/client/StateManager.js

@@ -139,6 +139,10 @@ define(function() {
                get: function()  { return getBool(storage, "allowJavaScript"); },
                set: function(v) { storage.setItem(prefix("allowJavaScript"), v); }
            },
+            allowAutoComplete: {


I have a general concern. In some places you use Autocomplete and in others AutoComplete. Once upon a time I implemented a bunch of code in Firefox related to the FullScreen API which later became the Fullscreen API, and it left me very nervous about the bugs that can creep in with such inconsistencies. When I Google this, most people seem to use "Autocomplete." Should we do that everywhere? I actually don't care as long as we're consistent in all places, here and in Thimble.

humphd · 2017-04-05T20:37:55Z

src/bramble/client/main.js

@@ -309,6 +311,14 @@ define([
                        _state.allowJavaScript = data.allowJavaScript;
                    } else if (eventName === "tutorialVisibilityChange") {
                        _tutorialVisible = data.visible;
+                    } else if (eventName === "TagHintsChange") {


This is a leaky abstraction, and I'd like to harmonize these event types into a single autoCompleteChange event. See my comment below.

hkirat · 2017-04-06T09:38:17Z

Thanks for the review @humphd
I really should've kept track of the variable names. Apologies for that.
I've shifted to a common convention of autocomplete now.

humphd

Two small wording changes, and this is ready to go! Great work.

humphd · 2017-04-06T12:34:59Z

src/bramble/client/main.js

@@ -909,6 +914,14 @@ define([
        this._executeRemoteCommand({commandCategory: "bramble", command: "BRAMBLE_DISABLE_SCRIPTS"}, callback);
    };

+    BrambleProxy.prototype.enableAutocomplete = function(callback) {
+        this._executeRemoteCommand({commandCategory: "bramble", command: "START_AUTO_COMPLETE"}, callback);


change the command name to BRAMBLE_ENABLE_AUTOCOMPLETE

humphd · 2017-04-06T12:35:10Z

src/bramble/client/main.js

+    };
+
+    BrambleProxy.prototype.disableAutocomplete = function(callback) {
+        this._executeRemoteCommand({commandCategory: "bramble", command: "STOP_AUTO_COMPLETE"}, callback);


change the command name to BRAMBLE_DISABLE_AUTOCOMPLETE

hkirat · 2017-04-06T13:03:44Z

Sorry I missed that.
Fixed now

humphd · 2017-04-06T13:17:48Z

Excellent work, @hkirat, thank you!

A note for Thimble devs about this change. This PR, along with similar ones in process right now, makes changes to the code used to create dist/bramble.js. Thimble uses this file in order to communicate with Brackets, and unless you manually rebuild it, it won't have the new code. A common error is bramble.someMethod() is not a function. It means you need to update Brackets. Here is the recommended way to do it:

git checkout master
git pull upstream master
git submodule update --init
npm install
npm run build

Updating your submodules and npm modules is not strictly necessary in this case, but it also won't hurt (i.e., it's always safe to do these two things). The final line npm run build will recreate dist/bramble.js with the new code, and will allow Thimble to find it.

If you do all this and Thimble still can find the function in needs on the bramble object, you probably need to clear your cache in Thimble so it forces it to reload dist/bramble.js.

* Included new state in StateManager.js * Added events to implement AutoComplete toggle * Added start and stop functions in bramble * Added allowAutoCorrect to main.js * Fixes typos * Added toggle functionality to JS and CSS hints * Changed all variable names to Autocomplete * Emitted a single event(autocompleteChange) when Hints Change * Added note in README related to new getAutocomplete function * Modified Remote Command Strings

hkirat added 5 commits March 31, 2017 02:35

Included new state in StateManager.js

69ac49e

Added events to implement AutoComplete toggle

85e3a53

Added start and stop functions in bramble

fb208bc

Merge branch 'master' of https://github.com/mozilla/brackets into aut…

96af8d3

…ocorrect

Added allowAutoCorrect to main.js

7bf5372

hkirat mentioned this pull request Mar 31, 2017

Added buttons to toggle the value of auto-complete mozilla/thimble.mozilla.org#1929

Merged

Fixes typos

7e423d1

hkirat force-pushed the autocorrect branch from ef546cf to 7e423d1 Compare March 31, 2017 12:34

hkirat changed the title ~~Added logic to allow toggling of auto-complete~~ Added logic to control auto-complete Apr 2, 2017

Added toggle functionality to JS and CSS hints

c82a850

humphd suggested changes Apr 5, 2017

View reviewed changes

humphd mentioned this pull request Apr 5, 2017

Add Bramble API for toggling whitspace view, part of mozilla/thimble.mozilla.org#1763" #664

Closed

hkirat added 2 commits April 6, 2017 14:50

Changed all variable names to Autocomplete

c50d758

Emitted a single event(autocompleteChange) when Hints Change

2b23253

hkirat changed the title ~~Added logic to control auto-complete~~ Added logic to control autocomplete Apr 6, 2017

Added note in README related to new getAutocomplete function

b1dc373

humphd suggested changes Apr 6, 2017

View reviewed changes

Modified Remote Command Strings

7099880

humphd merged commit 2d99ca7 into mozilla:master Apr 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added logic to control autocomplete #678

Added logic to control autocomplete #678

hkirat commented Mar 31, 2017 •

edited

Loading

Pomax commented Apr 3, 2017

hkirat commented Apr 3, 2017

flukeout commented Apr 5, 2017

humphd Apr 5, 2017

humphd Apr 5, 2017

humphd Apr 5, 2017

humphd Apr 5, 2017

humphd Apr 5, 2017

humphd Apr 5, 2017

hkirat commented Apr 6, 2017

humphd left a comment

humphd Apr 6, 2017

humphd Apr 6, 2017

hkirat commented Apr 6, 2017

humphd commented Apr 6, 2017

Added logic to control autocomplete #678

Added logic to control autocomplete #678

Conversation

hkirat commented Mar 31, 2017 • edited Loading

Pomax commented Apr 3, 2017

hkirat commented Apr 3, 2017

flukeout commented Apr 5, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hkirat commented Apr 6, 2017

humphd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hkirat commented Apr 6, 2017

humphd commented Apr 6, 2017

hkirat commented Mar 31, 2017 •

edited

Loading