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

Pop out remote screen #728

Closed
mtlynch opened this issue Jun 22, 2021 · 5 comments · Fixed by #1590
Closed

Pop out remote screen #728

mtlynch opened this issue Jun 22, 2021 · 5 comments · Fixed by #1590
Assignees
Labels
enhancement New feature or request medium

Comments

@mtlynch
Copy link
Contributor

mtlynch commented Jun 22, 2021

I received a request from a custom to offer the ability to pop out the remote screen to its own window. I know this is a feature that's available in VM managers like Proxmox, and it could be a useful feature in TinyPilot.

We should have a View option that pops out a minimal browser window that displays just the remote screen without any other menus, similar to what Proxmox does in displaying a new console connection.

image

Proxmox also causes the browser to hide the forward/back buttons and bookmark bar, which I'm not sure how they're doing but it would be good to apply similar behavior.

This is a bit similar to #523, but they're not quite the same feature. But implementing one would likely bring us very close to the other. (From thinking about this more, I feel like they effectively are the same feature)

@jotaen4tinypilot
Copy link
Contributor

jotaen4tinypilot commented Aug 21, 2023

Notes to self from the POC branch:

  • Consider wording of menu item. Candidate: “Dedicated Window”
  • Disable main remote screen while popup is active
    • It’s probably too complicated to track the open state of the popup dynamically, so we could just turn off the main remote screen and say something along the lines of: “Remote screen disabled due to dedicated window. Refresh page to get back the remote screen.” The user could of course still refresh the main window while the popup is open, but that’s okay, since this is only a convenience feature.
    • We also need to “mute” the keyboard/mouse forwarding from the main window while the popup is open. (We currently don’t have such functionality yet, so we need to implement it first.)

More notes to self:

  • Remove stray <div> from index.html

