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

Improve subtitle telemetry display and configurations #946

Conversation

ArturoManzoli
Copy link
Contributor

@ArturoManzoli ArturoManzoli commented May 10, 2024

src/libs/sensors-logging.ts Outdated Show resolved Hide resolved
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 work so far - super keen for this to be made available once it's cleaned up! :-)

src/libs/sensors-logging.ts Outdated Show resolved Hide resolved
src/libs/sensors-logging.ts Outdated Show resolved Hide resolved
src/libs/sensors-logging.ts Outdated Show resolved Hide resolved
src/libs/sensors-logging.ts Outdated Show resolved Hide resolved
src/libs/sensors-logging.ts Outdated Show resolved Hide resolved
src/views/ConfigurationLogsView.vue Outdated Show resolved Hide resolved
src/views/ConfigurationLogsView.vue Outdated Show resolved Hide resolved
src/views/ConfigurationLogsView.vue Outdated Show resolved Hide resolved
src/libs/sensors-logging.ts Outdated Show resolved Hide resolved
src/views/ConfigurationLogsView.vue Outdated Show resolved Hide resolved
@ArturoManzoli ArturoManzoli force-pushed the 621-Improve-subtitle-telemetry-configuration branch 2 times, most recently from e3bfdc9 to 6205255 Compare May 13, 2024 18:25
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.

Code seems reasonable, although I haven't looked super in depth.

Tried it out and it mostly works well (nice!), although there are a few weird UI things:

Screen.Recording.2024-05-14.at.11.01.55.PM.mov
  1. dragging a variable from the options shows it left-aligned in the grid positions
    • the expected behaviour works correctly when dragging it from a grid position, where it then shows in the actual location it would go in any other grid position it's dragged over
  2. it seems weird that it's possible to drag and leave a variable in a different input section than the one it came from, especially since the variables within each input section get automatically sorted, which makes them feel like they have a "correct" place
  3. it would be nice if the styling options were previewed on the variable names, so it's clearer what each style change looks like
  4. ideally the styling options would be able to apply to individual variables rather than the entire overlay
    • e.g. you could click a variable to select it, which would open the styling options for that variable
  5. it would be nice to allow typing in the "font size" dropdown, or at least provide some more sizes above 30
  6. window sizing can make the "shadow size" dropdown unreadable
  7. it would be nice if the input variable sections had a maximum height, after which they become scrollable, or at least just make the input variables column scrollable instead of the whole page
    • since the current approach just extends the page, you can end up in a situation where the whole grid is off screen and you can't drag your variables into it
  8. the mission name only gets recorded if it has been manually changed - the automatic names don't appear in the subtitles
  9. I can't seem to delete the videos from Cockpit once I've downloaded them - that's apparently just a bug in Cockpit, since I was able to replicate the issue in the latest beta as well 🤷‍♂️

They're not critical, but would be nice to fix if it's not too challenging to do so :-)

@ArturoManzoli ArturoManzoli force-pushed the 621-Improve-subtitle-telemetry-configuration branch from 6205255 to bfd14b4 Compare May 15, 2024 12:02
@ArturoManzoli
Copy link
Contributor Author

Latest updates from @ES-Alexander changes request:
1, 2, 5, 6 - Done;

3 and 4- This grid/variable individual styling and preview is planned to be implemented when porting this screen to the next UI;

7- There is an auto-scroll feature when you drag a chip to the top of the view area. Too many scrollbars usually degrades the UI harmony and inserting them in this case also did. Anyway, your suggestion was added to the next UI port.

8- The standard text for non named missions is 'cockpit', that goes onto the filename when downloading a video, for example. Now the 'Mission name' tag will show 'Cockpit' instead of simply not showing anything.
Getting the auto generated mission name is not practical for now, but it has been added as suggestion for the next UI port.

@ArturoManzoli ArturoManzoli force-pushed the 621-Improve-subtitle-telemetry-configuration branch from bfd14b4 to 7bebdb1 Compare May 15, 2024 12:05
@ES-Alexander
Copy link
Contributor

  1. Fixed for vehicle variables, but not mission variables or custom variables
  2. This seems partially fixed, but deleting a mission variable that was configured before this logs configuration session now seems to put it in the vehicle variables until you close and reopen the logs configuration page
  3. Fair enough
  4. Fair enough
  5. Nice! 👍
  6. The dropdown now no longer shrinks 👍, but also seems to be perpetually slightly too small:
    Screenshot 2024-05-16 at 12 00 44 PM
  7. Ahh, I had tried dragging up near the top of the page section, but not outside of it. Good that this feature exists, but I think it may be a little more intuitive if the scrolling started from a bit lower down in the screen (maybe if it gets dragged to within the top quarter or fifth of the page section?). Fair enough on the scrollbars adding clutter.
  8. Fair enough 👍

