-
Notifications
You must be signed in to change notification settings - Fork 24
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
Allow creation transformation functions over data lake variables #1706
base: master
Are you sure you want to change the base?
Allow creation transformation functions over data lake variables #1706
Conversation
4271164
to
ed8ee45
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.
Awesome feature!
UI wise is good to change some stuff:
1- The save button is blue (probably as primary color). Change it to our white/transparent standard;
2- The idea of using the placeholder for some quick use guide is very nice! Add some usage examples for boolean and text variables;
3- The placeholder on the description could simply be "optional description" or something more direct. The current placeholder is too descriptive I think;
4- The click-to-copy button isn't working. This is important to have imo;
Code will be reviewed next
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.
- It's confusing to have transforming functions in a separate menu to the data lake variable monitor, especially since the transforming functions are just compound variables
- Can we merge them? I think they can even be in the same table
- We could add a
Source
column (betweenID
andType
) that has icons to specify whether a variable is from MAVLink (logo), Cockpit internal (logo), Other (mdi-exit-run
), or compound (mdi-database-settings-outline
)- The compound ones could have the edit/delete icons included in the Source column, or we could use
database-edit-outline
as the icon and only allow deleting from inside the editor - In future clicking on an Other one could list the Actions and/or widgets that are known to set that variable, and possibly even jump to their configuration page
- The compound ones could have the edit/delete icons included in the Source column, or we could use
- We could add a "New compound variable" button under the table
- The
ID
column contents seem largely redundant with the Name (only humans are looking at the table)- we could regain some table width by removing the text for the IDs and just leaving the copy button
- We could add a
- More generally, I feel like Joystick, Actions, Data Lake, and MAVLink belong together, separated from the rest of the settings stuff, because they're all related to things you can do with Cockpit - I think they can all go in the "Tools" menu, which would also balance the load with the "Settings" menu
- It seems reasonable to have a shortcut from the MAVLink page to the Actions page, in case someone wants to manually send a message
- Can we merge them? I think they can even be in the same table
- Having the variables accessible is a a great idea, but it's seemingly quite awkward to use at the moment - could we make it inline instead of a separate form field?
- I'm thinking there could be an auto-complete dropdown where the cursor is, whenever it's at the end of a zero or more length set of valid ID characters (and searched using those characters), between either a pair of double curly braces, or an open double curly brace and the end of the input (e.g. when first typing)
- This could support normal clicking to select and replace the currently typed characters with the selected variable's ID, and ideally also tab-completion
- I would prefer for this to be implemented here, but it can be opened as an Issue and handled later if necessary, since it's polish / quality of life rather than fundamental to the feature
- I'm thinking there could be an auto-complete dropdown where the cursor is, whenever it's at the end of a zero or more length set of valid ID characters (and searched using those characters), between either a pair of double curly braces, or an open double curly brace and the end of the input (e.g. when first typing)
I liked almost all the suggestions, but I fear we can miss our deadline for the radcam support (this week) if we try to make this initial PR already the ideal one. Let me check what can be done that does not involves too much extra work and I get back to you. |
e07936d
to
81d5c61
Compare
f454fe6
to
ae61da8
Compare
New version with about all the changes requested: Kapture.2025-02-25.at.13.33.57.mp4After I recorded the video I still fixed some things:
|
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 have one more suggestion before approval:
On FHD screens (most of our users) the modal height is larger than the screen height, and so a scrollbar is present, making the user roll down the modal to see the save button.
But this can be solved quite simply by adding a density="compact" and hide-details to all the <v-text-fields />
, <v-select />
and <v-text-area />
. Also increase the gap to 7 on line 8 and invert the margin to -mt-1
on line 21.
This way we save important vertical space.
ae61da8
to
39d4764
Compare
Good catch! Fixed. |
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.
Nice feature. Will have a many uses over time
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.
Nice progress, and already much nicer to use! :D
Some suggestions/notes:
- Can the ID column be moved to the left of the name? It seems like it could take up much less space
- Comments are included in the placeholder, but using a comment currently stops the expression from evaluating (with no notice/warnings)
- Ideally comments would be allowed, and would just get stripped out before being passed to the evaluation function
- The "Description" field seems kind of redundant - could we just automatically extract a description using a comment on the first line, if there is one, and otherwise assume there's no description?
- Longer term it would be nice to include automatic descriptions for the MAVLink fields and the like, using metadata, but that can be raised in an Issue
- Could we have a single row with
[Variable Name ] -> [Variable ID ]
? I don't think names are expected (or encouraged) to be super long - If we don't expect the available variable types to change in the near future, could we replace the "Variable Type" dropdown with a row of radio-buttons, so it's a single click to change, and uses the space better?
- Editing the ID is disabled for existing variables, so maybe the icon could change to a copy one (so it can actually be used for something)?
- Compound variables don't get initialised on startup
- This should happen after the internal variables get initialised
- It would be nice to be able to delete variables that were created using Input Widgets and the like, but that can be raised as an Issue
- The new +/- buttons on the Dial don't actually update the underlying data lake variable (can be an Issue, I can open it later - short on time right now)
/> | ||
<div class="text-xs text-gray-400"> | ||
Create complex transformations by combining existing Data Lake variables using JavaScript expressions. | ||
Type '\{\{' to access available variables. Return is optional, but should be included in complex |
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.
Type '\{\{' to access available variables. Return is optional, but should be included in complex | |
Type <code>{{</code> to access available variables. Return is optional, but should be included in complex |
As-is it's showing up with the backslashes in the description
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.
Fixed.
This actually doesn't work, and we need to use {{
.
A transforming function is a new DataLake variable in which the value is calculated based on a JavsScript expression, that can include other DataLake variables in it.
39d4764
to
62a5a1a
Compare
Thanks!
Tried here, but it gets very strange. I don't think it's a big deal, specially since we already reduced the vertical space usage by a lot in the last change. It fits in both a FHD screen as well as a MacAir one.
This was actually a bug caused by my automatic addition of the
Actually the description is something available for all data-lake variables. I've replaced the text-field with a button that the user should click if they want to add a description, so we save some vertical space by default.
Same as 1.
Done.
I prefer not to do that. They already have the copy functionality elsewhere.
Nice catch. Fixed it!
Agree that it's useful, but out of scope here. Can you open the issue?
It's a bug in the Dial widget. Should be fixed there as well. Can you open the issue? |
Not sure what you're referring to here - I was asking about the ID column in the table, which is just copy icons, but currently is somewhat lost in space/padding between the names and types. Moving it to before the names seems like we could reduce the padding on the right and save a bunch of horizontal space, while also making it clearer that it's copying something related to the names :-) |
Oh, I thought you mentioned the ID input in the dialog. The ID column in the table was removed, like suggested in a previous review. |
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.
The ID column in the table was removed, like suggested in a previous review.
- Lol, it still exists (as intended) - it just only contains copy icons now. I'm suggesting to move those copy icons to before the Name column, instead of after it:
(ignore the background-colour changes from moving parts of a screenshot around)
This was actually a bug caused by my automatic addition of the return keyword on expressions missing it. Just fixed it. It now supports multi-line comments as well.
-
Tried this, and can confirm it now works for single line
//
comments, and multiline/**/
comments 👍It does still fail on
/**/
comments that are confined to a single line 😕
That's not a big problem, but is an unexpected failure case. I can raise an issue if you don't think that's worth fixing here.
... we already reduced the vertical space usage by a lot in the last change. It fits in both a FHD screen as well as a MacAir one.
I've replaced the text-field with a button that the user should click if they want to add a description, so we save some vertical space by default.
-
I use a slightly scaled display on my 16" MBP, so it has a more comfortable (larger) text size:
I have to vertically scroll on the menu to be able to access the save/cancel buttons, even with the "Description" changed to a button:
Outside of vertical space though, my suggestion to extract it from the first line comment in the code was also because we already show code comments as possible (in the placeholder text examples), so it seems intuitive to include some kind of description there, in which case it seems unintuitive to also have a description specified in a separate form field underneath the code window. Just an opinion 🤷
... the description is something available for all data-lake variables.
-
(part 2) Is it expected to be populated for the MAVLink message fields? Because it's not at the moment:
As mentioned in my previous review, this can be raised in an issue if we don't want to handle it here :-)
- Could we have a single row with [Variable Name ] -> [Variable ID ]? I don't think names are expected (or encouraged) to be super long
Same as 1.
-
Per 3, it may still be worth reducing vertical space usage. This suggestion was also ID to the right of the name, whereas you previously thought 1 was suggesting ID to the left of the name. Not a big deal, but may be worth a try if you haven't tested that option 🤷
-
Radio buttons look great - thanks! :-)
-
No worries :-)
-
Nice!
-
Sure (frontend: data lake: allow deleting variables in the monitor #1721) :-)
-
Sure (Dial: stop resizing value indicator with contents #1720) :-)
-
New bug found: if you edit a compound variable, press "cancel", then edit it again, it opens as an edit pane with new widget form values, and cannot be saved. It's possible to get back to the real edit pane by editing another variable first, or by closing and reopening the data lake monitor, but that shouldn't be required.
Screen.Recording.2025-02-27.at.11.49.33.am.mov
This PR adds a powerful new feature to Cockpit: being able to create transformation functions. What that means is that it basically allows the user to create new data lake variables based on existing ones.
Those functions can be as simple as summing two variables, or as complex as the user wants. Want
if
clauses? You have it. The transformation function is basically aneval
over the expression input.If the user does not include a
return
statement (as the video example below, where we use{{ var1 }} + {{ var2 }}
, the system does it automatically for them. The user can have complex functions with their ownreturns
as well.Kapture.2025-02-20.at.16.25.55.mp4
If the expression fails to be evaluated (e.g.: math error, variable access error, etc), it will try again in 10ms, than 20ms, than 40, doubling on every sequential error, till each reaches 10 seconds. That heuristic was put there to prevent error spamming.
This functionality will be used with the Radcam joystick variables to create the final zoom and focus values to be sent to the autopilot.
Fix #1642