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

demo: auto-resize and other tweaks #4695

Merged
merged 6 commits into from
Aug 20, 2023
Merged

demo: auto-resize and other tweaks #4695

merged 6 commits into from
Aug 20, 2023

Conversation

PerBothner
Copy link
Contributor

I had some annoyances with the demo. For example, switching to the dom renderer would make the terminal bigger - which would obscure part of the settings column, making it difficult to change things.

Having the terminal automatically resize itself to fit seemed a better approach for testing, as well as for demo purposes. It uses a ResizeObserver for this. Auto-resize is now the default, but you can still manually set COLSxROWS if you want to.

I also fixed some warts in the index.html and tweaked some other styling issues: most noticably making the terminal take 2/3 of the width, rather than 1/2.

PerBothner and others added 5 commits August 18, 2023 16:30
* id="grid" should be id="class" (since "grid" is not unique in the document).
* Remove space after 'id=' (multiple places).
* Group 3 texture-atlas elements in new <div class="texture-atlas-container">.
* Make terminal take 2/3 of the width, and the options etc get 1/3
* Remove options height limit, which could lead to annoying nested
  verical scroll regions.  (An inner region for the options inside a
  scrollable viewport.)
This is optional (but the default): You can still explicitly set COLSxROWS,
but "auto" uses a ResizeObserver sets the size based on window size.
@Tyriar Tyriar added this to the 5.3.0 milestone Aug 20, 2023
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.

Thanks, this is much nicer in general and we can just flip the boolean in client.ts temporarily if we need it to be static.

@Tyriar Tyriar enabled auto-merge August 20, 2023 14:35
@PerBothner
Copy link
Contributor Author

@jerch could you take a look at this failing test:

  1) ImageAddon
       image lifecycle & eviction
         get storageUsage:
     AssertionError: expected +0 to be close to 0.2048 +/- 0.05
      at Context.<anonymous> (addons/xterm-addon-image/test/ImageAddon.api.ts:200:14)

It fails when run by GitHub, but it passes when I do yarn test-api-firefox --headless --forbid-only on my laptop. Does the API test make use of demo/client.ts? If so, could the test be sensitive to the terminal size, which now by default depends on context (window size)? Or perhaps the test is a bit too specific?

@Tyriar Tyriar merged commit 6f0a5c9 into xtermjs:master Aug 20, 2023
12 of 14 checks passed
@PerBothner PerBothner deleted the demo branch August 20, 2023 16:38
@jerch
Copy link
Member

jerch commented Aug 20, 2023

@PerBothner Hmm thats strange- nope the API should not be sensitive to windows size, unless the window size is so small, that the image gets evicted right away. Then ofc the test fails at its preconditions. It almost looks like thats the case here 🤔

Edit: Where did you get that failure, seems I cant find it.

@PerBothner
Copy link
Contributor Author

@jerch: "Where did you get that failure, seems I cant find it."

It was in the results reported by GitHub when it auto-runs tests something is checked in. Specifically https://github.com/xtermjs/xterm.js/actions/runs/5917974173/job/16046844283?pr=4695 .

It might have fixed itself: https://github.com/xtermjs/xterm.js/actions/

@jerch
Copy link
Member

jerch commented Aug 20, 2023

It might have fixed itself: https://github.com/xtermjs/xterm.js/actions/

It either was the data center's cat, or we have here evidence of computers' nondeterminism in big cluster setups. Ah well, prolly the cat 🤣

@jerch
Copy link
Member

jerch commented Aug 21, 2023

Eww this removes the col/row inputs from the options, which I use alot to test behavior. Its much more cumbersome to do the same through the console with term.resize(). Can we get these back, maybe prepopulated from the measured values?

@Tyriar
Copy link
Member

Tyriar commented Aug 21, 2023

@jerch you can explicitly set the size via a textbox now, so when we need a specific size we can either set it there after each reload or disable auto resize in client.ts

@jerch
Copy link
Member

jerch commented Aug 21, 2023

Sorry for the rant - buts thats not better, for me it is actually worse. I have many tests, that rely on a certain terminal size, which I now have to fiddle out by resizing the browser window? Furthermore it hard-couples the fit addon, which should be tested separately.
Last but not least - the fact that switching renderer might change metrics is actually pretty helpful in the demo to spot possible dimension miscalculations, it helped me alot during DOM renderer rewrite to get the letter spacing right. Now thats hidden behind an implicit rows / cols change.

Imho the demo should behave like that:

  • A given cols x rows setting creates a certain div container, derived from font size. That should be the basic assumption.
  • If we want to test fit addon behavior, there could be a resizer edge handle, that applies resizes freely (so peeps can enlarge/shrink the terminal container in any direction)
  • The settings should not get into the way of the terminal widget, thus should be shown below, if there is not enough room at the right side (thats broken for some time now).

Your change here treats the terminal more like from integrator side, where ppl most likely want the terminal to stay within a certain container dimension. Well that does not work for many of my tests.

Edit:

@jerch you can explicitly set the size via a textbox now, so when we need a specific size we can either set it there after each reload or disable auto resize in client.ts

Via a textbox? How that and where?

@Tyriar
Copy link
Member

Tyriar commented Aug 21, 2023

From my perspective it's very useful to quickly maximize/restore/resize to resize the terminal, and during development I'd generally prefer a larger terminal.

Via a textbox? How that and where?

At the bottom:

image

And you can set this to disable it in client.ts locally to revert to the old behavior:

image

@jerch
Copy link
Member

jerch commented Aug 21, 2023

Oh sorry, did not see this - yeah thats good enough for me. 👍

(Sorry for the rant above, it felt like introducing more issues at new ends, when seeing the new options list. Did not spot the new entry at the end though.)

@Tyriar
Copy link
Member

Tyriar commented Aug 21, 2023

No worries, being passionate is a good thing 😉

@PerBothner
Copy link
Contributor Author

It might be useful to have a way to initialize the size to a specific (initial) size. For example, in the URL:

http://localhost:3000/#size=80x24

Perhaps we could try for a bit with the way it currently works. I can always add this feature later.

@Tyriar
Copy link
Member

Tyriar commented Aug 21, 2023

Something like that, I was always thinking a checkbox to remember all settings would be handy

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.

3 participants