Fix #5062: Update notification is no longer modal #5085
Conversation
ghost
assigned 
|
|
||
| AppInit.appReady(function () { | ||
| $dlg.find("button").focus(); | ||
| }); |
There was a problem hiding this comment.
This seems to differ from the button-focusing logic in the "shown" handler. Should be hoisted out into a shared helper function?
There was a problem hiding this comment.
We could, but this is only required when the dialog is opened before the app is ready, which will happen in very few places. Maybe we could just move the update notification logic to the app ready?
There was a problem hiding this comment.
I think we should move the update notification logic to the app ready.
|
Done initial review. Everything looks good except the button-focusing logic in App ready. |
|
@RaymondLim Done. Moved the first check for updates to be done on App ready. |
|
Removing @RaymondLim since he's out for a few days and I think we should try to merge this soon. Will reassign in tomorrow's standup. |
|
Assign back to Raymond as he will be back on 9/17 from his PTO. |
| @@ -330,6 +331,11 @@ define(function (require, exports, module) { | |||
| // Append locale to version info URL | |||
| _versionInfoURL = brackets.config.update_info_url + brackets.getLocale() + ".json"; | |||
|
|
|||
| // Check for updates on App Ready | |||
| AppInit.appReady(function () { | |||
| checkForUpdate(); | |||
There was a problem hiding this comment.
Now we're checking for update regardless of whether "skipUpdateCheck" is specified in the UrlParams or not. When this call is in the old location, it won't be invoked if "skipUpdateCheck" is specified. Not sure how to test with UrlParams. @gruehle, we need your opinion on this.
There was a problem hiding this comment.
The "skipUpdateCheck" is required for test windows to work correctly. Without that test, every test window would check for updated.
The call in the old location also checks "inBrowser", since we don't want to do update notification checks while running in browser.
@TomMalbran - is there a reason why this check was moved here? If not, it would be best to keep it in brackets.js.
There was a problem hiding this comment.
Thanks @gruehle. Tom moves it here because the old location is too early to show the update dialog and consequently the dialog is losing focus to the main editor causing issue #5602.
@TomMalbran I think we can keep this call back in brackets.js, but in a different location where we're done with asynchronous loading of the editor. That is, moves the entire if block that contains this call to the line above PerfUtils.addMeasurement("Application Startup");.
There was a problem hiding this comment.
@RaymondLim I was thinking of just moving this block to the old location. I will test if that works, but it might make sense to do it in that way. If it doesn't work i'll test that.
Note: It seems to work fine.
|
Looks good. Merging. |
Fix #5062: Update notification is no longer modal
Fix for issue #5062.