search

AnonymouX47 / term-image

Display images in the terminal with python
https://term-image.readthedocs.io
MIT License
209 stars 9 forks source link

Make term-img easier to unit test #11

Closed paw-lu closed 2 years ago

paw-lu commented 2 years ago

Is your feature request related to a problem? Please describe.

I'm having trouble actually using term-img when running unit tests, because term_img.exceptions.InvalidSize is raised when TermImage is initialized.

Describe the solution you'd like

I'm not sure what's best here, but one quick solution would be add check_width and check_height args to TermImage, so that they can be passed to set_size here

https://github.com/AnonymouX47/term-img/blob/44182ee63d0f0da281c9c57313e81dca1f3abc14/term_img/image.py#L91

Overall, I'm also wondering if the size shouldn't be validated at __init__? Might be more appropriate to evaluate at draw() time only.

Describe alternatives you've considered

I could just mock get_terminal_size to give me the size I'm trying to emulate in my tests as a fallback, but wondering if the library itself could use some way to control this.

Additional context

Just wanted to say that I've really enjoyed using this library, and want to make it the default image renderer in a cli I'm working on. Appreciate the work here!

AnonymouX47 commented 2 years ago

Thank you very much, I really appreciate the suggestion! 😃

The main reason why I designed the constructor as such was to simplify and separate functionality. I wanted the class constructor to be as simple as reasonably possible.

To explain what I mean by simplification and separation... At a point in development when I realized the need for more fine-tuned/extensive sizing control, just before I implemented TermImage.set_size(), I was thinking of just implementing that via the already-existing properties size, width and height by probably assigning a tuple of values but then realized how cloggy and complicated that would be to use. So, I the added set_size() as a separate method with the complete sizing controls and piped the existing properties through it while still keeping their functionalities as basic as they were before.

I know adding just two more parameters to __init__() probably wouldn't be too much but then, what reason would there be not to add the other parameters of set_size(), or frame_duration to __init__()? Someone using the library for the first time could easily get thrown off by the number of parameters and their maybe "complicated" mutual relationships.

I feel the constructor should only provide an interface to the basic/necessary functionalities and then extra things should be handled by other specialized interfaces via methods or properties... I guess that's why I mentioned "advanced" in the description of set_size() 🤷🏾‍♂️ 😀.

The case of unit testing seems to me as more of a special case, which would call for more than basic functionalities.

Instead of...

https://github.com/paw-lu/nbpreview/blob/4f8766309e56fa919913af85d5a3f3ad74963f6f/src/nbpreview/component/content/output/result/drawing.py#L266-L271

... what do you think about this?

        block_image = term_image.TermImage(pil_image)
        block_image.set_size(
            width=scaled_width,
            height=scaled_height,
            check_height=False,
            check_width=False,
        )
        string_image = str(block_image)
        pil_image.close()
AnonymouX47 commented 2 years ago

Also, thanks for the Additional context! 😊

AnonymouX47 commented 2 years ago

About not validating the size at __init__(), I'm not yet sure If that'll be good or not... aside eliminating exceptions at initialization, I think delaying the validation could also be quite consequential.

First of all, it introduces an inconsistency in that other means of setting the render size validate it immediately.

Also, for instance, if it isn't known ahead if a given size will be valid and various "candidate" sizes are being tried to see which of them are valid... delaying the validation of the given size could cause a problem when the size is actually valid because the image would already be drawn which might not be the user's intent.

AnonymouX47 commented 2 years ago

Going through your code again, I noticed https://github.com/paw-lu/nbpreview/blob/4f8766309e56fa919913af85d5a3f3ad74963f6f/src/nbpreview/component/content/output/result/drawing.py#L237-L239 and https://github.com/paw-lu/nbpreview/blob/4f8766309e56fa919913af85d5a3f3ad74963f6f/src/nbpreview/component/content/output/result/drawing.py#L289-L290

max_width and max_height being from rich.console.ConsoleOptions and knowing rich simply deals in textual data, I would guess the dimensions are in units of lines and columns.

Also, briefly going through DrawingDimension and CharacterDimensions, it seems most of the calculations being made are things already done by TermImage._valid_size() which is the method actually responsible for almost all size-related calculations.

If my inferences are correct, then it seems to me as though what you need exactly is the maxsize parameter of set_size()... you probably wouldn't even need width and height at all.

Here's what I propose:

        block_image = term_image.TermImage(pil_image)
        block_image.set_size(maxsize=(max_width, max_height))
        string_image = str(block_image)
        pil_image.close()

