Define subed-srt-mode

[riscy]

For your consideration re: https://github.com/melpa/melpa/pull/6255 -- this is how major modes should be defined, and we can use Emacs's derived-modes to help with the abstraction layer handling.

The first commit tries to remove any unaliased mention of subed-srt.el from subed.el -- I tried to keep it from the next (very large) commit.

The second commit reverses the dependency graph so that subed-srt.el relies on subed.el:

• subed.el now properly defines subed-mode as a major mode. Because subed-mode is now a proper mode, there is no need to run its hooks explicitly. The expectation is this mode will be derived from (per the abstraction layer).
• All defaliases are now defvar-locals, and they are nil -- the expectation is that the derived mode will set these. This necessitates calling functions using funcall, because these are now variables (that point to functions) and not actually functions.
• subed-srt.el defines subed-srt-mode, which derives from subed-mode, even using a defvar to grab its keybindings.

[riscy]

I'll add that this is a fairly large and dangerous change -- I recommend testing!

[rndusr]

How idea how I missed so many srt-specific stuff in subed.el. Quite embarrassing. But your solution isn't proper because the timestamp regexp is specific to srt. Other subtitle formats may have different timestamp formats or even no timestamps at all. Instead, subed-copy-player-pos-to-start/stop-time should be moved to subed-srt.el.

Your second commit sounds great. I'll have to read up on derived modes and take a closer look. Hopefully, I won't have to rewrite all my tests.

[rndusr]

I'm currently fixing all the tests and I've noticed that it's unclear whether a function must be called with funcall or not. For example, subed-jump-to-subtitle-id must be called with funcall, but subed-adjust-subtitle-time-start must not, and there is nothing that indicates this.

It also feels "unclean" and "hacky", but that may be because of my lack of Elisp experience.

It looks like buffer-local functions don't exist and all functions are global, correct?

Is there any other way to call the generic functions normally?

[rndusr]

Another issue I've found is that, in function docstrings, \\[...] doesn't translate to the keybinding anymore, e.g. \\[subed-move-subtitle-forward] would translate to C-M-n, but now it translates to M-x subed-move-subtitle-forward.

The help system is also broken: You can't C-h f subed-move-subtitle-forward anymore, because subed-move-subtitle-forward is a variable, not a function.

[rndusr]

Another issue: subed-mode isn't enabled automatically anymore when I'm opening a srt file. I've tried moving

(add-to-list 'auto-mode-alist '("\\.srt\\'" . subed-srt-mode))

to subed.el, but that doesn't work because subed-srt-mode isn't defined.

When I (require 'subed-srt) in subed.el, I get a "Recursive load" error.

At the moment, I can't get subed working at all.

I can run subed-mode just fine, but that doesn't load any of the functions in subed-srt.el and breaks horribly.

[rndusr]

I've added the melpa branch so I can keep working on master. Please use that for any future PRs.

[riscy]

But your solution isn't proper because the timestamp regexp is specific to srt. Other subtitle formats may have different timestamp formats or even no timestamps at all

Sorry about that -- I don't have a complete understanding of the package and thought it was a variable that wasn't srt-mode-specific, here.

I've noticed that it's unclear whether a function must be called with funcall or not

If the symbol points to a variable, then you have to run funcall on it. If it points to a function, then you can call it directly -- that's the only difference. Using the nomenclature, it's because elisp is a "lisp 2" -- functions and variables sit in separate namespaces.

One thing you could do would be to wrap each of those funcalls in a function of the same name, e.g. in subed.srt you could define:

(defun subed-jump-to-subtitle-id (arg1 arg2)
    "Executes whatever function variable `subed-jump-to-subtitle-id' refers to."
    (funcall subed-jump-to-subtitle-id arg1 arg2))

It looks like buffer-local functions don't exist and all functions are global, correct?

That's correct! Also, I didn't notice the issues with the help system being unable to understand the variable-functions. Sorry about that. And of course if you dislike the behavior, this was just my way of sketching up an idea for you. You don't have to use it. You could close it and tell me I have no idea what I'm talking about. :)