jotaen4tinypilot added a commit that referenced this issue 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](https://github.com/tiny-pilot/tinypilot/blob/069cf2d9829a4fe2b8df33cb03b26c141c60593b/app/templates/custom-elements/remote-screen.html#L11-L33),
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


https://github.com/tiny-pilot/tinypilot/assets/83721279/66758c7c-ce78-4f0a-8323-4990be47fe49

## Demo: Before


https://github.com/tiny-pilot/tinypilot/assets/83721279/f768b3c0-8b5b-44c3-8cbd-2ee724e46a02

## Notes

- We had introduced these breakpoints in
#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 #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.
<a data-ca-tag
href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1583"><img
src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review
on CodeApprove" /></a>
@jotaen4tinypilot
Copy link
Contributor

jotaen4tinypilot commented Aug 23, 2023

I just realized that (while the popup is open) in addition to hiding the remote screen and “muting” keyboard input on the main window, we also should consider what to do with other interactive elements on the main window:

  • On-screen keyboard: should the user be able to use the on-screen keyboard, to send keystrokes to the target machine?
  • Menu items: should the user be able to use actions from the menu? E.g. Virtual Media, changing the hostname, but also pasting, or sending “Alt+Ctrl+Del”?

Unless you have a clear preference here @mtlynch (since I don’t right now), we could consider to defer these questions, and only start out with hiding the remote screen and muting the keyboard input in the first step. Then we could see how it feels in real life, and optimize the experience later.

@mtlynch
Copy link
Contributor Author

mtlynch commented Aug 23, 2023

@jotaen4tinypilot - I think we can leave those as-is now.

My thinking with hiding the remote screen and muting keystrokes is that the duplicate screens would likely confuse users, and the user might not expect keystrokes to forward when they don't see the remote screen.

I'm less concerned with menus and other stuff because it's hard to interact with them accidentally, and I'd imagine if the user is interacting with them, they expect them to have their normal effect. Like, there's no reason to click Actions > Keyboard Shortcuts > Ctrl + Alt + Del if you expect that the functionality is disabled due to the popout screen.

jotaen4tinypilot added a commit that referenced this issue 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 issue 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>
@jotaen4tinypilot
Copy link
Contributor

jotaen4tinypilot commented Aug 24, 2023

I ran into a pretty tricky issue here, unfortunately. The good news is, however, that we might be able to work around the problem with a pretty pragmatic, unconventional approach. Therefore, I wanted to discuss things first, before opening a PR.

Problem

The issue is around disabling the remote screen on the main window. I realized that this isn’t just a mere convenience or a matter of aesthetics, but it’s actually a technical necessity to prevent duplicate video streaming, and hence duplicate bandwidth consumption.

Disabling the MJPEG stream is relatively simple, because the MJPEG stream stops as soon as we’d remove the src attribute of the <img> element. It’s not that simple for H.264, however: Janus appears to continue delivering the full video signal, no matter what I tried, be it pausing the <video> element via JS, removing the <video> element from the DOM, or even removing the entire <remote-screen> component from the DOM. It seems as if we’d have to issue a separate procedure to get Janus to stop sending data. To make matters worse, the Janus connection itself isn’t controlled from within the <remote-screen> component, but it’s actually having its own lifecycle that’s entirely facilitated via the webrtc-video.js script.

So apart from the fact that I couldn’t figure out so far how to make Janus stop reliably, I’m also worried that the control flows in the code would become complex and brittle. I eventually realized that the entire code of the frontend is set up under the implicit assumption that the app is running perpetually as long as the window is open. We don’t have any explicit teardown mechanisms for stopping keyboard input, stopping video, or deliberately disconnecting from the backend.

We also make some pragmatic assumptions in the code, such as that there is either MJPEG or H.264 enabled, but we don’t cover the case where it’s neither of those, i.e. when video isn’t available at all. Once we’d halt and hide the remote screen on the main window, we can’t know what the current streaming mode is, and hence the streaming-mode information in the status bar would be outdated or unreliable.

I’m not sure it’s worth to try to implement such teardown mechanisms, and harden the app against all these sorts of quirks, only for the sake of displaying a placeholder after opening the dedicated window. We wouldn’t need these mechanisms and control flows anywhere else at the moment, so I’d expect a rather high price in effort, code complexity, and error-proneness, for a rather low overall value.

Proposed Solution

To avoid the hassle that might stem from the dual window situation described above, we could provide the following experience:

Screen.Recording.2023-08-24.at.20.08.10.mov

This branch contains a working implementation of this approach.

  • When clicking “Dedicated Window” in the “View” menu, we open the popup window with ?viewMode=standalone.
  • In the main window, we issue an external (!) redirect to /dedicated-window-placeholder, which is a new and completely separate page. That’s how we’d effectively accomplish the “teardown” of the main window, and get all the following things for free:
    • All video signal is sure to be stopped
    • We aren’t connected to the web socket endpoint anymore
    • No keystrokes, mouse, or touch events can be forwarded
    • The status bar and menu items are gone, so we don’t risk showing inconsistent information
  • Since it might feel weird for the user to see /dedicated-window-placeholder (or whatever) as path in their address bar, the dedicated placeholder page dynamically erases the path from the URL history right away. That effectively makes it look as if the user stayed on the main page, and /dedicated-window-placeholder also wouldn’t appear in the browser history.
  • Since the URL continues to be /, the user can refresh their page as normal, to get back to the “real” TinyPilot web app.

I think the price in terms of code complexity is fairly low, and most of the “ugliness” of this approach is contained in one single file (dedicated-window-placeholder.html). Apart from some additional commentary, the branch would basically be good to go as is.

Placeholder Wording

Completely independent of the technicalities, it would be cool if you could revise the text of the placeholder message. My current, rather ragged draft is this:

The remote screen has been opened in a dedicated window.
You can close this window to continue using TinyPilot in the dedicated window.
If you want to restore this window to its original state, please refresh the page.

I find it borderline verbose, and it’s also confusing to talk about “this window” and “the dedicated window”, since it sounds very similar and might create ambiguity.

@mtlynch
Copy link
Contributor Author

mtlynch commented Aug 24, 2023

Oof, lots of hidden complexity in features this cycle!

I like your proposed solution. It feels hacky in principle but the implementation is pretty tidy and unobtrusive. We always have the option to do a more elegant teardown later if we feel like we have to.

it would be cool if you could revise the text of the placeholder message.

How about this?

The remote screen is displaying in another browser window.

To restore the remote screen in this window, please refresh the page.

jotaen4tinypilot added a commit that referenced this issue Aug 28, 2023
Part of #728.

This PR adds an item to the “View” menu, which is labelled “Dedicated
Window”, and which opens the remote screen in “standalone” view mode in
a popup window.


https://github.com/tiny-pilot/tinypilot/assets/83721279/f6bdbdf5-5387-4f6a-8401-9b33f92fb5cb

## Notes

- As discussed in [this issue
comment](#728 (comment)),
we decided to use a rather pragmatic and slightly unconventional
approach for hiding the remote screen from the main (parent) window, by
issuing an external redirect to a separate page. A “clean” and proper
teardown mechanism, which would allow us to stay on the main page, would
probably be rather complex to build, and also brittle to maintain.
- I’ve debated whether to use an `btn-action` (blue) or a `btn-success`
(green) for the “Refresh Page” button. I eventually went with the
former, since the action is optional, and the button is only a
convenience.
<a data-ca-tag
href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1589"><img
src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review
on CodeApprove" /></a>
jotaen4tinypilot added a commit that referenced this issue Aug 28, 2023
Resolves #728. Stacked on
#1589.

For convenience and smoother UX, this PR detects the size of the remote
screen in order to intialize the popup window with the exact same
dimensions as the remote screen. That also ensures that the remote
screen fits into the popup window exactly, with no extra padding.
<a data-ca-tag
href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1590"><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
Labels
enhancement New feature or request medium
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants