The Resize Window escape sequence doesn't support omitted and zero parameters #4417
Comments
ghost
added
Needs-Triage
My interpretation of ECMA-48 5.4.2 is that an empty parameter must be equivalent to a certain valid explicitly specified value. Given that the values in this context are expected to be unsigned integers, the default must also be an unsigned int, like 0, 1, 999 or so – leaving room for only one special action for the number 0, and not being able to express two different special actions ("unchanged" vs. "maximized"). I might be wrong, though. On a more important note, I believe that this escape sequence with a fixed size is borderline problematic/harmful, whereas this one with the "maximized" value, and other nearby escape sequences, are straight harmful (just like when webpages that are allowed to move/resize the browser window – everyone hates that, right?) and terminals should deliberately refuse to implement. (Yeah I know vttest tests for 80/132 column switching...) See also https://gitlab.gnome.org/GNOME/vte/issues/128#note_524609. |
I think you're reading too much into that. At any rate, this sort of thing isn't without precedent. The second parameter of Still, like it or not, that's how the operation is defined. Whether you choose to support it or not is a separate question, and not one I expect everyone to agree on. We already do support it (at least in conhost), so this issue is just about getting us more closely aligned with the specification.
Personally I don't have a problem with apps resizing the window, because a regular Windows console app would already be able to do that. And I wouldn't want to ban all apps from using certain functionality just because some trollish app might decide to do something annoying with that functionality - a resized window isn't going to kill me. And if that is a major concern for users, we could always add an option to disable sizing ops (I think XTerm has something like that). |
|
From the perspective of "conhost supports this, so WT should support this" I totally agree. Alternatively, I rather dislike how this works in a world where multiple terminals might be hosted in one application window. This will be uncomfortable to implement with split panes as well. 😄 |
DHowett-MSFT
added
Area-VT
|
(I do think that conhost should do something with |
|
I was only expecting this to work in conhost. WT's handling of programmatic resizing is completely broken, and I'm not sure if there is any way to fix it. At any rate, I think it's a big enough problem that it should probably be dealt with as a separate issue. |
|
Excellent. Re-tagged. |
This PR introduces a pair of classes for managing VT parameters that automatically handle range checking and default fallback values, so the individual operations don't have to do that validation themselves. In addition to simplifying the code, this fixes a few cases where we were mishandling missing or extraneous parameters, and adds support for parameter sequences on commands that couldn't previously handle them. This PR also sets a limit on the number of parameters allowed, to help thwart DoS memory consumption attacks. ## References * The new parameter class also introduces the concept of an omitted/default parameter which is not necessarily zero, which is a prerequisite for addressing issue #4417. ## Detailed Description of the Pull Request / Additional comments There are two new classes provide by this PR: a `VTParameter` class, similar in function to a `std::optional<size_t>`, which holds an individual parameter (which may be an omitted/default value); and a `VTParameters` class, similar in function to `gsl:span<VTParameter>`, which holds a sequence of those parameters. Where `VTParameter` differs from `std::optional` is with the inclusion of two cast operators. There is a `size_t` cast that interprets omitted and zero values as 1 (the expected behaviour for most numeric parameters). And there is a generic cast, for use with the enum parameter types, which interprets omitted values as 0 (the expected behaviour for most selective parameters). The advantage of `VTParameters` class is that it has an `at` method that can never fail - out of range values simply return the a default `VTParameter` instance (this is standard behaviour in VT terminals). It also has a `size` method that will always return a minimum count of 1, since an empty parameter list is typically the equivalent of a single "default" parameter, so this guarantees you'll get at least one value when iterating over the list with `size()`. For cases where we just need to call the same dispatch method for every parameter, there is a helper `for_each` method, which repeatedly calls a given predicate function with each value in the sequence. It also collates the returned success values to determine the overall result of the sequence. As with the `size` method, this will always make at least one call, so it correctly handles empty sequences. With those two classes in place, we could get rid of all the parameter validation and default handling code in the `OutputStateMachineEngine`. We now just use the `VTParameters::at` method to grab a parameter and typically pass it straight to the appropriate dispatch method, letting the cast operators automatically handle the assignment of default values. Occasionally we might need a `value_or` call to specify a non-standard default value, but those cases are fairly rare. In some case the `OutputStateMachineEngine` was also checking whether parameters values were in range, but for the most part this shouldn't have been necessary, since that is something the dispatch classes would already have been doing themselves (in the few cases that they weren't, I've now updated them to do so). I've also updated the `InputStateMachineEngine` in a similar way to the `OutputStateMachineEngine`, getting rid of a few of the parameter extraction methods, and simplifying other parts of the implementation. It's not as clean a replacement as the output engine, but there are still benefits in using the new classes. ## Validation Steps Performed For the most part I haven't had to alter existing tests other than accounting for changes to the API. There were a couple of tests I needed to drop because they were checking for failure cases which shouldn't have been failing (unexpected parameters should never be an error), or testing output engine validation that is no longer handled at that level. I've added a few new tests to cover operations that take sequences of selective parameters (`ED`, `EL`, `TBC`, `SM`, and `RM`). And I've extended the cursor movement tests to make sure those operations can handle extraneous parameters that weren't expected. I've also added a test to verify that the state machine will correctly ignore parameters beyond the maximum 32 parameter count limit. I've also manual confirmed that the various test cases given in issues #2101 are now working as expected. Closes #2101
Environment
Steps to reproduce
printf "\e[8;25;80t"printf "\e[8;30t"printf "\e[8;;100t"printf "\e[8;0;0t"Expected behavior
According to the XTerm documentation, omitted parameters should reuse the current height or width, while zero parameters should use the display's height or width.
Actual behavior
Step 2 works as expected, but the remaining steps have no effect.
That said, this functionality doesn't seem to be widely supported (the only terminals I've seen that got it right were XTerm and Mintty), so it's probably not that important.
The text was updated successfully, but these errors were encountered: