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

Jump to bottom

adds VM (domain) title and description to UI #1001

Closed
wants to merge 6 commits into from
Closed

adds VM (domain) title and description to UI #1001

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
dbe8909
adds VM (domain) title and description to UI
Britz Mar 31, 2023
ace05db
Adds Flex element with spaceItems
Britz Apr 3, 2023
fce1c87
fixes setter names and element ids
Britz Apr 3, 2023
c928110
refacoring functions to use domainSetXML
Britz Apr 3, 2023
90864c9
adds check VM status for title and description
Britz Apr 3, 2023
f7ef41b
Merge branch 'cockpit-project:main' into title_and_description
Apr 5, 2023
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
26 changes: 26 additions & 0 deletions po/de.po
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3454,3 +3454,29 @@ msgstr "Ja"

#~ msgid "show more"
#~ msgstr "Zeig mehr"

#: src/components/vm/overview/descriptionModal.jsx

#~ msgid "Description could not be changed"
#~ msgstr "Beschreibung konnte nicht geändert werden"

#~ msgid "New description"
#~ msgstr "Neue Beschreibung"

#~ msgid "Set description of $0"
#~ msgstr "Beschreibung von $0 festlegen"

#: src/components/vm/overview/titleModal.jsx

#~ msgid "Title could not be changed"
#~ msgstr "Titel konnte nicht geändert werden"

#~ msgid "New title"
#~ msgstr "Neuer Titel"

#~ msgid "Set title of $0"
#~ msgstr "Titel von $0 festlegen"

#: src/components/vm/overview/vmOverviewCard.jsx:256
#~ msgid "Title"
#~ msgstr "Titel"
64 changes: 64 additions & 0 deletions src/components/vm/overview/descriptionModal.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
import React, { useState } from 'react';
import cockpit from 'cockpit';
import { Button } from "@patternfly/react-core/dist/esm/components/Button/index.js";
import { Form, FormGroup } from "@patternfly/react-core/dist/esm/components/Form/index.js";
import { TextInput } from "@patternfly/react-core/dist/esm/components/TextInput/index.js";
import { Modal } from "@patternfly/react-core/dist/esm/components/Modal/index.js";
import { useDialogs } from 'dialogs.jsx';

import { ModalError } from 'cockpit-components-inline-notification.jsx';
import { domainSetDescription } from '../../../libvirtApi/domain.js';

const _ = cockpit.gettext;

export const DescriptionModal = ({ vm }) => {
const Dialogs = useDialogs();
const [error, setError] = useState({});
const [newDescription, setDescription] = useState(vm.description);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a nitpick - but we like to use the same name for the variable as the setter. Just use const [description, setDescription] = ...

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense. I will change it, but i will use [newDescription, setNewDescription] to be consistent with the rename dialog code.

const [isLoading, setIsLoading] = useState(false);

function save() {
setIsLoading(true);
domainSetDescription({
name: vm.name,
id: vm.id,
connectionName: vm.connectionName,
description: newDescription
}).then(Dialogs.close, exc => {
setIsLoading(false);
setError({ dialogError: _("Description could not be changed"), dialogErrorDetail: exc.message });
});
}

const defaultBody = (
<Form isHorizontal>
<FormGroup id="description-model-select-group" label={_("New description")}
fieldId="rename-dialog-new-description"
>
<TextInput id='rename-dialog-new-description'
value={newDescription}
onChange={setDescription} />
</FormGroup>
</Form>
);

return (
<Modal position="top" variant="small" isOpen onClose={Dialogs.close}
title={cockpit.format(_("Set description of $0"), vm.name)}
footer={
<>
<Button variant='primary' isDisabled={isLoading} isLoading={isLoading} id="description-config-dialog-apply" onClick={save}>
{_("Save")}
</Button>
<Button variant='link' onClick={Dialogs.close}>
{_("Cancel")}
</Button>
</>
}>
<>
{error && error.dialogError && <ModalError dialogError={error.dialogError} dialogErrorDetail={error.dialogErrorDetail} />}
{defaultBody}
</>
</Modal>
);
};
64 changes: 64 additions & 0 deletions src/components/vm/overview/titleModal.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
import React, { useState } from 'react';
import cockpit from 'cockpit';
import { Button } from "@patternfly/react-core/dist/esm/components/Button/index.js";
import { Form, FormGroup } from "@patternfly/react-core/dist/esm/components/Form/index.js";
import { TextInput } from "@patternfly/react-core/dist/esm/components/TextInput/index.js";
import { Modal } from "@patternfly/react-core/dist/esm/components/Modal/index.js";
import { useDialogs } from 'dialogs.jsx';

