Check for extensions updates #7330
Conversation
|
Thanks @zaggino! I played with it, trying to make the number smaller so it won't cover the extension icon but made the number hard to read. I think we discussed that's it's okay to not include the number. Thoughts? |
|
Yes, we can remove the number completely. I think @lkcampbell mentioned just making the icon green and that's fine with me, but I didn't really know how to do it fast and didn't want to spent half a day with SVG thingy now :) |
|
We can just use span like what @peterflynn suggested originally like this (ignore font-size: 0, we should just remove the text): |
|
It'll look like this: |
|
@larz0 done ;-) |
|
I have now realized that update check is done every time someone closes and opens the Brackets... Not only my new update check for extension but also the old check for new Sprint version... this is probably spamming the server? Ping @TomMalbran to clarify. |
|
I accidentally hit close! Hopefully this can be merged pretty quickly. |
|
Probably not so fast, I see these lines in master https://github.com/adobe/brackets/blob/master/src/brackets.js#L295-L305 as problematic. I don't think it's a good idea to do requests everytime Brackets are started - I do that quite a lot. |
|
Also Travis says |
|
Hmm I think I saw an email about that from @redmunds. |
|
@zaggino According to issue #6723 the idea was to call it every 24 hours, so it probably shouldn't call it on every launch. So you save the time you get an update and on you just start the timeout on every launch using that time. It should also get updated every time you open the registry and when you update extensions. In this case we should reset the timer. And finally, it might be a good idea to get the updates on the first launch after a Brackets update. The Brackets updates and the Registry live on different servers and while the Brackets update request might be short, the registry might be a lot longer. So it should use a lot less bandwidth to call the brackets update than the registry. So it might make sense that one resets on every launch but the other one doesn't. |
|
It might also be nice to add a new API to the registry to retrieve the data for a list of extensions, so that we don't need to get every single extension when only have a few installed. But that might be for another PR. |
|
Ah, it actually does check for the 24 hours, but much more deeper in the code, I'll try to rewrite it a bit. |
|
This works better now - check at most once a day + if extension is uninstalled or updated, do not show notification for it. |
There's a bug in our CLA check script where all checks are failing. We're working on it. |
|
Just my two cents on this, I always liked the design where the block turns completely green. This is a screenshot from issue #6723 as a comparison: |
|
Only changing the color is easy to miss for people who are partially colorblind (like me), so I like the idea of also adding a "+" to the icon. |
|
@TomMalbran can you take a look? |
| } | ||
|
|
||
| function launchAutomaticUpdate() { | ||
| var INTERVAL = 86520000; // repeat once a day, plus 2 minutes |
There was a problem hiding this comment.
This in my opinion could be a lot shorter - like an hour or two. The real interval is limited to 24 hours for both branches of the code.
There was a problem hiding this comment.
The update seems ok to be done every day or on every startup, we don't update all the time anyway and at most you get the update some hours later which isn't bad.
Notice that the registry is not in the same server as brackets.io and is not even owned by Adobe, is hosted by jrowny company. So we should ckeck wicth him about the bandwidth about checking a lot more often would be. Also, this might change with another API to request information about just the extensions required.
There was a problem hiding this comment.
Changing this interval even to 5 milliseconds doesn't change that the requests are made just once every 24-hours. Currently it's like this:
Monday - update at 9:00 (the time gets saved in user preferences)
Tuesday - turn on PC at 8:55 (it's not 24 hours yet, so no update check, schedule update check 24 hours + 2 minutes from 8:55 to Wednesday 8:57 - no update check for almost 24 hours)
This is scenario in a case Brackets are not restarted entire day.
There was a problem hiding this comment.
I think that it might be simpler if we use an interval for Brackets updates which works like this and a timeout for Extensions updates, where the time is savedTime + oneDay - currentTime and is recreated after the updates are checked.
|
I agree with @lkcampbell that green icon and plus notification sign together would be better than just the latter one. |
|
Can we just use what we discussed at #6723? There's a lot of good points there. |
|
@larz0 I wanted to make the green icon but I'm a fail graphics person. Is there a good reasonable free editor for Windows to work with SVG files? |
|
@zaggino Inkscape or Adobe Illustrator. Here's the SVG that you can use: http://cl.ly/1O2B2g150q35 |
|
Thanks 👍 |
|
Looks great now :) |
|
|
|
Finally found your problem - it wasn't introduced by my changes. If you had installed version for Brackets 35 and latest version was for Brackets 99, update was always disabled before, even though version for Brackets 37 existed in the registry. I fixed it so after the fix you can update to version for Brackets 37 even though it's not the latest ;-) I'd push here the fix and the tests for new methods but GitHub is down... [just for my reference] |
|
Finally pushed - ready for another look @TomMalbran |
|
No, that isn't the issue I was referring too. This change will allow you o install extensions not supported by Brackets. So you should undo that change, it is working fine. The issue is with the updates counter not with the actual updates. I'll explain it better: Lets say that I am running Brackets 37 (not master, but the actual released code). Now lets say that I have an extension fully updated that supports version 37. Lets say that Brackets 38 will release a new awesome API which is already available in master. The extension author loves this API and decides to use it and releases a new version of his extension but that supports Brackets 38. Now the user gets a update notification that his extension has an update, but when he wants to update it he can't since it requires Brackets 38 and he has version 37. He can't upgrade Brackets and doesn't want to use the code from master, so he will not update it until the next Brackets release. The issue now is that since there is an update the extension icon turned green, and it will stay like that until the next release. So now the user won't be notified of any new updates for other extensions that might support Brackets 37 and that he would like to upgrade. This can happen a lot, specially when upgrading an extension to use new APIs, and have it ready for the next release. So my point is, that we shouldn't count updates for extensions that can't be updated because your Brackets version doesn't supported. Is ok if you can't actually install it and that we show a disabled upgrade button in the manager. But both the counter in the manager and the icon in the sidebar shouldn't consider this extensions as updates. Notice that the counter in the dialog already counts them, but once we merge this, the problem can be a lot worse. About the tests, I am not sure if it is a great idea to just implement them in already created tests. Would be better to have them in a new describe. Inside the same file. Have you considered moving the extensions update functions to one of the extensibility files? |
|
I really can't find the stuff you're talking about - please fix me what I'm doing wrong:
|
|
|
Oh, I see. I had version 0.1.0. So I could update it to version 0.2.0 and I can't get the next update since is not supported and it is not showed in count. Awesome, I thought it might have been another error. Sorry about that. |
|
Ok. Tests have been moved into the new describe. I've looked at the code and I don't think there's anything I'd move but if you want to move some functions around just tell me which and to where :) |
|
Rebased and ready for review @TomMalbran |
|
Maybe it is ok there since you are only testing the new ExtensionManager functions. Unless we need more tests, the location is fine for now. The code looks good to me, but I am not an expert in the Extensions Manager code, and this touches it a lot, so I am not sure if it might break something else. Maybe @njx could give it a fast look? Also I don't know how we are on the next release, so I am not sure if we should wait for sprint 39 (we would have more time to test it) or if everything is fine to merge it for sprint 38. |
|
We missed the 38 (c3f2b50) so this could be merged early into 39 to catch potentional problems, even though I doubt there should be any. |
|
Yes, that was the plan. I forgot to post it here. The day I talked about it with Peter was going to probably be branch day, which at the end it wasnt, so there waasn't much time to test. This might seems like an easy feature but I think is a bit more complex and it touches some bits in extension manager that should be better tested. I'll give it a final look and merge it soon, since we are already in release 39. |
|
@zaggino and @TomMalbran, I am really glad this feature is going in and I am also really glad that @zaggino did it instead of me ;). Seriously though, thanks for all of your work on this. |
| @@ -506,39 +523,92 @@ define(function (require, exports, module) { | |||
| ); | |||
| } | |||
|
|
|||
| /** | |||
| * Gets an array of extensions that are currently installed and can be updated to a new version | |||
| * @return {Array} array of objects with properies id,installVersion,registryVersion | |||
There was a problem hiding this comment.
Should we change the type to Array.<{id: string, installVersion: string, registryVersion: string}> and change the description a bit to not mention with properies id,installVersion,registryVersion?
|
@zaggino Sorry for the delay. I did a final review of the code and found a few nits. After those are fixed, I think we can finally merge this. |
Updated design Check registry at most once a day Remove updated and uninstalled extensions from update list Use new icon sprite Calculate when an actual update should be launched Calculate updateCompatible for extensions toggle icon even when registry are downloaded through extension manager checkForUpdate is now called directly on the startup as it was before combine the functions download only if there're no updates available delete icon that is no longer user This sorts installed extensions by updateAvailable (#6634) align = in exports add an empty line use localeCompare instead of ifs use Array instead of array for preferences do not convert new preferences move event listener to the bottom of the file move function to the bottom to make JSLint happy count only compatible updates for notification icon add comments fix tests that were failing after changes to the model.notifyCount fix issue when update button is disabled but newer compatible version (not latest) exists add tests for ExtensionManager.getAvailableUpdates() method add tests for ExtensionManager.cleanAvailableUpdates() method move new tests
|
@TomMalbran comments should be fixed now. |
|
Ok. I am ready to merge this. If anything fails, we can fix it later :) |
Check for extensions updates
As mentioned here #6723
Ping @lkcampbell @larz0
P.S.: this is not a complete thing but I wanted to get it moving at least a bit (and design is ugly because I don't have a software to work with SVG :)) Feel free to push into this branch.