arch1t3cht / Aegisub

Cross-platform advanced subtitle editor, with new feature branches. Read the README on the feature branch.
http://www.aegisub.org
Other
755 stars 34 forks source link

Don’t truncate parsed times >=10h #104

Closed TheOneric closed 8 months ago

TheOneric commented 9 months ago

Such times are accepted by all modern renderers and with ffmpeg and mkvtoolnix it appears muxers support it as well.

Testing the new code, everything still seems to be fine:

    //  11:42:42.421 =  42162421 ms
    agi::Time time = agi::Time(42162421);
    std::cout << " 11:42:42.421 ASS = " << time.GetAssFormatted() << std::endl;
    std::cout << " 11:42:42.421 ASS = " << time.GetAssFormatted(true) << std::endl;
    std::cout << " 11:42:42.421 SRT = " << time.GetSrtFormatted() << std::endl;
    std::cout << "Parsed from string: " << agi::Time("11:42:42.421").GetSrtFormatted() << std::endl;
    // 111:00:00.001 = 399600001 ms
    time = agi::Time(399600001);
    std::cout << "111:00:00.001 ASS = " << time.GetAssFormatted() << std::endl;
    std::cout << "111:00:00.001 SRT = " << time.GetSrtFormatted() << std::endl;
    std::cout << "Parsed from string: " << agi::Time("111:00:00.001").GetSrtFormatted() << std::endl;
    //   1:42:42:120 =   6162120 ms
    time = agi::Time(6162120);
    std::cout << "  1:42:42.120 ASS = " << time.GetAssFormatted() << std::endl;
    std::cout << "  1:42:42.120 SRT = " << time.GetSrtFormatted() << std::endl;
    std::cout << "Parsed from string: " << agi::Time("01:42:42.120").GetSrtFormatted() << std::endl;
 11:42:42.421 ASS = 11:42:42.42
 11:42:42.421 ASS = 11:42:42.421
 11:42:42.421 SRT = 11:42:42,421
Parsed from string: 11:42:42,421
111:00:00.001 ASS = 111:00:00.00
111:00:00.001 SRT = 111:00:00,001
Parsed from string: 111:00:00,001
  1:42:42.120 ASS = 1:42:42.12
  1:42:42.120 SRT = 01:42:42,120
Parsed from string: 01:42:42,120

time_test

I tried to also deal with the input boxes, by allowing prepending a digit if the cursor is at the start and a modifier is pressed, but it appears those keyboard events don’t even reach TimeEdit::OnChar and I’m not sure how to resolve this (even with Shift as a modifier).
If there already are multi-digit hours, all digit can be edited as usual (unless the leading digit is replaced by a zero in which case it disappears).

As a hacky workaround until a proper fix: I also noticed if dead keys or an IME is involved OnChar also appears to be skipped. This can be abused to insert any character at the start and then subsequently replace it as usual with the desired digit.

(Side note: when this limit started to be enforced in 1d4c0c0712ce20fb4661e0f527223ac8d9680e55, there already was wxString::format available and used for SMPTE, so this was no reason to avoid multi-digits (anymore already))

TheOneric commented 9 months ago

Updated patch to not just lift the one-digit limitation, but to also instead enforce the < 596h limit stemming from 32-bit timestamps.

The new CI failure for MacOS and Windows seems unrelated:

Run-time dependency Boost found: NO (tried system)
Downloading boost source from https://boostorg.jfrog.io/artifactory/main/release/1.74.0/source/boost_1_74_0.tar.gz
A fallback URL could be specified using source_fallback_url key in the wrap file

meson.build:114:12: ERROR: Incorrect hash for source:
 afff36d392885120bcac079148c177d1f6f7730ec3d47233aa51b0afa4db94a5 expected
 5e89103d9b70bba5c91a794126b169cb67654be2051f90cf7c22ba6893ede0ff actual.
arch1t3cht commented 8 months ago

I could finally sit down to test this now - merged in 8fcb7c61e8577c982ca114cfcb8dde596ca7ce46, thanks a lot!

And, yeah, personally I'm not too worried about it not being possible to enter times >10h in the input boxes (certainly not enough to spend time on trying to make it work). In the very unlikely case that users do need it, they can enter frame numbers instead or use an automation script.

arch1t3cht commented 8 months ago

... aaand just as I merged this, I see a small issue: The widths of the grid columns for the start and end times don't account for times > 10h, so the times don't fit in completely. But I can just fix that myself.

TheOneric commented 8 months ago

Sorry, I missed that

Btw, if you’re worried about recalculating the column width each update, we could just always reserve enough space for MAX_TIME from libaegisub/ass/time.cpp. This will admittedly waste 4 characters for most (sub-10h) files, but relative to the total available space this doesn’t seem like a lot to me

arch1t3cht commented 8 months ago

Yeah, that was my backup solution if this turns out to cause bigger problems. But my thinking was that it shouldn't contribute too much, since there are already other O(#events) functions run per commit, like uploading the new file to the subtitle renderer.