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

feat: add OpenDocument thumbnail support (port #366) #545

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

CyanVoxel
Copy link
Member

This PR is a port of #366 which previously targeted thumbnails/Alpha-v9.4 to main (aka v9.5x).

This adds thumbnail/preview support for OpenDocument file types. This uses the code from #366 and allows any other OpenDocument file type to be rendered with the same method as MuseScore files.

Closes #379.

@CyanVoxel CyanVoxel added Type: Enhancement New feature or request Type: UI/UX User interface and/or user experience Priority: Low Doesn't require immediate attention Status: Review Needed A review of this is needed labels Oct 13, 2024
Comment on lines 13 to 27
def test_odt_preview(cwd, snapshot):
file_path: Path = cwd / "fixtures" / "sample.odt"
renderer = ThumbRenderer()
img: Image.Image = renderer._open_doc_thumb(file_path)

img_bytes = io.BytesIO()
img.save(img_bytes, format="PNG")
img_bytes.seek(0)
assert img_bytes.read() == snapshot(extension_class=PNGImageSnapshotExtension)


def test_ods_preview(cwd, snapshot):
file_path: Path = cwd / "fixtures" / "sample.ods"
renderer = ThumbRenderer()
img: Image.Image = renderer._open_doc_thumb(file_path)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these two tests (and the tests in #543 #540 #539 ) can be eventually DRY-ed by using pytest.mark.parametrize. It executes the test with values in the supplied list, which works great in case the only thing which changes is the fixture name (and thumbnailing method in case of the other PRs). So it can look like this:

Suggested change
def test_odt_preview(cwd, snapshot):
file_path: Path = cwd / "fixtures" / "sample.odt"
renderer = ThumbRenderer()
img: Image.Image = renderer._open_doc_thumb(file_path)
img_bytes = io.BytesIO()
img.save(img_bytes, format="PNG")
img_bytes.seek(0)
assert img_bytes.read() == snapshot(extension_class=PNGImageSnapshotExtension)
def test_ods_preview(cwd, snapshot):
file_path: Path = cwd / "fixtures" / "sample.ods"
renderer = ThumbRenderer()
img: Image.Image = renderer._open_doc_thumb(file_path)
@pytest.mark.parametrize(["fixture_file", "thumbnailer"], [
("sample.odt", ThumbRenderer._open_doc_thumb), # _open_doc_thumb needs to be changed to @classmethod
("sample.ods", ThumbRenderer._open_doc_thumb), # _open_doc_thumb needs to be changed to @classmethod
])
def test_document_preview(cwd, fixture_file, thumbnailer, snapshot):
file_path: Path = cwd / "fixtures" / fixture_file
img: Image.Image = thumbnailer(file_path)

so instead of two almost-identical tests, there will be just one, and the fixture file + thumbnailing method is passed inside via the parameters.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these two tests (and the tests in #543 #540 #539 ) can be eventually DRY-ed by using pytest.mark.parametrize. It executes the test with values in the supplied list, which works great in case the only thing which changes is the fixture name (and thumbnailing method in case of the other PRs). So it can look like this:

so instead of two almost-identical tests, there will be just one, and the fixture file + thumbnailing method is passed inside via the parameters.

I've tried merging the tests as shown here, including marking open_doc_thumb as a @classmethod, however it seems that the snapshots can not be found causing the tests to fail. Here's how I have the test set up:

@pytest.mark.parametrize(
    ["fixture_file", "thumbnailer"],
    [
        (
            "sample.odt",
            ThumbRenderer._open_doc_thumb,
        ),
        (
            "sample.ods",
            ThumbRenderer._open_doc_thumb,
        ),
    ],
)
def test_document_preview(cwd, fixture_file, thumbnailer, snapshot):
    file_path: Path = cwd / "fixtures" / fixture_file
    img: Image.Image = thumbnailer(file_path)

    img_bytes = io.BytesIO()
    img.save(img_bytes, format="PNG")
    img_bytes.seek(0)
    assert img_bytes.read() == snapshot(extension_class=PNGImageSnapshotExtension)

And the error from each run (same output as if the original tests had missing snapshots):

E       AssertionError: assert [+ received] == [- snapshot]
E         Snapshot 'test_document_preview[sample.odt-_open_doc_thumb]' does not exist!
E         + b'\x89PNG\r\n\x1

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the test was renamed, it's looking for a fixture matching the new test name + parameters, which doesnt exist yet. You have to run pytest with --snapshot-update the first time when a new test is created to generate the fixtures.

@CyanVoxel
Copy link
Member Author

I've addressed the merge conflicts by combining some of the new and old tests using the @pytest.mark.parametrize decorator. I'm not aware of a good way to pass optional parameters using this method (for extra arguments such as size), so this results in two sort-of-similar tests. Let me know if there's any way to combine these further, or if this looks good to you.

Thank you for your review so far, and your help with the tests 👍

@yedpodtrzitko
Copy link
Collaborator

I've addressed the merge conflicts by combining some of the new and old tests using the @pytest.mark.parametrize decorator. I'm not aware of a good way to pass optional parameters using this method (for extra arguments such as size), so this results in two sort-of-similar tests. Let me know if there's any way to combine these further, or if this looks good to you.

you can use functools.partial to pass a parameter into the function upfront:

from functools import partial


def foo(template, size=1000):
    print(template, size)


foo_200 = partial(foo, size=200)
foo_200('xxx')
# will print `xxx, 200`

so it will look like this:

from functools import partial

@pytest.mark.parametrize(
    ["fixture_file", "thumbnailer"],
    [
        (
            "sample.pdf",
            partial(ThumbRenderer._pdf_thumb, size=200),
        ),

@CyanVoxel CyanVoxel added Status: Mergeable The code is ready to be merged and removed Status: Review Needed A review of this is needed labels Oct 19, 2024
@yedpodtrzitko
Copy link
Collaborator

I'm not aware of a good way to pass optional parameters using this method (for extra arguments such as size)

The question for future consideration is if all the thumbnailing methods shouldnt accept the same parameters (ie. size)...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Low Doesn't require immediate attention Status: Mergeable The code is ready to be merged Type: Enhancement New feature or request Type: UI/UX User interface and/or user experience
Projects
Status: Ready for Review
2 participants