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

Implement urwid image widget #73

Merged
merged 46 commits into from
Feb 23, 2023
Merged

Implement urwid image widget #73

merged 46 commits into from
Feb 23, 2023

Conversation

AnonymouX47
Copy link
Owner

@AnonymouX47 AnonymouX47 commented Feb 4, 2023

Resolves #71

  • Adds term_image.widget subpackage and term_image.widget.urwid submodule.
  • Adds the following widgets and related classes for the displaying images with the urwid TUI framework.
    • UrwidImage
    • UrwidImageCanvas
    • UrwidImageJanitor
    • UrwidImageScreen

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.

@AnonymouX47
Copy link
Owner Author

AnonymouX47 commented Feb 4, 2023

Hello @danschwarz! 😃

Please, I'll appreciate if you can help test this out.

@danschwarz
Copy link

Thanks! I pulled the urwid-widget branch and did a make install locally into the venv of my test app.

import term_image seems to work

mywidget = UrwidImage(img,format='RGB',upscale=False)

fails with NameError: name 'UrwidImage' is not defined.

I'm not a Python expert so this is probably an environment config issue on my side, but in case I'm just not importing the right library, let me know and that'll be an easy fix.

@danschwarz
Copy link

also tried from term_image import widget
then attempted to print out all functions in widget.. it prints []. So probably an environmental issue?

@AnonymouX47
Copy link
Owner Author

AnonymouX47 commented Feb 4, 2023

First of all, I apologize for not giving any directions earlier. 🤦🏾‍♂️

Did you install with make install?

If so, then term_image.widget shouldn't be empty, it should contain two classes, UrwidImage and UrwidImageCanvas. I just made sure of this by testing with a fresh virtual environment.

The key thing is the urwid package must also be installed (in the same environment) for the widgets to be available. You can do that by install urwid normally, though running make install does that.

As for importing the required parts of the library, this should be sufficient:

from term_image.image import AutoImage
from term_image.widget import UrwidImage

To use the widget, the docs should be of help.

You might need to familiarize yourself with the library first, the tutorial should be quick and sufficient.

Finally, I'll try to give a minimal working sample soon.

@AnonymouX47
Copy link
Owner Author

AnonymouX47 commented Feb 4, 2023

mywidget = UrwidImage(img,format='RGB',upscale=False)

This won't work even if UrwidImage were properly imported because the argument types/values are wrong. The pages I linked in my previous comment should help with that.

Basically,

  • the first argument must be an instance of an image class defined in term_image.image.
  • The second is an image format specifier, which is shown in use here in the tutorial. For now, you may omit this argument, the default should be fine.
  • The third argument determines if the source image may be upscaled when rendering.

@AnonymouX47
Copy link
Owner Author

AnonymouX47 commented Feb 4, 2023

urwid_sample.py:

import sys

import urwid
from PIL import Image

from term_image.image import AutoImage
from term_image.widget import UrwidImage

class MyLoop(urwid.MainLoop):
    def process_input(self, keys):
        if "window resize" in keys:
            UrwidImage.clear()
        return super().process_input(keys)

def handle_input(key):
    if key == "q":
        UrwidImage.clear()
        raise urwid.ExitMainLoop
    return False

img = Image.open(sys.argv[1])
image = AutoImage(img)
image_w = UrwidImage(image)

base_widget = urwid.LineBox(
    urwid.Pile(
        [
            urwid.Columns(
                [urwid.LineBox(image_w, "Image")] * 2
            )
        ] * 2 
    ),
    "Images",
)

loop = MyLoop(base_widget, unhandled_input=handle_input)
loop.run()
python urwid_sample.py logo.png

produces

Screenshot_2023-02-04_19-29-18

on Kitty and

Screenshot_2023-02-04_19-30-53

on Terminator (VTE-based).

EDIT: Updated the sample code to clear graphics-based images properly.

@danschwarz
Copy link

danschwarz commented Feb 4, 2023 via email

@AnonymouX47
Copy link
Owner Author

without creating a sixel image-per-row, which is likely to slow down or break Xterm.

I see. Unfortunately, that's the only way to achieve that :(

@AnonymouX47
Copy link
Owner Author

AnonymouX47 commented Feb 4, 2023

Please note that I just updated the sample code above to clear graphics-based images properly.

When images need to be cleared will majorly depend on your use case but one common case is a window resize.

@danschwarz
Copy link

Thanks for the extra documentation. With that, I was able to get the widget working properly in my test app.
ANSI and Kitty images look good.

One anomaly I've found with the ANSI images - I have one that entirely filled my screen down to the lower right corner. You can see the [ character printed over the image. I don't know why that is appearing. This is an UrwidImage enclosed in an urwid.Filler container, nothing more. Displaying on Windows Terminal/Ubuntu 22.04.

This is repeatable with any image displayed as ANSI that is as large, or larger than the terminal width/height.

image

Another example, this is a NASA image

image

@danschwarz
Copy link

I know there's some weirdness with VT100 terminal emulation and the last cell in the lower right position - urwid raw_display.py _last_row(..) method does some extra work to set the last row, rightmost cell. Perhaps something there is interacting with the ANSI graphics output? Just a guess.

- Change: Prepend `UrwidImageCanvas._change_diguise()` with `_ti`.
- Change: Correct string formatting of an error message.
@AnonymouX47
Copy link
Owner Author

AnonymouX47 commented Feb 5, 2023

I looked into it and as you mentioned, it's due to urwid.raw_display.Screen._last_row().

I have implemented a workaround in e8f89b2. Also, includes some comments explaining the problem.

The issue still affects, at least, the iterm2 style on Wezterm where the last line of the image is not drawn because technically, the image escape sequence is truncated/modified.
The workaround resulted in a different problem (scrolling the screen) which seems to be worse IMO. So, I have excluded this case from the workaround. The best option is to avoid having an image fall at the bottom right cell.

Please, let me know how this works on your end, particularly on Windows Terminal.

Thanks for all your feedback. 🙏🏾

@AnonymouX47
Copy link
Owner Author

The major thing keeping this from being merged is unit tests, such a necessary evil 😡 but I'm almost done with them 😃.

@danschwarz
Copy link

danschwarz commented Feb 5, 2023 via email

@AnonymouX47
Copy link
Owner Author

Python 3.7 is the minimum supported version for the entire library.

@AnonymouX47 AnonymouX47 added graphics Related to the interface common to graphics-based render styles text Related to the interface common to text-based render styles new New feature implementation labels Feb 5, 2023
@danschwarz
Copy link

danschwarz commented Feb 22, 2023 via email

@danschwarz
Copy link

danschwarz commented Feb 22, 2023 via email

- Add: An urwid screen that clears *kitty* images at start and stop.
@AnonymouX47
Copy link
Owner Author

AnonymouX47 commented Feb 22, 2023

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:

  • By urwid.BaseScreen's design, it inherently takes care of unhandled exceptions via a context manager.
  • It doesn't have any of the quirks associated with the previous approach (No. 2).
  • It also handles clearing upon exit.
  • It can be easily used with a custom screen via inheritance.

@AnonymouX47 AnonymouX47 force-pushed the urwid-widget branch 2 times, most recently from 4f0cc29 to f0cb34b Compare February 22, 2023 17:23
@danschwarz
Copy link

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:

  • Handle terminals like Apple Terminal that only support BlockImage @ 256 colors - no truecolor. (The code may do this already? I can't test until I get back to a Mac.)
  • Sixel support 🙏

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!

- Change: Remove `UrwidImage.selectable`.
- Change: Remove `UrwidImage.keypress()`.
- Add: Tests for:
  - `.clear_all()`,
  - `.clear()`,
  - widgets rendering images of the kitty render style.
@AnonymouX47
Copy link
Owner Author

AnonymouX47 commented Feb 23, 2023

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
Copy link
Owner Author

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 added a commit that referenced this pull request Feb 23, 2023
@AnonymouX47
Copy link
Owner Author

AnonymouX47 commented Feb 23, 2023

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 😮‍💨.

@AnonymouX47 AnonymouX47 merged commit a4b1194 into main Feb 23, 2023
@AnonymouX47 AnonymouX47 deleted the urwid-widget branch February 23, 2023 15:19
AnonymouX47 added a commit that referenced this pull request Mar 26, 2023
- Add: Synchronize the following I/O methods of `UrwidImageScreen`
  with `.utils.lock_tty()`:

  - `get_available_raw_input()`
  - `flush()`
  - `write()`

Refs: #73, #80
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
graphics Related to the interface common to graphics-based render styles new New feature implementation text Related to the interface common to text-based render styles urwid Related to the urwid image widgets/canvas
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request: Refactor to provide reusable widgets that can be used in other Urwid projects
3 participants