search

jung-kim / atom-ungit

Atom plugin for Ungit project
MIT License
75 stars 12 forks source link

Ungit server doesn't get killed when toggled off #1

Closed jung-kim closed 10 years ago

ibnesayeed commented 10 years ago

This issue aside, but we will have to think about it if we really want to do this. What if multiple editor instances are open in different directory contexts, killing it from one instance of the editor will kill process for everyone. I would recommend the toggle functionality should be implemented in a way that if no Ungit process is running then run it, otherwise simple hide of show the view. This will make the experience of using this package very fluid and fast. Also to explicitly kill the running Ungit process, we can have another dedicated key-binding. Another approach will be to run multiple Ungit processes, i.e. create one and kill it every time toggle function is fired. This will require it to generate a random port number and use that to instantiate the Ungit server.

jung-kim commented 10 years ago

I'm not a big fan of multiple instances as they seems like over kill and I really don't think it is recommended. There could be potential issues if two different ungit instances navigate to same directory and work in there. And there can be swarm of headless children running around when atom crashes, although number of ungit instance should be low in many cases.

One instance for all is better I believe. On toggle, start if missing, then redirect. If there is "on atom terminate" event it would be a perfect place to place kill ungit command.

But exposing a short cut to manually kill ungit is a good idea in any cases I think.

jung-kim commented 10 years ago

In my environment, current execution command to pick up ungit pid, ps -ef | grep 'ungit --no-b|server.js --no-b' runs in different shell /bin/sh -c ps -ef | grep 'ungit --no-b|server.js --no-b'.

Which I have no idea why this is and this needs to be fixed.

ibnesayeed commented 10 years ago

I am afraid if this type of process query will be platform agnostic? Will it work on Windows too? Not that I care coding in Windows machine, but the tool should work on all platforms where Ungit can run.

ibnesayeed commented 10 years ago

@codingtwinky, is atom-ungit working for you? I have updated the package dependency and now I can't get Ungit to work. I have Ungit installed independently, that is working fine, but if I run it from the binary that is packaged inside atom-ungit/node_modules, it gives me errors in the editor as well as in the browser.

jung-kim commented 10 years ago

Last I checked atom.io wasn't available for Windows, but I guess it is runnable on windows now. (based on the quick looks around, need confirmation though.) I will add quick OS check and differentiated treatment.

I just need git pull and it works fine for me, I have not updated the ungit though. What I did noticed is sometimes page load happens before ungit is ready despite the logic to wait on ## Ungit Started ## stdout string. In such cases, pressing ctrl + alt + u to close the tab, and ctrl + alt + u again to reopen and it works fine.

jung-kim commented 10 years ago

Hmm, are you getting node: No such file or directory error when you open developer's tools in atom? This may answer your question, https://github.com/joyent/node/issues/3911. Based on my quick talk with atom guys this is due to OSX's $PATH configuration problem.

Depends on how you start atom, either you start by atom icon or atom command in command line, $PATH will be different and may not able to recognize the node command.

sudo ln -fs /usr/local/bin/node /usr/bin/node did the trick for me

ibnesayeed commented 10 years ago

This is not the kind of error I am getting. I am using it on Ubuntu. I tried to change the ungit dependency to the master branch and when I ran the update_package_dependencies command, it was all ruined up. Then I switched the dependency back to "=0.8.1" and now dependency update command is failing. Which simply means I will have to uninstall Atom, clean all the related directories and start it over. Right now if I am trying to run run atom from command line with the directory context on the atom-ungit package, I am getting an error in the developer console: "TypeError: Unable to watch path", followed by the stack trace.

ibnesayeed commented 10 years ago

Finally I got my editor working as before. Now I can contribute to this further. It turns out, the issue was very unrelated. I had a lower limit of inotify, I have just increased it and the problem is gone now. :)

jung-kim commented 10 years ago

Glad to hear it is resolved.

Oh and now ungit terminates definitely after https://github.com/codingtwinky/atom-ungit/commit/44ae111efee2d80eed284e74aa2615511a0a37cd check in but I forgot to attached to this one...