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

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 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
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
4 changes: 4 additions & 0 deletions src/components/common/needsShutdown.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,10 @@ export function needsShutdownSpice(vm) {
return vm.hasSpice !== vm.inactiveXML.hasSpice;
}

export function needsShutdownVideo(vm, video) {
Copy link
Member

Choose a reason for hiding this comment

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

Not used anymore, please remove.

return false;
}

export function getDevicesRequiringShutdown(vm) {
if (!vm.persistent)
return [];
Expand Down
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
136 changes: 109 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,18 @@
};

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

Check warning

Code scanning / CodeQL

Unused or undefined state property Warning

Component state property 'addVncInProgress' is
written
Loading
, but it is never read.
Component state property 'addVncInProgress' is
written
Loading
, but it is never read.
Component state property 'addVncInProgress' is
written
Loading
, but it is never read.
constructor (props) {
super(props);

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

Choose a reason for hiding this comment

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

As COdeQL says: addVncProgress is never read, we can remove it.

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 +74,10 @@
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 +108,116 @@
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;

this.setState({ addVncInProgress: true });
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))
.finally(() => this.setState({ addVncInProgress: false }));
}

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;
55 changes: 52 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 @@ -48,18 +52,26 @@
KEY_DELETE: 111,
};

class Vnc extends React.Component {

Check warning

Code scanning / CodeQL

Unused or undefined state property Warning

Component state property 'vncAddress' is
written
Loading
, but it is never read.
Component state property 'vncPort' is
written
Loading
, but it is never read.
Component state property 'vncPassword' is
written
Loading
, but it is never read.
Component state property 'dialogError' is
written
Loading
, but it is never read.
Component state property 'dialogErrorDetail' is
written
Loading
, but it is never read.
static contextType = DialogsContext;

constructor(props) {
super(props);

this.state = {
path: undefined,
isActionOpen: false,
vncAddress: props.consoleDetail.address || '',
Copy link
Member

Choose a reason for hiding this comment

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

Not used anymore.

vncPort: props.consoleDetail.port || '',
vncPassword: props.consoleDetail.password || '',
};

this.connect = this.connect.bind(this);
this.onDisconnected = this.onDisconnected.bind(this);
this.onInitFailed = this.onInitFailed.bind(this);
this.onExtraKeysDropdownToggle = this.onExtraKeysDropdownToggle.bind(this);
this.onValueChanged = this.onValueChanged.bind(this);
this.dialogErrorSet = this.dialogErrorSet.bind(this);
}

connect(props) {
Expand Down Expand Up @@ -114,7 +126,17 @@
this.setState({ isActionOpen: false });
}

onValueChanged(key, value) {
Copy link
Member

Choose a reason for hiding this comment

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

Unused.

const stateDelta = { [key]: value };
this.setState(stateDelta);
}

dialogErrorSet(text, detail) {
Copy link
Member

Choose a reason for hiding this comment

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

Unused.

this.setState({ dialogError: text, dialogErrorDetail: detail });
}

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 +183,17 @@
</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 +209,29 @@
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