adobe-research / theseus

A pretty darn cool JavaScript debugger for Brackets
Other
1.34k stars 69 forks source link

Added new version updating and prevented Theseus from debugging wrong Brackets window #33

Closed nhynes closed 10 years ago

nhynes commented 10 years ago

New version notification and option to update.

Added unique ID to new debuggable Brackets window so that Theseus can grab the right one

marcelgerber commented 10 years ago

2 improvements:

I will add an german translation, too.

alltom commented 10 years ago

This pull request has a lot of great stuff in it! However, for that reason, could you split it into one pull request for each change set? I see three changes:

  1. Notify users of new versions of Theseus.
  2. Don't debug the instance of Brackets that's doing the debugging.
  3. Send OS with usage data.

This will allow us to test and discuss the changes independently.

Hopefully that's not too big a deal. I think I would use git diff to make a patch with all those changes, then check out origin/master into three feature branches, git apply < patch in each, and edit out the irrelevant changes (probably with git add -p). I think the second two pull requests will go right in, but I think the first one needs some discussion.

I agree with SAPlayer (thanks, SAPlayer!) on the first point: we should probably throttle the update checks. Once every 24 hours if that's how Brackets updates work. However, to SAPlayer's second point, it looks like you're already checking that the Theseus update is compatible with the current version of Brackets (var checkAgainst and var compatibility). Is that right?

Also, I'm really hesitant to use Brackets' ExtensionManager API (since it's so new) and especially nervous about using its dialogs. And we definitely shouldn't use methods prefixed with an underscore. There should be a lot of error handling in there for when any of that stuff goes away, which it inevitably will, and if it requires using underscored methods to perform an update, we should settle for telling the user there is an update and opening the extension manager.

Thanks for pulling all these changes together! Despite my criticism, the code is looking great and this is valuable stuff.