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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Mouse Support] Track position slider is also activated without initial press #920

Closed
ThomasFrans opened this issue Aug 25, 2022 · 4 comments
Labels
bug Something isn't working

Comments

@ThomasFrans
Copy link
Contributor

Describe the bug
Yet another small problem with the mouse support (sorry 馃槃 ): When dragging the scroller on the right side, it's possible to drag the mouse onto the track position slider by accident, which causes it to jump to the position where it was entered with the mouse. This isn't a major bug of course, but just something small that could be fixed as it's probably not intended and sad when you were just listening to "that one good song with the nice drop" and you skip to the end by accident 馃槈 ...

To Reproduce
Steps to reproduce the behavior:

  1. Go to any tab with enough items for the scroller to appear (it also works without scrolling as the statusbar just checks for the MouseEvent::Hold(MouseButton::Press), but you'd never trigger it by accident without scrolling);
  2. Scroll down by clicking the scroller until the mouse is over the track position slider;
  3. Watch the slider jump to the mouse position.

Expected behavior
The statusbar should grab all mouse input when it has been clicked, and should not react to MouseEvent::Hold directly, until the mouse is released again anywhere on the screen. That way, it would only be possible to scroll on the statusbar when it is actually the intention of the user to do so.

I tried implementing this behavior using a result EventResult::Consumed(Some(Callback::on_*_fn)) after the statusbar was pressed, and using cursive::set_global_callback or cursive::set_on_{pre,post}_event to capture all the mouse events, but couldn't get it to work.

System (please complete the following information):

  • OS: Arch Linux
  • Terminal: Alacritty
  • Version: 0.11.0 and from source
  • Installed from: AUR and local build
@ThomasFrans ThomasFrans added the bug Something isn't working label Aug 25, 2022
@hrkfdn
Copy link
Owner

hrkfdn commented Aug 28, 2022

Yeah, can reproduce this for sure. Not really certain yet how to tackle that. The MouseEvent::Hold events need to be ignored inside the statusbar if the scroller is dragged. While thinking about this I was wondering how appropriate the Hold event actually is for skipping through tracks, maybe adjusting the position on clicks is enough.. 馃

@ThomasFrans
Copy link
Contributor Author

Yeah, can reproduce this for sure. Not really certain yet how to tackle that. The MouseEvent::Hold events need to be ignored inside the statusbar if the scroller is dragged. While thinking about this I was wondering how appropriate the Hold event actually is for skipping through tracks, maybe adjusting the position on clicks is enough.. 馃

Yes, I thought about that as well. The only other option I saw with the current implementation was to capture all mouse events after a press in the status bar, and to release them when any release event occurs. I don't know if it's possible to do that, and if it's possible, I don't know what would happen when a release happens outside the window.

As a quick solution, I think seeking with press only would be ok, as it's way more inconvenient when seeking is activated by accident...

@hrkfdn
Copy link
Owner

hrkfdn commented Aug 29, 2022

I think one problem in the existing implementation is here:

ncspot/src/ui/layout.rs

Lines 292 to 343 in 816d2f1

fn on_event(&mut self, event: Event) -> EventResult {
// handle mouse events in cmdline/statusbar area
if let Event::Mouse {
position,
event: mouseevent,
..
} = event
{
if position.y == 0 {
if mouseevent == MouseEvent::Press(MouseButton::Left)
&& !self.is_current_stack_empty()
&& position.x
< self
.get_current_screen()
.map(|screen| screen.title())
.unwrap_or_default()
.len()
+ 3
{
self.pop_view();
}
return EventResult::consumed();
}
let result = self.get_result();
let cmdline_visible = self.cmdline.get_content().len() > 0;
let mut cmdline_height = if cmdline_visible { 1 } else { 0 };
if result.as_ref().map(Option::is_some).unwrap_or(true) {
cmdline_height += 1;
}
if position.y >= self.last_size.y.saturating_sub(2 + cmdline_height)
&& position.y < self.last_size.y - cmdline_height
{
self.statusbar.on_event(
event.relativized(Vec2::new(0, self.last_size.y - 2 - cmdline_height)),
);
return EventResult::Consumed(None);
}
}
if self.cmdline_focus {
debug!("cmdline event");
return self.cmdline.on_event(event);
}
if let Some(view) = self.get_current_view_mut() {
view.on_event(event.relativized((0, 1)))
} else {
EventResult::Ignored
}

The events are passed to the statusbar before the inner view in the layout (i.e. ListView with scroller), which is maybe why they are never consumed by the scrolling logic..

@hrkfdn hrkfdn closed this as completed in 10213f9 Aug 29, 2022
@hrkfdn
Copy link
Owner

hrkfdn commented Aug 29, 2022

Went for the quick fix now, the loss in functionality very little and the bug is way more annoying 馃槃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants