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.

Sign up for GitHub

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

Add "view, edit, and add Video devices" to detailspage #1795

Closed
wants to merge 10 commits into from
+545 −42
Closed

Add "view, edit, and add Video devices" to detailspage #1795

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
be1a210
Add "view, edit, and add Video devices" to detailspage
Shotaro-Kawaguchi Aug 30, 2024
ef911a8
Fixed virt-xml command to avoid using --update for video devices.
Shotaro-Kawaguchi Sep 6, 2024
639c8a2
Modified to target only VNC devices for view and editing of video dev…
Shotaro-Kawaguchi Sep 6, 2024
afc6b32
Modified to hide the "Add video device" button when a VNC device is p…
Shotaro-Kawaguchi Sep 6, 2024
7577f3c
Change UI regarding vnc devices
Shotaro-Kawaguchi Nov 22, 2024
74b25dd
Fix the layout of the modal form
Shotaro-Kawaguchi Dec 6, 2024
499adeb
Fix display of VNC address and port
Shotaro-Kawaguchi Dec 6, 2024
748b2bc
Remove unused code
Shotaro-Kawaguchi Jan 22, 2025
69ef1bf
Remove unused code 'addVncInProgress'
Shotaro-Kawaguchi Jan 22, 2025
904d92e
Remove unused code
Shotaro-Kawaguchi Jan 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/components/vm/consoles/consoles.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
.pf-v5-c-console__vnc > div,
.pf-v5-c-console__vnc > div > div {
inline-size: 100%;
block-size: 100%;
block-size: 90%;
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason for this? To make space for the line with the VNC details and Edit button? I think we need to find a better solution in that case.

}

.pf-v5-c-console__actions {
Expand Down
133 changes: 106 additions & 27 deletions src/components/vm/consoles/consoles.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,20 @@
import React from 'react';
import PropTypes from 'prop-types';
import cockpit from 'cockpit';
import { AccessConsoles } from "@patternfly/react-console";
import { vmId } from "../../../helpers.js";
import { Button, Text, TextContent, TextVariants } from '@patternfly/react-core';
import { DialogsContext } from 'dialogs.jsx';
import { AddVNC } from './vncAdd.jsx';
import { EditVNCModal } from './vncEdit.jsx';

import SerialConsole from './serialConsole.jsx';
import Vnc from './vnc.jsx';
import DesktopConsole from './desktopConsole.jsx';
import {
domainAttachVnc,
domainCanConsole,
domainDesktopConsole,
domainGet,
domainSerialConsoleCommand
} from '../../../libvirtApi/domain.js';

Expand All @@ -43,11 +49,17 @@ const VmNotRunning = () => {
};

class Consoles extends React.Component {
static contextType = DialogsContext;

constructor (props) {
super(props);

this.state = {
serial: props.vm.displays && props.vm.displays.filter(display => display.type == 'pty'),
selectedConsole: this.getDefaultConsole(props.vm),
vncAddress: "",
Copy link
Member

Choose a reason for hiding this comment

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

Also, these three can be removed when add() is removed.

vncPort: "",
vncPassword: "",
};

this.getDefaultConsole = this.getDefaultConsole.bind(this);
Expand All @@ -61,6 +73,10 @@ class Consoles extends React.Component {
if (newSerial.length !== oldSerial.length || oldSerial.some((pty, index) => pty.alias !== newSerial[index].alias))
return { serial: newSerial };

if (nextProps.selectedConsole !== prevState.selectedConsole) {
Copy link
Member

Choose a reason for hiding this comment

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

This looks problematic. The getDerivedStateFromProps function might have already returned a new state before even reaching this. I think both serial and selectedConsole need to be processed on every call.

Copy link
Member

Choose a reason for hiding this comment

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

But: selectedConsole should not be in state in the first place. See below.

return { selectedConsole: nextProps.selectedConsole };
}

return null;
}

Expand Down Expand Up @@ -91,51 +107,114 @@ class Consoles extends React.Component {
domainDesktopConsole({ name: vm.name, id: vm.id, connectionName: vm.connectionName, consoleDetail: vm.displays.find(display => display.type == type) });
}

add() {
Copy link
Member

Choose a reason for hiding this comment

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

This function is not used, I think. Let's remove it.

const Dialogs = this.context;
const { vm } = this.props;

const vncParams = {
connectionName: vm.connectionName,
vmName: vm.name,
vncAddress: this.state.vncAddress || "",
vncPort: this.state.vncPort || "",
vncPassword: this.state.vncPassword || "",
};

domainAttachVnc(vncParams)
.then(() => {
domainGet({ connectionName: vm.connectionName, id: vm.id });
Dialogs.close();
})
.catch(exc => this.dialogErrorSet(_("Video device settings could not be saved"), exc.message))
}

render () {
const Dialogs = this.context;
const { vm, onAddErrorNotification, isExpanded } = this.props;
const { serial } = this.state;
const { serial, selectedConsole } = this.state;
Copy link
Member

Choose a reason for hiding this comment

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

selectedConsole should not be in state, IMO. It's already in props, and we can just use it from there.

const spice = vm.displays && vm.displays.find(display => display.type == 'spice');
const vnc = vm.displays && vm.displays.find(display => display.type == 'vnc');
const id = vmId(vm.name);

const openVncAdd = () => {
Dialogs.show(<AddVNC idPrefix={`${id}-add-vnc`}
vm={vm} />);
};

const openVncEdit = () => {
Dialogs.show(<EditVNCModal idPrefix={`${id}-edit-vnc`}
vmName={vm.name}
vmId={id}
connectionName={vm.connectionName}
consoleDetail={vnc} />);
};

if (!domainCanConsole || !domainCanConsole(vm.state)) {
return (<VmNotRunning />);
return (
<>
<VmNotRunning />
{selectedConsole === 'VncConsole' && !vnc && (
<Button variant="secondary" size="sm"
onClick={openVncAdd}>
{_("Add VNC")}
</Button>
)}
{selectedConsole === 'VncConsole' && vnc && (
<TextContent>
<Text component={TextVariants.p}>
{`VNC address=${vnc.address} port=${vnc.port === '-1' ? 'auto' : vnc.port} `}
Copy link
Member

Choose a reason for hiding this comment

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

It is confusing that "-1" is translated to "auto" here, but the dialogs still expose the raw "-1" number.

<Button variant="link"
onClick={openVncEdit}>
{_("Edit")}
</Button>
</Text>
</TextContent>
)}
</>
);
}

const onDesktopConsole = () => { // prefer spice over vnc
this.onDesktopConsoleDownload(spice ? 'spice' : 'vnc');
};

return (
<AccessConsoles preselectedType={this.getDefaultConsole()}
textSelectConsoleType={_("Select console type")}
textSerialConsole={_("Serial console")}
textVncConsole={_("VNC console")}
textDesktopViewerConsole={_("Desktop viewer")}>
{serial.map((pty, idx) => (<SerialConsole type={serial.length == 1 ? "SerialConsole" : cockpit.format(_("Serial console ($0)"), pty.alias || idx)}
key={"pty-" + idx}
connectionName={vm.connectionName}
vmName={vm.name}
spawnArgs={domainSerialConsoleCommand({ vm, alias: pty.alias })} />))}
{vnc &&
<Vnc type="VncConsole"
vmName={vm.name}
vmId={vm.id}
connectionName={vm.connectionName}
consoleDetail={vnc}
onAddErrorNotification={onAddErrorNotification}
isExpanded={isExpanded} />}
{(vnc || spice) &&
<DesktopConsole type="DesktopViewer"
onDesktopConsole={onDesktopConsole}
vnc={vnc}
spice={spice} />}
</AccessConsoles>
<>
{selectedConsole === 'SerialConsole' && serial.map((pty, idx) => (
<SerialConsole type={serial.length == 1 ? "SerialConsole" : cockpit.format(_("Serial console ($0)"), pty.alias || idx)}
Copy link
Member

Choose a reason for hiding this comment

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

When there is more than one serial console, this will just show them all, no? I think there needs to be dropdown here to select one of them when there are multiple.

key={"pty-" + idx}
connectionName={vm.connectionName}
vmName={vm.name}
spawnArgs={domainSerialConsoleCommand({ vm, alias: pty.alias })} />
))}
{selectedConsole === 'VncConsole' && !vnc && (
<Button variant="secondary" size="sm"
onClick={openVncAdd}>
{_("Add VNC")}
</Button>
)}
{selectedConsole === 'VncConsole' && vnc && (
<Vnc type="VncConsole"
vmName={vm.name}
vmId={vm.id}
connectionName={vm.connectionName}
consoleDetail={vnc}
onAddErrorNotification={onAddErrorNotification}
isExpanded={isExpanded} />
)}
{selectedConsole === 'DesktopViewer' && (vnc || spice) && (
<DesktopConsole type="DesktopViewer"
onDesktopConsole={onDesktopConsole}
vnc={vnc}
spice={spice} />
)}
</>
);
}
}
Consoles.propTypes = {
vm: PropTypes.object.isRequired,
onAddErrorNotification: PropTypes.func.isRequired,
selectedConsole: PropTypes.string.isRequired,
};

export default Consoles;
41 changes: 38 additions & 3 deletions src/components/vm/consoles/vnc.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,18 @@
* along with Cockpit; If not, see <http://www.gnu.org/licenses/>.
*/
import React from 'react';
import PropTypes from 'prop-types';
import cockpit from 'cockpit';

import { logDebug } from "../../../helpers.js";
import { VncConsole } from '@patternfly/react-console';
import { Dropdown, DropdownItem, DropdownList } from "@patternfly/react-core/dist/esm/components/Dropdown";
import { MenuToggle } from "@patternfly/react-core/dist/esm/components/MenuToggle";
import { Divider } from "@patternfly/react-core/dist/esm/components/Divider";
import { Button, Text, TextContent, TextVariants } from '@patternfly/react-core';
import { DialogsContext } from 'dialogs.jsx';
import { EditVNCModal } from './vncEdit.jsx';

import { logDebug } from '../../../helpers.js';
import { domainSendKey } from '../../../libvirtApi/domain.js';

const _ = cockpit.gettext;
Expand All @@ -49,8 +53,11 @@ const Enum = {
};

class Vnc extends React.Component {
static contextType = DialogsContext;

constructor(props) {
super(props);

this.state = {
path: undefined,
isActionOpen: false,
Expand Down Expand Up @@ -115,6 +122,7 @@ class Vnc extends React.Component {
}

render() {
const Dialogs = this.context;
const { consoleDetail, connectionName, vmName, vmId, onAddErrorNotification, isExpanded } = this.props;
const { path, isActionOpen } = this.state;
if (!consoleDetail || !path) {
Expand Down Expand Up @@ -161,8 +169,17 @@ class Vnc extends React.Component {
</Dropdown>
];

const openVncEdit = () => {
Dialogs.show(<EditVNCModal idPrefix={`${vmId}-edit-vnc`}
vmName={vmName}
vmId={vmId}
connectionName={connectionName}
consoleDetail={consoleDetail} />);
};

return (
<VncConsole host={window.location.hostname}
<>
<VncConsole host={window.location.hostname}
port={window.location.port || (encrypt ? '443' : '80')}
path={path}
encrypt={encrypt}
Expand All @@ -178,11 +195,29 @@ class Vnc extends React.Component {
consoleContainerId={isExpanded ? "vnc-display-container-expanded" : "vnc-display-container-minimized"}
resizeSession
scaleViewport
/>
/>
<TextContent>
<Text component={TextVariants.p}>
{`VNC address=${consoleDetail.address} port=${consoleDetail.port} `}
<Button variant="link"
onClick={openVncEdit}>
{_("Edit")}
</Button>
</Text>
</TextContent>
</>
);
}
}

// TODO: define propTypes
Vnc.propTypes = {
vmName: PropTypes.string.isRequired,
vmId: PropTypes.string.isRequired,
connectionName: PropTypes.string.isRequired,
consoleDetail: PropTypes.object.isRequired,
onAddErrorNotification: PropTypes.func.isRequired,
isExpanded: PropTypes.string.isRequired,
};

export default Vnc;
Loading