subed-mode isn't enabled automatically anymore when I'm opening a srt file

I just restarted Emacs fresh and M-x load-lib'd subed-srt.el using the code in this branch, and when I opened a file called "new.srt" it switched to "Subed-SRT Mode" as expected -- let me know if this persists for you, maybe I can help debug.

I've added the melpa branch so I can keep working on master. Please use that for any future PRs

Will do -- if someone forgets, I think you can always change the target branch in GitHub before you merge.

[rndusr]

wrap each of those funcalls in a function of the same name

But then I'd have to maintain the docstrings of the general function and for each subtitle format. That's tedious, and I'm pretty sure I'd constantly forget to update every docstring when things change.

M-x load-lib'd subed-srt.el

Ah, I didn't know about load-library. That works, thanks. But have you tried using any keybindings? E.g. M-n raises (wrong-type-argument commandp subed-forward-subtitle-text). Probably because that key is bound to #'subed-forward-subtitle-text and that's not a command anymore. I guess I would have to wrap all the keybindings in lambdas. Or use the wrapper approach from above.

But I think I've found a better solution: Simply name all the public functions in subed-{srt,ass,...}.el generically, so subed-srt--forward-subtitle-text would be renamed to subed-forward-subtitle-text and stay in subed-srt.el.

I could also move the common functions to a new file called subed-common.el and move the config stuff to subed.el.

What do you think? That sould solve all the issues. The only downside is that the Emacs docs say function names should always start with the name of the file they're in. Would this case warrant an exception to this rule?

[riscy]

But then I'd have to maintain [snip]

Yeah, it's not great, but I have faith you'll make elisp bend to your will. :)

... Probably because that key is bound to #'subed-forward-subtitle-text and that's not a command anymore.

Yeah, I neglected to touch the -config.el file. :)

What do you think?

If I'm understanding the solution, you would end up with two files, subed-ass.el and subed-srt.el, and both of them would define a function called subed-forward-subtitle-text. The problem is that no matter what kind of file the user is editing, s/he would always be using the functions defined in the most recently loaded file.

As far as I can see, though, your package and your namespace (at least for the package as it is right now) are both "subed", so as long as your functions within this package all start with "subed" then you're fine.

Another alternative would be to duplicate a bit of code, either within this package or across two separate packages called "subed-srt" and "subed-ass". It's not DRY, but abstractions are really only useful if they make the code easier to work with/understand.

If you keep it all in this package, you'll find yourself factoring generic bits of code out all the time, so it won't be that bad. You could even add unit tests that try to enforce the common abstraction you're going for.

As for getting this into MELPA, my main advice is to make sure you're using Emacs conventions for defining a major mode.

[rndusr]

Another alternative would be to duplicate a bit of code

That would be a majority of the code. subed-srt.el has only 423 lines of code while the other files have 1203 lines. Definitely not an option. Maybe I misunderstood what you mean?

But my simple solution also won't work because (appart from the reason you mentioned) it would make it impossible, again, to edit different subtitle formats in the same Emacs session.

Why are there no buffer-local functions? It would make everything so clean and easy!

I guess I'll either go with your approach of storing functions in buffer-local variables and adding a wrapper function for each one or just drop support for other subtitle formats. I'll get back to you when I have something I can live with.

my main advice is to make sure you're using Emacs conventions for defining a major mode.

Am I currently violating Emacs conventions? I remember spending the better part of a weekend reading MELPA/Emacs docs and getting checkdoc and package-lint to work before submitting my packaging PR. I've read [1] at least twice while going through my code.

[1] https://www.gnu.org/software/emacs/manual/html_node/elisp/Major-Mode-Conventions.html

[riscy]

I guess I'll either go with your approach of storing functions in buffer-local variables and adding a wrapper function for each one or just drop support for other subtitle formats. I'll get back to you when I have something I can live with.

It's tricky -- sorry for the gymnastics. :)

The "conventions" for using a major mode are fixed in this PR -- in particular these lines.