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

fix broken completion of screen on osx, test on ubuntu and mac #3271

Merged
merged 6 commits into from Aug 19, 2016

Conversation

Projects
None yet
4 participants
@arkbriar
Contributor

arkbriar commented Jul 28, 2016

Description

Change sed expression of function '__fish_complete_screen' in share/completion/screen.fish to support completion on both mac and linux.
I have tested on my mac and ubuntu server.

Fixes issue #

fish does not complete screen -r when tapping Tab on mac (both GNU and FAU version)

Output example

on mac:

~/W/G/f/s/completions (master)$ screen -r 819
81930.test1  (Screen: Detached)  81951.test3  (Screen: Detached)
81940.test2  (Screen: Detached)  81961.test4  (Screen: Detached)

on linux (ubuntu):

xxx@localhost ~> screen -r 4
4171.test1  ((07/28/16 08:09:41) Screen:Detached)  4272.test3  ((07/28/16 08:10:19) Screen:Detached)
4255.test2  ((07/28/16 08:10:17) Screen:Detached)  4289.test4  ((07/28/16 08:10:21) Screen:Detached)

You will find that they are a little different because screen -list on mac doesn't show time.

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Jul 28, 2016

Member

Can you post an example of the output? Ideally, this'd be switched over to our string tool. I haven't done so so far because I couldn't properly test it, since I don't use screen.

Member

faho commented Jul 28, 2016

Can you post an example of the output? Ideally, this'd be switched over to our string tool. I haven't done so so far because I couldn't properly test it, since I don't use screen.

@faho faho added the completions label Jul 28, 2016

@floam floam changed the title from fix broken completion of screen on osx, test on ubuntu and mac with f… to fix broken completion of screen on osx, test on ubuntu and mac Jul 28, 2016

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Jul 30, 2016

As long as you're here, might want to replace __fish_sgrep (uses grep, should be considered defunct) with a string match.

floam commented on 85923c8 Jul 30, 2016

As long as you're here, might want to replace __fish_sgrep (uses grep, should be considered defunct) with a string match.

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Aug 1, 2016

Member

Cool.

In context though, before you type the first "4" in examples at the top, these appear along with many other completions. On Linux, the date means these no longer fit in a single column and you have to scroll down through more pages to get to them:
screenshot 2016-08-01 at 1 47 53 am
screenshot 2016-08-01 at 1 39 41 am

The omitted space after "Screen:" is kinda ugly. I think I'd rather drop the "Screen:" here than add it though. These columns in the pager can be narrow. Maybe it could be -r32189.pts-3.aaron (08/01/2016 08:45:00 AM attached)?

I wonder what we can do to make it less of a pain to scroll past all the other options upon screen -r<tab>. It'd be nice if they appeared at the front or were suggested right after a screen <tab> instead of the files in the current directory it can't do anything with.

Member

floam commented Aug 1, 2016

Cool.

In context though, before you type the first "4" in examples at the top, these appear along with many other completions. On Linux, the date means these no longer fit in a single column and you have to scroll down through more pages to get to them:
screenshot 2016-08-01 at 1 47 53 am
screenshot 2016-08-01 at 1 39 41 am

The omitted space after "Screen:" is kinda ugly. I think I'd rather drop the "Screen:" here than add it though. These columns in the pager can be narrow. Maybe it could be -r32189.pts-3.aaron (08/01/2016 08:45:00 AM attached)?

I wonder what we can do to make it less of a pain to scroll past all the other options upon screen -r<tab>. It'd be nice if they appeared at the front or were suggested right after a screen <tab> instead of the files in the current directory it can't do anything with.

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Aug 1, 2016

Member

Hm, one can adjust LC_TIME to perhaps get a more narrow format/ISO 8601 through en_DK:

screen -list
There are screens on:
    32189.pts-3.aaron   (08/01/2016 08:45:00 AM)    (Attached)
…

begin; set -lx LC_TIME en_DK.UTF-8; screen -list; end
There are screens on:
    32189.pts-3.aaron   (2016-08-01 08:45:01)   (Attached)
…
begin; set -lx LC_TIME POSIX; screen -list; end
There are screens on:
    32189.pts-3.aaron   (08/01/16 08:45:01) (Attached)
Member

floam commented Aug 1, 2016

Hm, one can adjust LC_TIME to perhaps get a more narrow format/ISO 8601 through en_DK:

screen -list
There are screens on:
    32189.pts-3.aaron   (08/01/2016 08:45:00 AM)    (Attached)
…

begin; set -lx LC_TIME en_DK.UTF-8; screen -list; end
There are screens on:
    32189.pts-3.aaron   (2016-08-01 08:45:01)   (Attached)
…
begin; set -lx LC_TIME POSIX; screen -list; end
There are screens on:
    32189.pts-3.aaron   (08/01/16 08:45:01) (Attached)
@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Aug 2, 2016

Member

Wow!

Member

floam commented Aug 2, 2016

Wow!

@floam floam self-assigned this Aug 2, 2016

Show outdated Hide outdated share/completions/screen.fish
function __fish_detect_screen_socket_dir --description "Detect which folder screen uses"
set screen_bin screen
if not set -q __fish_screen_socket_dir
set -g __fish_screen_socket_dir (eval $screen_bin -ls __fish_i_don_t_think_this_will_be_matched | string match -r "(?<=No Sockets found in ).*(?=\.)")

This comment has been minimized.

@floam

floam Aug 2, 2016

Member

Looking over Apple's released screen code, while the per-user TMPDIR thing is an Apple addition, I don't think it's the case that only Macs will have an old screen with no date output (I could be wrong?).

I worry checking for this specific string might break for localizations. Could we try to detect the path whether or not there were attached screens? -q -list and exit status might help but probably isn't necessary.

@floam

floam Aug 2, 2016

Member

Looking over Apple's released screen code, while the per-user TMPDIR thing is an Apple addition, I don't think it's the case that only Macs will have an old screen with no date output (I could be wrong?).

I worry checking for this specific string might break for localizations. Could we try to detect the path whether or not there were attached screens? -q -list and exit status might help but probably isn't necessary.

This comment has been minimized.

@arkbriar

arkbriar Aug 2, 2016

Contributor

The socket directory is a compiler option so we could't know the path of a screen binary before it is run. Also, TMPDIR is used by the builtin screen on mac (not those installed with brew which use /tmp/uscreens/S-$USER, not even others). So I have to tell which path current screen command of current user is using If we want to steal the time from those socket files.

And we also could not know whether a screen on other platform shows time or not before there are sessions.

-q -ls can only tell whether there are sockets matched in the SockDir.

-list Do nothing, just list our SockDir [on possible matches].

-q Suppress printing of error messages. In combination with "-ls" the exit value
is as follows: 9 indicates a directory without sessions. 10 indicates a direc-
tory with running but not attachable sessions. 11 (or more) indicates 1 (or
more) usable sessions.

So as far as I see, the only way to get the sockdir is to run screen -ls.

Localization is a big problem. However, think about it: they could translate "Detached" to "分离" while they translate "No Socket in ..." into "没有..." . We can not prevent anyone from doing this.
So I think it is not strictly necessary to consider localization.

@arkbriar

arkbriar Aug 2, 2016

Contributor

The socket directory is a compiler option so we could't know the path of a screen binary before it is run. Also, TMPDIR is used by the builtin screen on mac (not those installed with brew which use /tmp/uscreens/S-$USER, not even others). So I have to tell which path current screen command of current user is using If we want to steal the time from those socket files.

And we also could not know whether a screen on other platform shows time or not before there are sessions.

-q -ls can only tell whether there are sockets matched in the SockDir.

-list Do nothing, just list our SockDir [on possible matches].

-q Suppress printing of error messages. In combination with "-ls" the exit value
is as follows: 9 indicates a directory without sessions. 10 indicates a direc-
tory with running but not attachable sessions. 11 (or more) indicates 1 (or
more) usable sessions.

So as far as I see, the only way to get the sockdir is to run screen -ls.

Localization is a big problem. However, think about it: they could translate "Detached" to "分离" while they translate "No Socket in ..." into "没有..." . We can not prevent anyone from doing this.
So I think it is not strictly necessary to consider localization.

@arkbriar

This comment has been minimized.

Show comment
Hide comment
@arkbriar

arkbriar Aug 2, 2016

Contributor

Setting LC_TIME doesn't change the output of screen on my ubuntu server. Maybe some compiler options were not opened to support this.

begin; set -lx LC_TIME POSIX; screen -list; end
There are screens on:
    30035.test  (08/01/16 21:49:14) (Detached)
begin; set -lx LC_TIME en_DK.UTF-8; screen -list; end
There are screens on:
    30035.test  (08/01/16 21:49:14) (Detached)

They just behave the same way.


I use a trick for screen on mac to detect the time and status. Let me show you how it works.

I have these screen sockets in socket directory.

/p/t/u/S-xxx$ ll
total 0
srw-------  1 xxx  staff     0B Aug  2 09:52 90828.name
srw-------  1 xxx  staff     0B Aug  2 09:12 90843.test
srw-------  1 xxx  staff     0B Aug  2 09:12 90861.so
srw-------  1 xxx  staff     0B Aug  2 09:14 91053.fdsjk
srw-------  1 xxx  staff     0B Aug  2 09:14 91087.tes

They are all detached.

There are screens on:
    90828.name  (Detached)
    90843.test  (Detached)
    90861.so    (Detached)
    91053.fdsjk (Detached)
    91087.tes   (Detached)
5 Sockets in /tmp/uscreens/S-xxx.

And then I change the file permission of one of them.

