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

Britz · 2023-03-31T17:37:29Z

The libvirt domain XML has the title and description element which is not represented in the UI.
This commit adds, if available, the Title to the VM List in the form "<TITLE> ()" and adds Title and description to the VM detail page including the option to modify it.

Blocked on new design mockups for the overview from @garrett

KKoukiou

@Britz thanks a lot for the enhancement

@garrett can you please do a design review?

@Britz As we are doing a test driven development, would you be interested to add tests? if yes I am happy to guide you, if not I can write them for you and push to your branch.

KKoukiou · 2023-04-01T09:23:16Z

src/libvirtApi/domain.js

@@ -865,6 +865,36 @@ export function domainSetCpuMode({
    ], opts);
 }

+export function domainSetTitle({


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

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 ;-).

KKoukiou · 2023-04-01T09:24:46Z

src/components/vm/vmDetailsPage.jsx

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


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' }}>

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.

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.

KKoukiou · 2023-04-01T09:28:05Z

src/components/vm/vmDetailsPage.jsx

@@ -87,15 +87,29 @@ export const VmDetailsPage = ({
        // eslint-disable-next-line react-hooks/exhaustive-deps
    }, []);

+    const vmNameHeadline = (


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.

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

KKoukiou · 2023-04-01T09:30:15Z

src/components/vm/overview/descriptionModal.jsx

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


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

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

KKoukiou · 2023-04-01T09:31:30Z

src/components/vm/overview/titleModal.jsx

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


s/model/modal/g

KKoukiou · 2023-04-01T09:31:58Z

src/components/vm/overview/titleModal.jsx

+    const defaultBody = (
+        <Form isHorizontal>
+            <FormGroup id="title-model-select-group" label={_("New title")}
+                fieldId="rename-dialog-new-title"


It's not rename dialog;

s/rename/set-title/

skobyda · 2023-04-02T23:36:02Z

Hey @Britz . Thanks a lot for this PR.

Before we dive deeper into code review, do you mind explaining why did you decide to implement this specific feature? I'm asking, since I have not seen an issue or request for it before, so I don't really know what use case this solves, and what kind of additional information would a user input into the title/description which cannot be part of the VM's name.

In cockpit-machines, we are quite careful about adding new features and expanding the UI, because we don't want the UI to become overcrowded with a lot of information, inputs, modals, etc.. So I'm just trying to find out what kind of use case this functionality solves, and how many users would use it..., so we can find the best place for this functionality in our UI :)

Did you decide to implement it based on some issue or some request from somebody?
Or did you implement it because you personally set title/description for your own VMs? Can you then perhaps explain and give is examples what kind of information do you input into it?

Lastly, virt-manager also supports setting title/description. But a lot of virt-manager users use it for quasi-categories. Since virt-manager doesn't support VM categories, users input the category into the title/description, so they can categorize it that way. That's not ideal from virt-manager, and if this is the aim here too, I would like to avoid going exactly the same way.
(btw we have a request for adding categories too)

skobyda · 2023-04-02T23:42:52Z

Btw, here are the screenshots I took when I built this PR locally, so other people who just wanna talk about the design don't have to build it:

(seems like this PR breaks our grid in overview tab, that needs to be fixed)

ng)

Britz · 2023-04-03T09:39:34Z

@KKoukiou: According to the test driven development, It would be great if you could this, I will try to help then. I've see that there are tests but no documentation on what and how to test and how to setup tests, so I just skipped it for now. I had not time to reverse engineer that by myself.

Britz · 2023-04-03T09:59:16Z

@skobyda: Thanks for adding the screenshots. The rename dialog actually did not change, I will add additional screenshots to pinpoint the changes.

Your question about why is quite simple. I simply implemented it, be cause we need that feature to better describe the purpose of the VM. Of cause you can code a lot of information into naming patterns, but i don't like extensively long domain names, especially when you need to type them regularly on shell, e.g., using virsh. We started with naming patterns, but since the setup gained a lot of complexity, this was just not feasible anymore and we switch to a way cleaner naming using the hostnames and domain names.

To our setup/project: We are currently developing a small-scale on premises trusted research environment to do research on medical data in Germany (so strict laws like GDPR apply) and we use plain libvirt on our servers with an increasing amount of machines. We also tried oVirt, proxmox and even OpenStack and they do not fit our current requirements, so we need to use pure libvirt for now. Cockpit machine is a nice way to have a quick overview over the machines but it is far away from being a productive replacement of the underlaying console tools, thus we need both. If you are interested in our infrastructure, you can have a look at https://semla.dfki.de.

Britz · 2023-04-03T11:32:57Z

I committed my changes and tried to fix all already mentioned issues, except the missing tests.
I also did not really understand how to edit the PO files, since they seem to be generated.

Since I am a german, I only added the German translations of the additional text snippets. Should I add these the other languages without translation, such that other can easily spot and contribute to missing translations? How is the process there?

Britz · 2023-04-03T15:05:29Z

@KKoukiou, according to the tests, I also need some help to understand the output. For me it is unclear how to work with the failed tests in the pull request. Looking at the logs, which are hard to read, it seems that the errors has nothing to do with my changes.

Britz · 2023-04-03T15:26:31Z

Here are some screenshot of the changes including explanations.

The VM List

If the title is set, it is shown instead of the name and the vm-name is shown in parenthesis (see intra and ipa-intra entries). Since VM names cannot contain whitespaces and there is one between the title and name, this is unambiguous.

VM details Page

Title, name and description are set and shown both in the headline and in the overview.
The elements in the overview are for clarification and to be able to edit the values, since the VM is running, the edit buttons are not active. An alternative would be to add "edit Title" and "edit Description" entries to the VM actions menu.

If the VM is shut down, the edit buttons are active and can be clicked.

The change title dialog: For some unkown reasons, the specified translations do not work here. Empty values are allowed to delete the title, perhaps it is better to add a dedicated delete button in addition. In a first version I just put additional fields to the rename dialog, but since renaming also fails if there are no changes on the name, that did not worked out as expected

The change description dialog: Basically the same as the title dialog, empty values are also allowed.

If the title is not set, the name element is not shown in the overwiew page, since it is indistinguishably shown in the header and it can be edited via the VM actions menu. Empty title and empty description entries are shown, since otherwise there would be no possibility to set them.

KKoukiou · 2023-04-06T09:00:04Z

@Britz I had a discussion yesterday with our designer (@garrett) and he believes adding these few extra options in the overview makes the UI rather cramped. He is working on a new design which will fit this in nicely, and then after I can pick up your commits to incorporate in the new design, add tests etc.

Thanks for the contributions and sorry for the waiting time on this. I hope this is not super urgent.

Britz · 2023-04-06T10:02:04Z

@KKoukiou That's great to here. It's in deed not the best design and really pragmatic. My first approach was to only put it in the header and add three field inside the rename dialogue, but that actually also fails, if the name of the vm does not change and thus it would always create errors if only the title or the description would be changed, which is not nice. Since it is easy to install from Git, we can also use the version in my branch until the feature is there.

When you have final idea of how to incorporate this information, perhaps I can help to implement. I also has other required changes on my todo list, but I had not yet time to implement or to check if there are already issues. This includes, e.g., the name of the created vnet interface on the host in the network interface overview table, which is a very important information.

mvollmer · 2024-05-20T13:04:55Z

My first approach was to only put it in the header and add three field inside the rename dialogue,

I think that's a reasonable approach since it leads to fewer changes to the page. I think it's also discoverable: if people care about the names of their machines and want to improve them, they will see the additional options like title and description, and might change those instead of the id.

I wouldn't use "Title" in the UI, but rather something like "Display name" or "Long name".
Or "Full name" like we do with accounts.

but that actually also fails, if the name of the vm does not change and thus it would always create errors if only the title or the description would be changed, which is not nice.

I am pretty sure we can make this work somehow.

mvollmer · 2024-05-20T14:05:51Z

I have done my own version of this (mostly to get more familiar with the code in general): #1644. Let's continue there, ok?

adds VM (domain) title and description to UI

dbe8909

KKoukiou requested changes Apr 1, 2023

View reviewed changes

Britz added 3 commits April 3, 2023 13:26

Adds Flex element with spaceItems

ace05db

fixes setter names and element ids

fce1c87

refacoring functions to use domainSetXML

c928110

adds check VM status for title and description

90864c9

Merge branch 'cockpit-project:main' into title_and_description

f7ef41b

KKoukiou added the blocked label Apr 6, 2023

KKoukiou mentioned this pull request Apr 12, 2023

VSOCK interface #969

Merged

skobyda mentioned this pull request Apr 13, 2023

Details page redesign #1027

Open

mvollmer closed this May 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Britz commented Mar 31, 2023 •

edited by KKoukiou

Loading

KKoukiou left a comment

KKoukiou Apr 1, 2023

Britz Apr 3, 2023

KKoukiou Apr 1, 2023

Britz Apr 3, 2023

KKoukiou Apr 3, 2023

KKoukiou Apr 1, 2023

Britz Apr 3, 2023

KKoukiou Apr 1, 2023

Britz Apr 3, 2023

KKoukiou Apr 1, 2023

KKoukiou Apr 1, 2023

skobyda commented Apr 2, 2023 •

edited

Loading

skobyda commented Apr 2, 2023 •

edited

Loading

Britz commented Apr 3, 2023

Britz commented Apr 3, 2023 •

edited

Loading

Britz commented Apr 3, 2023 •

edited

Loading

Britz commented Apr 3, 2023 •

edited

Loading

Britz commented Apr 3, 2023 •

edited

Loading

KKoukiou commented Apr 6, 2023

Britz commented Apr 6, 2023

mvollmer commented May 20, 2024 •

edited

Loading

mvollmer commented May 20, 2024

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

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

Conversation

Britz commented Mar 31, 2023 • edited by KKoukiou Loading

KKoukiou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skobyda commented Apr 2, 2023 • edited Loading

skobyda commented Apr 2, 2023 • edited Loading

Britz commented Apr 3, 2023

Britz commented Apr 3, 2023 • edited Loading

Britz commented Apr 3, 2023 • edited Loading

Britz commented Apr 3, 2023 • edited Loading

Britz commented Apr 3, 2023 • edited Loading

The VM List

VM details Page

KKoukiou commented Apr 6, 2023

Britz commented Apr 6, 2023

mvollmer commented May 20, 2024 • edited Loading

mvollmer commented May 20, 2024

Britz commented Mar 31, 2023 •

edited by KKoukiou

Loading

skobyda commented Apr 2, 2023 •

edited

Loading

skobyda commented Apr 2, 2023 •

edited

Loading

Britz commented Apr 3, 2023 •

edited

Loading

Britz commented Apr 3, 2023 •

edited

Loading

Britz commented Apr 3, 2023 •

edited

Loading

Britz commented Apr 3, 2023 •

edited

Loading

mvollmer commented May 20, 2024 •

edited

Loading