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

Add support to close a recording #2972

Merged
merged 3 commits into from Aug 14, 2023
Merged

Add support to close a recording #2972

merged 3 commits into from Aug 14, 2023

Conversation

abey79
Copy link
Contributor

@abey79 abey79 commented Aug 14, 2023

What

This PR adds support to close a recording. Currently, this is available through the Rerun menu and the command palette. A close button will be added to the recording list UI in a subsequent PR.

My choices:

  • No warning, etc. This is a rather "lossy" feature.
  • No shortcut for the "Close recording" command (the natural one would be cmd-W, but that usually close an actual window).
  • When a recording is closed, the next one is selected when sorted by (app ID, recording time). If the closed recording was the last, then the previous one is selected.

Note: For the web builds, this feature is accessible from the command panel, but not from the rerun menu (which, as discussed, is ok). The drawback is that when all records are closed, the welcome screen shows "Loading..." instead of "Rerun is ready" (as in native builds). This will be addressed when revisiting the welcome screen, and relates to:

image

Checklist

@Wumpf Wumpf self-requested a review August 14, 2023 08:27
@abey79 abey79 added enhancement New feature or request ui concerns graphical user interface labels Aug 14, 2023
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

👌

@@ -86,6 +87,8 @@ impl UICommand {
#[cfg(not(target_arch = "wasm32"))]
UICommand::Open => ("Open…", "Open a Rerun Data File (.rrd)"),

UICommand::CloseCurrentRecording => ("Close recording", "Close the current Recording"),
Copy link
Member

Choose a reason for hiding this comment

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

Maybe the tooltip should mention that all data will be lost unrecoverably unless saved?

Copy link
Member

Choose a reason for hiding this comment

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

Also consider adding the actual name of the recording to the tooltip for added clarity.

Copy link
Member

Choose a reason for hiding this comment

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

Could also consider using the label "Close current recording" to be more specific. If it doesn't take too much space I think that specificity would make things clearer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nikolausWest "dynamic" tooltips would require a significant refactor of that area of code I believe. I'd rather avoid that can of worm in this PR so I'll open an issue. Also, this might be a little-used feature anyways once I add the "delete" button in the recording list.

Copy link
Member

@nikolausWest nikolausWest left a comment

Choose a reason for hiding this comment

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

Nice!

A couple comments plus:
Screenshot 2023-08-14 at 10 57 26

The command pallet version would be much clearer if you wrote out the recording's name (I guess that's App Id + recording Id?) since this isn't done directly on the recording name in the recordings list.

@@ -86,6 +87,8 @@ impl UICommand {
#[cfg(not(target_arch = "wasm32"))]
UICommand::Open => ("Open…", "Open a Rerun Data File (.rrd)"),

UICommand::CloseCurrentRecording => ("Close recording", "Close the current Recording"),
Copy link
Member

Choose a reason for hiding this comment

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

Also consider adding the actual name of the recording to the tooltip for added clarity.

@@ -86,6 +87,8 @@ impl UICommand {
#[cfg(not(target_arch = "wasm32"))]
UICommand::Open => ("Open…", "Open a Rerun Data File (.rrd)"),

UICommand::CloseCurrentRecording => ("Close recording", "Close the current Recording"),
Copy link
Member

Choose a reason for hiding this comment

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

Could also consider using the label "Close current recording" to be more specific. If it doesn't take too much space I think that specificity would make things clearer

@abey79 abey79 mentioned this pull request Aug 14, 2023
2 tasks
# Conflicts:
#	crates/re_viewer/src/ui/recordings_panel.rs
@abey79
Copy link
Contributor Author

abey79 commented Aug 14, 2023

Thanks for the reviews. I've changed the command name to "Close current Recording" and opened an issue to improve the tooltip with dynamic info:

@abey79 abey79 merged commit 04f974f into main Aug 14, 2023
23 of 24 checks passed
@abey79 abey79 deleted the antoine/close-recording branch August 14, 2023 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ui concerns graphical user interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants