consoles: Improve VNC dialogs

- The dialogs talk about "server" and "listening" to make that clearer. - There are now placeholder texts to explain the defaults - We use the empty string instead "-1" to signify automatic port assignment in the UI. - The port does validation of its value. - The password field takes the whole row and has can reveal its value.
cockpit-project · Jan 23, 2025 · fe91c79 · fe91c79
1 parent a7aa875
commit fe91c79
Show file tree

Hide file tree

Showing 6 changed files with 175 additions and 61 deletions.
diff --git a/src/components/vm/consoles/consoles.jsx b/src/components/vm/consoles/consoles.jsx
@@ -136,8 +136,9 @@ class Consoles extends React.Component {
             return 'SerialConsole';
         }
 
-        // no console defined
-        return null;
+        // no console defined, but the VncConsole is always there and
+        // will instruct people how to enable it for real.
+        return 'VncConsole';
     }
 
     onDesktopConsoleDownload (type) {
@@ -160,6 +161,8 @@ class Consoles extends React.Component {
             this.onDesktopConsoleDownload(spice ? 'spice' : 'vnc');
         };
 
+        console.log("R");
+
         return (
             <AccessConsoles preselectedType={this.getDefaultConsole()}
                             textSelectConsoleType={_("Select console type")}

diff --git a/src/components/vm/consoles/vncAdd.jsx b/src/components/vm/consoles/vncAdd.jsx
@@ -23,7 +23,7 @@ import { Button, Form, Modal, ModalVariant } from "@patternfly/react-core";
 import { DialogsContext } from 'dialogs.jsx';
 
 import { ModalError } from 'cockpit-components-inline-notification.jsx';
-import { VncRow } from './vncBody.jsx';
+import { VncRow, validateDialogValues } from './vncBody.jsx';
 import { domainAttachVnc, domainGet } from '../../../libvirtApi/domain.js';
 
 const _ = cockpit.gettext;
@@ -40,6 +40,7 @@ export class AddVNC extends React.Component {
             vncPort: "",
             vncPassword: "",
             addVncInProgress: false,
+            validationErrors: { },
         };
         this.add = this.add.bind(this);
         this.onValueChanged = this.onValueChanged.bind(this);
@@ -60,6 +61,12 @@ export class AddVNC extends React.Component {
         const Dialogs = this.context;
         const { vm } = this.props;
 
+        const errors = validateDialogValues(this.state);
+        if (errors) {
+            this.setState({ validationErrors: errors });
+            return;
+        }
+
         this.setState({ addVncInProgress: true });
         const vncParams = {
             connectionName: vm.connectionName,
@@ -84,15 +91,17 @@ export class AddVNC extends React.Component {
 
         const defaultBody = (
             <Form onSubmit={e => e.preventDefault()} isHorizontal>
-                <VncRow idPrefix={idPrefix}
-                                 dialogValues={this.state}
-                                 onValueChanged={this.onValueChanged} />
+                <VncRow
+                    idPrefix={idPrefix}
+                    dialogValues={this.state}
+                    validationErrors={this.state.validationErrors}
+                    onValueChanged={this.onValueChanged} />
             </Form>
         );
 
         return (
             <Modal position="top" variant={ModalVariant.medium} id={`${idPrefix}-dialog`} isOpen onClose={Dialogs.close} className='vnc-add'
-                title={_("Add VNC")}
+                title={_("Add VNC server")}
                 footer={
                     <>
                         <Button isLoading={this.state.addVncInProgress}

diff --git a/src/components/vm/consoles/vncBody.jsx b/src/components/vm/consoles/vncBody.jsx
@@ -17,47 +17,88 @@
  * along with Cockpit; If not, see <http://www.gnu.org/licenses/>.
  */
 
-import React from 'react';
+import React, { useState } from 'react';
 import PropTypes from 'prop-types';
-import { FormGroup, Grid, GridItem, TextInput } from "@patternfly/react-core";
+import {
+    FormGroup, FormHelperText, HelperText, HelperTextItem,
+    Grid, GridItem,
+    InputGroup, TextInput, Button
+} from "@patternfly/react-core";
+import { EyeIcon, EyeSlashIcon } from "@patternfly/react-icons";
 
 import cockpit from 'cockpit';
 
 const _ = cockpit.gettext;
 
-export const VncRow = ({ idPrefix, onValueChanged, dialogValues }) => {
+export const VncRow = ({ idPrefix, onValueChanged, dialogValues, validationErrors }) => {
+    const [showPassword, setShowPassword] = useState(false);
+
     return (
-        <Grid hasGutter md={6}>
-            <GridItem span={6}>
-                <FormGroup fieldId={`${idPrefix}-address`} label={_("VNC address")}>
-                    <TextInput id={`${idPrefix}-address`}
-                           value={dialogValues.vncAddress}
-                           type="text"
-                           onChange={(event) => onValueChanged('vncAddress', event.target.value)} />
-                </FormGroup>
-            </GridItem>
-            <GridItem span={6}>
-                <FormGroup fieldId={`${idPrefix}-port`} label={_("VNC port")}>
-                    <TextInput id={`${idPrefix}-port`}
-                           value={dialogValues.vncPort}
-                           type="number"
-                           onChange={(event) => onValueChanged('vncPort', event.target.value)} />
-                </FormGroup>
-            </GridItem>
-            <GridItem span={6}>
-                <FormGroup fieldId={`${idPrefix}-password`} label={_("VNC password")}>
-                    <TextInput id={`${idPrefix}-password`}
-                           value={dialogValues.vncPassword}
-                           type="password"
-                           onChange={(event) => onValueChanged('vncPassword', event.target.value)} />
-                </FormGroup>
-            </GridItem>
-        </Grid>
+        <>
+            <Grid hasGutter md={6}>
+                <GridItem span={6}>
+                    <FormGroup fieldId={`${idPrefix}-address`} label={_("Listening address")}>
+                        <TextInput id={`${idPrefix}-address`}
+                            value={dialogValues.vncAddress}
+                            type="text"
+                            placeholder={_("default")}
+                            onChange={(event) => onValueChanged('vncAddress', event.target.value)} />
+                    </FormGroup>
+                </GridItem>
+                <GridItem span={6}>
+                    <FormGroup fieldId={`${idPrefix}-port`} label={_("Listening port")}>
+                        <TextInput id={`${idPrefix}-port`}
+                            value={dialogValues.vncPort}
+                            type="text"
+                            placeholder={_("automatic")}
+                            validated={validationErrors.vncPort ? "error" : null}
+                            onChange={(event) => onValueChanged('vncPort', event.target.value)} />
+                        { validationErrors.vncPort &&
+                            <FormHelperText>
+                                <HelperText>
+                                    <HelperTextItem variant='error'>{validationErrors.vncPort}</HelperTextItem>
+                                </HelperText>
+                            </FormHelperText>
+                        }
+                    </FormGroup>
+                </GridItem>
+            </Grid>
+            <FormGroup fieldId={`${idPrefix}-password`} label={_("Password")}>
+                <InputGroup>
+                    <TextInput
+                        id={`${idPrefix}-password`}
+                        type={showPassword ? "text" : "password"}
+                        value={dialogValues.vncPassword}
+                        onChange={(event) => onValueChanged('vncPassword', event.target.value)} />
+                    <Button
+                        variant="control"
+                        onClick={() => setShowPassword(!showPassword)}>
+                        { showPassword ? <EyeSlashIcon /> : <EyeIcon /> }
+                    </Button>
+                </InputGroup>
+            </FormGroup>
+        </>
     );
 };
 
+export function validateDialogValues(values) {
+    const res = { };
+
+    console.log("port", JSON.stringify(values.vncPort), values.vncPort.match("^[0-9]+$"));
+
+    if (values.vncPort == "")
+        ; // fine
+    else if (!values.vncPort.match("^[0-9]+$"))
+        res.vncPort = _("Port must be a positive number.")
+    else if (Number(values.vncPort) < 5900)
+        res.vncPort = _("Port must be 5900 or larger.")
+
+    return Object.keys(res).length > 0 ? res : null;
+}
+
 VncRow.propTypes = {
     idPrefix: PropTypes.string.isRequired,
     onValueChanged: PropTypes.func.isRequired,
     dialogValues: PropTypes.object.isRequired,
+    validationErrors: PropTypes.object.isRequired,
 };
diff --git a/src/components/vm/consoles/vncEdit.jsx b/src/components/vm/consoles/vncEdit.jsx
@@ -23,7 +23,7 @@ import { Button, Form, Modal, ModalVariant } from "@patternfly/react-core";
 
 import { ModalError } from 'cockpit-components-inline-notification.jsx';
 import { DialogsContext } from 'dialogs.jsx';
-import { VncRow } from './vncBody.jsx';
+import { VncRow, validateDialogValues } from './vncBody.jsx';
 import { domainChangeVncSettings, domainGet } from '../../../libvirtApi/domain.js';
 
 const _ = cockpit.gettext;
@@ -41,8 +41,9 @@ export class EditVNCModal extends React.Component {
             vmId: props.vmId,
             connectionName: props.connectionName,
             vncAddress: props.consoleDetail.address || "",
-            vncPort: props.consoleDetail.port || "",
+            vncPort: Number(props.consoleDetail.port) == -1 ? "" : props.consoleDetail.port || "",
             vncPassword: props.consoleDetail.password || "",
+            validationErrors: { },
         };
 
         this.save = this.save.bind(this);
@@ -51,7 +52,7 @@ export class EditVNCModal extends React.Component {
     }
 
     onValueChanged(key, value) {
-        const stateDelta = { [key]: value };
+        const stateDelta = { [key]: value, validationErrors: { [key]: null } };
         this.setState(stateDelta);
     }
 
@@ -62,6 +63,12 @@ export class EditVNCModal extends React.Component {
     save() {
         const Dialogs = this.context;
 
+        const errors = validateDialogValues(this.state);
+        if (errors) {
+            this.setState({ validationErrors: errors });
+            return;
+        }
+
         const vncParams = {
             connectionName: this.state.connectionName,
             vmName: this.state.vmName,
@@ -86,17 +93,19 @@ export class EditVNCModal extends React.Component {
 
         const defaultBody = (
             <Form onSubmit={e => e.preventDefault()} isHorizontal>
-                <VncRow idPrefix={idPrefix}
-                                 dialogValues={this.state}
-                                 onValueChanged={this.onValueChanged} />
+                <VncRow
+                    idPrefix={idPrefix}
+                    dialogValues={this.state}
+                    validationErrors={this.state.validationErrors}a
+                    onValueChanged={this.onValueChanged} />
             </Form>
         );
         const showWarning = () => {
         };
 
         return (
             <Modal position="top" variant={ModalVariant.medium} id={`${idPrefix}-dialog`} isOpen onClose={Dialogs.close} className='vnc-edit'
-                   title={_("Edit VNC settings")}
+                   title={_("Edit VNC server settings")}
                    footer={
                        <>
                            <Button isDisabled={this.state.saveDisabled} id={`${idPrefix}-save`} variant='primary' onClick={this.save}>

diff --git a/src/components/vm/vmDetailsPage.jsx b/src/components/vm/vmDetailsPage.jsx
@@ -132,24 +132,22 @@ export const VmDetailsPage = ({
             title: _("Usage"),
             body: <VmUsageTab vm={vm} />,
         },
-        ...(vm.displays.length
-            ? [{
-                id: `${vmId(vm.name)}-consoles`,
-                className: "consoles-card",
-                title: _("Console"),
-                actions: vm.state != "shut off"
-                    ? <Button variant="link"
-                          onClick={() => {
-                              const urlOptions = { name: vm.name, connection: vm.connectionName };
-                              return cockpit.location.go(["vm", "console"], { ...cockpit.location.options, ...urlOptions });
-                          }}
-                          icon={<ExpandIcon />}
-                          iconPosition="right">{_("Expand")}</Button>
-                    : null,
-                body: <Consoles vm={vm} config={config}
-                            onAddErrorNotification={onAddErrorNotification} />,
-            }]
-            : []),
+        {
+            id: `${vmId(vm.name)}-consoles`,
+            className: "consoles-card",
+            title: _("Console"),
+            actions: vm.state != "shut off"
+                ? <Button variant="link"
+                      onClick={() => {
+                          const urlOptions = { name: vm.name, connection: vm.connectionName };
+                          return cockpit.location.go(["vm", "console"], { ...cockpit.location.options, ...urlOptions });
+                      }}
+                      icon={<ExpandIcon />}
+                      iconPosition="right">{_("Expand")}</Button>
+                : null,
+            body: <Consoles vm={vm} config={config}
+                      onAddErrorNotification={onAddErrorNotification} />,
+        },
         {
             id: `${vmId(vm.name)}-disks`,
             className: "disks-card",

diff --git a/test/check-machines-consoles b/test/check-machines-consoles
@@ -19,6 +19,7 @@
 
 import os
 import time
+import xml.etree.ElementTree as ET
 
 import machineslib
 import testlib
@@ -253,6 +254,59 @@ fullscreen=0
         self.allow_browser_errors("Disconnection timed out.",
                                   "Failed when connecting: Connection closed")
 
+    def testAddEditVNC(self):
+        b = self.browser
+
+        # Create a machine without any consoles
+
+        name = "subVmTest1"
+        self.createVm(name)
+
+        self.login_and_go("/machines")
+        self.waitPageInit()
+        self.waitVmRow(name)
+        self.goToVmPage(name)
+
+        # "Console" card shows empty state
+
+        b.wait_in_text(f"#vm-{name}-consoles .pf-v5-c-empty-state", "Graphical console not supported")
+
+        # Shut down machine, card shows a button to add VNC
+
+        self.performAction("subVmTest1", "forceOff")
+        b.wait_in_text("#vm-not-running-message", "Graphical console: not supported")
+        b.click("#vm-not-running-message button:contains(Add support)")
+
+        b.wait_visible("#add-vnc-dialog")
+        b.set_input_text("#add-vnc-address", "0.0.0.0")
+        b.set_input_text("#add-vnc-port", "5000")
+        b.click("#add-vnc-add")
+        b.wait_visible("#add-vnc-dialog .pf-m-error:contains('Port must be 5900 or larger.')")
+        b.set_input_text("#add-vnc-port", "5901")
+        b.click("#add-vnc-add")
+        b.wait_not_present("#add-vnc-dialog")
+
+        b.wait_in_text("#vm-not-running-message", "Graphical console: supported, port 5901")
+
+        root = ET.fromstring(self.machine.execute(f"virsh dumpxml {name}"))
+        graphics = root.find('devices').findall('graphics')
+        self.assertEqual(len(graphics), 1)
+        self.assertEqual(graphics[0].get('port'), "5901")
+        self.assertEqual(graphics[0].get('listen'), "0.0.0.0")
+
+        b.click("#vm-not-running-message button:contains(Edit)")
+        b.wait_visible("#edit-vnc-dialog")
+        b.set_input_text("#edit-vnc-address", "")
+        b.set_input_text("#edit-vnc-port", "")
+        b.click("#edit-vnc-save")
+        b.wait_not_present("#edit-vnc-dialog")
+
+        root = ET.fromstring(self.machine.execute(f"virsh dumpxml {name}"))
+        graphics = root.find('devices').findall('graphics')
+        self.assertEqual(len(graphics), 1)
+        self.assertEqual(graphics[0].get('port'), "-1")
+        self.assertEqual(graphics[0].get('listen'), None)
+
 
 if __name__ == '__main__':
     testlib.test_main()