More feature integration with mpv

[i-blis]

Despite being alpha, subed is a pleasure to use. I've even never had any crash yet. Thanks for creating it :)

I've been tinkering with your code, to discover that most of what I needed was already baked in. I still had a couple of requirements to ease my workflow and hacked a couple of functions out. There are certainly not worth a PR but I am sharing them nonetheless.

1. Insert mpv player's current timestamp as current subtitle's stop timestamp

This is pretty useful when after creating the sub at a given point, you let the video flow and stop it where the subtitle should disappear.

(defun ego/subed-insert-player-stop-timestamp ()
  "Replace current subtitle's stop timestamp with mpv player's current timestamp."
  (interactive)
  (subed-srt--jump-to-subtitle-time-stop)
  (when (and subed-mpv-playback-position
         (looking-at subed-srt--regexp-timestamp))
    (replace-match (subed-srt--msecs-to-timestamp subed-mpv-playback-position))
    (subed--run-subtitle-time-adjusted-hook)))

I never needed the same for the start timestamp yet.

2. Amend current start and stop timestamp to match previous one

When adding a new subtitle between two existing ones, I mostly need it to be next to the previous one, as if I created it at the end of the file. Currently subed fits smartly the newly created subs between the two. Meaning that, if I add a single one, it will match the next sub's time stamp. The following function allows the change the timestamp to make it fit the previous one.

(defun ego/subed-have-current-sub-timestamp-match-previous ()
  "Changes the current start and stop timestamp to match the previous one according to default spacing and length settings"
  (interactive)
  (let* ((current-id  (subed-srt--subtitle-id))
     (previous-id (- current-id 1)))
    (when (> previous-id 0)
      (subed-srt--jump-to-subtitle-id previous-id)
      (let* ((prev-stop-ts (subed-srt--subtitle-msecs-stop))
         (new-start-ts (+ prev-stop-ts subed-subtitle-spacing)))
    (subed-srt--jump-to-subtitle-id current-id)
    (subed-srt--jump-to-subtitle-time-start)
    (when (looking-at subed-srt--regexp-timestamp)
      (replace-match (subed-srt--msecs-to-timestamp new-start-ts))
      (subed-srt--jump-to-subtitle-time-stop)
      (when (looking-at subed-srt--regexp-timestamp)
        (replace-match (subed-srt--msecs-to-timestamp
                (+ new-start-ts (* 1000 subed-default-subtitle-length)))))
      (subed--run-subtitle-time-adjusted-hook))))))

3. Insert subtitle with player's timestamp

This one is particularly useful for inserting a subtitle at the end, when you skipped a long way forward without subtitles.

(defun ego/subed-insert-with-player-timestamp ()
  (interactive)
  (when-let* ((current-position subed-mpv-playback-position))
    (subed-srt--jump-to-subtitle-id)
    (subed-srt--jump-to-subtitle-end)
    (forward-line)
    (insert "\n")
    (insert (format "0\n%s --> %s\n\n\n"
                    (subed-srt--msecs-to-timestamp current-position)
                    (subed-srt--msecs-to-timestamp (+ current-position
                              (* 1000 subed-default-subtitle-length)))))
    (subed-srt--sort) ;; regenerate-ids seems to be enough in all cases, still keeping it
    ;; (subed-srt--regenerate-ids)
    (subed-srt--jump-to-subtitle-text)))

I also made basic commands to rewind and forward mpv frame by frame.

Probably most of them are not worth incorporating to subed but who knows. I thought it might be useful to share them.

I'd be happy to contribute, if you open issues with new feature requests for instance. I can provide one of these as a PR if needed.

[rndusr]

I've been busy developing subed and didn't have any time to actually use it, so I'm very happy to know I have actual users and about any kind of feedback. Thank you very much for yours! Also, this is my first Elisp project, so if you see anything that seems funky to you, it's probably because of that, and I'm open to suggestions.

1. Grabbing the current playback time from the player has been on my todo list for a long time. I'll bump it up now that I know people actually use this.

2. I'll take a closer look at that tomorrow.

On a side note: This can be shortened from

(subed-srt--jump-to-subtitle-id current-id)
(subed-srt--jump-to-subtitle-time-start)

to

(subed-srt--jump-to-subtitle-time-start current-id)

3. This should be redundant:

(subed-srt--jump-to-subtitle-id)
(subed-srt--jump-to-subtitle-end)

You should be able to jump to the end of a subtitle from anywhere. If that doesn't work, it's a bug.

On first glance, all your additions seem worthy to be incorporated upstream instead of leaving the burden of implementing them on the user. Maybe they don't all need default keybindings, but the functions should be there. Not every Emacs user speaks Elisp.

Talking about default keybindings: Do you like them? I've been struggling setting them up, and because I've never really used subed, I'm still not quite sure if they are any good.

Regarding those issues for contributors, I'll make my local todo list public on GitHub tomorrow. I'd be happy to accept PRs, but I always think everyone who could contribute is busy making their own tools (as I am).

[i-blis]

Thanks for your answer and tips.

Regarding the key bindings, I did not remap even a single one. I mapped the three functions above respectively to C-M-s, C-M-S-s and C-M-i. I do think too that this is best left to the user.

I've been using subed heavily during a couple of days for a project at work and to say the truth I did not encounter a single bug yet.

I'd be happy to help with contributions if I can and I'll try to match your coding style as much as possible.

[rndusr]

It's really great to hear that you didn't encounter any issues! I guess that means I can remove the alpha warning.

I opened two issues you can work on if you really want. Feel free to add your own or open PRs for the stuff you mentioned earlier; you probably know better than me what is missing as I only edit subtitles occasionally. But Emacs not having a proper subtitle editor is not an acceptable situation, so I bit the bullet.

Regarding the coding style, please follow the official coding conventions: https://www.gnu.org/software/emacs/manual/html_node/elisp/Coding-Conventions.html#Coding-Conventions I've spent much of the weekend to get the code up to snuff because I didn't really care that much in the beginning, and when I submitted a packaging PR with Melpa, I learned they take this more seriously. I hope I found the worst stuff, but there might still be style breaches and you shouldn't copy them.

[rndusr]

@i-blis, I'm taking a closer look at ego/subed-have-current-sub-timestamp-match-previous, and I think it would be best to drop all the clever code that makes inserted subtitles as long as possible. Instead inserted subtitles should just be subed-default-subtitle-length long, regardless of how much space is available.

We would still need to be smart to make them shorter when there isn't enough space, but I think inserting a three minute subtitle because is a three minute gap is just annoying. What do you think?

---

Also, should the default value of subed-default-subtitle-length be increased to something like 1500 or even 2000? Most subtitles are probably much longer than 1 second, which means it takes longer to fix them with the default 100ms steps.

[rndusr]

I just finished reimplementing the insertion code. Each subtitle format implementation now only needs to provide very simple functions that append or prepend single subtitles while all the complex argument parsing and time stamp calculations are done generically.

While doing this, I also implemented my idea from above: Subtitles are now always subed-default-subtitle-length milliseconds long and evenly spaced between two subtitles.

I'll also add some way of adding subtitles immediately after/before the current subtitle instead of spreading them out. Probably with another generic function. I'll have to think more about that. Once that's done, that should make your function #2 redundant.