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

Use a new windows console screen buffer to preserve window contents. #288

Closed
wants to merge 2 commits into from

Conversation

gcla
Copy link
Contributor

@gcla gcla commented Jun 17, 2019

This change attempts to eliminate some lingering console issues when a windows
tcell application terminates. Here's a link to one such issue, in termshark:
gcla/termshark#25.

Rather than restoring the original console window settings, this change saves
the current console buffer, then creates and switches to a new console window
buffer, using CreateConsoleScreenBuffer and SetConsoleActiveScreenBuffer. The
Fini function restores the saved console buffer. This eliminates the
above-linked termshark issue, and has the side benefit of preserving the
existing console contents which are restored after the tcell application
terminates.

Tested on both cmd.exe and Windows Powershell.

This change attempts to eliminate some lingering console issues when a windows
tcell application terminates. Here's a link to one such issue, in termshark:
gcla/termshark#25.

Rather than restoring the original console window settings, this change saves
the current console buffer, then creates and switches to a new console window
buffer, using CreateConsoleScreenBuffer and SetConsoleActiveScreenBuffer. The
Fini function restores the saved console buffer. This eliminates the
above-linked termshark issue, and has the side benefit of preserving the
existing console contents which are restored after the tcell application
terminates.

Tested on both cmd.exe and Windows Powershell.
@codecov
Copy link

codecov bot commented Jun 17, 2019

Codecov Report

Merging #288 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #288   +/-   ##
=======================================
  Coverage   19.54%   19.54%           
=======================================
  Files          17       17           
  Lines        1448     1448           
=======================================
  Hits          283      283           
  Misses       1149     1149           
  Partials       16       16

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca8fb5b...1097a48. Read the comment docs.

@codecov
Copy link

codecov bot commented Jun 17, 2019

Codecov Report

Merging #288 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #288   +/-   ##
=======================================
  Coverage   19.54%   19.54%           
=======================================
  Files          17       17           
  Lines        1448     1448           
=======================================
  Hits          283      283           
  Misses       1149     1149           
  Partials       16       16

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca8fb5b...061ef01. Read the comment docs.

@gdamore
Copy link
Owner

gdamore commented Jun 18, 2019

Does this change the initial position or size of the window, or does the new buffer inherit these from the prior buffer? I guess if the user changes the window size when the old one is restored, it will go back to the original size?

Commit 1097a48 changed the implementation of
the tcell windows console screen to use a new console screen buffer via
CreateConsoleScreenBuffer. The screen buffer is initialized with a size that
is determined by the console window. If the user uses a mouse to make the
window smaller, the next call to cScreen.Show() calls resize() which sets the
console screen buffer to the smaller size. This has the side effect of
disabling the ability of the mouse to make the window larger again. Although
the window can be enlarged via right-click->Properties on the window title
bar, it's inconvenient.

This change removes the call to adjust the size of the console screen
buffer. Tcell's rendering functions are aware of the reduced window size, and
operate correctly in the reduced dimensions, but the windows buffer object
keeps its original size. This preserves the ability of the user to grab the
bottom right corner of the window with the mouse and resize the window back up
to its original size.
@gcla
Copy link
Contributor Author

gcla commented Jun 19, 2019

Hi - by default the new buffer seems to have its size determined by the dimensions of the window. If the prior buffer is larger than the window (seen via right-click -> Properties and a vertical scroll bar on the right), when the new tcell application starts, the scroll bar disappears, indicating the new buffer has been made the same size as the console window.

Yes you're right - if the user changes the window size, or the font size, the old size is restored when the tcell application terminates.

Your question made me fiddle with the behavior more, and I think a change to my PR is warranted. Without this change, if the user uses the mouse to make the running tcell application window smaller, then they can't use the mouse to make it bigger again. With this change, it can be resized with the mouse back up to its original size. Worth adding though that with or without this additional change, the user can right click, select Properties, and adjust the window and buffer size manually to make the application window bigger than the size at which it started.

Here are some videos to demonstrate. First, before patch 2, this shows the application being made smaller, and then it only being possible to use Properties to make it bigger again:
https://drive.google.com/file/d/1cxssORk94TiWb2v7T5gGxe6YMzJ1yv4_/view?usp=sharing

Then after patch 2, this shows that the application can be made smaller, and then enlarged to its original size:
https://drive.google.com/file/d/1EwttuhGCwMFwUzFVfmomo5AsH7IS7Pt9/view?usp=sharing

Finally, font changes made when the tcell app is running are undone when the application terminates:
https://drive.google.com/file/d/1U8f91qBNahWPnNdY0krqRXTaLAE6AKoh/view?usp=sharing

@gdamore
Copy link
Owner

gdamore commented Jun 26, 2019

I'm a little concerned by the last change. If you don't change the buffer size, does the application get a chance to understand the window size? The expectation is that applications get a chance to be responsive on Window resize events...

@gdamore
Copy link
Owner

gdamore commented Jun 26, 2019

It looks to me, based on your videos, that what happens is that the caller doesn't see the resize events, and winds up just scribbling to areas under the scroll bar. I don't think this is the behavior we want.

What we want is for the application to react. There may be some other way we can enable the buffer to be resized with the Window....

@gdamore
Copy link
Owner

gdamore commented Jun 28, 2019

So the other thing, at least one user has requested the opposite behavior of this, on UNIX terminals where this behavior is the default. (XTerm initialization sequences use a secondary buffer for curses style applications.)

More and more I'm thinking this isn't such a hot idea. The behavior needs to be tunable at a minimum.

@gcla
Copy link
Contributor Author

gcla commented Jun 28, 2019

Hi - thanks for all the feedback. Just to make sure I understand, do you have reservations about patch 2 specifically - if the application window is made smaller, allowing it to be made larger again - or the overall idea of a windows tcell application using a new windows console? If it's about patch 2, and you think making it optional is best, I can definitely add that.

@gdamore
Copy link
Owner

gdamore commented Aug 1, 2019

At least one concern is about using a new console. This behavior is present with xterm (where it uses an alternate screen buffer instead), but some folks have requested not to have this behavior. (In xterm's case it is driven by specific terminal initialization sequences, which can be altered to taste. In windows we have no way to tune this.)

gcla added a commit to gcla/termshark that referenced this pull request Nov 5, 2019
These are used to address this issue: #25

The tcell PR discussion is here: gdamore/tcell#288.
@gdamore
Copy link
Owner

gdamore commented Sep 20, 2020

I may wish to revisit this -- perhaps with a tunable.

More and more tcell is learning about terminal capabilities that are not expressed in termino, and I think the ability to use an alternate buffer may be one of them. This could then be an application choice (or possibly an environment variable.)

@gdamore
Copy link
Owner

gdamore commented Apr 18, 2021

I'm not sure if you still want this PR under consideration (it's been a long time) -- some of the other issues around suspend and resume are now sorted, but not using an alternate buffer.

If you do want it, can you rebase it and test it again? Sorry. Thanks.

@gcla
Copy link
Contributor Author

gcla commented Sep 4, 2021

It's been a long time since I thought about this, and it's polluting your PR list, so I'll close it. I can always re-submit if it looks like a reasonable thing with respect to the latest tcell.

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.

None yet

2 participants