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

Introduce a multi slider widget and use it to choose tax rates #2057

Merged
merged 4 commits into from
Nov 27, 2023

Conversation

lmoureaux
Copy link
Contributor

I would like to have a generic version of fc_double_edge (the widget to choose tax rates) available. The general idea is that there is a fixed number of items and the user can distribute them across multiple categories. This could be useful for specialists in big cities. In order to level up the field a bit, I intend to make the new widget usable with the keyboard only.

This PR implements the generic widget and uses it in the tax rates dialog. Both keyboard and mouse navigation should work properly. The widget was developed with NightStalker and I quickly checked that it's not doing anything crazy under Classic.

If/when this gets merged, documentation screenshots will need an update.

I would like to have a generic version of fc_double_edge available. The general
idea is that there is a fixed number of items and the user can distribute them
across multiple categories. This could be useful for specialists in big cities.
In order to level up the field a bit, I intend to make the new widget useable
with the keyboard only.

This commits starts implementing such a widget. Exploratory keyboard
interaction has been implemented but I'm not satisfied with the result.
The first implementation of the multi slider allowed the user to move the
handles with the keyboard. The result was confusing when handles came to
overlap in the middle of a change. The diagram below explains the situation,
with ABC three categories and the pipes representing the handles:

We start with:                  AAAA|B|CCCCC
Moving the left handle:         AAAAA||CCCCC

Here the two handles are on top of each other and cannot be distinguished. The
implementation was chosing the rightmost handle, so moving the handle further
right would result in:
                                AAAAA|B|CCCC

So the user was increasing the amount of A, but by repeating the same action
the amount of A remains fixed and the amount of B increases.

There are several possible remedies to this situation:

 * Use the left handle instead of the right one and keep increasing A. This
   doesn't work in the opposite direction (when increasing C).
 * Stop merging overlapping handles. The visual representation sounds
   complicated.
 * Remember what the user did last and apply some heuristics. Heuristics are
   bound to fail.

Instead a completely different approach is needed. This is necessarily
different from the mouse interaction, for which handles work very well (as
fc_double_edge has proven). This commit choses to focus on the categories
themselves, letting the user increase or decrease the number of items in the
"current" category. Other categories are affected in the process and the code
does its best to accomodate the user's desires (searching for available items
first to the right, then to the left).

Having experimented a bit with this new interface, I find it more natural to
work with than the previous one. With some practice one can probably become
quite efficient at using it.
Mouse interaction for the multi slider bears a lot to fc_double_edge. It was
improved in a few areas:

 * Single clicks no longer make the handles jump around. Drag or double click
   is now required.
 * Better rounding results in smoother moving of the handles.
 * Small visual hints indicate which handle is active.

The visual style is significantly different. More work is likely needed to
properly integrate the widget with Classic (though it is usable).
This removes fc_double_edge, of which multi_slider is a more capable version.
@lmoureaux lmoureaux requested a review from jwrober November 22, 2023 18:25
Copy link
Collaborator

@jwrober jwrober left a comment

Choose a reason for hiding this comment

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

This is a great new feature that I can see us re-using all over the UI. I just have the one review question.

Should we also remove the old sickle icons used?

I think we can update docs here too, but also fine with another PR on that "soon" so we don't forget.

} // namespace metrics
} // anonymous namespace

namespace freeciv {
Copy link
Collaborator

Choose a reason for hiding this comment

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

When we refactor something why don't we use a diff namespace such as fc21 or freeciv21?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The namespace freeciv is actually new. Could switch to fc21 but I find it a bit obscure and it's also harder to type for me (I need shift to type numbers). freeciv21 is probably too long.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok learned something new.

@lmoureaux
Copy link
Contributor Author

Should we also remove the old sickle icons used?

I wasn't sure. I think the new handles are graphically less engaging, but the sickles were also not great. I went with what was easier to code. Better styling support would be good...

@jwrober
Copy link
Collaborator

jwrober commented Nov 24, 2023

Should we also remove the old sickle icons used?

I wasn't sure. I think the new handles are graphically less engaging, but the sickles were also not great. I went with what was easier to code. Better styling support would be good...

The new handles are def not sexy, but they do work well. Do you need code to provide better styling support or is this something we can play with in a theme once merged?

1 similar comment
@jwrober
Copy link
Collaborator

jwrober commented Nov 24, 2023

Should we also remove the old sickle icons used?

I wasn't sure. I think the new handles are graphically less engaging, but the sickles were also not great. I went with what was easier to code. Better styling support would be good...

The new handles are def not sexy, but they do work well. Do you need code to provide better styling support or is this something we can play with in a theme once merged?

@lmoureaux
Copy link
Contributor Author

The new handles are def not sexy, but they do work well. Do you need code to provide better styling support or is this something we can play with in a theme once merged?

Changing them would need more code. Even the colors are hard-coded right now

@jwrober
Copy link
Collaborator

jwrober commented Nov 27, 2023

I added an issue to capture the code for theme support. I do think we should either update this PR with the new images for the client manual or do it very soon in another PR.

@lmoureaux
Copy link
Contributor Author

I would say it should come with the theming PR if it comes in time for 3.1. Let's open an issue and add it to the 3.1 milestone

@lmoureaux lmoureaux merged commit f7c8195 into longturn:master Nov 27, 2023
20 checks passed
@lmoureaux lmoureaux deleted the feature/multi-slider branch November 27, 2023 23:38
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.

2 participants