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

osc: avoid showing a double-minus-sign for time remaining #15255

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rcombs
Copy link
Contributor

@rcombs rcombs commented Nov 3, 2024

Previously, if the current timestamp exceeded the nominal duration of the file (possible if the duration was incorrect, stream durations mismatched, etc), the time-remaining field would show eg "--00:00:00".

With this change, it instead shows "00:00:00".

The single minus sign is still present (to keep the field width consistent), but drawn fully transparent.

I considered showing a plus sign instead, but this ends up looking pretty awkward baseline-rendering-wise, and results in a size jump as "-" is replaced by the wider "+" if the unicodeminus setting isn't enabled.

Previously, if the current timestamp exceeded the nominal duration of the file
(possible if the duration was incorrect, stream durations mismatched, etc),
the time-remaining field would show eg "--00:00:00".

With this change, it instead shows "00:00:00".

The single minus sign is still present (to keep the field width consistent),
but drawn fully transparent.
@Dudemanguy
Copy link
Member

time-remaining should already be returning M_PROPERTY_UNAVAILABLE if the calculated time is bogus so I think we should just take that into account instead of trying to apply cosmetics?

mpv/player/command.c

Lines 853 to 854 in a61518d

if (!time_remaining(ctx, &remaining))
return M_PROPERTY_UNAVAILABLE;

@rcombs
Copy link
Contributor Author

rcombs commented Nov 4, 2024

time_remaining can (and does) output a negative number (and return true) if the playhead is beyond the demuxer-provided duration. In most cases this is just a frame or so beyond the nominal duration, but it can be longer in some cases, and I think it's still worth displaying for consistency if nothing else; "the playhead is currently 5 seconds beyond where we expected the file to end" is meaningful information.

Copy link

github-actions bot commented Nov 4, 2024

Download the artifacts for this pull request:

Windows
macOS

@Dudemanguy
Copy link
Member

Oh I see I misread the condition. I kind of disagree though. I don't see what's useful about negative numbers in the time-remaining and similar properties. Seems like just junk information to me.

local alpha = normal_alpha
if value:sub(1, 1) == '-' then
value = value:sub(2)
alpha = 255
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps prepending a + would be simpler than changing the color?

Copy link
Contributor Author

@rcombs rcombs Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See PR description; I tried this and it didn't look good. (It'd probably look better in a different font.)

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.

3 participants