import { ModalError } from 'cockpit-components-inline-notification.jsx';
import { domainSetTitle } from '../../../libvirtApi/domain.js';

const _ = cockpit.gettext;

export const TitleModal = ({ vm }) => {
const Dialogs = useDialogs();
const [error, setError] = useState({});
const [newTitle, setTitle] = useState(vm.title);
const [isLoading, setIsLoading] = useState(false);

function save() {
setIsLoading(true);
domainSetTitle({
name: vm.name,
id: vm.id,
connectionName: vm.connectionName,
title: newTitle
}).then(Dialogs.close, exc => {
setIsLoading(false);
setError({ dialogError: _("Title could not be changed"), dialogErrorDetail: exc.message });
});
}

const defaultBody = (
<Form isHorizontal>
<FormGroup id="title-model-select-group" label={_("New title")}
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/model/modal/g

fieldId="rename-dialog-new-title"
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not rename dialog;

s/rename/set-title/

>
<TextInput id='rename-dialog-new-title'
value={newTitle}
onChange={setTitle} />
</FormGroup>
</Form>
);

return (
<Modal position="top" variant="small" isOpen onClose={Dialogs.close}
title={cockpit.format(_("Set title of $0"), vm.name)}
footer={
<>
<Button variant='primary' isDisabled={isLoading} isLoading={isLoading} id="title-config-dialog-apply" onClick={save}>
{_("Save")}
</Button>
<Button variant='link' onClick={Dialogs.close}>
{_("Cancel")}
</Button>
</>
}>
<>
{error && error.dialogError && <ModalError dialogError={error.dialogError} dialogErrorDetail={error.dialogErrorDetail} />}
{defaultBody}
</>
</Modal>
);
};
92 changes: 89 additions & 3 deletions src/components/vm/overview/vmOverviewCard.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ import { Flex, FlexItem } from "@patternfly/react-core/dist/esm/layouts/Flex/ind
import { Switch } from "@patternfly/react-core/dist/esm/components/Switch/index.js";
import { DialogsContext } from 'dialogs.jsx';

import { RenameDialog } from '../vmRenameDialog.jsx';
import { TitleModal } from './titleModal.jsx';
import { DescriptionModal } from './descriptionModal.jsx';
import { VCPUModal } from './vcpuModal.jsx';
import { CPUTypeModal } from './cpuTypeModal.jsx';
import MemoryModal from './memoryModal.jsx';
Expand Down Expand Up @@ -58,6 +61,9 @@ class VmOverviewCard extends React.Component {
this.state = {
virtXMLAvailable: undefined,
};
this.openName = this.openName.bind(this);
this.openTitle = this.openTitle.bind(this);
this.openDescription = this.openDescription.bind(this);
this.openVcpu = this.openVcpu.bind(this);
this.openCpuType = this.openCpuType.bind(this);
this.openMemory = this.openMemory.bind(this);
Expand All @@ -81,6 +87,23 @@ class VmOverviewCard extends React.Component {
});
}

openName() {
const Dialogs = this.context;
Dialogs.show(<RenameDialog vmName={this.props.vm.name}
vmId={this.props.vm.id}
connectionName={this.props.vm.connectionName} />);
}

openTitle() {
const Dialogs = this.context;
Dialogs.show(<TitleModal vm={this.props.vm} />);
}

openDescription() {
const Dialogs = this.context;
Dialogs.show(<DescriptionModal vm={this.props.vm} />);
}

openVcpu() {
const Dialogs = this.context;
Dialogs.show(<VCPUModal vm={this.props.vm} maxVcpu={this.props.maxVcpu} />);
Expand Down Expand Up @@ -125,6 +148,46 @@ class VmOverviewCard extends React.Component {
label={_("Run when host boots")} />
</DescriptionListDescription>
);
const nameLinkButton = (
<Button variant="link" isInline isAriaDisabled={vm.state != 'shut off'} onClick={this.openName}>
{_("edit")}
</Button>
);
const nameLink = (
<DescriptionListDescription id={`${idPrefix}-name`}>
<Flex spaceItems={{ default: 'spaceItemsSm' }}>
<FlexItem>
{vm.name}
</FlexItem>
{vm.state == 'shut off' ? nameLinkButton : <Tooltip content={_("Only editable when the guest is shut off")}>{nameLinkButton}</Tooltip>}
</Flex>
</DescriptionListDescription>
);
const titleLink = (
<DescriptionListDescription id={`${idPrefix}-title`}>
<Flex spaceItems={{ default: 'spaceItemsSm' }}>
<FlexItem>
{vm.title}
</FlexItem>
<Button variant="link" isInline isDisabled={!vm.persistent} onClick={this.openTitle}>
{_("edit")}
</Button>
</Flex>
</DescriptionListDescription>
);
const descriptionLink = (
<DescriptionListDescription id={`${idPrefix}-description`}>
<Flex spaceItems={{ default: 'spaceItemsSm' }}>
<FlexItem>
{vm.description}
</FlexItem>
<Button variant="link" isInline isDisabled={!vm.persistent} onClick={this.openDescription}>
{_("edit")}
</Button>
</Flex>
</DescriptionListDescription>
);

const memory = convertToBestUnit(vm.currentMemory, units.KiB);
const memoryLink = (
<DescriptionListDescription id={`${idPrefix}-memory-count`}>
Expand Down Expand Up @@ -176,10 +239,33 @@ class VmOverviewCard extends React.Component {
);

return (
<Flex className="overview-tab" direction={{ default: "column", "2xl": "row" }}>
<Flex className="overview-tab" direction={{ default: "column" }}>
<FlexItem>
<DescriptionList isHorizontal>
{ // Only show the name, if the title is set
vm.title &&
(
<DescriptionListGroup>
<DescriptionListTerm>{_("Name")}</DescriptionListTerm>
{nameLink}
</DescriptionListGroup>
)
}

<DescriptionListGroup>
<DescriptionListTerm>{_("Title")}</DescriptionListTerm>
{titleLink}
</DescriptionListGroup>

<DescriptionListGroup>
<DescriptionListTerm>{_("Description")}</DescriptionListTerm>
{descriptionLink}
</DescriptionListGroup>
</DescriptionList>
</FlexItem>
<FlexItem>
<DescriptionList isHorizontal>
<Text component={TextVariants.h4}>
<Text component={TextVariants.h3}>
{_("General")}
</Text>

Expand Down Expand Up @@ -240,7 +326,7 @@ class VmOverviewCard extends React.Component {
</FlexItem>
<FlexItem>
<DescriptionList isHorizontal>
<Text component={TextVariants.h4}>
<Text component={TextVariants.h3}>
{_("Hypervisor details")}
</Text>

Expand Down
16 changes: 15 additions & 1 deletion src/components/vm/vmDetailsPage.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,15 +87,29 @@ export const VmDetailsPage = ({
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

const vmNameHeadline = (
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to keep this seperate. Just create a const like this:

    const vmHeadline = (
    <Flex spaceItems={{ default: 'spaceItemsSm' }} component='h2'>
            {vm.title && <FlexItem>{vm.title}</FlexItem>}
            <FlexItem style={{ fontSize: "var(--pf-global--FontSize--xl)">
                {vm.name}
            </span>
    </Flex>

I did not test it, please adjust if needed.

Copy link
Author

Choose a reason for hiding this comment

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

Great, this is how spaceItems is used. This code does not the same, but I now understand the idea and will change mine accordingly.

<h2 className="vm-name">{vm.name}</h2>
);

const vmTitleHeadline = (
<h2 className="vm-name">
{vm.title}
<span style={{ fontSize: "var(--pf-global--FontSize--xl)", paddingLeft: "0.83em" }}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This 0.83 feels like a bit magic number.. Can you just maybe wrap this in a flex container with some spacing?

<Flex spaceItems={{ default: 'spaceItemsSm' }}>

Copy link
Author

Choose a reason for hiding this comment

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

The intention was having the same space ob both sides and the space in the right side is , according to the chrome debugger, determined by margin-block-end of h2 wich is set to 0.83em. I could not find it as a css variable so I just hardcoded it. There are probably better ways but I'm not a React expert. The Flex spaceItems seems to put spaces on both sides.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Additionally - I just realized that here we put the vm.name in parenthesis. Since most users don't use title, we defiintely want to have the name outside of parenthesis, when the user did not specify any title for the VM.

({vm.name})
</span>
</h2>
);

const vmActionsPageSection = (
<PageSection className="actions-pagesection" variant={PageSectionVariants.light}>
<div className="vm-top-panel" data-vm-transient={!vm.persistent}>
<h2 className="vm-name">{vm.name}</h2>
{vm.title ? vmTitleHeadline : vmNameHeadline}
<VmActions vm={vm}
config={config}
onAddErrorNotification={onAddErrorNotification}
isDetailsPage />
</div>
{ vm.description && <h4>{vm.description}</h4> }
</PageSection>
);

Expand Down
2 changes: 1 addition & 1 deletion src/components/vms/hostvmslist.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ const HostVmsList = ({ vms, config, ui, storagePools, actions, networks, onAddEr
isDisabled={vm.isUi && !vm.createInProgress}
component="a"
href={'#' + cockpit.format("vm?name=$0&connection=$1", encodeURIComponent(vm.name), vm.connectionName)}
className="vm-list-item-name">{vm.name}</Button>
className="vm-list-item-name">{vm.title ? vm.title + ' (' + vm.name + ')' : vm.name}</Button>
},
{ title: rephraseUI('connections', vm.connectionName) },
{
Expand Down
4 changes: 4 additions & 0 deletions src/libvirt-xml-parse.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,8 @@ export function parseDomainDumpxml(connectionName, domXml, objPath) {
const metadataElem = getSingleOptionalElem(domainElem, "metadata");

const name = domainElem.getElementsByTagName("name")[0].childNodes[0].nodeValue;
const title = domainElem.getElementsByTagName("title")[0]?.childNodes[0]?.nodeValue;
const description = domainElem.getElementsByTagName("description")[0]?.childNodes[0]?.nodeValue;
const id = objPath;
const osType = osTypeElem.nodeValue;
const osBoot = parseDumpxmlForOsBoot(osBootElems);
Expand Down Expand Up @@ -267,6 +269,8 @@ export function parseDomainDumpxml(connectionName, domXml, objPath) {
return {
connectionName,
name,
title,
description,
id,
osType,
osBoot,
Expand Down
30 changes: 30 additions & 0 deletions src/libvirtApi/domain.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -865,6 +865,36 @@ export function domainSetCpuMode({
], opts);
}

export function domainSetTitle({
Copy link
Collaborator

Choose a reason for hiding this comment

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

domainSetTitle and domainSetDescription have essentially the same function body. Can you maybe parameterize it?

Copy link
Author

Choose a reason for hiding this comment

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

Yes I will do it, no problem. Acually I thought the same, but tried to match the rest of the code and doing as little changes as possible. All other field seems to have their own functions and there are a lot cockpit.spawn methods which call virt-xml with basically the same arguments and I didn't want to refactor a whole file on my first contribution ;-).

name,
id: objPath,
connectionName,
title,
}) {
const opts = { err: "message", environ: ['LC_ALL=C'] };
if (connectionName === 'system')
opts.superuser = 'try';

return cockpit.spawn([
'virt-xml', '-c', `qemu:///${connectionName}`, '--metadata', `title="${title}"`, name, '--edit'
], opts);
}

export function domainSetDescription({
name,
id: objPath,
connectionName,
description,
}) {
const opts = { err: "message", environ: ['LC_ALL=C'] };
if (connectionName === 'system')
opts.superuser = 'try';

return cockpit.spawn([
'virt-xml', '-c', `qemu:///${connectionName}`, '--metadata', `description="${description}"`, name, '--edit'
], opts);
}

export function domainSetMemoryBacking({ connectionName, vmName, type }) {
const options = { err: "message" };
if (connectionName === "system")
Expand Down