/p/t/u/S-xxx$ chmod 700 90843.test
/p/t/u/S-xxx$ ll
total 0
srw-------  1 xxx  staff     0B Aug  2 09:52 90828.name
srwx------  1 xxx  staff     0B Aug  2 09:12 90843.test
srw-------  1 xxx  staff     0B Aug  2 09:12 90861.so
srw-------  1 xxx  staff     0B Aug  2 09:14 91053.fdsjk
srw-------  1 xxx  staff     0B Aug  2 09:14 91087.tes
/p/t/u/S-xxx$ screen -ls
There are screens on:
    90828.name  (Detached)
    90843.test  (Attached)
    90861.so    (Detached)
    91053.fdsjk (Detached)
    91087.tes   (Detached)
5 Sockets in /tmp/uscreens/S-xxx.

The 90843.test is attached now without executing screen -r. So I think that screen uses the execute permission to tell whether one socket is in use, at least on mac.

Thus I change the list function for mac by using stat command to get the time, name and status.

It relies on stat and can not distinguish irrelevant files. :(

I'm sorry I have no time to come up with another good idea.

Contributor

arkbriar commented Aug 2, 2016

Setting LC_TIME doesn't change the output of screen on my ubuntu server. Maybe some compiler options were not opened to support this.

begin; set -lx LC_TIME POSIX; screen -list; end
There are screens on:
    30035.test  (08/01/16 21:49:14) (Detached)
begin; set -lx LC_TIME en_DK.UTF-8; screen -list; end
There are screens on:
    30035.test  (08/01/16 21:49:14) (Detached)

They just behave the same way.


I use a trick for screen on mac to detect the time and status. Let me show you how it works.

I have these screen sockets in socket directory.

/p/t/u/S-xxx$ ll
total 0
srw-------  1 xxx  staff     0B Aug  2 09:52 90828.name
srw-------  1 xxx  staff     0B Aug  2 09:12 90843.test
srw-------  1 xxx  staff     0B Aug  2 09:12 90861.so
srw-------  1 xxx  staff     0B Aug  2 09:14 91053.fdsjk
srw-------  1 xxx  staff     0B Aug  2 09:14 91087.tes

They are all detached.

There are screens on:
    90828.name  (Detached)
    90843.test  (Detached)
    90861.so    (Detached)
    91053.fdsjk (Detached)
    91087.tes   (Detached)
5 Sockets in /tmp/uscreens/S-xxx.

And then I change the file permission of one of them.

/p/t/u/S-xxx$ chmod 700 90843.test
/p/t/u/S-xxx$ ll
total 0
srw-------  1 xxx  staff     0B Aug  2 09:52 90828.name
srwx------  1 xxx  staff     0B Aug  2 09:12 90843.test
srw-------  1 xxx  staff     0B Aug  2 09:12 90861.so
srw-------  1 xxx  staff     0B Aug  2 09:14 91053.fdsjk
srw-------  1 xxx  staff     0B Aug  2 09:14 91087.tes
/p/t/u/S-xxx$ screen -ls
There are screens on:
    90828.name  (Detached)
    90843.test  (Attached)
    90861.so    (Detached)
    91053.fdsjk (Detached)
    91087.tes   (Detached)
5 Sockets in /tmp/uscreens/S-xxx.

The 90843.test is attached now without executing screen -r. So I think that screen uses the execute permission to tell whether one socket is in use, at least on mac.

Thus I change the list function for mac by using stat command to get the time, name and status.

It relies on stat and can not distinguish irrelevant files. :(

I'm sorry I have no time to come up with another good idea.

Show outdated Hide outdated share/completions/screen.fish
stat -f "%Lp %Sm %N" -t "%D %T" $__fish_screen_socket_dir/* | string match -r '^7\d{2} .*$' | string replace -r '^7\d{2} (\S* \S*) \S*/(\S+)' '$2\t$1 Attached'
pushd $__fish_screen_socket_dir > /dev/null
ls
set sockets (ls)

This comment has been minimized.

@floam

floam Aug 18, 2016

Member

set -l

@floam

floam Aug 18, 2016

Member

set -l

@@ -1,14 +1,58 @@
function __fish_detect_screen_socket_dir --description "Detect which folder screen uses"
set screen_bin screen

This comment has been minimized.

@floam

floam Aug 18, 2016

Member

What is this screen_bin for? Let's drop that

@floam

floam Aug 18, 2016

Member

What is this screen_bin for? Let's drop that

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Aug 18, 2016

Is this a typo? Should this be $sockets?

Is this a typo? Should this be $sockets?

@floam floam merged commit 044efef into fish-shell:master Aug 19, 2016

1 check passed

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

@faho faho added this to the next-2.x milestone Aug 27, 2016

@krader1961 krader1961 modified the milestones: fish 2.4.0, next-2.x Sep 3, 2016

nomaed added a commit to nomaed/fish-shell that referenced this pull request Jul 17, 2017

fix broken completion of screen on osx, test on ubuntu and mac (#3271)
* fixes broken completion of screen on osx, test on ubuntu and mac with fish 2.3.1
* replaces sed, __fish_sgrep with fish builtin string
* add completion for `screen -x`
* adjust format (e.g. 12345.socket\t01/01/16 09:55:00 Detached)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment