Slider widget for options window #215

bendoughty · 2025-02-26T08:37:25Z

Here’s a PR for a new “slider” widget type I have been working on. It is intended for use in the options window as the first part of an effort to make that area of the interface less cluttered and more intuitive for the user.

As is typically the case with UI sliders, the absolute value of the option is now no longer directly shown to the user within the interface. To make this a little more palatable to those who may miss it, I have added an extra line to the right-click context menu which displays the current value of the option. This change applies to slider options only.

I have updated some candidate options to use the new option type, these are very conservative changes based on Human Interface Guidelines from Gnome and Apple (thanks for the guidance, @pjbroad!). If the new widget type is approved I am happy to make adjustments to these selections as discussion progresses 😊

It would be useful if android folks could have a play around with the new widget, as I unfortunately do not possess a device capable of running the android client.

Screen.Recording.2025-02-26.at.08.33.11.mov

… the new slider widget.

…o allow direct defaulting/resetting.

…alue is displayed.

pjbroad · 2025-03-02T13:25:38Z

Nice work @bendoughty, I really like it. A few initial comments:

There is an issue if you have "User Interface Scaling Factor" anything other than 1.0. The scaling of the widget in the window is fine, but the slider does not move to the correct position when clicked, and lags when sliding. For > 1.0 value you cannot get to the most right side.
The code works on Android (apart form the above issue), but:

the keyboard is opened when you click. You should just be able to remove the ANDROID #defs that call SDL_StartTextInput() in the el_config.c handler functions.
the scaling is too wide, overlapping the option text. I tested removing the 2 x scaling just for the width and start position, keeping the 2 x height. That looks fine to me.

I see you have use space characters rather than TABs for indent throughout the patch. The convention for EL is TABS. Though not 100% followed, it is particularly an issue in code that does use TABS as the space using code shows non-aligned. PITA I know, but hopefully your text editor supports fixing this easily.
In slider_click() of widgets.c, there's a compile warning because of the late declaration of pos; due to C90 comparability being enabled. Perhaps time to change that but its what we currently have.
For the current value shown in the context menu, perhaps limit floats to 2 significant figures.
The context menu stays open when sliding the bar, but the value does not update, perhaps close it in the el_config handler. - Looks to be only on Android, I can investigate if you like.

…k_handler.

… correctly at different UI scaling levels.

bendoughty · 2025-03-03T17:41:11Z

Thank you for taking the time to test out the widget, I really appreciate the constructive feedback 😊

The integer truncation bug causing the slider positioning issue should now be fixed. I have tested scaling up, scaling down, and restarting the client at various scaling levels.
I am glad the widget is (mostly) working as expected for Android, but clearly I need to take a look at picking up a cheap device I can use to test things like this in future. The keyboard issue you noted has been resolved, however I think it might be best for me to tap you in (if that’s okay) re: the Android scaling issue as I am a little dubious about making changes blindly.
Sorry about the formatting problem, I have a new machine and it would appear I was blissfully unaware that Xcode now defaults to indenting with spaces(?!) - this should now be resolved. 
I have fixed the compiler warning, thank you for pointing it out. 
The “current value” and other context menu option lines for slider floats have now been limited to 2 significant figures. 
I have added a fix which I believe should resolve the context menu issue for Android - let me know.

pjbroad · 2025-03-08T12:23:46Z

Thanks for making the changes, looks good.

The following patch fixes the Android with scaling. It also replaces the cm_destroy() call with the poorly named cm_post_show_check() that close the context menu window rather then destroy it. We could remove the #def around it too. android-patch.txt

One other thing I realised while testing, other widgets use sound for actions. What do you think about adding sounds for the slider? At least for clicks, but what about dragging. For the Gnome desktop, you get sounds for clicks, and a sound when you stop dragging. Both could potentially be based on mouse up events but there's nothing the the window/widget framework to provide such events; adding that would be a fair sized change.

If you apply the Android patch, and assuming we pick up the sound thing later, are you ready for this to be merged?

pjbroad · 2025-03-08T12:35:08Z

elconfig.c

+				widget_width = slider_width;
+				widget_id = slider_add_extended(window_id, elconfig_free_widget_id++, NULL,
+#ifdef ANDROID
+					window_width - right_margin - 2.0f * widget_width, current_y, 2.0f * widget_width, 2.0f * line_height,


As discussed, the width and x position scaling should be removed to avoid making the widget too wide.

pjbroad · 2025-03-08T12:35:21Z

elconfig.c

+				widget_width = slider_width;
+				widget_id = slider_add_extended(window_id, elconfig_free_widget_id++, NULL,
+#ifdef ANDROID
+					window_width - right_margin - 2.0f * widget_width, current_y, 2.0f * widget_width, 2.0f * line_height,


As discussed, the width and x position scaling should be removed to avoid making the widget too wide.

pjbroad · 2025-03-08T12:37:34Z

elconfig.c

+
+#ifdef ANDROID
+		if (cm_valid(CM_INIT_VALUE))
+			cm_destroy(CM_INIT_VALUE);


As discussed, suggest replacing if () cm_destroy() with cm_post_show_check(1)

pjbroad · 2025-03-08T12:37:45Z

elconfig.c

+
+#ifdef ANDROID
+		if (cm_valid(CM_INIT_VALUE))
+			cm_destroy(CM_INIT_VALUE);


As discussed, suggest replacing if () cm_destroy() with cm_post_show_check(1)

pjbroad · 2025-03-08T12:39:01Z

elconfig.c

@@ -2151,14 +2220,23 @@ static void call_option_menu(var_struct *option)
 	cm_set_colour(cm_id, CM_GREY, 201.0/256.0, 254.0/256.0, 203.0/256.0);
 	cm_set(cm_id, option->name, context_option_handler);
 	cm_grey_line(cm_id, 0, 1);
+	add_cm_current_value_line(cm_options_current_str, option, *(float*)option->var);
+	cm_grey_line(cm_id, 1, 1);


The cm_grey_line(cm_id, 1, 1) is harmless at this stage but could be moved to the add_cm_current_value_line() function or made conditional on the return value.

…resultant context menu line grey.

bendoughty · 2025-03-09T09:16:10Z

Thank you very much for the patch, I have applied it and also made the change in call_option_menu that you recommended 😊

Re: an action sound for the slider, this is actually something I have also been wondering about. Although a perfect implementation is not currently possible, I agree that it is better to at least have something in place for clicks. I have added a confirmationary do_click_sound to the onclick handler. This results in a "click!" when the slider is clicked (and, weirdly, at the beginning of a drag).

If you are happy with this then I am happy for the merge to go ahead!

pjbroad · 2025-03-09T12:06:59Z

All merged. Thanks very much for adding this function.

bendoughty added 7 commits February 25, 2025 09:24

Add draft of new slider widget type.

ba3e45a

Add new OPT_FLOAT_S and OPT_INT_S option types to elconfig, utilising…

d5f6c03

… the new slider widget.

Enable the right-click context menu for the new slider option types t…

bf967a4

…o allow direct defaulting/resetting.

Update several candidate options to use the new slider option types.

623570c

Tidy up set_var functions a little.

7a028e6

Add a "current value" line to the context menu for slider options.

c5d214a

Small fix for option context menu checkbox positioning when current v…

28ccf98

…alue is displayed.

pjbroad self-requested a review March 2, 2025 12:07

bendoughty added 6 commits March 3, 2025 08:11

Fix improper code indentation.

1dc70f5

Fix small compile warning in widgets.c.

fc18708

Limit context menu entries for slider options to 2 significant figures.

cd1f3d2

Removed the erroneous SDL_StartTextInput for Android in slider_onclic…

b29c55e

…k_handler.

Fix integer truncation bug causing the slider widgets to not function…

b91cd1f

… correctly at different UI scaling levels.

Resolve some Android-related issues with the new slider widget.

7532297

pjbroad self-assigned this Mar 8, 2025

pjbroad reviewed Mar 8, 2025

View reviewed changes

bendoughty added 3 commits March 9, 2025 08:26

Slider patch for Android compatibility.

2a63dec

Check add_cm_current_value_line executes correctly before making the …

d01510c

…resultant context menu line grey.

Add a click sound for slider onclick events.

e8af309

pjbroad merged commit 1717d9b into raduprv:master Mar 9, 2025
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Slider widget for options window #215

Slider widget for options window #215

Uh oh!

bendoughty commented Feb 26, 2025

Uh oh!

pjbroad commented Mar 2, 2025 •

edited

Loading

Uh oh!

bendoughty commented Mar 3, 2025 •

edited

Loading

Uh oh!

pjbroad commented Mar 8, 2025 •

edited

Loading

Uh oh!

pjbroad Mar 8, 2025 •

edited

Loading

Uh oh!

pjbroad Mar 8, 2025 •

edited

Loading

Uh oh!

pjbroad Mar 8, 2025

Uh oh!

pjbroad Mar 8, 2025

Uh oh!

pjbroad Mar 8, 2025 •

edited

Loading

Uh oh!

bendoughty commented Mar 9, 2025

Uh oh!

Uh oh!

pjbroad commented Mar 9, 2025

Uh oh!

Uh oh!

Slider widget for options window #215

Slider widget for options window #215

Uh oh!

Conversation

bendoughty commented Feb 26, 2025

Uh oh!

pjbroad commented Mar 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bendoughty commented Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pjbroad commented Mar 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pjbroad Mar 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pjbroad Mar 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pjbroad Mar 8, 2025

Choose a reason for hiding this comment

Uh oh!

pjbroad Mar 8, 2025

Choose a reason for hiding this comment

Uh oh!

pjbroad Mar 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bendoughty commented Mar 9, 2025

Uh oh!

Uh oh!

pjbroad commented Mar 9, 2025

Uh oh!

Uh oh!

pjbroad commented Mar 2, 2025 •

edited

Loading

bendoughty commented Mar 3, 2025 •

edited

Loading

pjbroad commented Mar 8, 2025 •

edited

Loading

pjbroad Mar 8, 2025 •

edited

Loading

pjbroad Mar 8, 2025 •

edited

Loading

pjbroad Mar 8, 2025 •

edited

Loading