Skip to content

Make for smooth vertical resizing of remote screen#1583

Merged
jotaen4tinypilot merged 1 commit intomasterfrom
focus-mode/1-vertical-resizing
Aug 23, 2023
Merged

Make for smooth vertical resizing of remote screen#1583
jotaen4tinypilot merged 1 commit intomasterfrom
focus-mode/1-vertical-resizing

Conversation

@jotaen4tinypilot
Copy link
Contributor

@jotaen4tinypilot jotaen4tinypilot commented Aug 23, 2023

Part of #728.

The vertical resizing behaviour of the remote screen is currently so that the screen element’s height appears to “jump” at certain steps due to CSS breakpoints, while you resize the window vertically.

This PR implements “smooth” and step-less vertical resizing, while maintaining a constant margin between remote screen and status-bar during that process.

The breakpoint-less resizing is important for the “dedicated window” (popup) that we are about to implement in #728, where it would probably feel weird if the remote screen height jumped around the way it currently does, or if we wouldn’t be able to fill up the entire vertical space of the popup window due to the breakpoint setup.

Demo: After

Screen.Recording.2023-08-23.at.13.52.12.mov

Demo: Before

Screen.Recording.2023-08-23.at.13.51.32.mov

Notes

  • We had introduced these breakpoints in Scale remote screen size to fit browser height #878 originally, as kind of “heuristic”/pragmatic approach for resizing the remote screen, without having the remote screen visually collide with the status bar. The breakpoints itself are arbitrary, and they don’t serve any other purpose except for facilitating the vertical screen size while resizing the window.
  • I discovered Cursor position off in full screen mode (only for “full width” aspect ratio) #1581 while testing this PR; so this behaviour is not a regression, but it’s currently broken on master.
  • This PR leaves a “naked” <div> in index.html which we technically don’t need anymore. I’ll clean that up in a later PR, to avoid a noisy whitespace diff here.
  • The page-content CSS class is orphaned, so I’ve removed it.

I’ve tested on device with Firefox and Chrome, and I tried to check all imaginable scaling constellations of window sizes and aspect ratios, both with MJPEG and H.264, and both in full screen and regular window mode. The biggest risk of this PR would be that we accidentally mess with the mouse cursor positioning/alignment. It would be cool if you could double-check on device as well, with a real target screen, just so that we are safe.
Review on CodeApprove

Copy link
Contributor

@jdeanwallace jdeanwallace left a comment

Choose a reason for hiding this comment

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

Automated comment from CodeApprove ➜

Approved on CodeApprove
✔️ Approved

Nice, LGTM! I've tested on device.

I discovered tinypilot#1581 while testing this PR; so this behaviour is not a regression, but it’s currently broken on master.

Oh wow. For me in Chrome, I experience this bug only when I'm in full screen mode and the browser's dev console is closed.


👀 @jotaen4tinypilot it's your turn please take a look

Copy link
Contributor Author

@jotaen4tinypilot jotaen4tinypilot left a comment

Choose a reason for hiding this comment

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

Automated comment from CodeApprove ➜

In: Discussion
Awesome, thanks for testing!

@jotaen4tinypilot jotaen4tinypilot merged commit aa80dbd into master Aug 23, 2023
@jotaen4tinypilot jotaen4tinypilot deleted the focus-mode/1-vertical-resizing branch August 23, 2023 15:36
jotaen4tinypilot added a commit that referenced this pull request Aug 24, 2023
Part of #728, follow up of
#1583. Non-functional
refactoring.

#1583 left a stray `<div>`
in the `index.html` file, which doesn’t serve any purpose anymore.

This PR removes the `<div>`, and lifts up the `<remote-screen>` and
`<on-screen-keyboard>` components. That doesn’t have any actual effect,
as all HTML elements are positioned absolutely anyway, but to me it
makes for a slightly more intuitive code structure: i.e., we instantiate
the basic elements in the order in which they appear on the screen, and
have all dialog elements underneath.
<a data-ca-tag
href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1586"><img
src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review
on CodeApprove" /></a>
jotaen4tinypilot added a commit that referenced this pull request Aug 24, 2023
Part of #728, stacked onto
#1586.

As demonstrated and discussed [in the original proof-of-concept
branch](#1574), we want to
hide the status bar and menu bar in the popup window [via a query
parameter](https://github.com/tiny-pilot/tinypilot/pull/1574/files#diff-66e482e36d313dd641cd92c815c5fa30fb380bbaa601810827b30a5b84428856R351).
So by appending `?viewMode=standalone` to the TinyPilot URL, this PR
hides the status bar, the menu bar, and the on-screen keyboard.

We will use this in a later PR, when adding the menu item for opening a
“real” popup window.

<img width="783" alt="Screenshot 2023-08-23 at 18 38 09"
src="https://github.com/tiny-pilot/tinypilot/assets/83721279/c3a22b9b-b3b8-49a3-a5b1-88d2368337ec">

The query parameter can also be used independent of the popup window,
which e.g. allows for [the iframe
use-case](#523).

## Notes

- `prettier-ignore` is needed above `<remote-screen>`, since Prettier
cannot handle Jinja2 templates, and would enforce a pretty [bulky
formatting
otherwise](https://github.com/tiny-pilot/tinypilot/pull/1574/files#diff-a18b3b5de30df1bcf7531723d24c214d69b2acff3cd88540e1ff186409879b0aR81-R88).
- I had to pull out a `--bar-padding` CSS variable in the
`<remote-screen>` component, so that we can dynamically override the
value. I forgot that in the [earlier
PR](#1583).
- In `index.html`, I injected the `display: none` via a plain `<style>`
block to hide the elements from the page. Since `index.html` itself
isn’t a web component, I’m not sure we can mimic our usual pattern with
HTML attributes here
([example](https://github.com/tiny-pilot/tinypilot/blob/aa80dbdd3e58e79401008665f2199d98fa14b3d0/app/templates/custom-elements/change-hostname-dialog.html#L12-L14)),
at least not without introducing more complexity and indirection. The
`<style>` block is not super beautiful, but I thought it’s simple and
straightforward at least.
- Note sure `standalone` is the greatest name for the query parameter
value. Potential alternatives: “screen-only”, “focus-mode”,
“dedicated-screen”. (It’s maybe not *that* critical, however… 🤔) In any
event, I found something like `?viewMode=standalone` more descriptive
and flexible than `standaloneMode=true`.
<a data-ca-tag
href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1587"><img
src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review
on CodeApprove" /></a>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants