ofirgall / tmux-window-name

A plugin to name your tmux windows smartly.
MIT License
205 stars 19 forks source link

Added support for MacOS (Darwin) #2

Closed jaclu closed 2 years ago

jaclu commented 2 years ago

The Darwin (MacOS) ps is not compatible with the params you are using, so I made a workaround splitting the task up in ps + grep. However subprocess.check_output() does not support piped commands directly, and using shell=True is not recommended, and chaining two Popen adds code complexity, so I opted to use the available sh packet, which inherently supports chaining commands. If you totally object to another external package, I still have my chained Popen code lying around, so I could add that approach in a separate PR.

To ensure my change has no impact on the rest of this tool, I made my best effort to emulate the GNU ps output.

I rearranged the order of the first 3 columns of ps output to put ppid first, so that grep ^pid could identify relevant lines. Since you cut off the first 6 items it does not have any impact at this point, I just thought I should mention it so that if it leads to confusion later there is a note about the reason for this change.

The very wide tty I have given the ps command is to ensure the line isn't cut off at this point, and let the @tmux_window_name_max_name_len setting make the decision when the window name is assigned.

The default tty width for sh commands is 80, and from that the first 6 output columns used by rename_session_windows.py, eats up 53, only leaving 27 spaces of actual content.

As I removed my use_tilde new feature to only add Darwin support in this PR, I unintentionally also removed one change that should be in this PR. This line should be added to tmux_window_name.tmux to ensure auto-installation of the dependency sh

[ "$(uname -s)" = "Darwin" ] && python -m pip install sh --user

jaclu commented 2 years ago

If you are ok with shell=True, the change is simpler, no dependency on sh and it can be in the the same try: excpt: statement as the GNU ps call: program = subprocess.check_output(f'ps -o ppid,pid,user,cpu,stime,tty,time,command | grep ^{pid}', shell=True)

ofirgall commented 2 years ago

Thanks for the detailed explanation, I don't like the use of shell=True either, can we use your ps command and cut the relevant text later with python regex?

jaclu commented 2 years ago

Not sure what you mean by 'can we use your ps command' I submitted it as a PR, so now its up to you to accept or refuse it :) You can't really use that ps command in a single statement since then you still need to grep to filter out relevant processes, but I guess you could remove the Darwin check and use the same nested sh commands on all platforms if that is what you meant, or just use a python regex as you said, I'm to much of a shell person to have ever bothered learning those. But you are welcome to cherry-pick changes and just insert them in your codebase of course, in that case, please credit me in some form :)

jaclu commented 2 years ago

If you do you also have to add the additional change at the end of my initial msg, I couldn't be bothered to repeat the entire process to get that in :) As soon as you have made your decision on this PR I will submit another PR for the added use_tilde feature

ofirgall commented 2 years ago

I have pushed a code that doesn't rely on ppid in ps can you check out if it works in darwin? I would love to add support for the use_tilde, it sounds nice.

jaclu commented 2 years ago

Works fine!