stats.lua: clip lines with ${term-clip-cc}

[guidocella]

Use the new property introduced in bf025cd289 to clip the lines of stats.lua with accurate unicode width detection and considering --msg-module and --msg-time.

no_ass_indent is changed from \t to 4 spaces because the width of \t is not calculated correctly.

mpv --idle --script-opts=stats-bindlist=yes uses io.write, so don't use ${term-clip-cc} in that case because it doesn't work.

[github-actions[bot]]

Download the artifacts for this pull request:

Windows

• mpv-i686-w64-mingw32
• mpv-x86_64-w64-mingw32
• mpv-x86_64-windows-msvc

macOS

• mpv-macos-13-intel
• mpv-macos-14-arm
• mpv-macos-15-arm

[avih]

Generally looks OK.

Few things:

The default prefix is changed from tab to 4 spaces because presumably the tab width calculation is incorrect. Maybe the tab width calculation should be fixed instead? especially since all the output goes through the log, it knows exactly the whole line content, so there shouldn't be an issue of "don't know how many columns the tab advanced because we don't know at which column the text starts", right?

And terminal clipping is not unique to stats. Other scripts might want to use it as well, with tabs. So I think tabs should be fixed at the clipping calc.

Also, as this is a prefix on first column, shouldn't tab be replaced with 8 spaces to keep the output looking the same (in most terminals)?

Escape sequences can be disabled with --script-opts-append=stats-no_ass_b1= --script-opts-append=stats-no_ass_b0=.

I don't like this. What if tomorrow the bindlist output also have other escape sequences? so the user would need to update their options and disable every single one of the terminal ass sequences manually? Why not a single option to disable all the escapes? (I don't mind that it was changed from the previous compound value of -, but it should be reasonably easy to disable all of it IMO). The bindlist code knows exactly which ass sequences it uses, and it should be trivial to disable all of them together like the previous code did.

Maybe add a method to override all the escapes together - either only for bindlist or to stats in general or even for the mpv logging in general (and document it), and mention that specific escape sequences can still be overridden normally, e.g. using --script-opts-append=stats-no_ass_b0= etc?

Not entirely sure of the specifics, but I think it should be easy to disable all the escapes together.

Also, what if stdout is not a tty? does it still output escape sequences? If not, can it be forced anyway, because e.g. less does know to display SGR sequences (with -R).

I presume that the output is not clipped if stdout is not a tty, e.g. pipe to less, right? I wonder whether the docs should mention something about it? (not sure, because it's a good default general practice to not do tty-specific things if it's not a tty, and this is how it behaves already, presumably at least as far as clipping goes).

Not specific to this PR, but in general I think it would be useful to have an option to override the terminal dimensions in case we don't get that right or whatever other reason that the user wants to behave as if the terminal is of a specific size. In this case it would affect to the clip width, but there are also other cases, e.g. IIRC both the sixel and tct vo have their own options to specify the terminal width, so maybe unify all of those into a common width/height which the log clipping and any terminal VO can use instead?

[kasper93]

After this changes I get empty line in output when there is none output. For example page 2 with dmabuf-wayland.

[guidocella]

After this changes I get empty line in output when there is none output. For example page 2 with dmabuf-wayland.

${term-clip-cc} alone prints a newline, is that expected?

[guidocella]

The default prefix is changed from tab to 4 spaces because presumably the tab width calculation is incorrect. Maybe the tab width calculation should be fixed instead? especially since all the output goes through the log, it knows exactly the whole line content, so there shouldn't be an issue of "don't know how many columns the tab advanced because we don't know at which column the text starts", right?

And terminal clipping is not unique to stats. Other scripts might want to use it as well, with tabs. So I think tabs should be fixed at the clipping calc.

Also, as this is a prefix on first column, shouldn't tab be replaced with 8 spaces to keep the output looking the same (in most terminals)?

I actually prefer 4 spaces to tabs because they waste too much space.

I don't like this. What if tomorrow the bindlist output also have other escape sequences? so the user would need to update their options and disable every single one of the terminal ass sequences manually? Why not a single option to disable all the escapes? (I don't mind that it was changed from the previous compound value of -, but it should be reasonably easy to disable all of it IMO). The bindlist code knows exactly which ass sequences it uses, and it should be trivial to disable all of them together like the previous code did.

Maybe add a method to override all the escapes together - either only for bindlist or to stats in general or even for the mpv logging in general (and document it), and mention that specific escape sequences can still be overridden normally, e.g. using --script-opts-append=stats-no_ass_b0= etc?

Not entirely sure of the specifics, but I think it should be easy to disable all the escapes together.

Added print_escape_sequences.

Also, what if stdout is not a tty? does it still output escape sequences? If not, can it be forced anyway, because e.g. less does know to display SGR sequences (with -R).

They are outputted to less.

I presume that the output is not clipped if stdout is not a tty, e.g. pipe to less, right? I wonder whether the docs should mention something about it? (not sure, because it's a good default general practice to not do tty-specific things if it's not a tty, and this is how it behaves already, presumably at least as far as clipping goes).

It is not clipped when piping to less.

[avih]

Not entirely sure of the specifics, but I think it should be easy to disable all the escapes together.

Added print_escape_sequences.

So why this instead of the =-yes method? surely you agree that it's easier for the user to replace =yes with =-yes at the CLI instead of adding --script-opts-append=stats-print_escape_sequences=no, no?

Obviously compound values are not the nicest, but this is a very local and specific use case, which I think should be obvious that it becomes much much less convenient for the user to apply it, so personally I'd have kept the =-yes method.

@kasper93 any opinion on print_escape_sequences? if you both think it's better than the =-yes method then I'd be OK with that. But do keep the poor user in mind.

Also, this can become moot if escape filtering becomes central. See below.

I actually prefer 4 spaces to tabs because they waste too much space.

This is a different reason than what the commit message states, which personally I don't think is valid enough.

Tab is a common indentation in the terminal, and I think it's fine as default. Whoever wants a different prefix (like yourself) can set it in an option.

Also, what if stdout is not a tty? does it still output escape sequences? If not, can it be forced anyway, because e.g. less does know to display SGR sequences (with -R).

They are outputted to less.

@kasper93 IMO if you want central tty output in mpv, then you can't do only truncation based on isatty, because following common conventions, escape sequences should be stripped as well by default if it's not a tty, at the very least SGR sequences.

You did mention "input processing" as a reason to remove io.write, so here's a chance to make it meaningful.

Therefore I think that there should be common tty output options in mpv: term-width, term-height, term-truncate, and term-strip-esc (or sgr, or whatever names, maybe with 0 or -1 value of width/height to disable any dimensions considerations).

These should be automated based on isatty or equivalent, and would still allow the user to override and/or control the specific regardless of isatty.

And if tab is broken in truncation, then it should be fixed, and it should be entirely possible. The truncation facility is public, and it's silly to not support tabs, especially when it's valid and common terminal output which shouldn't be hard to support, and when this PR replaces an existing truncation which does support tab (even if it can be problematic due to unknown prefix columns - an issue which central logging shouldn't have).

[guidocella]

So why this instead of the =-yes method? surely you agree that it's easier for the user to replace =yes with =-yes at the CLI instead of adding --script-opts-append=stats-print_escape_sequences=no, no?

Obviously compound values are not the nicest, but this is a very local and specific use case, which I think should be obvious that it becomes much much less convenient for the user to apply it, so personally I'd have kept the =-yes method.

Since the width is already removed from the bindlist I thought we can just turn bindlist into a plain boolean, and allow disabling escape sequences in stats also outside of bindlist without duplicating script-opt functionalities, but I don't have strong feelings about it and will restore -yes if you prefer.

This is a different reason than what the commit message states, which personally I don't think is valid enough.

Tab is a common indentation in the terminal, and I think it's fine as default. Whoever wants a different prefix (like yourself) can set it in an option.

It's not the reason, it's a bonus. I don't care if tab width calculation is fixed because 4 spaces look better anyway. If you want to fix it, go right ahead.

More terminal options can be added if later if someone wants to. This PR just aims at removing duplicate code and using the more accurate width detection.

[kasper93]

@kasper93 any opinion on print_escape_sequences?

No, not really.

Therefore I think that there should be common tty output options in mpv: term-width, term-height, term-truncate, and term-strip-esc (or sgr, or whatever names, maybe with 0 or -1 value of width/height to disable any dimensions considerations).

These should be automated based on isatty or equivalent, and would still allow the user to override and/or control the specific regardless of isatty.

This is outside of the scope of this PR. Patches welcome though, more filtering would be good addition if scripts want to do own formatting.

[avih]

@kasper93 any opinion on print_escape_sequences?

No, not really.

OK. Let's remove print_escape_sequences and restore/keep the =-yes.

This is outside of the scope of this PR

Correct. And these don't block this PR.

It's your idea to make it central truncation based on isatty, so it's only appropriate to not leave it half baked, and with appropriate override controls.

Regardless, the tab handling should be fixed, and this is a blocker here.

[kasper93]

It's your idea to make it central truncation based on isatty, so it's only appropriate to not leave it half baked, and with appropriate override controls.

As discussed on IRC tab width is hardcoded now to 8 in #15137

Regardless, the tab handling should be fixed, and this is a blocker here.

I don't see any \t used in this PR, so it seems unrelated to this PR.

[avih]

As discussed on IRC tab width is hardcoded now to 8 in #15137

Thanks.

I don't see any \t used in this PR, so it seems unrelated to this PR.

Tab is the default terminal ass prefix. Originally the PR replaced it with 4 spaces, but I requested to restore the tab, so now that part is unmodified and doesn't show at the changes, but the terminal output (which now gets clipped) does contain tabs by default.

[kasper93]

Originally the PR replaced it with 4 spaces, but I requested to restore the tab, so now that part is unmodified and doesn't show at the changes, but the terminal output (which now gets clipped) does contain tabs by default.

no_ass_indent is set to 4 spaces currently. See https://github.com/mpv-player/mpv/pull/15052/files#diff-31996b4d49cf0be804c8d510283bcebfa9a245b63a68bcba1d6e9126f41a2688R83

Which is closer in-line with ass_indent which is 5 spaces, which depending on the font with monospaced one looks more like 3 spaces, but 4 are good too. This makes it look uniform with terminal and ass output. To quote @guidocella

I actually prefer 4 spaces to tabs because they waste too much space.

[avih]

I requested to restore the tab, and I misread the reply as if it was agreed, and you said you didn't see \t in the PR so I assumed it was indeed restored to use tab, but now I looked at the updated commits, and I see that the PR still replaces \t with 4 spaces, so indeed by default it's without tabs currently at the PR.

no_ass_indent is set to 4 spaces currently ... Which is closer in-line with ass_indent which is 5 spaces, which depending on the font with monospaced one looks more like 3 spaces, but 4 are good too. This makes it look uniform with terminal and ass output. To quote @guidocella

I actually prefer 4 spaces to tabs because they waste too much space.

I still think tab is better as default, because it's a common indentations at the terminal, and it's also configurable by the user.

But looks like you two think 4 is better, so whatever.

That being said, at least don't hide it between unrelated changes. Changing default values should be in a commit of its own so that it's easy to identify when scanning the log.

[guidocella]

Yeah I also like that 4 spaces is closer to ass_indent, also users can still configure no_ass_indent.

[avih]

Thanks.