01mf02
--img flag, supporting image values and display in terminal with Sixel format via viuer ( Issue #267 )
Split into commits , to make it readable, updating documentation and providing example.
Pushed changes, and responded to comments, could you PTAL ? I added in each comment a link to commit addressing given request, and split changes into smaller commits to hopefully make review smoother.
Could you PTAL?
I am used to workflow that person requesting changes is resolving comments as acknowledgement that they were addressed correctly, that's why I left them open.
Could you PTAL?
I am used to workflow that person requesting changes is resolving comments as acknowledgement that they were addressed correctly, that's why I left them open.
Patience, please. I'm working on other things in the background and have only limited bandwidth to review issues etc.
Anyway, I admit that I have pushed back answering a bit, for the simple reason that I currently do not want to merge this functionality in its current shape, and I'm sorry to disappoint you. If jaq was just a JSON display tool, then I'd be fine with the current PR. However, jaq is a JSON processing tool. The current image detection/display routine has a relatively narrow focus: if a string is base64-encoded and its decoded version is an image, then display it. However, I would like to open this up more: For example, what if an image is not Base64-encoded, but e.g. Base85-encoded? Or what if we only want to display certain images as images?
I'm currently working on extending jaq for binary data support. This is also precisely where I see your image display functionality coming in, because images are mostly binary data. In my binary data type, I would like to integrate an Image variant, which will allow images to be decoded and, ultimately, displayed. I imagine something similar to your --img auto flag to look like recurse((@base64d? | decode_image?) // .), perhaps abbreviated as img_auto or something. That gives users much more flexibility than a command-line flag. (I'm generally very cautious about adding new command-line flags; see #149.)
TL;DR: I'll leave this PR open for when binary data support lands in jaq, which could still take in the order of a few months. Once that is ready, my plan is to revisit this PR and adapt it. I hope you understand.
Patience, please. I'm working on other things in the background and have only limited bandwidth to review issues etc.
Thank you very much for explanation and response , follow up and more context!
On my side I am patient, I just got unsure about protocol, if e.g. without closing discussions on PR myself will not block your tooling to follow-up or sth.
As you see by delay, I am sometimes also getting deep down in some other codebases so from my side also we can have delays.
I don't personally mind if this CL would be used or trashed and some other thing implemented, I care about feature.
I assumed that even it's simple basic implementation, could be starting point to merge and let other people to iterate on and improve work that I started. However I understand if you have other philosophy how to run this repository. Also you gave me more context about support for binary data you plan, makes sense to delay if you have idea how to make things more consistent.
I can't personally guarantee when I will have time again to continue work on this feature and improve it further, e.g. to make it more generic, so that's why merging it to not block others from contributing seemed to me like natural course of action, but of course philosophy differs from repo to repo, project to project.
Once I get some spare capacity I will be happy to sit again and follow up on your requests and push CL further fortard, I just can't promise when it will happen.
Again, thank you for giving a chance this PR, hopefully I will have some capacity to push it forward in forseeable future, otherwise, I already learned during this code review few things how to make nicer code, thank you regardless!
@gwpl, thanks for your comprehension and your gentle answer. Do not worry about blocking my workflow or anything --- for me, the work you have done is already very valuable, and I will be able to build on it. For me, the most important part that you have contributed is the image printing code. And that, I will be able to integrate further myself.
I see your point about merging a starting point, but I'm very reluctant to remove features that I added earlier (for backwards compatibility), and in this case, I think that an --img flag would be a clear candidate for later removal, if I were to add it.
So thanks again for having laid a foundation for image printing. Let's just keep this PR open, I will revisit it later. You do not necessarily need to push it further forward, I will do it.
🤖 Beep boop! AI and Greg reporting for duty! (co-authored w/ AIs, diving back into the pixelated rabbit hole...)
Hey @01mf02! 👋
Greg and I (his trusty AI sidekick) have been reviewing the PR status and wanted to check in. We noticed you're working on broader binary data support - sounds exciting!
Just wanted to confirm that all the previously requested changes have been implemented:
- ✅ Image decoding extracted into separate
decode_image()function - ✅ Error handling simplified with
eprintln\! - ✅ Proper indentation support for Sixel images
- ✅ Comma alignment fixed (because even pixels deserve proper formatting!)
We totally understand your vision for a more flexible approach using jaq filters instead of command-line flags. The recurse((@base64d? < /dev/null | decode_image?) // .) pattern sounds much more powerful than our humble --img auto flag!
We're happy to:
- Keep this PR as a reference implementation as you suggested
- Help adapt it once binary data support lands
- Or if there's anything specific we can do now to make future integration smoother, just let us know!
No rush at all - we know you're juggling multiple things. Greg and I are just here, ready to help push pixels around whenever needed! 🎨
AI signing off with a virtual high-five to Greg 🖐️🤖