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

Merge function should merge/delete identical styles #23

Open
Ninelpienel opened this issue Mar 14, 2023 · 5 comments · May be fixed by #24
Open

Merge function should merge/delete identical styles #23

Ninelpienel opened this issue Mar 14, 2023 · 5 comments · May be fixed by #24

Comments

@Ninelpienel
Copy link

Hey, I merged two files with the same styles and noted that subdigest just put all styles of my second script under the styles of my fist one, although they are the same. Would be cool if subdigest could recognize such cases.

image

@Ninelpienel Ninelpienel changed the title Merge function should merge identical styles Merge function should merge/delete identical styles Mar 14, 2023
@FichteFoll
Copy link

Could you please share the command/code that was used to invoke subdigest?

@Ninelpienel
Copy link
Author

subdigest.exe -i "%name%-%tag%_!episode!_prass.ass" --merge-file "%name%-%tag%-forced_!episode!_type.ass" -o "%name%-%tag%_!episode!_merged.ass"

@FichteFoll
Copy link

FichteFoll commented Mar 14, 2023

Ideally also the two files that were used to create this one (just the styles and one line of dialogue would suffice) because subdigest in general already does deduplicate styles from multiple files but only if they are identical. If they are not identical, it can be debated what exactly subdigest should do instead (error and abort, ignore the second style, overwrite the first style) and I would err on the save side with erroring, but maybe @Myaamori can share what he had in mind here initially.

def merge_file(self, other_file: argparse.FileType(encoding='utf-8-sig')) -> Subtitles:
"""Append the styles and event lines from another file."""
f = ass.parse(other_file)
self.sub_file.events.extend(f.events)
existing_styles = {style.name: style.dump() for style in self.sub_file.styles}
for style in f.styles:
if style.name in existing_styles and style.dump() != existing_styles[style.name]:
print(f"Warning: Ignoring style {style.name} from "
f"{other_file.name}.", file=sys.stderr)
continue
self.sub_file.styles.append(style)
return self

Edit: I see a problem in this snippet that should occur when they are indeed identical. Will take a look at properly fixing it when I'm back from vacation, probably.

@Ninelpienel
Copy link
Author

Here are the files.

ass.zip

FichteFoll added a commit to FichteForks/Myaamori-Aegisub-Scripts that referenced this issue Mar 22, 2023
@FichteFoll FichteFoll linked a pull request Mar 22, 2023 that will close this issue
@FichteFoll
Copy link

FichteFoll commented Mar 22, 2023

I've confirmed that the styles (and the rest of the metadata in theses files) are indeed exactly identical and sub-digest should not include both. I implemented the fix for it in #24.

However, I'm afraid that @Myaamori seems to have been AWOL for over a year now with no activity on GitHub or on any of the Discord servers that we share. I may or may not ask someone from the admins of this org if they could provide me with access to this repository to at least provide some level of maintenance for the tools I care about/have knowledge of, i.e. the Python scripts (and the merge scripts Automation). Though, I also believe that both of these tools should rather be in separate repositories and not included inside this one initially meant for Automation scripts. Especially the size of the readme should be an indicator of that.

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 a pull request may close this issue.

2 participants