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 manual scrolling #81

Merged
merged 18 commits into from
Dec 29, 2020
Merged

Conversation

lucaspoffo
Copy link
Contributor

Fixes #74

egui/src/ui.rs Outdated Show resolved Hide resolved
egui/src/ui.rs Outdated Show resolved Hide resolved
@lucaspoffo
Copy link
Contributor Author

lucaspoffo commented Dec 26, 2020

Added the scroll_to_me inside Response,
Needed to pass the center_factor to the Context, so we can correctly centralize inside the ScrollArea.

@lucaspoffo lucaspoffo changed the title [WIP] Add manually scrolling Add manually scrolling Dec 27, 2020
@lucaspoffo lucaspoffo changed the title Add manually scrolling Add manual scrolling Dec 27, 2020
@lucaspoffo
Copy link
Contributor Author

Update the scroll demo (made it similar to imgui) and moved it to its own module.
Added documentation to the methods.
Added ScrollArea::scroll_offset().

Copy link
Owner

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Very nice! Thanks for taking your time to work on this.

I left a lot of comments. Most contentious will probably be center_factor vs Align. This is basically a tradeoff between flexibility and simplicity, and I'm keen to hear your thoughts on it!

egui/src/containers/scroll_area.rs Outdated Show resolved Hide resolved
egui/src/containers/scroll_area.rs Outdated Show resolved Hide resolved
egui/src/context.rs Outdated Show resolved Hide resolved
egui/src/containers/scroll_area.rs Outdated Show resolved Hide resolved
egui/src/types.rs Outdated Show resolved Hide resolved
egui/src/ui.rs Outdated
/// The scroll centering is based on the `center_factor`:
/// * 0.0 - top
/// * 0.5 - middle
/// * 1.0 - bottom
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
/// * 1.0 - bottom
/// * 1.0 - bottom
///
/// ```
/// # let mut ui = egui::Ui::__test();
/// egui::ScrollArea::auto_sized().show(ui, |ui| {
/// for i in 0..1000 {
/// if ui.button(format!("Button {}", i)).clicked {
/// ui.scroll_to_here(0.5);
/// }
/// }
/// });
/// ```

Copy link
Owner

Choose a reason for hiding this comment

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

...though in this case, this would make more sense:

let response = ui.button(format!("Button {}", i));
if response.clicked {
    response.scroll_to_me(0.5);
}

Is there any other more plausible use case of this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the code example from the scroll_to_cursor to use the scroll_bottom example similar from the demo. Makes more sense for this case. Added the button example for the scroll_to_me.

egui/src/ui.rs Outdated
/// * 0.5 - middle
/// * 1.0 - bottom
pub fn scroll_to_here(&mut self, center_factor: f32) {
let scroll_target = self.min_rect().bottom();
Copy link
Owner

Choose a reason for hiding this comment

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

This will scroll to the bottom of the current size of the Ui, not to the current cursor position (a Ui can be resized to something large before adding elements to it).

I think scroll_to_cursor and let scroll_target = self.region.cursor.y; is better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point 👍 Didn't knew about that.

egui/src/context.rs Outdated Show resolved Hide resolved
egui/src/demos/demo_window.rs Show resolved Hide resolved
@emilk
Copy link
Owner

emilk commented Dec 28, 2020

Another minor addition: when clicking a button to scroll to the top of that button, I think it would make sense to add a small offset (e.g. item_spacing.y) to the scroll. Otherwise the top of the button frame will be clipped by the scroll areas clip rect. In other words, it will look better if the top and bottom scrolls always came with an offset of item_spacing.y.

Comment on lines 177 to 179
let height_offset = content_ui.clip_rect().height() * center_ratio;
let top = content_ui.min_rect().top();
let offset_y = scroll_target - top - height_offset;
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
let height_offset = content_ui.clip_rect().height() * center_ratio;
let top = content_ui.min_rect().top();
let offset_y = scroll_target - top - height_offset;
let offset_y = scroll_target - lerp(content_ui.clip_rect().y_range(), center_ratio);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This does not work, as the clip_rect is always the same, but I updated the code to be more easy to understand. I hope.

Comment on lines +33 to +39
pub(crate) fn scroll_center_factor(&self) -> f32 {
match self {
Self::Min => 0.0,
Self::Center => 0.5,
Self::Max => 1.0,
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know if this is the best place to add this.

Copy link
Owner

Choose a reason for hiding this comment

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

This is the right place! Maybe could be called simply to_factor

/// ui.scroll_to_cursor(Align::bottom());
/// }
/// });
/// ```
Copy link
Owner

Choose a reason for hiding this comment

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

Good example 👍

Copy link
Owner

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Awesome - this works great!

Thank you!

@emilk emilk merged commit 19b4d87 into emilk:master Dec 29, 2020
emilk added a commit that referenced this pull request Dec 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Manually control ScrollArea scroll offset
2 participants