Make term-img easier to unit test

[paw-lu]

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]

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]

Also, thanks for the Additional context! 😊

[AnonymouX47]

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]

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]

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]

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

[AnonymouX47]

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]

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

• ASCII-only characters
• braille characters
• Greyscale rendering
• kitty, iterm2 and sixel protocols support (drawing images with actual pixels)

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]

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

[AnonymouX47]

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]

I guess they might be be useful for your project.

You'd be right.

• iterm2 image support paw-lu/nbpreview#438
• Support Kitty image protocol paw-lu/nbpreview#439
• Add sixel rendering paw-lu/nbpreview#254

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]

Thanks 😃

I'll try to make sure development moves quickly.

[AnonymouX47]

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]

@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.