Allow forwarding resize events

[nagylzs]

I'm working on a project that is used to start different commands on different servers, and for this, it needs to forward terminals through TCP pipes.

There are various reasons why I don't want to directly open a local ssh command on the client side; here are some of them:

• ssh may not be available on the client side, or it might be incompatible with the ssh server
• there may not be an ssh server running on the server side at all
• possibly, logging in with ssh requires 2FA, and once authenticated, I want to be able to open many terminals without having to do 2FA many times

This is how I use it right now:

1. create a process on the server side using exec.Command and then pty.Start
2. forward the input and output of the tty (*os.File, as returned by pty.Start to the "other side", using a TCP connection / pipe
3. open fyne.Terminal on the client side using terminal.RunWithConnection and display the terminal there

However, I'm missing a functionality: when the terminal is resized, then I'm not able to reconfigure the pty size, because that interface is hidden. It would be nice to add this feature. This would allow Terminal to be used through any pipe/network connection.

Suggested changes are:

1. Add the possiblity to listen for pty.WinSize changes, e.g. add a Terminal.SetResizeHandler method
2. In term_unix.go, updatePTYSize method should first call the resize handler, and after that call pty.Setsize, but only if t.pty != nil

Using this modified interface, an end user could forward not just the pty through a pipe, but also forward terminal size changes.

[andydotxyz]

Like this?

https://github.com/andydotxyz/sshterm/blob/57557c8e6ab083370f00acd21d228b0bbd692518/main.go#L62

With Fyne widgets you can "intercept resize" by extending and replacing the Resize method with your own and delegating to the original.
This is preferable to having lots of fields or setters for every sort of method / change event that can occur.

[nagylzs]

All right, but there are some problems with that approach:

1. guessCellSize is private, it cannot possibly be used from a different Resize method. At least would it be possible to make it public?
2. updatePTYSize already calculates X, Y Rows and Cols. That calculation is independent of the pty library itself, and this calculation is an implementation detail that is also "hidden". You are right in that fyne.Terminal should not expose a type from a third party library, but if we leave this calculation hidden/private, then anybody who wants to respond to size changes will have to re-implement this calculation again and again. Would it be possible to put this calculation into a public method, and use that from updatePTYSize?

[nagylzs]

With Fyne widgets you can "intercept resize" by extending and replacing the Resize method with your own and delegating to the original.

I can accept that I have to reimplement guessCellSize, and the calculation of rows/cols; if you think that is the best way to do it. I'm not an expert in fyne, so that might be the best solution. However, I fail to "intercept Resize() by extending", because I do not know how to extend terminal.Terminal. I was trying to do so for hours, and I don't see how. I looked at your example (sshterm) and it seems to be extending widget.Icon, and intercept its Resize method. I do not want to create icons or layouts or other extra widgets; I only need to be able to extend and intercept "Resize".

In sshterm, the extended widget it is created like this:

https://github.com/andydotxyz/sshterm/blob/57557c8e6ab083370f00acd21d228b0bbd692518/main.go#L51

	t := &termResizer{win: w, debug: debug}
	t.ExtendBaseWidget(t)

It is doable because both termResizer and widget.Basewidget can be simply constructed without calling an initializer. But terminal.Terminal cannot be constucted like this:

t := &terminal.Terminal{}

It must be created with terminal.New() because it must initialize some private fields:

terminal/term.go

Line 465 in eddb379

t := &Terminal{

Also, terminal.New already calls ExtendWidget, so it both cannot be used for widget extension and it should be used (because there is no other way to initialize the internal/private struct).

So how I can "extend and intercept"? Sure, you can put terminal into another widget and intercept its Resize method. But that is not good enough. Here is a much better question that is closer to real use caces: how can I extend Resize, TypedShortcut Print by "extending"?

[andydotxyz]

I can accept that I have to reimplement guessCellSize, and the calculation of rows/cols; if you think that is the best way to do it.

Not necessarily - I was responding in the generic resize case. If a "terminal sized" callback is what is the desired outcome then that could be pursued - with appropriate values so as to not expose dependencies.

[andydotxyz]

But terminal.Terminal cannot be constucted like this:

t := &terminal.Terminal{}
It must be created with terminal.New() because it must initialize some private fields:

That should be considered a bug, it should work as you describe so that it can be extended.

Also, terminal.New already calls ExtendWidget, so it both cannot be used for widget extension and it should be used (because there is no other way to initialize the internal/private struct).

The "New" constructors are supposed to call ExtendWidget - it's the lack of the ability to create it from the base struct that is the issue, as noted above. A PR would be most welcome, otherwise we will get to it later I'm sure.

[nagylzs]

I can do a PR, but I'm new to fyne. What is the convention for creating extensible widgets? Should I add an Init() method for initializing internal structures and call that from New()?

Something like this:

// New sets up a new terminal instance with the bash shell
func New() *Terminal {
	t := &Terminal{}
	t.ExtendBaseWidget(t)
	t.Init()
	return t
}

// Init initializes a Terminal.
// Does not call ExtendWidget. Call this for extending Terminal
func (t *Terminal) Init() {
	t.mouseCursor = desktop.DefaultCursor
	t.highlightBitMask = 0x55
	t.in = discardWriter{}
	t.content = widget2.NewTermGrid()
	t.setupShortcuts()
}

And then extend it this way:

type MyTerminal struct {
	terminal.Terminal
}

func NewMyTerminal() *MyTerminal {
	t := &MyTerminal{}
	t.ExtendBaseWidget(t)
	t.Init()
	return t
}

but it does not work, gives me nil pointer reference when I tap on the widget.

[nagylzs]

@andydotxyz regarding the guessCellSize and X / Y calculation, would it be a good idea to change the terminal.Config type to include Width Height and cell size? I think it would be a backward compatible change, and that would provide all information that is needed to connect a custom tty.

[andydotxyz]

I can do a PR, but I'm new to fyne. What is the convention for creating extensible widgets? Should I add an Init() method for initializing internal structures and call that from New()?

No, there should be no new method. The types should work without initialisation.

However if it cannot be avoided then it is possible to hook into ExtendBaseWidget because extending instances will have to call that anyway.

[andydotxyz]

@andydotxyz regarding the guessCellSize and X / Y calculation, would it be a good idea to change the terminal.Config type to include Width Height and cell size? I think it would be a backward compatible change, and that would provide all information that is needed to connect a custom tty.

Hey that sounds like a smart idea yes - we could document that the size of a pty is part of the configuration.

[nagylzs]

All right, should we add both CellWidth + CellHeight and Width + Height, or only one of them?

type Config struct {
	Title                 string
	Rows, Columns         uint
	Width, Height         uint
	CellWidth, CellHeight uint
}

and I guess Width, Height, CellWidth and CellHeight should already be converted to pixels using canvas.Scale()?

[nagylzs]

Something like this nagylzs@27a5288

[andydotxyz]

I don't think it needs CellSize - that was just used internally to calculate...

Hold on a second... Isn't the Rows and Columns of a Config what you want?...
Not sure why I didn't see that before!

[nagylzs]

Hold on a second... Isn't the Rows and Columns of a Config what you want?...

Yes, except that pty.Setsize requires X and Y (width and height in pixels) to work properly. I think for mouse interactions. So we should add either cellsize or size. But possibly not both. Of course, size can also be calculated from widget Size + canvas scale but that is already calculated by Terminal.updatePtySize, and it is not trivial. I really think that that should be available to the end user. (Or recalculated by anyone who wish to connect Terminal to a custom tty...)

[andydotxyz]

I didn't know you were talking about pixels.
If that is truly required we'd need a clearer API because no Fyne APIs work with pixel data...

[nagylzs]

I didn't know you were talking about pixels.

Actually, I didn't know it either when I created this issue. :-) But I dive into the source code and realized that rows, columns and widow size are needed.

I'm not sure why but pty.Setsize requires a pty.Winsize, and pty.Winsize has this X and Y fields that are supposed to contain the virtual terminal size in "pixels". (They are not real pixels, because they can be scaled.) I suspect that it is required because a virtual tty can receive mouse events, and the window size must be known in order to convert mouse coordinates to row/col coordinates. I really don't know the exact details. I only know that Terminal.updatePtySize already calculates pty.Winsize.X and Y fields, and passes them to pty.Setsize. So I think it is safe to assume that any virtual terminal would need this information. So if we want to be able to connect any pty to terminal.Terminal, then we should be notified when the number of rows and cols, or the size changes.

I really don't know how pseudo-terminals work. It might turn out that the X / Y values (window size) is not needed - but then why it is calculated in updatePtySize? All I did is that I tried to forward all pty related calls through a network connection to "the other side", and then I realized that I can't, because some required data is private/unavaliable.

Once again, I do not know enough about pseudo-terminals, so I'll believe what you say. :-) My actual problem can be solved without changing terminal.Terminal. I can copy the code from guessSize and updatePtySize, and re-calculate these parameters. I just tought that it would be nice to have support for terminals that are not running locally.