search

vezel-dev / cathode

A terminal-centric replacement for the .NET console APIs.
https://docs.vezel.dev/cathode
BSD Zero Clause License
91 stars 7 forks source link

Tiny adjustment to SetScrollMargin #180

Closed scottbilas closed 2 months ago

scottbilas commented 3 months ago

I noticed that the minimum size supported by a scroll region is 2 lines. Docs for DECSTBM say top must be less than bottom, not less-or-equal.

So this little check could use adjusting:

https://github.com/vezel-dev/cathode/blob/6bd76d8836c1988d96c58283de037de63f5e8ab9/src/core/Text/Control/ControlBuilder.cs#L404

scottbilas commented 3 months ago

Hmm, maybe have an exception for 1-line-high terminals. This can happen when shrinking a tmux pane to be a single line - I do this sometimes to show just the status part of a cli app. In that case, top == bottom == 0 would be reasonable to allow.

alexrp commented 3 months ago

It's been a hot minute since I wrote that code. I do remember that I wrote those argument checks to explicitly deviate from the DECSTBM docs based on some practical observations in various terminal emulators. I don't remember the details though...

alexrp commented 3 months ago

Hmm, here's a comment in a past version of the code that might provide a clue: https://github.com/vezel-dev/cathode/commit/fd39193da1bd33f765dcca6f54a48eba6f090fac#diff-e6adf24d6af11f0dd8712eef8db7057bce1e9cd5638a34c85c6bf57f40c83de1L154-L159

Which was also removed in that same commit.

alexrp commented 3 months ago

I think the odd behavior I observed was with Windows Terminal. Any chance I could get you to test loosening those checks and see how Windows Terminal behaves with such values? (Especially if you resize the terminal window to be as small as possible.)

scottbilas commented 3 months ago

In Windows Terminal (I am on 1.20.11381.0) it behaves according to docs and the same as tmux: bottom <= top is an error case, any DECSTBM with error params is ignored, and will not change whatever the margins were previously.

scottbilas commented 3 months ago

However, I do believe I've found some odd behavior in Windows Terminal when the window is resized...

If I do not set scroll margins, then resizing the window will always keep the margins to the full window. But if I manually set the margins to 0,height-1, then - sometimes - when I make the window taller, it will not keep the bottom margin stuck to the bottom of the window. Doing a DECSTBM with no coords (i.e. apply defaults) restores the correct behavior.

I haven't tried to pin this issue down specifically, instead added a ResetScrollMargins() => _cb.Print(CSI).Print(";r").

Note that tmux behaves correctly - the bottom scroll margin in a resizing pane is always sticky when set to height-1.

alexrp commented 3 months ago

Thinking about it a bit more, I think I'm just inclined to follow the DECSTBM documentation on this. So I'll loosen that check.

Also, good call on needing a ResetScrollMargins() method!

alexrp commented 2 months ago

@scottbilas just to clarify regarding https://github.com/vezel-dev/cathode/issues/180#issuecomment-2183426066, is there actually a case where top == bottom == 0 has an effect? My understanding is that this is technically an error and will go ignored. See e.g. https://github.com/microsoft/terminal/issues/1849.