New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement progress callback so node domains are able to notify the Brackets before they finish #10761

Merged
merged 2 commits into from Jun 12, 2015

Conversation

Projects
None yet
4 participants
@zaggino
Contributor

zaggino commented Mar 19, 2015

Related PR: adobe/brackets-shell#509
Related Issue: #7356

Please review @nethip , I'd like to get this into Brackets as this would help to improve user experience on some of my work I'm doing with extensions.

@zaggino

This comment has been minimized.

Show comment
Hide comment
@zaggino

zaggino Mar 19, 2015

Contributor

If we don't want to introduce progressCallback as a new argument to node functions, we can make a call callback(null, null, "progress") to invoke a progress and not resolve the promise checking for arguments.length === 3

Contributor

zaggino commented Mar 19, 2015

If we don't want to introduce progressCallback as a new argument to node functions, we can make a call callback(null, null, "progress") to invoke a progress and not resolve the promise checking for arguments.length === 3

@nethip

This comment has been minimized.

Show comment
Hide comment
@nethip

nethip Mar 19, 2015

Contributor

@zaggino Sure! I will take a look at this.

Contributor

nethip commented Mar 19, 2015

@zaggino Sure! I will take a look at this.

@zaggino

This comment has been minimized.

Show comment
Hide comment
@zaggino

zaggino Mar 19, 2015

Contributor

The more I'm thinking about this, the more callback(null, null, "progress") version makes sense, but I'll wait for some opinions before fixing this PR.

Contributor

zaggino commented Mar 19, 2015

The more I'm thinking about this, the more callback(null, null, "progress") version makes sense, but I'll wait for some opinions before fixing this PR.

@zaggino zaggino added this to the Release 1.3 milestone Mar 23, 2015

@nethip

This comment has been minimized.

Show comment
Hide comment
@nethip

nethip Apr 3, 2015

Contributor

@zaggino Sorry. I could not find some time to go through this PR as I was caught up with lot of other stuff. I will for sure take a look at this PR next week.

Contributor

nethip commented Apr 3, 2015

@zaggino Sorry. I could not find some time to go through this PR as I was caught up with lot of other stuff. I will for sure take a look at this PR next week.

@nethip nethip self-assigned this Apr 3, 2015

@zaggino

This comment has been minimized.

Show comment
Hide comment
@zaggino

zaggino Apr 23, 2015

Contributor

So I'm guessing nobody found time to review this for 1.3 ...

Contributor

zaggino commented Apr 23, 2015

So I'm guessing nobody found time to review this for 1.3 ...

@nethip

This comment has been minimized.

Show comment
Hide comment
@nethip

nethip Apr 23, 2015

Contributor

Yes @zaggino That is right. Sorry for not being able to dedicate some time to this PR in this release.

Contributor

nethip commented Apr 23, 2015

Yes @zaggino That is right. Sorry for not being able to dedicate some time to this PR in this release.

@zaggino zaggino modified the milestones: Release 1.4, Release 1.3 Apr 23, 2015

@zaggino

This comment has been minimized.

Show comment
Hide comment
@zaggino

zaggino Apr 27, 2015

Contributor

Can we get someone to merge this into 1.4?

Contributor

zaggino commented Apr 27, 2015

Can we get someone to merge this into 1.4?

@prafulVaishnav prafulVaishnav self-assigned this May 6, 2015

@@ -192,6 +192,42 @@ define(function (require, exports, module) {
});
});
it("should receive progress events from asynchronous commands", function () {

This comment has been minimized.

@abose

abose Jun 11, 2015

Contributor

@zaggino I ran the Live preview test for Node connection and the test is timing out at should receive progress events from asynchronous commands [win 8.1]. Any idea why?

@abose

abose Jun 11, 2015

Contributor

@zaggino I ran the Live preview test for Node connection and the test is timing out at should receive progress events from asynchronous commands [win 8.1]. Any idea why?

This comment has been minimized.

@prafulVaishnav

prafulVaishnav Jun 12, 2015

Contributor

@abose Have you taken shell changes also? adobe/brackets-shell#509

@prafulVaishnav

prafulVaishnav Jun 12, 2015

Contributor

@abose Have you taken shell changes also? adobe/brackets-shell#509

This comment has been minimized.

@abose

abose Jun 12, 2015

Contributor

oh! i didn't that explains. Did you verify this change?

@abose

abose Jun 12, 2015

Contributor

oh! i didn't that explains. Did you verify this change?

@abose abose assigned abose and unassigned prafulVaishnav Jun 12, 2015

abose added a commit that referenced this pull request Jun 12, 2015

Merge pull request #10761 from adobe/zaggino/domain-progress
Implement progress callback so node domains are able to notify the Brackets before they finish

@abose abose merged commit 3348ab0 into master Jun 12, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@abose abose deleted the zaggino/domain-progress branch Jun 12, 2015

@abose

This comment has been minimized.

Show comment
Hide comment
@abose

abose Jun 12, 2015

Contributor

Verified, merging.
The corresponding PR adobe/brackets-shell#509 in brackets shell has been merged.
Our linux builds will not pick up this change on brackets shell as we build from the older CEF branch.
So will need to manually add this before release. Adding to release 1.4 checklist #11256 .

Contributor

abose commented Jun 12, 2015

Verified, merging.
The corresponding PR adobe/brackets-shell#509 in brackets shell has been merged.
Our linux builds will not pick up this change on brackets shell as we build from the older CEF branch.
So will need to manually add this before release. Adding to release 1.4 checklist #11256 .

@nethip

This comment has been minimized.

Show comment
Hide comment
@nethip

nethip Jun 12, 2015

Contributor

This is going to make @zaggino to see this once he is back from vacation 😄

Contributor

nethip commented Jun 12, 2015

This is going to make @zaggino to see this once he is back from vacation 😄

@zaggino

This comment has been minimized.

Show comment
Hide comment
@zaggino

zaggino Jul 9, 2015

Contributor

Thanks a lot guys for merging this 👍

Contributor

zaggino commented Jul 9, 2015

Thanks a lot guys for merging this 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment