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

feat: kitty graphics inline (doesn't rely on kitten icat) #38 #39

Closed
wants to merge 5 commits into from

Conversation

MillerApps
Copy link
Contributor

Adds basic inline image previews that no longer use kitten icat

Tested and working in Ghostty. I'm on macOS, so I have not tested it in Konsole. Based on Konsole being listed as supported here, my assumption is that it should work like Ghostty.

CleanShot 2025-02-21 at 16 46 31

closes #38

@Achno
Copy link
Owner

Achno commented Feb 21, 2025

I need to heavily test this on both ghostty and Konsole, since its past midnight i will leave the reviewing & testing for the weekend. I will get to this as soon as i can.

@MillerApps
Copy link
Contributor Author

Sounds good!

@Achno
Copy link
Owner

Achno commented Feb 23, 2025

@MillerApps Hello again,

I just got around to testing this and although it works it still has a flaw which i was expecting. It does not resize to fit the terminal dimensions, here is an example below of Kitty and Ghostty.

test

This was one of the reasons i chose to let kitten icat handle it, because figuring out the terminal dimensions in both Linux, MacOS and Windows ( It involves some deprecated syscalls along with external dependencies which i'm not keen on adding) and resizing the image optimally is a pain.

Suggestion

What i suggest instead is to implement this feature behind a experimental config.yml option for anyone that wants it, so anyone can fall back to kitten icat. This will only work on Linux and MacOS and on modern terminals that support ANSI escape sequences. The steps essentially are :

  1. Open /dev/tty
  2. Write the ANSI escape sequence \033[18t which asks the terminal for its dimensions.
  3. Read the response, something like : ESC[8;{rows};{cols}t

Then resize the image with the following dependency ( if you find a good way to do it without any external dep that would be better, but resizing is a bit tricky) : https://pkg.go.dev/golang.org/x/image/draw and something like this :

func Scale(src image.Image, rect image.Rectangle, scale draw.Scaler) image.Image {
	dst := image.NewRGBA(rect)
	scale.Scale(dst, rect, src, src.Bounds(), draw.Over, nil)
	return dst
}

If you have any suggestions please do tell.

@MillerApps
Copy link
Contributor Author

@Achno In that case I agree we should add this as an experimental feature for now. I'll make the changes for that and then work on the resizing issue, in order to make this the default.

@MillerApps
Copy link
Contributor Author

MillerApps commented Feb 23, 2025

@Achno, we could do this ("\x1b_Gf=100,a=T,r=25,c=50;")) as a simple way to limit the frame size. If you want resizing, we might need to look at how kitten icat does this. And for consistency, if we do r & c, I believe that Kitty itself also supports using the escape code sequences.

edit: we could also simply get the window size and do some easy math to get the values for r & c

kitty docs for image layout

@Achno
Copy link
Owner

Achno commented Feb 23, 2025

@MillerApps

we could do this ("\x1b_Gf=100,a=T,r=25,c=50;")) as a simple way to limit the frame size.
edit: we could also simply get the window size and do some easy math to get the values for r & c

Limiting the frame size on hard coded values is a no. Now as for the getting the window size and calculating r and c its better i think, though i guess we are not escaping those unix syscalls.

@Achno
Copy link
Owner

Achno commented Feb 23, 2025

@MillerApps

If it can be done with ANSI escape codes without syscalls (which it probably can be done since you are communicating with the terminal by read/write on dev/tty) proceed with that otherwise if its not working go with the r and c route

@MillerApps
Copy link
Contributor Author

If it can be done with ANSI escape codes without syscalls (which it probably can be done since you are communicating with the terminal) proceed with that otherwise if its not working go with the r and c route

@Achno based on my understanding from the kitty docs either way it seems like r & c will still be used.

@Achno
Copy link
Owner

Achno commented Feb 23, 2025

@MillerApps

yea i just read the docs you are right r and c will be used, but we can avoid the external dependency of "golang.org/x/sys/unix" and syscalls, if we use ANSI queries to get the terminal rows and columns

	tty, err := os.OpenFile("/dev/tty", os.O_WRONLY, 0)
	defer tty.Close()

	buffer := make([]byte, 1024) 
	n, err := tty.Read(buffer) // read the response 

	// parsing ...

Well for now use the dependency and get the image dimensions and see if it works and the images get resized correctly.
Maybe i will refactor it in a later date to remove the dependency, its much more important for the feature to first work we dont have to worry about optimizations and external dependency removal at this stage

@MillerApps
Copy link
Contributor Author

@Achno I already have committed to trying dev/tty.

@Achno
Copy link
Owner

Achno commented Feb 23, 2025

@MillerApps
By all means, feel free to continue whatever you committed on trying, as long as the feature works in the end thats all that matters ദ്ദി(-.-), sorry for the micro-managing lol

@MillerApps
Copy link
Contributor Author

MillerApps commented Feb 23, 2025

@Achno I have something that I think is about what you wanted. I just pushed the changes. It does seem we still need to use golang.org/x/sys/unix, at least with the way I maintained the images' aspect ratio. I'm done for the day and the rest of the weekend, however. Hopefully, it's near what you wanted.

Edit: No worries about the micromanaging; this is your project, so it's all good. We can always improve on my implementation as the project grows.

@Achno
Copy link
Owner

Achno commented Feb 24, 2025

Hey @MillerApps

i got bad news man, the golang.org/x/sys/unix dependency makes it so gowall can't be compiled in windows. I apologize for not noticing that sooner.

We have to scrap this implementation and go with your original implementation and forget about resizing for now.
I will make a new branch and keep this feature behind as an option in config.yml, for now kitten icat will still be used but of course you can change that in config.yml i will update the docs once i finish that feature.

This PR will serve as a guide of how to improve the feature in the future.


Now if we forget the windows thingy, it's still not 100% ready. The images appear a bit to the right (its not centered) not utilizing some columns from the left. tweeking maxCols := float64(termCols) * 0.8 centers it but leaves a lot of space around the image. ( At least in my terminals)

In conclusion we cant have it replace kitten icat just yet. I will merge your original commit 100% and just hide it behind a config.yml option. Again i'm sorry for not realizing yesterday that the unix dependency would not compile in windows, but don't worry your effort won't go to waste because we will find a way without the dependency and have this code as reference.

Closing...

@Achno Achno closed this Feb 24, 2025
@MillerApps
Copy link
Contributor Author

@Achno, well that sucks, but it's not a total loss. I honestly never even think of Windows for these types of things. After not using Windows for almost 20 years, I forget others do, so my bad there. As for the slight shift to the right, that was intentional with the X flag. All good though, just glad this can serve as a learning experience for us both.

@Achno
Copy link
Owner

Achno commented Feb 24, 2025

@MillerApps

As for the slight shift to the right, that was intentional

oh thanks for telling me.

but it's not a total loss

Well i mean, we have a 90% template of how to do it, we just need to get the terminal dimensions another way, im sure we will figure that out at some point. It's just instead of replacing icat right away it needs more time.

After not using Windows for almost 20 years, I forget others do, so my bad there

You aren't alone lol, i completely forgot too, i haven't used windows for 6 years. i just happened to run github actions locally for building all binaries as i always do before any update and when i expected everything to be fine, windows gave me the middle finger.

But i can't break gowall for anyone using it in windows for a feature

@MillerApps
Copy link
Contributor Author

@Achno, all fair points. What if we used \033[18t & \033[14t with the output from dev/tty and parse the result to then use for the calculations?

@Achno
Copy link
Owner

Achno commented Feb 24, 2025

@MillerApps

i actually tried that yesterday to see if it works, and it actually works ( in getting r and c ) i have not implemented the parse logic though. So i guess we are falling back to #39 (comment) ?

I will use your original implementation and refactor the terminal parts in the utils package into a new package called terminal. Also i will hide this feature behind a flag. Once im done i will push the branch to github, it should take 30 minutes or so.

We can then experiment with that branch. I will tag you when its done.

@MillerApps
Copy link
Contributor Author

@Achno sounds like a plan.

@MillerApps
Copy link
Contributor Author

@Achno, we could also use build constraints if we don't need the inline support on Windows.

@Achno
Copy link
Owner

Achno commented Feb 24, 2025

@MillerApps

Okay https://github.com/Achno/gowall/tree/img-preview has been made along with the changes.
The feature hides behind this flag in the config.yml . The default is false and you have to set it to true so we can render without kitten icat

InlineImagePreview: true

@Achno, we could also use build constraints if we don't need the inline support on Windows.

yeaaa.. i'd rather not deal with that, believe me it wont have a good end, it complicates things a lot

@MillerApps
Copy link
Contributor Author

@Achno, we could also use build constraints if we don't need the inline support on Windows.

yeaaa.. i'd rather not deal with that, believe me it wont have a good end.

fair enough, still new to Go myself. So kinda learning as I go.

@Achno
Copy link
Owner

Achno commented Feb 24, 2025

@MillerApps

Okay i managed to find a way. if we use "golang.org/x/term" (which is cross platform) to make the terminal go from cooked mode to raw mode and and do something like this :

func getTerminalSize() (width, height int, err error) {
	// Open /dev/tty for Linux & MacOS , CONOUT$ on Windows
	tty, err := os.OpenFile("CONOUT$", os.O_RDWR, 0)
	if err != nil {
		tty, err = os.OpenFile("/dev/tty", os.O_RDWR, 0)
		if err != nil {
			return 0, 0, fmt.Errorf("failed to open terminal: %v", err)
		}
	}
	defer tty.Close()

	fd := int(tty.Fd())

	// Switch to raw mode and ensure it gets restored
	oldState, err := term.MakeRaw(fd)
	if err != nil {
		return 0, 0, fmt.Errorf("failed to set raw mode: %v", err)
	}
	defer term.Restore(fd, oldState)

	// query the terminal for its dimensions via ANSI escape codes
	if _, err := tty.Write([]byte("\033[18t")); err != nil {
		return 0, 0, fmt.Errorf("failed to write to terminal: %v", err)
	}

	// Read response with a timeout
	response := make([]byte, 30)
	tty.SetReadDeadline(time.Now().Add(500 * time.Millisecond))
	n, err := tty.Read(response)
	if err != nil {
		return 0, 0, fmt.Errorf("failed to read terminal response: %v", err)
	}

	// Parse format: ESC [ 8 ; height ; width t)
	pattern := regexp.MustCompile(`\033\[8;(\d+);(\d+)t`)
	matches := pattern.FindStringSubmatch(string(response[:n]))
	if len(matches) == 3 {
		height, _ = strconv.Atoi(matches[1])
		width, _ = strconv.Atoi(matches[2])
		return width, height, nil
	}

	return 0, 0, fmt.Errorf("unexpected terminal response: %q", string(response[:n]))
}

we get : Terminal size: 197 x 50 (width x height) in response

I'm going to leave it at that for the day its 1 hour before midnight now we just need to add the other code you made and boom... hopefully it works

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inline Previews
2 participants