-
Notifications
You must be signed in to change notification settings - Fork 74
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.
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
vm: Support for VM descriptions #1644
vm: Support for VM descriptions #1644
Conversation
a2a1286
to
d9dc7b9
Compare
This looks a bit sad, retrying for comparison. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Depending on how much time you can/want to throw at this, I can be convinced to give a +1. But I'm not super happy about this just yet.
3d21e50
to
feb47bf
Compare
Yay for the new pixel test! |
Very cool, thanks! Looks good, just needs the new pixel test mopped up. |
feb47bf
to
99dee05
Compare
99dee05
to
1310449
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I was about to mop up the pixel update, but you already did it -- I didn't see the force-push here. Force-pushed a rebase, and re-running the tests to check the flakes.
LGTM, assuming green. Thanks!
This still needs a proper release note, but otherwise looks good to land. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought this was already discussed before and we specifically decided to not do it this way. (Note: it was different people working on cockpit-machines at that point in time.)
A VM is not a person with a first, middle, and last names. It does not have a "Full name".
If we want to get around the limitation for the name that serves as a type of identifier (ID) for the VM, then we should have "Name" be a display name and generate the machine's unique and limited (in characters) name based on the display name. I believe that's the intent for this, to work around an implementation detail where the name itself is restricted to various limits (mainly, which characters it can use).
It does not makes sense to have yet another field to do the exact same thing. We already have name for a way to identify the machine and description to explain what it does. Having both will be confusing and also take up extra space everywhere where there's a name, for no good reason.
So, instead of having the split here's how we might handle it instead:
Completely move the short ID name to the background, have it based on the display name, and always show the display name (treating the non-display name as an implementation detail). We could always set both the display name and ID name or only set the display if it doesn't fit within the ID's restrictions.
When displaying VMs, we'd do something like "display name || ID name" where it would default to showing the display name (if it exists) and falling back to the ID name if a display name is not shown. And we could even do something where we transform the display name to the ID name and if they match, only show display name, else show "Display name (ID name)". I think we're already doing this (if it already exists) without a way to set it, right?
I'm not a fan of this TBH. The name which we currently display is the one you'd see in I was screptical about this already, and @garrett 's comments pushed me over the line. How about we entirely drop this title/full name again, and only retain the optional description? That seems marginally more useful to me, and isn't so much "in your face" and redundant. |
Makes sense to me. |
@garrett's design also makes a lot of sense to me. :-) |
But does it then make sense to use the Rename dialog to set the description? |
Good point -- dang, looks like this is back to square one wrt. design then? 😢 |
4dfcf5c
to
10072f9
Compare
Done. (It's called the "inactive" definition.) |
10072f9
to
cb75374
Compare
cb75374
to
beaf9ae
Compare
This failure is a new flake... let's see. |
beaf9ae
to
10af9f1
Compare
One apparently can't ever use |
10af9f1
to
dd4339b
Compare
@vollmer: Please don't use |
Hmm, can we make it so that set_input_text("\n") does the right thing? I'll make a PR. |
dd4339b
to
b74ad42
Compare
The test now dioes this:
It would be really nice to replace that with
Not only because it is less to type, but also because this is the natural first thing to try, and it is not at all obvious what to do once you figure out that it doesn't work. |
b74ad42
to
24d90a3
Compare
Based on work by @Britz, thanks a lot!
Whoops, dropped the ball on this, sorry. Let's get this back on track. based on the discussion above, this PR leaves "title" and "id" alone. It is only about the description now. The description of a VM is shown on the details page, and there is an action in the menu to edit it. The description of this PR is up-to-date with the code. |
24d90a3
to
cd80563
Compare
We were not able to find or create Copr project
Unless the HTTP status code above is >= 500, please check your configuration for:
|
} catch (exc) { | ||
dialogErrorSet({ | ||
dialogError: cockpit.format(_("Failed to set description of VM $0"), vm.name), | ||
dialogErrorDetail: exc.message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 4 added lines are not executed by any test.
<Form onSubmit={e => { | ||
e.preventDefault(); | ||
onSubmit(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 3 added lines are not executed by any test.
onSubmit(); | ||
}} | ||
isHorizontal> | ||
{!isObjectEmpty(error) && <ModalError dialogError={error.dialogError} dialogErrorDetail={error.dialogErrorDetail} />} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This added line is not executed by any test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small comment/question, otherwise looks good (!)
@@ -888,6 +888,33 @@ export function domainSendNMI({ | |||
return call(connectionName, objPath, 'org.libvirt.Domain', 'InjectNMI', [0], { timeout, type: 'u' }); | |||
} | |||
|
|||
function shlex_quote(str) { | |||
// yay, command line apis... | |||
return "'" + str.replaceAll("'", "'\"'\"'") + "'"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, what about it? :-) This is supposed to be a JavaScript implementation of it. Is something missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mistaken this for a Python file, don't ask :)
|
||
# Non-ascii chars, unbalanced quotes, backslash, multiple lines | ||
desc1 = '"Döscrü\\ptiän \'\'\' كرة القدم' | ||
desc2 = 'Second <b>line</b>' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope this does not get rendered as such :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed by not showing/setting the title at all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool!
Based on work by @Britz, thanks a lot!
This is also a proving ground for robust and convenient virt-xml invocations. I plan to renovate the rest of domain.js and other files accordingly.
The description is a
<p>
so that it doesn't get too shouty when people put a lot of text there."Edit description" action:
Machines: Support for VM descriptions
Cockpit can now show and change the descriptions of virtual machines.