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.

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

rafaellehmkuhl
Copy link
Member

@rafaellehmkuhl rafaellehmkuhl commented Feb 20, 2025

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 an eval 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 own returns 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

@rafaellehmkuhl rafaellehmkuhl force-pushed the allow-transforming-data-lake-variables branch from 4271164 to ed8ee45 Compare February 20, 2025 19:36
Copy link
Contributor

@ArturoManzoli ArturoManzoli left a 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;

image

Code will be reviewed next

@ES-Alexander ES-Alexander added the docs-needed Change needs to be documented label Feb 24, 2025
Copy link
Contributor

@ES-Alexander ES-Alexander left a comment

Choose a reason for hiding this comment

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

  1. 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 (between ID and Type) 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
      • 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
    • 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
  2. 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

@rafaellehmkuhl
Copy link
Member Author

  1. 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 (between ID and Type) 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
      • 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
    • 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
  2. 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 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.

@rafaellehmkuhl rafaellehmkuhl force-pushed the allow-transforming-data-lake-variables branch 5 times, most recently from e07936d to 81d5c61 Compare February 24, 2025 22:45
@rafaellehmkuhl rafaellehmkuhl force-pushed the allow-transforming-data-lake-variables branch 5 times, most recently from f454fe6 to ae61da8 Compare February 25, 2025 16:50
@rafaellehmkuhl
Copy link
Member Author

New version with about all the changes requested:

Kapture.2025-02-25.at.13.33.57.mp4

After I recorded the video I still fixed some things:

  • Save button is now white and disabled when the forms is not filled
  • There's a placeholder text to help the user
  • Sorting by source was fixed

Copy link
Contributor

@ArturoManzoli ArturoManzoli left a 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.

Check the result:
Before
image

After
image

@rafaellehmkuhl rafaellehmkuhl force-pushed the allow-transforming-data-lake-variables branch from ae61da8 to 39d4764 Compare February 25, 2025 18:34
@rafaellehmkuhl
Copy link
Member Author

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.

Good catch! Fixed.

Copy link
Contributor

@ArturoManzoli ArturoManzoli left a 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

Copy link
Contributor

@ES-Alexander ES-Alexander left a 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:

  1. Can the ID column be moved to the left of the name? It seems like it could take up much less space
  2. 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
  3. 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
  4. Could we have a single row with [Variable Name ] -> [Variable ID ]? I don't think names are expected (or encouraged) to be super long
  5. 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?
  6. 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)?
  7. Compound variables don't get initialised on startup
    • This should happen after the internal variables get initialised
  8. 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
  9. 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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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

Copy link
Member Author

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 &#123;&#123;.

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.
@rafaellehmkuhl rafaellehmkuhl force-pushed the allow-transforming-data-lake-variables branch from 39d4764 to 62a5a1a Compare February 26, 2025 20:10
@rafaellehmkuhl
Copy link
Member Author

rafaellehmkuhl commented Feb 26, 2025

Nice progress, and already much nicer to use! :D

Thanks!

Some suggestions/notes:

  1. Can the ID column be moved to the left of the name? It seems like it could take up much less space

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.

  1. 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

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.

  1. 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

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.

  1. 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.

  1. 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?

Done.

  1. 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)?

I prefer not to do that. They already have the copy functionality elsewhere.

  1. Compound variables don't get initialised on startup

    • This should happen after the internal variables get initialised

Nice catch. Fixed it!

  1. 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

Agree that it's useful, but out of scope here. Can you open the issue?

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

It's a bug in the Dial widget. Should be fixed there as well. Can you open the issue?

@ES-Alexander
Copy link
Contributor

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.

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 :-)

@rafaellehmkuhl
Copy link
Member Author

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.

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.

Copy link
Contributor

@ES-Alexander ES-Alexander left a 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.

  1. 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:
    Screenshot 2025-02-27 at 11 13 33 am
    (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.

  1. 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.

  1. I use a slightly scaled display on my 16" MBP, so it has a more comfortable (larger) text size:
    Screenshot 2025-02-27 at 11 25 08 am

    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:
    Screenshot 2025-02-27 at 11 31 06 am

    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.

  1. (part 2) Is it expected to be populated for the MAVLink message fields? Because it's not at the moment:
    Screenshot 2025-02-27 at 11 43 53 am

    As mentioned in my previous review, this can be raised in an issue if we don't want to handle it here :-)

  1. 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.

  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 🤷

  2. Radio buttons look great - thanks! :-)

  3. No worries :-)

  4. Nice!

  5. Sure (frontend: data lake: allow deleting variables in the monitor #1721) :-)

  6. Sure (Dial: stop resizing value indicator with contents #1720) :-)


  1. 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-needed Change needs to be documented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow "transforming functions" on data-lake variables
3 participants