Skip to content

shell: Show some pages in fullscreen #21670

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mvollmer
Copy link
Member

@mvollmer mvollmer commented Mar 5, 2025

The Machines application of Cockpit lets people interact with the graphical console of a virtual machine. This is useful for baby sitting a VM through its boot process or doing the occasional rescue work, but it might also be used to do real day-to-day work in a graphical desktop environment that runs in the VM.

The graphical console in Cockpit Machines is fundamentally limited by having to run in a browser. The browser might eat some keys, for example, and serious work might thus require a external viewer application such as GNOME Connections, TigerVNC, or virt-viewer. Nevertheless, we should make Cockpit's own viewer work as well as possible.

One aspect of improving our own viewer is to give more space to the actual console, and less to Cockpit's own UI. Cockpit Machines can already "expand" the console to fill most of the content iframe.

Purpose

The purpose of this task is to make it possible to remove as much of the Shell UI elements that normally surround the content iframe as possible when people are interacting with the expanded view of a VM console.


Demo: https://youtu.be/9kDEZkvI2eo

This needs a change to the cockpit-machines manifest to recreate the demo. There is also a new "fullscreen" playground.

Some questions:

  • Should every page be able to go fullscreen? Then we need a reliable way to get out of fullscreen again, and can't rely on the page to offer it itself. We always need to show a little bit of Shell UI with a "go back to normal" action in it. We of course also need to find a place for the "go fullscreen" action.

  • Should fullscreen-ness be part of the navigation state? If you click on something that make the shell go fullscreen, and then you click on the Browser back button, do you go back to normal mode?

  • If we have a "go fullscreen" action in the Shell itself and you activate that on the details page for a VM, should the page automatically switch to showing only the console? I don't think we can do that since people might want to use all of Machines fullscreen, including the details page. So we would have two "go fullscreen" actions that behave differently. (One in the Shell, one in the Console card.) I think that can be ok.

  • Likewise, we would have two "go normal" buttons when the console has been expanded: the one in the Shell would just add the Shell chrome back around the expanded console, the one in the expanded console would also navigate back to the details page. This gets confusing, no?

To avoid answering these questions, the current code here does not add a general "any page can be fullscreen" feature to the Shell. Only specific pages will (always) be fullscreen, and those pages are expected to have a "back to normal" action.

  • Docs for the new "fullscreen" manifest field.

@mvollmer mvollmer added the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Mar 5, 2025
@mvollmer mvollmer removed the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Mar 6, 2025
@@ -1047,6 +1047,39 @@
b.wait_visible("#fsreplace1-create")
set_content_and_validate(path, "user data", custom_context)

def testFullscreen(self):
b = self.browser
m = self.machine

Check notice

Code scanning / CodeQL

Unused local variable Note test

Variable m is not used.
@mvollmer mvollmer changed the title shell: Full browser! shell: Show some pages in fullscreen Mar 6, 2025
let fs = this.current_manifest.fullscreen;
const path_prefix = location.path.split("/", 1)[0];
if (Array.isArray(fs))
fs = { "": fs };
Copy link
Contributor

Choose a reason for hiding this comment

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

This added line is not executed by any test.

if (Array.isArray(fs))
fs = { "": fs };
for (const path_suffix in fs) {
if ((path_suffix == "" && path_prefix == location.path)
Copy link
Contributor

Choose a reason for hiding this comment

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

This added line is not executed by any test.

@mvollmer mvollmer assigned mvollmer and garrett and unassigned mvollmer Apr 2, 2025
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