AutoUpdate Framework #14177

mbhavi · 2018-03-27T16:01:10Z

This PR includes changes for Auto Update of Brackets.
The changes are with respect to:

Windows implementation
Mac implementation
A new UI

ping @swmitra @nethip for review

The Auto Update workflow along with the screenshots is described below :

Notification of Update : through current UpdateNotification dialog, which is triggered either from Help->CheckForUpdates or from the side bar button notification

Download of Update : Once user clicks on GetItNow button, a download of the latest installer is triggered in the background, with a progress bar shown on the status bar.

Download retrial in case of failure : If the download fails, it is retried for a maximum of 5 attempts, and the same is reflected on the status bar UI.

Validation of the downloaded update : Once download is successful, the SHA256 checksum is validated, and the info is displayed on status bar.

Download Failure : In case download fails, error is shown to the user towards bottom of the editor.

Download and Validation success : User is prompted with the update bar, with buttons for "Restart" and "Later" options. Clicking Restart button would start the update procedure by quitting the app, and launching the installer. Clicking on Later button allows user to continue with the normal flow of Brackets.

Validation Failure : If the validation of the downloaded update fails, a validation error is displayed to the user in update bar.

Update in Progress : Once the user clicks Restart button, the app is quit and installer gets launched.

Update Success : If update is successful, new version of Brackets is launched automatically, with the success message on the update bar.

Update Failure : If the update fails, the older version of Brackets is relaunched automatically, and the error message is displayed to the user in update bar, with a link to the brackets website for a manual update operation.

Any warnings or errors encountered during the worflow are reflected in the update bar.
Screenshots for one of the warnings is attached here. It corresponds to the scenario where user clicked on Restart button, but had dirty files to which he/she chose Cancel in File Save dialog. In this case, the update will happen on the next quit of Brackets, and the same is displayed to the user as a warning on update bar.

On MAC, instead of the installer, the update procedure is carried out by an update shell script. Rest everything is same for MAC, as above.

abose · 2018-03-31T05:49:32Z

Awesome! @mbhavi Can you add some screenshots of the new flow?

abose · 2018-04-02T10:55:06Z

Is there a preference to disable auto updates?

mbhavi · 2018-04-03T02:23:57Z

@abose , there isn't a preference to disable auto updates.
Going forward, auto update will be the default update mechanism in Brackets.
The feature is implemented as an extension AutoUpdate, which if not present or not loaded successfully, the update mechanism will fallback to the current manual update wherein if the user clicks on GetItNow button in UpdateNotification dialog, he/she is redirected to brackets website for a manual update.

abose · 2018-04-03T03:41:47Z

Sometimes, users disable auto-updates to prevent breaking changes to their setup[extension compatibilities Et.Al.]. It would be good if it is configurable by a preference without having to remove the extension.
Also is there an opt-in at first launch? or is the update process kicked off without confirmation from the user?
@petetnt @swmitra

mbhavi · 2018-04-03T06:56:56Z

@abose, I have attached the screenshots and the step-by-step workflow description.
The update is kicked off only after the confirmation from the user as an input of GetItNow button click in UpdateNotification dialog.

…ni/autoUpdate

nethip · 2018-04-05T07:53:59Z

src/document/DocumentCommandHandlers.js

