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

Limit the size of component tooltips with UiVerbosity::Reduced #3171

Merged
merged 7 commits into from Sep 1, 2023

Conversation

abey79
Copy link
Contributor

@abey79 abey79 commented Aug 31, 2023

What

Limit the size of component tooltips with UiVerbosity::Reduced

Fixes #3154

image

Checklist

@abey79 abey79 added ui concerns graphical user interface enhancement New feature or request labels Aug 31, 2023
@nikolausWest
Copy link
Member

nikolausWest commented Aug 31, 2023

Regarding screenshot: why is the tooltip so wide?

@abey79
Copy link
Contributor Author

abey79 commented Aug 31, 2023

@nikolausWest I rushed that before leaving earlier and didn't even realise 🤦🏻 Anyways, I found the culprit.

@emilk requesting a review from you as I'm wondering if there was a reason for that autoshrink not to be set to true in the first place. (I couldn't find one by playing around with the UI.)

@emilk
Copy link
Member

emilk commented Sep 1, 2023

@emilk requesting a review from you as I'm wondering if there was a reason for that autoshrink not to be set to true in the first place. (I couldn't find one by playing around with the UI.)

I can't see any. Maybe a copy-paste bug :T

crates/re_data_ui/src/component.rs Outdated Show resolved Hide resolved
crates/re_data_ui/src/component.rs Outdated Show resolved Hide resolved
abey79 and others added 3 commits September 1, 2023 10:57
Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
Comment on lines 49 to 60
let max_row = match verbosity {
UiVerbosity::Small => 0,
UiVerbosity::Reduced => num_instances.at_most(4), // includes "…x more" if any
UiVerbosity::All => num_instances,
};

let displayed_row = if num_instances <= max_row {
num_instances
} else {
// this accounts for the "…x more" using a row
max_row - 1
};
Copy link
Member

Choose a reason for hiding this comment

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

I think we can simplify this a lot:

Suggested change
let max_row = match verbosity {
UiVerbosity::Small => 0,
UiVerbosity::Reduced => num_instances.at_most(4), // includes "…x more" if any
UiVerbosity::All => num_instances,
};
let displayed_row = if num_instances <= max_row {
num_instances
} else {
// this accounts for the "…x more" using a row
max_row - 1
};
let num_rows_to_show = match verbosity {
UiVerbosity::Small => 0,
UiVerbosity::Reduced => num_instances.at_most(3),
UiVerbosity::All => num_instances,
};

Copy link
Contributor Author

@abey79 abey79 Sep 1, 2023

Choose a reason for hiding this comment

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

My behaviour (4 rows is strictly respected):

3 4 5 6
x x x x
x x x x
x x x x
x …2 more …3 more

Your behaviour (4 rows is not respected and the "...1 more" feels weird as you might as well put the corresponding item there):

3 4 5 6
x x x x
x x x x
x x x x
x x x
…1 more …2 more

The key difference is the max_row - 1 in my code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed, I updated the docs.

And luckily I just experienced the panic-inducing case of max_row = 0 (=> max_row - 1 panics) and fixed it.

Comment on lines +130 to +135
if num_instances > displayed_row {
ui.label(format!(
"…and {} more.",
re_format::format_large_number((num_instances - displayed_row) as _)
));
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if num_instances > displayed_row {
ui.label(format!(
"…and {} more.",
re_format::format_large_number((num_instances - displayed_row) as _)
));
}
if num_rows_to_show < num_instances {
ui.label(format!(
"…and {} more.",
re_format::format_large_number((num_instances - num_rows_to_show) as _)
));
}

Copy link
Collaborator

@martenbjork martenbjork left a comment

Choose a reason for hiding this comment

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

Looks good!

@github-actions
Copy link

github-actions bot commented Sep 1, 2023

Size changes

Name main 3171/merge Change
plots.rrd 194.56 kiB 184.32 kiB -5.26%

@abey79 abey79 merged commit 2f01670 into main Sep 1, 2023
26 checks passed
@abey79 abey79 deleted the antoine/smaller-component-tooltips branch September 1, 2023 20:48
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.

The hover previews on events in the Streams View flicker and block
4 participants