The best fitting size is automatically calculated by TermImage._valid_size() and no exception should be raised except the given maxsize is too small... even if maxsize and the resulting render size are larger than the terminal size 😃.

Please, try this and let me know what the result is.

Thanks once again.

paw-lu commented 2 years ago

Firstly, thank you so much for all the reponses here. Basically: I agree with your latest advice, .TermImage.set_size(maxsize=(max_width, max_height)) is the right fit for me—and it's what I'll be using.

More details

I see what you say about keeping __init__() simple. I'm cool with that and understand the resoning to not have an overwhelming amount of options.

The case of unit testing seems to me as more of a special case, which would call for more than basic functionalities.

I think the unit tests are where this "problem" showed up, but in reality this is a problem of being able to carefully control your image. Maybe—like me—you don't want to print it directly to the terminal, and want to output to text to do something with it. You later showed some workarounds to deal with this (TermImage.set_size())

About not validating the size at __init__(), I'm not yet sure If that'll be good or not... aside eliminating exceptions at initialization, I think delaying the validation could also be quite consequential.

I think this part I'm still not 100% sold on. I think it's still really suprising to have the size be validated if it's not clear that the user is even interested in printing to the terminal. Maybe they wish to save it to a file or—like me—want to export it to a string to process it further, and don't really care about the terminal size. Intuitively to me it would make more sense to validate when the user actually tries to render to a terminal, because they definitley care at that point.

To be clear this isn't a big deal for me, and you showed me some workarounds that work perfectly for my case. But as a user—who read the docs poorly—it was something that threw me off.

        block_image = term_image.TermImage(pil_image)
        block_image.set_size(maxsize=(max_width, max_height))
        string_image = str(block_image)
        pil_image.close()

This is perfect. You're totally right, I can get rid of both DrawingDimension and CharacterDimensions, and let img-term take care of the math here. Really appreciate this. This is what I'll be using. This not only solved my original problem, but simpliefied my class further. Big thanks!

paw-lu commented 2 years ago

Ended up implimenting what we discussed. As you can see, a lot of code removed—so (again) thanks a lot for the guidance!

AnonymouX47 commented 2 years ago

You're very welcome! 😃

I think it's still really suprising to have the size be validated if it's not clear that the user is even interested in printing to the terminal.

True 🤔... For some reason, I never saw it from this perspective until this statement 🥲. Thanks, I'll make appropriate changes here.

Though, the terminal size will still be used when setting automatic size i.e via image.width = None, image.height = None and the likes... Otherwise, what would the size calculations be based on 🤔? The good side is, there's no validation of the resulting size... though the available terminal size has to be validated i.e checked if it's too small (cols <= 0 or lines <= 0) to ensure calculations don't go out of hand.

But as a user—who read the docs poorly—it was something that threw me off.

I know that pain, when you realize there was actually one function/method or function parameter that could do something you've been racking your brains over 🥲... happens to everyone once in a while.

Thanks for everything!

AnonymouX47 commented 2 years ago

You might also wanna watch out for some upcoming features...

I guess they might be be useful for your project.

The first three should be coming with 0.3.x and the last one, probably later (though, definitely coming soon). I'll be releasing 0.2.0 real soon, majorly including bug fixes and significant performance improvements plus some feature additions that were necessary for the performance improvements.

Also, I'm planning to take the rendering performance up some notches 😉.

AnonymouX47 commented 2 years ago

I think I'll leave this open till I implement the changes to __init__().

AnonymouX47 commented 2 years ago

I noticed you're using str(block_image), you can control the alignment (amongst other things) using any string formatting method with the TermImage instance e.g

f"{block_image:{max_width}.{max_height}}"

to always have the images centered within the available space i.e (max_width, max_height).

See here for more.

paw-lu commented 2 years ago

I guess they might be be useful for your project.

You'd be right.

Really looking forward to seeing this developing. Will be following closely. Of course, no rush! Really happy already with how the renders turned out.

you can control the alignment (amongst other things)

Yeah I saw that! I don't see this leveraged a lot so I thought this was an interesting API design!

AnonymouX47 commented 2 years ago

Thanks :smiley:

I'll try to make sure development moves quickly.

AnonymouX47 commented 2 years ago

Hello @paw-lu !

I have made some changes to size setting and validation in #12. Please, can you take a look. In case, you don't want to go through the code, the commit messages are quite explicit and you may also pull that branch to test the changes.

I'll gladly appreciate your feedback. Thanks. 🙏🏾

AnonymouX47 commented 2 years ago

@paw-lu Thank you very much.

I believe this issue has been fixed and the changes will be rolling out with version 0.2.0 by the beginning of April.