+    var DIRTY_FILESAVE_CANCELLED = "dirtyFileSaveCancelled";
+
+    /**
+    Event triggered when error is returned when tried to check if Auto Update is progress


nit: trying instead of tried.

Addressed this in 537430b

nethip · 2018-04-05T07:55:27Z

src/document/DocumentCommandHandlers.js

+     * Checks and handles if auto update is currently in progress
+     */
+    function checkIfAutoUpdateInProgress() {
+        brackets.app.isAutoUpdateInProgress(function (err, isAutoUpdateInProgress) {


Add if (brackets.app.isAutoUpdateInProgress) { to check if this function is registered or not.

isAutoUpdateInProgress() function has been added to the core. It is not a registered function. Hence, we need not have this check.

@mbhavi It is a registered in appshell_extensions.js. Consider this scenario where a user who is running the older shell but with newer brackets code, will never find this function, so it is a good idea to bailout. Please add a check here.

I strongly believe suppressing the error would turn out to be harmful in this case, not showing the error to the user would be misleading to him/her believing that the feature can work with just the brackets changes.
Moreover, I don't see that with any of the other functions' invocations. I believe doing that will be unnecessary bloating of the code, as this is most likely to happen only in the dev machines, which I think should be okay.

nethip · 2018-04-05T08:01:51Z

src/document/DocumentCommandHandlers.js

+     */
+    function checkIfAutoUpdateInProgress() {
+        brackets.app.isAutoUpdateInProgress(function (err, isAutoUpdateInProgress) {
+            if (err === 0 && isAutoUpdateInProgress) {


No need for an explicit check, general rule we follow in Brackets is to check for error condition first. So your statements should be

if (err) { } else { }

Addressed this in 537430b

nethip · 2018-04-05T08:03:31Z

src/document/DocumentCommandHandlers.js

+    function checkIfAutoUpdateInProgress() {
+        brackets.app.isAutoUpdateInProgress(function (err, isAutoUpdateInProgress) {
+            if (err === 0 && isAutoUpdateInProgress) {
+                CommandManager.trigger(exports.DIRTY_FILESAVE_CANCELLED);


Use execute over trigger.

Help me understand. The name of the method is checkIfAutoUpdateInProgress but you are executing exports.DIRTY_FILESAVE_CANCELLED ? Shouldn't this be something like whoever is calling this method, should be returned a promise and the client can then act upon the promise state.

I have now placed all the events related to Auto Update, needed in core, in UpdateNotification in order to ensure consistency.
Also, I now chose a better name for checkIfAutoUpdateProgress(). It is actually a handler function. It iinternally calls a function isAutoUpdateInProgress(), which returns a bool, and the wrapper function handles this bool. So, I have renamed the wrapper function to handleIfAutoUpdateInProgress().
Please refer to 537430b

nethip · 2018-04-05T08:20:10Z

src/document/DocumentCommandHandlers.js

@@ -996,6 +1020,7 @@ define(function (require, exports, module) {
                    if (selectedPath) {
                        _doSaveAfterSaveDialog(selectedPath);
                    } else {
+                        checkIfAutoUpdateInProgress();


Could you explain (workflow) why this call is required here?

This is needed for the workflow where user clicked Restart button for the update to happen, but had some dirty files, because of which the app could not be quit, and instead, he got the Unsaved Files dialog, This dialog has 3 buttons : Save/Don't Save/Cancel. If Don't Save is pressed, we quit the app and run the update. If Save is pressed and user saves the file successfully, then also we quit the app and update. But when the user chooses to either press Cancel button in the above dialog, or presses Save followed by Cancel in the Save As dialog, we show him a message saying that the update will now be reflected on relaunch, which inherently means that the update will now be carried out the next time the app quits.
So, for this, we need to know whether or not the save file was cancelled by either of the two scenarios mentioned here, so that we can prompt him with the required message.

Thanks for explanation @mbhavi

nethip · 2018-04-05T09:24:05Z

src/document/DocumentCommandHandlers.js

@@ -1217,6 +1242,7 @@ define(function (require, exports, module) {
            )
                .done(function (id) {
                    if (id === Dialogs.DIALOG_BTN_CANCEL) {
+                        checkIfAutoUpdateInProgress();


Looks like most of the auto update code is written as an extension. In that case, you should only send notifications about the relevant events here and let the extension react to those notifications.

Eg: You should send a notification which says save cancelled/file save failed e.t.c.

I am indeed sending the notification of a Cancel click on dirty file save operation. Please refer to my comments above.

nethip · 2018-04-05T09:27:05Z

src/extensions/default/AutoUpdate/StateHandler.js

+            _file = FileSystem.getFileForPath(this.path);
+
+        _file.exists(function (err, exists) {
+            if (exists) {


Check for err.

Addressed in 537430b

nethip · 2018-04-05T09:39:21Z

src/extensions/default/AutoUpdate/StateHandler.js

+     */
+    function _write(filePath, json) {
+        var result = $.Deferred(),
+            _file = FileSystem.getFileForPath(filePath);


check for validity of _file

_write is an internal function, which I have wrapped inside the function writePromise(), which in turn, is wrapped inside the write() function, which checks for the validity of the file. Hence, checking here is redundant.

I think there is not harm in checking for validity of _file given the fact that you are going to stringify the JSON irrespective of whether _file is valid or not.

@nethip , the requirement here is to do a force write. If the file exists, the content is overwritten. If it doesn't exist, the file is created and written. I am passing the boolean to be "true" in writeText, which ensures this.
Hence, the validity is not needed here.

@mbhavi Let us not have discussion of having a null check or not. It is a simple null check. If you still feel that is not required, post merging this PR, one of us will add it.

@nethip , addressed in 4fb6582

nethip · 2018-04-05T09:44:18Z

src/extensions/default/AutoUpdate/UpdateInfoBar.js

+            });
+
+            $restart.keyup(function (e) {
+                if (e.which === 32) { //32 is the keycode for space key


use a named variable for better readability.

Addressed in 537430b

nethip · 2018-04-05T10:09:06Z

src/extensions/default/AutoUpdate/main.js

+        updateDir = appSupportDirectory + '/updateTemp',
+        updateJsonPath = updateDir + '/' + 'updateHelper.json';
+
+    var updateJsonHandler = new StateHandler.StateHandler(updateJsonPath);


StateHandler.StateHandler?

Addressed in 537430b

nethip · 2018-04-05T10:18:01Z

src/extensions/default/AutoUpdate/main.js

+
+    /**
+     * Checks if it is the first iteration of download
+     * @returns {boolean} - true if first iteration, false if is a retrial of download


nit: if it is a

Addressed in 537430b

nethip · 2018-04-05T10:19:56Z

src/extensions/default/AutoUpdate/main.js

+     *                            to be called in case of
+     *                            successful setting of update state
+     */
+    function setUpdateStateInJSON(key, value, callback) {


Use a promise instead of a callback.

This is indeed a promise based function. The same promise handling code snippet was getting used at multiple places, and so, in order to avoid the repetition of code, I have templatized the code snippet.

nethip · 2018-04-05T10:21:22Z

src/extensions/default/AutoUpdate/main.js

+     */
+    function handleSafeToDownload() {
+
+        var downloadCB = function() {


Use promises rather than callbacks.

Please refer to my comment above.

nethip · 2018-04-05T10:30:07Z

src/extensions/default/AutoUpdate/main.js

+    functionMap["brackets.registerBracketsFunctions"] = registerBracketsFunctions;
+
+    UpdateNotification.on(UpdateNotification.GET_AUTOUPDATE_INSTALLER, handleGetNow);
+    CommandManager.on(DocumentCommandHandlers.DIRTY_FILESAVE_CANCELLED, dirtyFileSaveCancelled);


Refer to the comment that I put above in DocumentCommandHandler about generic command handlers.

Please refer to my comments in DocumentCommandHandlers and UpdateNotification, where I have explained the use cases and requirements.

nethip · 2018-04-05T10:32:11Z

src/extensions/default/AutoUpdate/main.js

+    });
+    updateDomain.on('data', receiveMessageFromNode);
+
+    AppInit.appReady(function () {


Check for platform and only then initialize AppInit.appReady as auto update is targeted for only Win and Mac.

Addressed in 537430b

nethip · 2018-04-05T10:38:47Z

src/extensions/default/AutoUpdate/main.js

+        UpdateInfoBar           = require("UpdateInfoBar");
+
+
+    var _modulePath = FileUtils.getNativeModuleDirectoryPath(module),


Execute the steps only for Win and Mac

Addressed this in 537430b

nethip · 2018-04-05T10:45:03Z

src/extensions/default/AutoUpdate/node/AutoUpdateDomain.js

+            []
+        );
+
+        domainManager.registerEvent(


Empty function?

This is actually an event registration, for which no naming is required.

Got it! Thanks

mbhavi · 2018-04-09T05:28:46Z

@nethip , I have addressed and committed all the required changes. Also, I have replied to all the comments.
The PR is ready for review again.

nethip · 2018-04-09T10:00:09Z

@mbhavi I think the preference to disable is still pending.

nethip · 2018-04-09T10:27:22Z

@mbhavi I don't think the changes which I had proposed with DocumentCommandHandler.js are incorporated.

There should be no reference of AutoUpdate anywhere inside DocumentCommandHandler.js. You could still borrow the mechanism but no reference to auto update. I still see methods like handleIfAutoUpdateInProgress or in the PR. I am not sure if there is any any strong reason for hard wiring auto update into DocumentCommandHandler.js, as you could create new events and emit those events from here. The AutoUpdate extension can then register for those events and react accordingly..

nethip · 2018-04-09T10:34:11Z

src/extensions/default/AutoUpdate/main.js

+    UpdateNotification.on(UpdateNotification.DIRTY_FILESAVE_CANCELLED, dirtyFileSaveCancelled);
+    UpdateNotification.on(UpdateNotification.AUTOUPDATE_ERROR, handleAutoUpdateError);
+
+    if(brackets.platform !== "linux") {


How about checking for the shell functions existence like setUpdateParamsAndQuit, and if not present, just bail out?

nethip · 2018-04-09T10:36:22Z

src/utils/UpdateNotification.js

@@ -333,6 +447,12 @@ define(function (require, exports, module) {
        var result = new $.Deferred();
        var versionInfoUrl;

+        //AUTOUPDATE_UNITTESTING
+        var updateJSONLocalhostURL = PreferencesManager.get("updateJSONLocalhostURL");


This needs to be removed.

nethip · 2018-04-09T10:52:38Z

src/utils/UpdateNotification.js

+
+                        //If no checksum field is present then we're setting it to 0, just as a safety check,
+                        // although ideally this situation should never occur in releases post its introduction.
+                        var platformInfo = getPlatformInfo(),


If you want to retain this , you might want to move all of this to notifyGetUpdateInstaller

…ni/autoUpdate

…date, making AutoUpdate no-op for Linux, Replacing icons

mbhavi · 2018-04-10T09:35:30Z

@nethip , the review comments have been addressed now. I have added the preference to enable/disable auto update, made AutoUpdate no-op on Linux, replaced the icons, and moved most of the core code onto extension side. Please find the changes in b50f8de
The changes are ready to be reviewed again.

nethip · 2018-04-10T09:48:15Z

src/utils/UpdateNotification.js

@@ -61,6 +71,10 @@ define(function (require, exports, module) {
    // Data about available updates in the registry
    PreferencesManager.stateManager.definePreference("extensionUpdateInfo", "Array", []);

+    //Preference for hosting latest installer on a local server
+    //AUTOUPDATE_UNITTESTING
+    PreferencesManager.stateManager.definePreference("autoUpdate.testUrl", "string", "");


Do you want to still retain this preference?

@nethip , Yes we need to retain this preference, as this will be very much needed for unit testing purposes.

Please have this preference in a separate branch. We can't be shipping with test URLs preference.

@nethip, I have removed the preference from this branch. I will probably include the changes in a separate branch then, for unit testing purposes.

nethip · 2018-04-10T09:58:13Z

src/extensions/default/AutoUpdate/main.js

+    AppInit.appReady(function () {
+
+        // Auto Update is supported on Win and Mac, as of now
+        if (brackets.platform !== "linux") {


I think one of the review comments got missed here. You need to check if brackets.app.setUpdateParams is registered or not. Only then run through the flow.

nethip · 2018-04-10T09:59:50Z

Thanks for addressing the review comments @mbhavi . I have added more comments. Please address those comments and I think we are good to merge from the dev perspective.

mbhavi · 2018-04-10T11:27:14Z

@nethip, I have addressed the new comments in 4fb6582. Please review.

nethip · 2018-04-11T05:23:33Z

LGTM.

@mbhavi Thanks for this hugely requested PR 😄 ! Great job 💯

@ayushrathi15 We are waiting for the test results from your sanity testing. Please update this thread once you are done testing the private builds, so that we can go ahead with the merge.

nethip · 2018-04-13T04:57:24Z

@mbhavi One last check. What if the node domain fails? Can we figure that out and bail out in your extension?

mbhavi · 2018-04-17T09:47:14Z

@nethip , I have addressed the failure scenario where the node domain fails in b272289. Please have a quick review for the same.

ayushrathi15 · 2018-04-17T10:30:06Z

LGTM.

@nethip @mbhavi I am done with the private build testing. We are good to go ahead with the merge.

nethip · 2018-04-17T12:23:25Z

src/extensions/default/AutoUpdate/main.js

+                 }
+             })
+             .fail(function() {
+                 return;


A console message message here would be good.

@nethip, addressed this in 9b89697.

…sole log message on node domain failure

AutoUpdate Framework : Initial Changes

409d4ed

mbhavi changed the title ~~[WIP] AutoUpdate Framework : Initial Changes~~ [WIP] AutoUpdate Framework Mar 27, 2018

vickramdhawal requested a review from nethip March 28, 2018 11:26

mbhavi added 5 commits April 4, 2018 10:42

Addressing unit testing issues, error handling, code cleanup

ac37cde

Merge branch 'master' into miglani/autoUpdate

446e389

Merge branch 'master' of https://github.com/adobe/brackets into migla…

ca0000e

…ni/autoUpdate

Clean up of code and spaces

0ff81d1

Error handling and code cleanup changes

ada6b28

nethip reviewed Apr 5, 2018

View reviewed changes

nethip reviewed Apr 9, 2018

View reviewed changes

mbhavi added 2 commits April 10, 2018 10:41

Merge branch 'master' of https://github.com/adobe/brackets into migla…

4efe928

…ni/autoUpdate

Merge branch 'master' of https://github.com/adobe/brackets into migla…

bcb2a3e

…ni/autoUpdate

mbhavi force-pushed the miglani/autoUpdate branch from 801a51e to bcb2a3e Compare April 10, 2018 09:03

Addressing review comments, adding feature-flag/preference for AutoUp…

b50f8de

…date, making AutoUpdate no-op for Linux, Replacing icons

nethip reviewed Apr 10, 2018

View reviewed changes

Addressing review comments, minor code cleanup

4fb6582

nethip approved these changes Apr 11, 2018

View reviewed changes

abose mentioned this pull request Apr 17, 2018

Auto update data logging #14220

Closed

Handling the node domain failure scenarios

b272289

Adding update_download_url to config.json and brackets.config.dist.json

9ae2fc5

mbhavi changed the title ~~[WIP] AutoUpdate Framework~~ AutoUpdate Framework Apr 17, 2018

nethip reviewed Apr 17, 2018

View reviewed changes

Fixing eslint and jslint errors, cleaning up package.json, adding con…

9b89697

…sole log message on node domain failure

nethip merged commit 92b872e into adobe:master Apr 18, 2018

This was referenced Aug 30, 2021

[CLOSED] AutoUpdate Framework brackets-archive/bracketsIssues#11762

Open

[CLOSED] Auto update data logging brackets-archive/bracketsIssues#11789

Open

		UpdateInfoBar = require("UpdateInfoBar");


		var _modulePath = FileUtils.getNativeModuleDirectoryPath(module),

AutoUpdate Framework #14177

AutoUpdate Framework #14177

Conversation

mbhavi commented Mar 27, 2018 • edited Loading

abose commented Mar 31, 2018

abose commented Apr 2, 2018

mbhavi commented Apr 3, 2018

abose commented Apr 3, 2018

mbhavi commented Apr 3, 2018

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

nethip Apr 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nethip Apr 5, 2018 • edited Loading

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

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

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

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

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

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

Choose a reason for hiding this comment

mbhavi commented Apr 9, 2018

nethip commented Apr 9, 2018

nethip commented Apr 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbhavi commented Apr 10, 2018

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

nethip commented Apr 10, 2018

mbhavi commented Apr 10, 2018

nethip commented Apr 11, 2018

nethip commented Apr 13, 2018

mbhavi commented Apr 17, 2018

ayushrathi15 commented Apr 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbhavi commented Mar 27, 2018 •

edited

Loading

nethip Apr 9, 2018 •

edited

Loading

nethip Apr 5, 2018 •

edited

Loading