Allow recording data-lake variables in the telemetry/sensor-logging/video-overlays#2406
Conversation
ES-Alexander
left a comment
There was a problem hiding this comment.
Exciting feature! 😃
Haven't had a chance to try yet, but a couple of concerns around the current approach:
src/libs/sensors-logging.ts
Outdated
There was a problem hiding this comment.
👂 Isn't this backwards, and unnecessary? I thought the IDs existed to always be used as the source/reference to fetch names and/or values, because neither names nor values need to be unique or constant?
There was a problem hiding this comment.
This is because ExtendedVariablesData use the keys as display names.
I will take a deeper look to see if its acceptable to convert that to use IDs (avoiding migrations).
| variablesData = { ...variablesData, ...veryGenericData } | ||
|
|
||
| const dataLakeData = this.collectDataLakeData(variablesData, timeNowObj) | ||
| variablesData = { ...variablesData, ...dataLakeData } |
There was a problem hiding this comment.
Could this have conflicts/overrides?
I think the ExtendedVariablesData type should probably be expanded to have an optional display name field, with IDs used as the keys. Finishing implementing part 1 of #2281 may also help avoid conflicts (I think the remaining steps are adding a prefix for the internal variables, and possibly adding a default prefix for user calls to the variable creation function).
There was a problem hiding this comment.
It's what's causing the need for what you mentioned in the previous comment.
Despite being desirable to enter at some point, the standardization is not a must here for this functionality. We can work step by step.
There was a problem hiding this comment.
Agreed that standardisation isn't a requirement for this, but while it isn't implemented it's likely worth at least a comment or an Issue mentioning we know it's a potential source of conflicts, just in case a user actually runs into issues with it down the line :-)
2a822eb to
45f41ed
Compare
|
@ES-Alexander I have refactored the implementation to enforce display names, even if blank. This way I could use the variable IDs as keys, avoiding conflicts. |
ES-Alexander
left a comment
There was a problem hiding this comment.
Nice feature - seems to work well :-)
Some minor non-blocking issues, which you can choose to address or ignore:
- There are lots of "(Legacy)" data-lake variables, which are lexicographically sorted to the top of the list (both in the Data Lake Variables section, and in the auto-complete in the monaco editor)
- It would be nice if they could be pushed down to the bottom of the list instead, but it's probably also ok to just filter them out, since we don't want to encourage people to use the legacy specifiers?
- The monaco editor adds syntax highlighting, and code-style auto-complete for the normal text parts, which can make it seem like the whole "message" gets evaluated rather than being a slightly special text field
- It would be ideal if the autocomplete could be only for the data-lake variables
- Is it possible to disable the syntax highlighting? Seems unlikely to cause many issues (since most users will be just trying to write text there), but it could be one less source of confusion
- Very long strings apparently get pushed down to the bottom of the screen in the subtitles
- There are several things this might occur on, since the data-lake variable names get used as the display names, but it's already possible to work around by just including the variable's value via a custom message field, or a VGI
- If we want to reduce the issues, it could help to set the display name of data-lake variables up to the first open bracket, to at least remove the MAVLink system and stuff?
- This could be conditional on the name being above some length, or containing "MAVLink", but probably good to avoid overly complex checks/processing
- A potentially more effective but also less smart solution would be to clip data-lake display names to a fixed number of characters
| value: string | ||
| lastChanged: number | ||
| hideLabel?: boolean | ||
| displayName: string |
There was a problem hiding this comment.
| displayName: string | |
| displayName?: string |
If we make this optional then if left unspecified it can default to the key, and if specified as an empty string it could replace the previous hideLabel: true usage (with the special behaviour of no name section being rendered).
Not a blocker, just could make the code slightly cleaner / require fewer changes to the existing definitions.
There was a problem hiding this comment.
This one I prefer to stay as it is. I believe the right approach is to remove all non-data-lake sensor/telemetry logging approaches.
45f41ed to
2020169
Compare
|
@ES-Alexander can you take one more look? I've implemented all suggested changes. |
ES-Alexander
left a comment
There was a problem hiding this comment.
Nice improvements!
One tiny bug got introduced, and one of the suggestions was only partially implemented, but happy for it to be merged already :-)
src/libs/sensors-logging.ts
Outdated
There was a problem hiding this comment.
| if (entry.includes('/mavlink') && displayName.includes('(')) { | |
| if (entry.includes('mavlink/') && displayName.includes('(')) { |
(Legacy) HEARTBEAT/mavlink_version comes up as an empty string if we search for /mavlink 😅
src/views/ConfigurationLogsView.vue
Outdated
There was a problem hiding this comment.
| if (entry.includes('/mavlink') && name.includes('(')) { | |
| if (entry.includes('mavlink/') && name.includes('(')) { |
As above, although I wonder whether it's possible to read the displayNames directly from the variables, or import the function for them, instead of needing to redefine it here?
src/views/ConfigurationLogsView.vue
Outdated
There was a problem hiding this comment.
I've decided to completely remove the legacy variables from both lists. As you said previously, it doesn't make sense to allow people to use old variables when the new ones are available there. The legacy variables are there just to make sure existing features don't break, but we don't need to support them on new features.
2020169 to
2947f91
Compare
…e ID Enforces the use of a displayName for all variables. Removed the `hideLabel` field.
They can be used both standalone and on custom messages
This adds auto-completing to data-lake variables.
2947f91 to
74db352
Compare
|
@ES-Alexander based on your approval and having fixed the last bug mentioned and removing the legacy support. |


This allows simply dragging data-lake variables, as one normally would do, but it also adds support for data-lake syntax in the Custom Message overlays! For it to be easy to use (with auto-completing), I've also changed that to use the Monaco Editor.
Fix #2185