Feature/change window size request for ssh client #1062

glenkleidon · 2022-12-11T05:43:54Z

Implemented ShellStream.WindowChangeRequest by surfacing existing implementation in (private) _channel::IChannelSession.

…equestInfo

… window-change:WindowChangeRequestInfo

drieseng

Can you also contribute a small self-contained application that excercises this new method?
That application should signal an error if that method was not called exactly as expected (eg. if rows and columns where switched in our implementation).

I'd also like to discuss the name of this method. Are there better alternatives (eg. ChangeWindow)?

drieseng · 2022-12-11T09:06:20Z

src/Renci.SshNet/ShellStream.cs

+        /// <returns>true when change is successful, or false when channel is NOT open or the request </returns>
+        public bool WindowChangeRequest(uint columns, uint rows, uint width, uint height )
+        {
+            if (_channel==null || !_channel.IsOpen) return false;


Please have it throw an ObjectDisposedException when _channel is null, and an InvalidOperationException if the channel is closed. Please document this as such and add corresponding unit tests.

I would've prefered to have the method on ChannelSession also throw if the channel were closed, but that would be considered a breaking change. That method currently either throws or returns true, never false. It also returns true if the channel is closed 😌

This method is essentially a wrapper for SendWindowChangeRequest which returns a boolean. It makes sense that this method also returns boolean. Nothing will break if the request fails, so therefore it is not exceptional that the method fails.

I always ask myself - as library developers, can we KNOW that it is Exceptional that a method is called when in the wrong state or is it simply unexpected. Often the answer is that you can't know the answer because we don't know the context of how the method is being used. The developer using the library will know if the situation is exceptional and can throw if it is. All we need to do is say whether or not it succeeded.

If you throw an exception in a library - it has to be fundamentally wrong to do so. In this case, I don't think it is - it is merely unexpected and of no consequence. There wont be any data coming back to be "in the wrong shape" so it wont matter that the method failed. There might be an argument for Throwing the exception if the channel had never been opened, but as this happens in the constructor that is not possible.

The scenario that comes to mind is, in the CanClose Event of a Desktop App, OR the Close browser event, the shape of the window may change during clean up. If that happens, the event to change window shape MAY still be attached causing the event to fire when the channel has already closed. It is much simpler to handle for the developer if the method does not throw an exception and the unexpected event can be ignored. With an exception, in order to properly clean up, the developer must remember to unsubscribe to events or risk an exception being raised when it is of no consequence.

Returning false allows to the developer to decide if failed call to change window size is exceptional. So I think returning a boolean is the right call here.

Yes, ChangeWindow might be a better name. Happy to do this. Should I proceed?

Ok changed in commit [61d5324]

With regards to an example application - I have a fully worked example in branch glenkleidon:feature/add_vs_2022_solution

Should consider doing a pull request on that one but I need some help with a failed test case.

Actually the blocker is the failed test case on #1024

drieseng · 2022-12-11T09:13:22Z

src/Renci.SshNet/ShellStream.cs

+        /// <param name="width">New screen width in Pixels</param>
+        /// <param name="height">New screen height in Pixels</param>
+        /// <returns>true when change is successful, or false when channel is NOT open or the request </returns>
+        public bool WindowChangeRequest(uint columns, uint rows, uint width, uint height )


Given the rationale below, we could consider having this method return void.
I prefer to discuss this with other team members before we decide how to proceed with this.

Please remove the extra whitespace before the closing parenthesis.

ok removed white space. commit [ec60725]

Should return false as it is not necessary to throw an exception in this case is often of no consequence that the method does not succeed. Also it is a wrapper for the underlying method which returns boolean.

drieseng · 2022-12-11T09:23:17Z

src/Renci.SshNet.Tests/Classes/ShellStreamTest.cs

+        {
+            var shellStream = CreateShellStream();
+            _channelSessionMock.Setup(s => s.IsOpen).Returns(true);
+            _channelSessionMock.Setup(s => s.SendWindowChangeRequest(


Not very useful since SendWindowChangeRequest(...) always returns true, but let's discuss.

Hmm, I dont think SendWindowChangeRequest SHOULD always be true. For example a change to size of 0,0,0,0 should fail and probably will on some pty implementations.

drieseng · 2022-12-11T09:23:50Z

src/Renci.SshNet.Tests/Classes/ShellStreamTest.cs

+            _channelSessionMock.Setup(s => s.SendWindowChangeRequest(
+                  It.IsAny<uint>(), It.IsAny<uint>(),
+                  It.IsAny<uint>(), It.IsAny<uint>())).Returns(true);
+            Assert.IsTrue(shellStream.WindowChangeRequest(80, 25, 0, 0));


Please also verify that SendWindowChangeRequest(...) was actually invoked (once).

scott-xu · 2024-02-13T02:07:49Z

Would you mind resolving the conflicts? @glenkleidon

glenkleidon · 2024-02-13T02:10:26Z

I can Scott. I am busy today but it has been quite a few months since I worked on it, so I’ll need to get myself back up to speed on it. In the next week or so. Glen. From: Scott Xu ***@***.***> Sent: Tuesday, February 13, 2024 1:08 PM To: sshnet/SSH.NET ***@***.***> Cc: Glen Kleidon ***@***.***>; Mention ***@***.***> Subject: Re: [sshnet/SSH.NET] Feature/change window size request for ssh client (PR #1062) Would you mind resolving the conflicts? @glenkleidon<https://github.com/glenkleidon> — Reply to this email directly, view it on GitHub<#1062 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AE6JMP2P7TKISELNZQOGV5LYTLDIFAVCNFSM6AAAAAAS2XYF66VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNBQGIZTINRZGE>. You are receiving this because you were mentioned.Message ID: ***@***.******@***.***>>

glenk@galkam.com.au added 3 commits December 11, 2022 16:23

Added support for sshnet#1061 Window Change Request in ShellStream

6452fda

sshnet#1061 Added not connected test case window-change:WindowChangeR…

485adff

…equestInfo

sshnet#1061 Added Request Failed and Request Successed test cases for…

a5d7b01

… window-change:WindowChangeRequestInfo

glenkleidon requested a review from drieseng as a code owner December 11, 2022 05:43

drieseng requested changes Dec 11, 2022

View reviewed changes

glenk@galkam.com.au added 3 commits December 15, 2022 07:45

Removed extra whitespace before closing parenthesis

ec60725

Ensure SendWindowChangeRequest is called once.

f77df6a

Changed WindowChangeRequest to friendlier name.

61d5324

scott-xu mentioned this pull request Dec 9, 2023

How can I modify the cols rows in CreateShellStream according to the size of the front-end terminal? #1252

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/change window size request for ssh client #1062

Feature/change window size request for ssh client #1062

glenkleidon commented Dec 11, 2022

drieseng left a comment •

edited

drieseng Dec 11, 2022

glenkleidon Dec 14, 2022 •

edited

glenkleidon Dec 14, 2022 •

edited

glenkleidon Dec 14, 2022

glenkleidon Dec 14, 2022

glenkleidon Dec 14, 2022

drieseng Dec 11, 2022 •

edited

glenkleidon Dec 14, 2022 •

edited

glenkleidon Dec 14, 2022 •

edited

drieseng Dec 11, 2022

glenkleidon Dec 14, 2022

drieseng Dec 11, 2022

scott-xu commented Feb 13, 2024

glenkleidon commented Feb 13, 2024 via email

Feature/change window size request for ssh client #1062

Are you sure you want to change the base?

Feature/change window size request for ssh client #1062

Conversation

glenkleidon commented Dec 11, 2022

drieseng left a comment • edited

Choose a reason for hiding this comment

drieseng Dec 11, 2022

Choose a reason for hiding this comment

glenkleidon Dec 14, 2022 • edited

Choose a reason for hiding this comment

glenkleidon Dec 14, 2022 • edited

Choose a reason for hiding this comment

glenkleidon Dec 14, 2022

Choose a reason for hiding this comment

glenkleidon Dec 14, 2022

Choose a reason for hiding this comment

glenkleidon Dec 14, 2022

Choose a reason for hiding this comment

drieseng Dec 11, 2022 • edited

Choose a reason for hiding this comment

glenkleidon Dec 14, 2022 • edited

Choose a reason for hiding this comment

glenkleidon Dec 14, 2022 • edited

Choose a reason for hiding this comment

drieseng Dec 11, 2022

Choose a reason for hiding this comment

glenkleidon Dec 14, 2022

Choose a reason for hiding this comment

drieseng Dec 11, 2022

Choose a reason for hiding this comment

scott-xu commented Feb 13, 2024

glenkleidon commented Feb 13, 2024 via email

drieseng left a comment •

edited

glenkleidon Dec 14, 2022 •

edited

glenkleidon Dec 14, 2022 •

edited

drieseng Dec 11, 2022 •

edited

glenkleidon Dec 14, 2022 •

edited

glenkleidon Dec 14, 2022 •

edited