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.

Sign up for GitHub

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

Jump to bottom

stats.lua: display file tags #13913

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

stats.lua: display file tags #13913

wants to merge 3 commits into from
+43 −15

Conversation

na-na-hi
Copy link
Contributor

This adds file tags to display along with the title, including album/artist etc. for music, and series etc. for some videos. The list of tags to display is identical to the tags printed to the terminal and is controlled by the --display-tags option.

@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 17, 2024 •
edited

Download the artifacts for this pull request:

Windows
macOS

@guidocella
Copy link
Contributor

You can remove 1 line by looping through filtered-metadata.

@na-na-hi
Copy link
Contributor Author

You can remove 1 line by looping through filtered-metadata.

This cannot be done because the entries in filtered-metadata are unordered. I want the entries to be displayed in the same order specified by the --display-tags option, like what it does for the terminal printing.

@guidocella
Copy link
Contributor

I see. You may also want to truncate the fields for who uses --display-tags-append=ytdl_description and for song lyrics embedded in metadata.

@na-na-hi
Copy link
Contributor Author

You may also want to truncate the fields for who uses --display-tags-append=ytdl_description and for song lyrics embedded in metadata.

I added an option to filter out overlength tags, currently the default limit is 128 bytes but can be customized. I don't think it's worthwhile to hardcode some arbitrary lists of metadata to truncate, which is also problematic in itself since Lua before 5.3 has no built-in UTF-8 utils.

@kasper93
Copy link
Contributor

We don't have much vertical space on page 1 of stats.lua, do we really want to add non determined number of lines based on input file?

@na-na-hi
Copy link
Contributor Author

Added another option for the maximum amount of tags to be displayed. I would like to at least display artist and album if available, so I set the limit to 3 tags for now.

@kasper93
Copy link
Contributor

kasper93 commented Apr 17, 2024 •
edited

I was thinking about adding new page with track info.

Page 1 would be reserved for technical parameters like display/decoding etc.
New page would be used for things from track-list like track language, name, description and tags would go on this page.

guidocella
guidocella reviewed Apr 17, 2024
View reviewed changes
player/lua/stats.lua Outdated Show resolved Hide resolved
@na-na-hi
Copy link
Contributor Author

Page 1 would be reserved for technical parameters like display/decoding etc.

It's already displaying the title. I'm OK if you want to move it to a new page but I would like to keep all tags (including title) in one place.

New page would be used for things from track-list like track language, name, description and tags would go on this page.

Tags belong to files, not tracks, so I don't think they belong to the track info section. If you want metadata on a new page, it's better for the page to have separate metadata and track info sections.

@kasper93
Copy link
Contributor

Tags belong to files, not tracks, so I don't think they belong to the track info section. If you want metadata on a new page, it's better for the page to have separate metadata and track info sections.

Exact split is to be agreed, but I was thinking

File:
...

Video Track:
...

Audio Track:
...

Subtitle Track:
...

Subtitle Track:
...

and remove file from page 1, maybe move page 1 to page 2 and make page 1 new page with more generic file/current track info.

Not sure if such page would be useful, maybe not, just thinking how we can display more things. Page 1 is already dense, so we should look at other solutions.

@na-na-hi
Copy link
Contributor Author

IMO page 1 should remain displaying mostly dynamic info (aka stats in "stats".lua). Moving the file section away would give us more space for actual stats on page 1 so I think moving the section to a new page for file/track info is a good plan.

@kasper93 kasper93 added the priority:needs-author-feedback The original author of the issue/PR needs to come back and respond to something label Apr 18, 2024
@na-na-hi
Copy link
Contributor Author

@kasper93
Since you're interested to add track info, I currently have the following high-level plan for stats.lua reorganization after some consideration:

  • Page 1: This displays mostly non-file information about the playback, like display and sound. For the sound section we can put more detailed AO info there. The info of the current file are moved to page 5.
  • Page 2-4: Unchanged.
  • Page 5 (new): This displays the info of the current file. Think it as the mpv equivalent of MediaInfo. This means we put the file info, video info, audio info, and subtitle info there, for all tracks available.

I am thinking about merging this PR first, considering that I have implemented a limit on the tags displayed. Then you can open a PR to add the track info and move the file info on page 1 to that page. What do you think of this plan?

@Argon-
Copy link
Member

Argon- commented Apr 20, 2024

@kasper93 Since you're interested to add track info, I currently have the following high-level plan for stats.lua reorganization after some consideration:

  • Page 1: This displays mostly non-file information about the playback, like display and sound. For the sound section we can put more detailed AO info there. The info of the current file are moved to page 5.

  • Page 2-4: Unchanged.

  • Page 5 (new): This displays the info of the current file. Think it as the mpv equivalent of MediaInfo. This means we put the file info, video info, audio info, and subtitle info there, for all tracks available.

Sounds reasonable.

I am thinking about merging this PR first, considering that I have implemented a limit on the tags displayed. Then you can open a PR to add the track info and move the file info on page 1 to that page. What do you think of this plan?

Given the current state of things (i.e., we cannot detect if the text overflows the screen - or can we now?) you can always create (or stumble upon) a situation where the stats overflow/cut off, no matter how low your limit is. For a workaround, what about enabling scrolling for page one so users can still work with it if worst comes to worst (additional to your limit)? I don't think it's good UX to expect users to tweak with configuration options in such cases (e.g., display-tags).

@na-na-hi
stats.lua: display file tags
06413df 
This adds file tags to display along with the title, including
album/artist etc. for music, and series etc. for some videos.
The list of tags to display is identical to the tags printed to
the terminal and is controlled by the --display-tags option.
@na-na-hi
stats.lua: add file_tag_max_length and file_tag_max_count options
d8924b0 
To filter out overlength tags (such as long comments and lyrics) and
files with too many tags, add file_tag_max_length and file_tag_max_count
options so that tags longer than this length are not displayed, and only
the first few tags are displayed.
@na-na-hi
stats.lua: make page 1 scrollable
d64cf0e 
This makes page 1 scrollable with up and down keys.

The trick here to avoid a large append parameter refactoring is to
make the append function add the table index only if the newline
character is not empty. Otherwise, new strings are appended to the
existing string.
@na-na-hi
Copy link
Contributor Author

For a workaround, what about enabling scrolling for page one so users can still work with it if worst comes to worst (additional to your limit)?

Good suggestion. I added a commit to make page 1 scrollable.

@kasper93
Copy link
Contributor

I am thinking about merging this PR first, considering that I have implemented a limit on the tags displayed. Then you can open a PR to add the track info and move the file info on page 1 to that page. What do you think of this plan?

@na-na-hi: It is very good idea, but I'm not interested in making a new page myself. If I wasn't clear, I don't think there is space on page 1 for more lines (3+). I think we can spare 1 line more, but that's all for this page.

Good suggestion. I added a commit to make page 1 scrollable.

I think page 1 should be designed in a way to fit on one screen.

@na-na-hi
Copy link
Contributor Author

na-na-hi commented Apr 21, 2024 •
edited

but I'm not interested in making a new page myself.

You will need to do so anyway for #13866 according to your own limit, won't you? Either in this PR or that one, someone needs to do the job eventually.

I think page 1 should be designed in a way to fit on one screen.

It is still useful when someone changes the default font size, the vidscale option is set to false, or the text overflows the screen.

@Argon-
Copy link
Member

Argon- commented Apr 22, 2024

It is still useful when someone changes the default font size, the vidscale option is set to false, or the text overflows the screen.

Definitely agree. With the right file, you could make it overflow the screen with default font settings even years ago and this hardly got any better over time. ;)

@kasper93 kasper93 added priority:on-ice may be revisited later and removed priority:needs-author-feedback The original author of the issue/PR needs to come back and respond to something labels May 1, 2024
@kasper93 kasper93 mentioned this pull request May 15, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guidocella guidocella guidocella left review comments

Assignees
No one assigned
Labels
priority:on-ice may be revisited later scripting:stats
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@na-na-hi @guidocella @kasper93 @Argon-