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

Fix: volume slider behavior in music gui #7209

Closed
wants to merge 1 commit into from

Conversation

@nikolas
Copy link
Member

nikolas commented Feb 10, 2019

In the music gui, the volume sliders don't respond to mouse drags
that extend outside of their small area.

This changes the event code to give them more typical dragging behavior
that's easier to work with.

@@ -26,6 +26,7 @@
#include "string_func.h"
#include "settings_type.h"
#include "settings_gui.h"
#include "tilehighlight_func.h"

This comment has been minimized.

Copy link
@glx22

glx22 Feb 10, 2019

Contributor

It's strange to require tilehighlight_func when not working with tiles at all, but I know it's needed for drag stuff (used in other GUI too)

This comment has been minimized.

Copy link
@nikolas

nikolas Feb 10, 2019

Author Member

I agree -_- this is required to use SetObjectToPlaceWnd(). depot_gui.cpp uses this function in a similar way, to allow for dragging vehicles to be sold in the depot window.

Maybe the window system's dragging events were only anticipated to be used with tiles? I think it may make sense to move SetObjectToPlaceWnd() out of tilehighlight_func.h, as it's useful for dragging events inside windows that don't work with tiles at all.

src/music_gui.cpp Outdated Show resolved Hide resolved
@PeterN
Copy link
Member

PeterN commented Feb 10, 2019

In master, moving the mouse away from the widget stops all further response, and you need to click and drag again to change volume.

As it stands, this PR makes the sliders continue to respond if you move the mouse away from the widget and then back.

I think it would be better if the sliders continued to respond whilst the cursor is off the widget, until dragging is stopped.

@nikolas nikolas force-pushed the nikolas:volume-slider-fix branch from c58f18c to 89b2dbd Feb 10, 2019
@nikolas
Copy link
Member Author

nikolas commented Feb 10, 2019

I've updated this PR to behave in the way you described, and yeah, it is much better.

src/music_gui.cpp Show resolved Hide resolved
@nikolas nikolas force-pushed the nikolas:volume-slider-fix branch from 89b2dbd to c7c9f76 Feb 10, 2019
src/music_gui.cpp Outdated Show resolved Hide resolved
In the music gui, the volume sliders don't respond to mouse drags
that extend outside of their small area.

This changes the event code to give them more typical dragging behavior
that's easier to work with.
@nikolas nikolas force-pushed the nikolas:volume-slider-fix branch from c7c9f76 to 9a82307 Feb 10, 2019
@PeterN
Copy link
Member

PeterN commented Feb 12, 2019

This works but I'd like to look at how scrollbars achieve the same result.

@nikolas
Copy link
Member Author

nikolas commented Feb 12, 2019

Good point.. It looks like scrollbar dragging is handled in window.cpp's MouseLoop(), with HandleScrollbarScrolling: https://github.com/OpenTTD/OpenTTD/blob/master/src/window.cpp#L2835

It might be appropriate to add a HandleSliderScrolling() handler here, creating some consistency with any future sliders that are added to the window system.

Though, that may add some complexity, as we'll still want to do the actual volume-setting in music_gui.cpp, not window.cpp...

@PeterN
Copy link
Member

PeterN commented Feb 14, 2019

#7227 extends scrollbar handling to allow generic widget dragging without needing to use bits of tile highlight functions.

@nikolas
Copy link
Member Author

nikolas commented Feb 14, 2019

Great, that's a nicer solution, and the volume sliders are working well for me on that branch.

@nikolas nikolas closed this Feb 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.