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

Patch for setting window title of Windows Console and Xterm #51

Merged
0 commits merged into from
Jan 4, 2011
Merged

Patch for setting window title of Windows Console and Xterm #51

0 commits merged into from
Jan 4, 2011

Conversation

grawity
Copy link
Contributor

@grawity grawity commented Jan 3, 2011

A patch for properly setting title of Windows Console without relying on os.system() or external modules (issue #3), and of Xterm and compatible terminal emulators.

@rg3
Copy link
Collaborator

rg3 commented Jan 3, 2011

Thanks for the code. I will not pull as it is, but here's my list of complaints so they can be addressed:

I'd like to have a single commit to pull. git rebase -i is your friend for this (git help rebase, INTERACTIVE MODE section). The commit should have a good description, specially about the Windows part.

I'd like the feature to be optional using a command line option. The thing is, the method you're using to detect the terminal type under Unix systems does not work as expected. For example, you can have a screen session inside a TTY and in that case the code does not work and the title stuff is printed to the terminal, at least in my tests. So, IMHO, the best way is to have this code activated selectively by an option, and disabled by default.

Third and last, change the code so that the imports from ctype are called only once, and the string to use as the text and the terminal title is composed only once.

@grawity
Copy link
Contributor Author

grawity commented Jan 4, 2011

I'd like to have a single commit to pull. git rebase -i is your friend for this (git help rebase, INTERACTIVE MODE section).

At the moment I have no idea how to do this :/

the method you're using to detect the terminal type under Unix systems does not work as expected. For example, you can have a screen session inside a TTY and in that case the code does not work and the title stuff is printed to the terminal, at least in my tests.

It does not matter where a screen session is running; inside the session, $TERM should always be set to screen1, so that my code would output the screen-specific sequence \033k%s\033\\. Any other terminal type is misconfiguration - exactly like it would be if you had $TERM set to xterm while being on bare Linux TTY - which there is no proper way to handle.

1 I'll admit that I forgot the possibility of $TERM being screen.linux or screen.xterm, as it happens on some distros; however, my current code would just fall back to no title at all.

Third and last, change the code so that the imports from ctype are called only once,

Will do. (Although Python never imports the same module twice anyway.)

and the string to use as the text and the terminal title is composed only once.

Do you mean titlestring? I'm not entirely sure where I can store it (self._titlestring?)

@rg3
Copy link
Collaborator

rg3 commented Jan 4, 2011

Thanks a lot for your time and comments. I've reviewed this matter with more depth. Here are my comments:

Regarding the rebasing stuff. Let's say you fork my repository. You could then create a new branch to work on this feature and call the branch "title-progress", for example. You start making changes, committing them, testing, etc. In the end and after much tunning, you made 20 commits. You can, then, run something like git rebase -i master to interactively rebase everything to the master branch. git lets you choose which commits to discard, which to apply, and which ones to squash into the previous commit. This way, you can squash everything as a single commit, with a proper commit message, and ask me to pull from the title-progress branch. See the manpage and stuff like this:

http://book.git-scm.com/4_interactive_rebasing.html

Regarding the screen stuff. Thanks for pointing out everything you said. I've read a bit about the TERM environment variable and it turns out my system was misconfigured. Hence the problem I was experiencing. I have corrected my system configuration regarding screen, but I completely object to the method you're using here. I don't want youtube-dl to mess with my screen window name at all. It's completely annoying.

If I merge this feature, it's an absolute requirement that the feature will be disabled by default and only activated by a program option. This option will then suppose the program is being run inside an xterm or xterm-compatible terminal, and use the escape sequences for xterm, because the feature is meant to help people read the progress in the task bar in a supposed GUI. It should not peek at the TERM environment variable and, for example, change the screen session's window name. That would not change the terminal title in the GUI.

As for composing the text only once, I meant something like:

text = u'%s of %s at %s ETA %s' % (...)
self.to_screen(u'\r[download] %s' % text)
if User Chose To Change Title:
    self.to_cons_title(u'youtube-dl - %s' % text)

@grawity
Copy link
Contributor Author

grawity commented Jan 4, 2011

Regarding the rebasing stuff. Let's say you fork my repository. You could then create a new branch to work on this feature and call the branch "title-progress", for example. You start making changes, committing them, testing, etc. In the end and after much tunning, you made 20 commits. You can, then, run something like git rebase -i master to interactively rebase everything to the master branch.

Thanks for the explanation.

but I completely object to the method you're using here. I don't want youtube-dl to mess with my screen window name at all. It's completely annoying

Noted. I will remove the screen seq.

it's an absolute requirement that the feature will be disabled by default and only activated by a program option.

Noted.

This option will then suppose the program is being run inside an xterm or xterm-compatible terminal, and use the escape sequences for xterm

Noted.

As for composing the text only once, I meant something like:

text = u'%s of %s at %s ETA %s' % (...)
self.to_screen(u'\r[download] %s' % text)
if User Chose To Change Title:
    self.to_cons_title(u'youtube-dl - %s' % text)

The reason my patch was doing it twice is that report_progress() uses multiple spaces in percent_str and data_len_str for layout. While this works good for monospaced terminal output...

[download]    1% of 42 MB at   32 kB/s
[download]   99% of 42 MB at  120 kB/s

...it doesn't work for titlebars, which almost always use a variable-width font (with digits being much wider than spaces):

youtube-dl -    1% of 42 MB at   32 kB/s
youtube-dl -   99% of 42 MB at  120 kB/s

@rg3
Copy link
Collaborator

rg3 commented Jan 4, 2011

OK. The text unification was just a minor detail anyway, so that's your call.

@FelixChery FelixChery mentioned this pull request Apr 24, 2019
joedborg referenced this pull request in joedborg/youtube-dl Nov 17, 2020
[pull] master from rg3:master
tsukumijima pushed a commit to tsukumijima/youtube-dl that referenced this pull request Dec 2, 2020
This pull request was closed.
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

Successfully merging this pull request may close these issues.

2 participants