Skip to content
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

list_screens() returns new instances of Screen #15

Open
rhatgadkar opened this issue Mar 3, 2017 · 2 comments
Open

list_screens() returns new instances of Screen #15

rhatgadkar opened this issue Mar 3, 2017 · 2 comments

Comments

@rhatgadkar
Copy link
Contributor

When the user types list_screens(), new instances of already existing Screen instances are returned. This is a problem, because if a user has enabled logs in a screen session, the user will not be able to get the logs from Screen instances returned by list_screens(). Here is an example scenario when the user is trying to disable and delete logs for all current screen sessions:

scr1 = Screen('scr1', True)
[detached]
There is a screen on:
46165.scr1 (Detached)
There is no screen to be detached matching 46165.

list_screens()
[<Screen 'scr1'>]
scr1.enable_logs('scr1.log')
screen_list = list_screens()
screen_list
[<Screen 'scr1'>]
for i in range(0, len(screen_list)):
... screen_list[i].disable_logs(True)
...
Traceback (most recent call last):
File "", line 2, in
File "screenutils/screen.py", line 104, in disable_logs
system('rm ' + self._logfilename)
TypeError: cannot concatenate 'str' and 'NoneType' objects

In the example above, the user created a Screen with name 'scr1' and a log file with name 'scr1.log'. But when using the Screen instance returned from list_screens(), the user is unable to delete the log file, because list_screens() creates new instances of an already existing Screen. This shouldn't be allowed. list_screens() should return references to already existing instances of Screen.

@kingllama
Copy link
Contributor

Yes. list_screens() right now returns a NEW instance for each existing screen. This might not be the best way of handling this, and the documentation certainly doesn't reflect that fact.

I'm not 100% sure if the function should return new instances or existing one. Perhaps it would be better to deprecate the function in favour of a create_for_all, a get_or_create_for_all and a get_all. I personally can't imagine a situation where you'd want >1 instance for existing screen, but thats not to say it wouldn't come up.

The current workaround for the issue you brought up (deleting the logfiles) would be to .disable_logs(False) and then delete the files some other way.

@Christophe31
Copy link
Owner

hum, I didn't know there was real users of this script ^^, I don't use it since… many years and don't even use screen anymore (tmux switch)… I'm not so motivated to maintain this project but understand your issue…
kingllama, you want to fork?

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

No branches or pull requests

3 participants