@ArturoManzoli ArturoManzoli force-pushed the 621-Improve-subtitle-telemetry-configuration branch 2 times, most recently from 3ced665 to 3a6924f Compare May 16, 2024 12:51
@ArturoManzoli
Copy link
Contributor Author

ArturoManzoli commented May 16, 2024

Hey @ES-Alexander, thanks for taking the time to review this PR and for providing such detailed and valuable feedback. 👍

As a MacOS user, things might be presented a little differently for you (and this is very useful), such as the hotspot for auto-scrolling being outside the view area and the number inside the selector for the shadow size being cut off.

About 2- Is nice to reset the variable positions between UI updates. Their references may change and odd behavior may occur.

Updates:
1, 2, and 6 - Fixed.
7 - The target area for auto-scrolling changes slightly depending on the browser and OS. Following your suggestion, the area was increased to the upper 40% of the view.

@ES-Alexander
Copy link
Contributor

Hey @ES-Alexander, thanks for taking the time to review this PR and for providing such detailed and valuable feedback. 👍

No worries - glad I can help find and resolve some issues before they end up in a release :-)

1, 6 - Nice 👍
2, 7 - Seemingly unchanged from my last comment?

Screen.Recording.2024-05-16.at.11.10.43.PM.mov

2 is definitely some kind of bug, 7 might just be an OS difference or something (I don't think that one's a big deal, just not quite as nice as it could be - I think it's ok to merge this once 2 is fixed if 7 is hard to track down, although it might just be an element layering thing in terms of where pointer events are registered - not sure 🤷‍♂️ ).

@ES-Alexander
Copy link
Contributor

For 2, I'm pretty sure the issue is caused by line 307 - that likely needs a filter for "unless it's in the mission variables" :-)

@ArturoManzoli ArturoManzoli force-pushed the 621-Improve-subtitle-telemetry-configuration branch from 3a6924f to f12c295 Compare May 16, 2024 13:56
@ArturoManzoli
Copy link
Contributor Author

For 2, I'm pretty sure the issue is caused by line 307 - that likely needs a filter for "unless it's in the mission variables" :-)

Nice catch. The filtering was being triggered too late on line 317. Moved to line 307.

@ArturoManzoli
Copy link
Contributor Author

2 is definitely some kind of bug, 7 might just be an OS difference or something (I don't think that one's a big deal, just not quite as nice as it could be - I think it's ok to merge this once 2 is fixed if 7 is hard to track down, although it might just be an element layering thing in terms of where pointer events are registered - not sure 🤷‍♂️ ).

About 7, I think I saw it working (but a little laggy) on the first seconds of the video you posted.
It's working on non safari browsers and it shouldn't be different on safari based.. but Apple...

If you change the line:
document.getElementById('draggable-container')!.scrollIntoView({ behavior: 'smooth' })
to
document.getElementById('draggable-container')!.scrollIntoView({ behavior: 'instant' })
the screen will simply center on the display grid with no scrolling. Is it a better solution?

Check the regular behavior on Chrome:
https://github.com/bluerobotics/cockpit/assets/14910201/44925375-4507-464c-b62e-6f9cfaee29ba

@ES-Alexander
Copy link
Contributor

2 is fixed 👍

About 7, I think I saw it working (but a little laggy) on the first seconds of the video you posted.

Nah, that's when the chip goes into one of the grid cells, which then scrolls the page to bring that full cell into view.

It's working on non safari browsers and it shouldn't be different on safari based.. but Apple...

I'm on Chrome, so not a Safari issue.

If you change the line:
document.getElementById('draggable-container')!.scrollIntoView({ behavior: 'smooth' })
to
document.getElementById('draggable-container')!.scrollIntoView({ behavior: 'instant' })
the screen will simply center on the display grid with no scrolling. Is it a better solution?

I tried this and it does work but it's super jarring, so I wouldn't say it's a better solution.

Check the regular behavior on Chrome:
...

The behaviour in your video looks as I'd expect/hope mine to work :-)

I played around a little and found out that if I flip the threshold check on line 389 I get the desired behaviour, so it seems the Y direction in my Chrome is upwards (graph style), whereas I suppose in yours it's downwards (image/matrix style)? I tried a different browser and the original threshold comparison worked as expected, so I think we can just assume it's a bug in my Chrome version or something and leave it for now.

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.

LGTM - thanks for sorting out those issues! :-)

@rafaellehmkuhl rafaellehmkuhl linked an issue May 16, 2024 that may be closed by this pull request
6 tasks
@rafaellehmkuhl rafaellehmkuhl merged commit 5dc2896 into bluerobotics:master May 16, 2024
8 checks passed
@ES-Alexander ES-Alexander added the docs-needed Change needs to be documented label Dec 30, 2024
@ES-Alexander ES-Alexander added the docs-in-progress Included in an open docs PR label Jan 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-in-progress Included in an open docs PR docs-needed Change needs to be documented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sensors logging: improve control on logging and subtitle overlays
4 participants