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

Support for window options #2393

Merged
merged 36 commits into from
Feb 4, 2020
Merged

Support for window options #2393

merged 36 commits into from
Feb 4, 2020

Conversation

jerch
Copy link
Member

@jerch jerch commented Aug 24, 2019

PR for support of window option reports and manipulations (CSI ... t).

To get this working we have several things to fiddle out:

  • reporting: Its not yet clear which size should be reported where. My current guess is that text area is meant for exact cell * row (pixel) size while our closest match as "window" would be the outer terminal container. Needs investigation.
  • separation of concerns: Some parts of CSI ... t address the offscreen parts, other are meant as "window" thingy thus need calls into browser related code. This might need some internal helpers to avoid tons of _private._private access. Resizing itself would need something like the fit addon in the codebase.
  • features to support: Fullscreen stuff seems questionable to me, some commands dont apply at all as xterm.js typically doesnt run in a windowed env. Still standalone apps like hyper might benefit from interfaces exposing those things. Needs to be discussed.
  • security: Many of the commands are seen as instrusive as they can leak/manipulate things on the terminal's host machine from application side. To not earn a CVE here, we need to decide whether to:
    • support feature xy at all
    • hide feature behind ctor options like options.allowTitleOps, options.allowGeometryOps and such so ppl/integrators are still in control over those features

The early stub only contains a hacked version of grid size report and resize working with the demo.

Closes #1762.

@jerch
Copy link
Member Author

jerch commented Sep 6, 2019

@Tyriar This turns out to be a bigger construction site than initially thought, parts of the features xterm offers under this sequence are deeply linked to X11 stuff (thus for xterm.js use cases prolly platform display/window manager dependent). I think we should not try to implement things here on our own, maybe just forward it via some API so integrators can fill the gaps they want to support. Will see if I can come up with an easy to intercept interface.

src/InputHandler.ts Outdated Show resolved Hide resolved
src/InputHandler.ts Outdated Show resolved Hide resolved
src/InputHandler.ts Outdated Show resolved Hide resolved
@jerch
Copy link
Member Author

jerch commented Sep 30, 2019

@Tyriar Reviewed the sequence again and indeed - it makes not much sense to build a sub API for all those platform dependent things, instead we should only deal with reports we can directly handle and leave all other things to a custom addCsiHandler implementation.

Things we can directly support/handle from within xterm.js:

  • report textarea/window size in pixels (Ps 14)
  • report cell pixel size (Ps 16)
  • report textarea size in cols/rows (Ps 18)
  • report window icon label (Ps 20)
  • report window title (Ps 21)
  • title and label stack handling (Ps 22/23)

Things left to integrators:

  • iconify (Ps 1 / 2)
  • window move (Ps 3)
  • window resize (Ps 4)
  • window stack handling (Ps 5 / 6)
  • window refresh (Ps 7)
  • textarea resize (Ps 8)
  • maximize / fullscreen handling (Ps 9 / 10)
  • report window state (Ps 11)
  • report window/textarea position (Ps 13)
  • report screen size in pixels (Ps 15)
  • report screen size in cols/rows (Ps 19)
  • line resize (Ps >= 24)

As you can see there is not much left to implement for us atm. To help integrators to get the other things done, we might need to implement some helpers on public API (like directly exposing some container metrics and such), but we can do that as it gets requested.
Gonna cleanup the PR accordingly.

Sidenote: I added icon label and window title handling on our side. Imho we can provide these features with a tiny implementation as we always know the propagated values from the OSC calls. There is a minor downside to this - if an implementation does not adopt the titleChanged event in a certain way the visual representation might be different to what gets reported here. Imho thats not a biggie as an emulator should care for title changes if it puts the title somewhere. xterm.js integrations that dont deal with the title will just keep working as expected.

Edit: I still think we will need some security settings here to not earn a CVE, maybe we can just adopt the xterm's security settings as ctor options.

@jerch jerch self-assigned this Oct 4, 2019
@jerch
Copy link
Member Author

jerch commented Oct 29, 2019

@Tyriar Updated the PR with implementations for params we actually can handle without further knowledge about the embedding env.

About security: Put all handling behind an option allowedWindowOps derived from xterm names. It is currently a comma separated string with either option names or param numbers, that gets sanitized during setting (setOption('allowedWindowOps', 'RestoreWin, 16, SetWinLines') ==> [1, 16, 24]). This option gate is also active for custom CSI Ps ; Ps ; Ps t handlers registered later on, thus a handler for a missing window option will never be called.

About DECCOLM: I also put DECCOLM handling behind 'SetWinLines' (24). That option is meant for DECSLPP which allows a more fine-grained column setting. We dont support that yet, still DECCOLM is just an older version of that with 80 vs. 132 column support.

Need some help with the first line here:

xterm.js/src/InputHandler.ts

Lines 2084 to 2098 in da3ae79

const rs = (this._terminal as any)._renderService; // FIXME: import renderService to get rid of any type here
switch (params.params[0]) {
case 14: // GetWinSizePixels, returns CSI 4 ; height ; width t
if (rs && second !== 2) {
const w = rs.dimensions.canvasWidth.toFixed(0);
const h = rs.dimensions.canvasHeight.toFixed(0);
this._coreService.triggerDataEvent(`${C0.ESC}[4;${h};${w}t`);
}
break;
case 16: // GetCellSizePixels, returns CSI 6 ; height ; width t
if (rs) {
const w = rs.dimensions.actualCellWidth.toFixed(0);
const h = rs.dimensions.actualCellHeight.toFixed(0);
this._coreService.triggerDataEvent(`${C0.ESC}[4;${h};${w}t`);
}

What is the supposed way to get a hold of the pixel dimensions? Also the values I return under 14/16 might not be right ones. Please have a look at this.

src/InputHandler.ts Outdated Show resolved Hide resolved
src/InputHandler.ts Outdated Show resolved Hide resolved
src/common/services/OptionsService.ts Outdated Show resolved Hide resolved
@jerch
Copy link
Member Author

jerch commented Nov 14, 2019

@Tyriar Changed the windowOptions into a const enum for internal usage (number). The external API turns it into a {[key: string]: boolean} mapping. Sadly we dont support different types for internal vs. external options in OptionService, thus I put the internal representation on the service object for now. Maybe you have a better idea how to simplify this.

Imho a new settings impl should support external type --> internal type conversions (my stub does not yet).

@jerch jerch closed this Nov 14, 2019
@jerch jerch reopened this Nov 14, 2019
@jerch
Copy link
Member Author

jerch commented Nov 14, 2019

@Tyriar The const enum indirection is quite ugly and we dont need the speed benefit, thus I removed it.

@jerch jerch removed the work-in-progress Do not merge label Nov 14, 2019
@jerch jerch requested a review from Tyriar November 23, 2019 16:37
src/Terminal.ts Outdated Show resolved Hide resolved
src/Terminal.ts Outdated Show resolved Hide resolved
src/common/Types.d.ts Outdated Show resolved Hide resolved
test/api/InputHandler.api.ts Outdated Show resolved Hide resolved
typings/xterm.d.ts Outdated Show resolved Hide resolved
src/InputHandler.ts Show resolved Hide resolved
src/InputHandler.ts Outdated Show resolved Hide resolved
src/InputHandler.ts Outdated Show resolved Hide resolved
src/InputHandler.ts Outdated Show resolved Hide resolved
src/InputHandler.ts Show resolved Hide resolved
@jerch
Copy link
Member Author

jerch commented Dec 7, 2019

Ready for the next review 😄

Edit: Should the windowOptions get a dedicated guide? Imho the security aspects and the impl of several commands might be abit tricky to comprehend without. (Edit: covered by xtermjs/xtermjs.org#116)

@jerch jerch added this to the 4.4.0 milestone Dec 7, 2019
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there 😃

src/InputHandler.ts Outdated Show resolved Hide resolved
typings/xterm.d.ts Show resolved Hide resolved
src/common/services/OptionsService.ts Outdated Show resolved Hide resolved
src/common/services/Services.ts Outdated Show resolved Hide resolved
@Tyriar Tyriar merged commit 7a8e2c6 into xtermjs:master Feb 4, 2020
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.

Support for Extended Window Manager Hints
2 participants