search

AnonymouX47 / term-image

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

Implement `urwid` image widget #73

Closed AnonymouX47 closed 1 year ago

AnonymouX47 commented 1 year ago

Resolves #71

One major feature left out in this PR is horizontal trimming of graphics-based images because it requires the implementation a non-trivial feature at the core of this library. This will happen in a few releases to come (tentatively planned for 0.8.0).

Credit: Major thanks to @danschwarz for his consistent suggestions and help with testing things out.

danschwarz commented 1 year ago

Will do. I assume the widget won’t be available through pip until it lands in master branch and you create a release.

On Wed, Feb 22, 2023 at 3:02 AM Toluwaleke Ogundipe < @.***> wrote:

My image11 branch of toot https://github.com/danschwarz/toot/tree/image11 integrates the latest term-image widget.

As a reminder, when adding term_image as a dependency, please remember to pin it to a specific minor version, until version 1.0 is released. See here https://github.com/AnonymouX47/term-image#usage.

— Reply to this email directly, view it on GitHub https://github.com/AnonymouX47/term-image/pull/73#issuecomment-1439580270, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAY4FJRH4W37C6GK5VPVW5TWYXBYFANCNFSM6AAAAAAURC4P4I . You are receiving this because you were mentioned.Message ID: @.***>

danschwarz commented 1 year ago

I think it’d be best for an app to catch all unhandled exceptions and clear images in the exception handler before exiting. I haven’t tried this but it seems possible by using sys.excepthook. Responsibility of the app developer.

As for clearing images on startup, option 1 seems like it’d work best, but it’s a lot to ask of an urwid developer who isn’t necessarily familiar with sending commands directly to the TTY. Sample code would help, or monkey patch urwid.Screen?

2 is more developer friendly, but if I read your note correctly, it

depends on rendering an urwid image widget immediately on startup. Toot, for example, doesn’t do that (yet). It throws up a big text splash screen while it loads data.

With #2 It would be necessary to render an image as part of the splash screen just to get the screen clearing benefits?

On Wed, Feb 22, 2023 at 2:52 AM Toluwaleke Ogundipe < @.***> wrote:

Another thing I should note at this point is that I haven't figured out a way to clear images upon a crash i.e when an unhandled exception is raised within MainLoop,run(). The closest solution to this is to clear images upon startup of the TUI i.e after urwid has switched to to alternate buffer. I've tried a few ideas:

  1. The user overrides Screen.start() and clear all images after calling super().start().
    • This requires either flushing the output buffer before clearing the images OR manually switching to the alternate buffer and clearing the images (and possibly switching back to the normal buffer) all by writing directly to the TTY.
  2. Let it be handled by UrwidImagejanitor by clearing all images upon its first render.
    • I would prefer this in order not to bother users with No. 1 but there are a couple of issues that depend entirely on the user.
    • What if the first render of widget does not occur immediately after the urwid screen start up i.e before Screen.start() is called or later after urwid has switched to the alternate buffer and drawn the UI?
    • One way to solve the problem above is to make the clear at first render optional and disabled by default.

What do you see to this?

— Reply to this email directly, view it on GitHub https://github.com/AnonymouX47/term-image/pull/73#issuecomment-1439571041, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAY4FJQPZDTZWNIAUBKVZJ3WYXAUJANCNFSM6AAAAAAURC4P4I . You are receiving this because you were mentioned.Message ID: @.***>

AnonymouX47 commented 1 year ago

I assume the widget won’t be available through pip until it lands in master branch and you create a release.

True.

About clearing images at startup (and exit)...

Let it be handled by UrwidImageJanitor by clearing all images upon its first render.

I had actually implemented (and even pushed) this already but it smelt so much of "hackiness". So I reverted it.

I've just added a screen class to handle this in commit f0cb34b. This is definitely a better approach:

danschwarz commented 1 year ago

Wow. Do you sleep?

The latest changes are great. Added UrwidImageScreen support to toot, verified it clears images on startup/shutdown, even when ctrl-c exiting the app. (Yeah, subclassing Screen was a much better idea than monkeypatching the urwid Screen class 🙄)

Implemented a test for pixel vs block style rendering by creating an AutoImage and checking isinstance(ai, BlockImage). Works well. Now I display UrwidImage widgets using fewer rows when they're rendered using pixels, and more when they're rendered using blocks.

Can't think of anything more this needs, other than a release in pip. It's really working well.

My wishlist after that would be:

On my side, it's just polishing up the image display and refactoring some code. Oh, and I have to check with @ihabunek about dropping Python 3.6 support in toot, so we're at the same Python 3.7 baseline as term-image.

Thanks!

AnonymouX47 commented 1 year ago

Finally, this PR seems to be coming to a wrap. I must say, we've really come a long way with this 🥲.

At this point, I think all initial features I had in mind and all that have come up along the way have been fully implemented (except for horizontal trim of images of graphics-based render styles) and I have written adequate tests for all.

Please help test it out and let me know of any issues you encounter or any other suggestions you have.

Thank you.

EDIT: Just saw your comment above after sending this.

AnonymouX47 commented 1 year ago

Do you sleep?

🤔 Probably not :D

I'm really happy to hear that everything is working well.

Just two major things left to do before the next release, restructure the docs and tests (due to the fact that I've now separated the TUI). Not adding anything new, just re-organizing what's already there.

As for your wishlist, all that plus more will be coming in the release following this. See this milestone. I'll gladly appreciate your input in any of the issues listed.

AnonymouX47 commented 1 year ago

I guess I'll wait till after I wake up before I merge... It's always best to go over one last time with a fresh brain :D

I can finally close all these terminal emulators and power off my PC in peace after days 😮‍💨.