mpv-player / mpv

🎥 Command line video player
https://mpv.io
Other
26.76k stars 2.84k forks source link

loadfile: improve the format of terminal track information #14192

Closed guidocella closed 1 month ago

guidocella commented 1 month ago

Make terminal output consistent with the symbols introduced in 14462dafe4.

There is no need to print ◌ for unselected tracks for alignment since terminal text is always monospace.

na-na-hi commented 1 month ago

* and aren't necessarily the same width.

kasper93 commented 1 month ago

looking at this, we could also do minor alignment fixes

diff --git a/player/loadfile.c b/player/loadfile.c
index 86d4f7cf48..31d052cdbe 100644
--- a/player/loadfile.c
+++ b/player/loadfile.c
@@ -236,6 +236,7 @@ static void uninit_demuxer(struct MPContext *mpctx)
 }

 #define APPEND(s, ...) mp_snprintf_cat(s, sizeof(s), __VA_ARGS__)
+#define FILL(s, n) mp_snprintf_cat(s, sizeof(s), "%-*s", n, "")

 static void print_stream(struct MPContext *mpctx, struct track *t)
 {
@@ -261,10 +262,13 @@ static void print_stream(struct MPContext *mpctx, struct track *t)
         if (forced_opt)
             forced_only = t->selected;
     }
-    APPEND(b, " %s %-5s", t->selected ? (forced_only ? "*" : "●") : " ", tname);
-    APPEND(b, " --%s=%d", selopt, t->user_tid);
-    if (t->lang && langopt)
-        APPEND(b, " --%s=%s", langopt, t->lang);
+    APPEND(b, "%s %-5s", t->selected ? (forced_only ? "*" : "\xE2\x97\x8F") : " ", tname);
+    APPEND(b, " --%s=%-2d", selopt, t->user_tid);
+    if (t->lang && langopt) {
+        APPEND(b, " --%s=%-7s", langopt, t->lang);
+    } else {
+        FILL(b, 16);
+    }
     if (t->default_track)
         APPEND(b, " (*)");
     if (t->forced_track)
@@ -282,13 +286,16 @@ static void print_stream(struct MPContext *mpctx, struct track *t)
     if (t->type == STREAM_VIDEO) {
         if (s && s->codec->disp_w)
             APPEND(b, " %dx%d", s->codec->disp_w, s->codec->disp_h);
-        if (s && s->codec->fps)
-            APPEND(b, " %.3ffps", s->codec->fps);
+        if (s && s->codec->fps) {
+            char *fps = mp_format_double(NULL, s->codec->fps, 4, false, false, true);
+            APPEND(b, " %s fps", fps);
+            talloc_free(fps);
+        }
     } else if (t->type == STREAM_AUDIO) {
         if (s && s->codec->channels.num)
-            APPEND(b, " %dch", s->codec->channels.num);
+            APPEND(b, " %d ch", s->codec->channels.num);
         if (s && s->codec->samplerate)
-            APPEND(b, " %dHz", s->codec->samplerate);
+            APPEND(b, " %d Hz", s->codec->samplerate);
     }
     APPEND(b, ")");
     if (s && s->hls_bitrate > 0)
github-actions[bot] commented 1 month ago

Download the artifacts for this pull request:

Windows * [mpv-i686-w64-mingw32](https://nightly.link/mpv-player/mpv/actions/artifacts/1572102485.zip) * [mpv-x86_64-w64-mingw32](https://nightly.link/mpv-player/mpv/actions/artifacts/1572107002.zip) * [mpv-x86_64-windows-msvc](https://nightly.link/mpv-player/mpv/actions/artifacts/1572129032.zip)
macOS * [mpv-macos-12-intel](https://nightly.link/mpv-player/mpv/actions/artifacts/1572106802.zip) * [mpv-macos-13-intel](https://nightly.link/mpv-player/mpv/actions/artifacts/1572105481.zip) * [mpv-macos-14-arm](https://nightly.link/mpv-player/mpv/actions/artifacts/1572098844.zip)
kasper93 commented 1 month ago

* and aren't necessarily the same width.

They should be in monospaced font, no?

na-na-hi commented 1 month ago

They should be in monospaced font, no?

There is the issue with using an East Asian font as the primary font, which has monospace ASCII characters but with this glyph (and some other glyph such as kanji) being double width.

guidocella commented 1 month ago

We can use ∗ which is also wider in Japanese fonts.

guidocella commented 1 month ago

Do we need both the asterisk and [F] with --sub-forced-events-only?

kasper93 commented 1 month ago

Do we need both the asterisk and [F] with --sub-forced-events-only?

It was added here https://github.com/mpv-player/mpv/commit/d8bd1c35ef6fb397a8ad2987cc41bab805f43e6c not sure how useful it is. To print [F] and (*)

Dudemanguy commented 1 month ago

--sub-forced-events-only is not worth showing imo. Nobody uses the option in the first place (except by mistake when thinking it had something to do with selecting forced tracks) and you'll struggle to find a file that actually would use a mix of forced and non-forced events.

guidocella commented 1 month ago

Nuked it.

guidocella commented 1 month ago

I made this leave spaces only if at least a track has a language, because it looked silly when none did:

● Video --vid=1                  [P] 'cover.jpg' (mjpeg) (external)
● Audio --aid=1                  (mp3 2 ch 48 kHz)
● Subs  --sid=1                  '8 Hallowed Be Thy Name.lrc' (text) (external)

I also hid the fps of images like I did in stats.lua and select.lua, as it's just a bogus value taken from --mf-fps.

guidocella commented 1 month ago

Can we remove the quotes around track titles?

kasper93 commented 1 month ago

I think they are good to distinguish title from other fields.

kasper93 commented 1 month ago

If is a problem, we could mark selected elements with bold or some color. In case we are outputting to tty.

guidocella commented 1 month ago

Bold or colored tracks seem too invasive, and the escapes sequence show as garbled in the console log.

kasper93 commented 1 month ago

Bold or colored tracks seem too invasive, and the escapes sequence show as garbled in the console log.

Console log can be fixed, as for the rest it is probably matter of preference, I've seen nice cli interfaces with colors.

guidocella commented 1 month ago

I tried to make unselected tracks grey instead.

guidocella commented 1 month ago

There is also an escape sequence to make text invisible: "\e[8m○\e[0m" fixes the alignment on mlterm and the clutter and differentiates selected tracks without UTF-8 support.

na-na-hi commented 1 month ago

Both grey color and invisible escape sequence look good to me.

kasper93 commented 1 month ago

Either way is fine for me too. Circles works better for log, but could use circles if !tty and gray if tty, but I don't have strong opinion of either as long it also marks them in log somehow.

guidocella commented 1 month ago

--log-file still logs the isatty path output.

sfan5 commented 2 weeks ago

I think printing unicode circles is useful for OSD text but there's nothing wrong with the simple ASCII representation for terminal usage.

As for the spacing: I think it made sense that tracks are indented. Just like the File tags: section.