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

0.9.10 reloaded - load support for file and in memory content #32

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

Conversation

sosie-js
Copy link

Hello,

I refactorized the logic adding functions
-load(path) : Load file content or content
-input(path) ; change the input path
-output(path): change the output path
-validate(data) : true if data is a valid ass content
-add_pyonfx_extension() launch calculations to extend data

and cover security checks,
-when no file is given, we default to the Aegisub default Untitled.ass . (Bonus)
-when no video is attached, play_res_x or play_res_y are missing from info section,
prevents add_pyonfx_extension crash.

Example 0 has been added for demo in Basic Examples. Enjoy!

@CoffeeStraw
Copy link
Owner

Thank you very much for your PR! Reading the changelog only, everything sound reasonable. Unfortunately, I'm not able to read the code/propose changes before a week.

Feel free to keep working on this meanwhile, if there is anything else!

@CoffeeStraw
Copy link
Owner

Hey, I'm again with my laptop and I've reviewed your PR.
There are some interesting ideas you've implemented to better organize the code, but overall I think the implementation could be done better.

Here's what I propose:

  • Rename "input" function in "set_input" (and so "output" in "set_output");
  • Rename "load" function in "parse_ass";
  • Remove "add_pyonfx_extension" and move it back in "parse_ass", I don't like it to be separated nor I find it usefull;
  • Delete "validate" function. Also, as mentioned in issue LBYL -> EAFP #29, we should follow EAFP approach, so we should avoid all the checks whenever possible;
  • "Untitled.ass" when no input is specified does not work. Instead, we should hardcode an empty .ass and use it as input when no one is specified. I can see the use-case of this option, as one could want to just generate new lines;
  • I should investigate whether it is really not possible to extract any information from lines if no video is specified, I'm not totally sure. Thank you anyway for pointing out this issue!

I hope I've written everything I thought. Feel free to point out other changes, I will also contribute to your branch to speed up the process (but this could take some weeks as I'm busy now).

@sosie-js
Copy link
Author

sosie-js commented Aug 12, 2021

Here's what I propose:

Rename "input" function in "set_input" (and so "output" in "set_output");
Rename "load" function in "parse_ass";

Accepted.

Remove "add_pyonfx_extension" and move it back in "parse_ass", I don't like it to be separated nor I find it usefull;

As just ass creation without karaoke, the extension is not needed, so i kept it

Delete "validate" function. Also, as mentioned in issue

ok i integrated the test

LBYL -> EAFP #29, we should follow EAFP approach, so we should avoid all the checks whenever possible;

It's your religion or politic , I choose FREEDOM ;p

"Untitled.ass" when no input is specified does not work. Instead, we should hardcode an empty .ass and use it >as input when no one is specified.

This is the purpose of Untitled.ass, matching the spirit of handling it in lua with aegisub ; yes it was buggy, now it is fixed

I can see the use-case of this option, as one could want to just generate new lines;

Here is a common usage of it with an exporter:

# coding:utf8

# messages2ass.py,   dictionary to subtitle exporter to automate translation
# author sosie-js
# require my pythonfx mofied version adding fixes and del_line facility
#==========================================

# For relative imports to work
import os, sys; sys.path.append(os.path.dirname(os.path.realpath(__file__)))
from pyonfx import *

# will include message_en.py a dictiatary of English messages for the syncplay gui
# https://raw.githubusercontent.com/Syncplay/syncplay/master/syncplay/messages_en.py
import messages_en

dict_message_file="message_en" #.py
ass_message_file=dict_message_file+".ass"
print("Welcome on the %s dictionary to ass file %s exporter" % (dict_message_file,ass_message_file) )
print("-------------------------------------------------------------------------------------")
 
io = Ass() #Will use aegisub template Untitled.ass as basis instead of "in.ass"
io.set_output(ass_message_file)
meta, styles, lines = io.get_data()

#messages will hold the message dict
messages=messages_en.en
i=len(lines)
pos_time=0

#the fist line of Untitled.ass, empty, will serve as template
line= lines[0].copy()
duration=2000

for key, value in messages.items():
    print("Exporting value of key %s as subtiyle line" % key)
    l= line.copy()
    i=i+1
    l.i=i
    l.start_time = pos_time
    l.end_time = pos_time+duration
    l.text =value
    io.write_line(l)
    pos_time=pos_time+duration

#Don't forget to remove the pollution lines of the template 
# in our case remove the empty single line of Untitled.ass.
io.del_line(1)   

io.save()
io.open_aegisub()

Download:
messages2ass.py.txt

I should investigate whether it is really not possible to extract any information from lines if no video is specified, >I'm not totally sure. Thank you anyway for pointing out this issue!

I explained why on the line, if (not hasattr(self.meta,"play_res_x") or not hasattr(self.meta,"play_res_y")):
it depends on playres_x ans playres_y available only in the ass file if video was provided
This the case thus when dummy video is choosen

I hope I've written everything I thought. Feel free to point out other changes, I will also contribute to your branch >to speed up the process (but this could take some weeks as I'm busy now).

Bunny Busy too ;p

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.

None yet

2 participants