Skip to content
This repository has been archived by the owner on Sep 2, 2021. It is now read-only.

[REVIEW-ONLY] Implement node-module add-on for native Windows file watching #412

Closed
wants to merge 1 commit into from

Conversation

bchintx
Copy link
Contributor

@bchintx bchintx commented Jan 28, 2014

Fixes brackets issue #6551- "[File Watchers] Can not externally rename a directory with subfolders on Windows"

Requires associated pull request #6673 from brackets repo.

Calling a native node-module add-on in Windows requires that the node process be named "node.exe", rather than our renamed "Brackets-node.exe". This change reverts our previous renaming so that we ship with "node.exe" instead.

@jasonsanjose
Copy link
Member

@bchintx this pull request targets master. Is that right or should it be release?

@bchintx
Copy link
Contributor Author

bchintx commented Jan 28, 2014

oh, hey! Good catch, @jasonsanjose . Yes, before merging, I'll re-submit this to release instead.

@bchintx
Copy link
Contributor Author

bchintx commented Jan 29, 2014

Closing this pull request to re-submit against the release branch.

@bchintx bchintx closed this Jan 29, 2014
@iwehrman
Copy link
Contributor

It would be easy and helpful to also set the Node process title back to something unique from within appshell/node-core, probably in Launcher.js:

process.title = "Brackets-Node";

@bchintx
Copy link
Contributor Author

bchintx commented Jan 29, 2014

FYI...submitted Pull Request #413 to replace this one.

@bchintx
Copy link
Contributor Author

bchintx commented Jan 29, 2014

@iwehrman Good observation. Actually @JeffryBooher tried that suggestion but it doesn't work on Windows, which is where we're having the issue. Even with that change, TaskManager continues to show the actual filename.

@iwehrman
Copy link
Contributor

Yes, but it helps differentiate the process on Mac and Linux at least. It's just a convenience for when you ps and find a bunch of node processes.

@iwehrman
Copy link
Contributor

(Plus it seems like the kind of thing that Node will get around to fixing on Windows eventually.)
(Plus it's easy.)

@ingorichter
Copy link
Contributor

I agree with Ian. We should do this anyway. It doesn't do any harm and helps at least on 2 out of 3 supported platforms.

@iwehrman
Copy link
Contributor

You know what they say: two outta three ain't bad!

@bchintx
Copy link
Contributor Author

bchintx commented Jan 29, 2014

@iwehrman @ingorichter
I understand your concerns, but please note that this change is only for Windows. The Mac binary is still "Brackets-node", just like it was before.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants