Page MenuHomeWildfire Games

[CSlider] On click move the slider-button to the mouse position
ClosedPublic

Authored by Imarok on Sep 1 2018, 11:13 AM.

Details

Summary

If the user clicks somewhere on the slider, the slider-button immediately moves to the mouse position. This allows changing slider values faster.

Test Plan

Open Settings and click on some slider.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Imarok created this revision.Sep 1 2018, 11:13 AM
Vulcan added a subscriber: Vulcan.Sep 1 2018, 11:14 AM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/differential/725/display/redirect

Vulcan added a comment.Sep 2 2018, 8:44 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/729/display/redirect

vladislavbelov requested changes to this revision.Dec 28 2018, 2:19 AM
vladislavbelov added inline comments.
source/gui/CSlider.cpp
81 ↗(On Diff #6876)

It'd be good to avoid the code duplication. Especially if we can change the logic: D406.

This revision now requires changes to proceed.Dec 28 2018, 2:19 AM
Imarok marked an inline comment as done.Jan 3 2019, 2:01 AM
Imarok added inline comments.
source/gui/CSlider.cpp
81 ↗(On Diff #6876)

About which lines are you talking? 83 and 84?
If so, what about creating a new function like void changeValue(float difference) for those two lines??

vladislavbelov added inline comments.Jan 13 2019, 8:03 PM
source/gui/CSlider.cpp
1 ↗(On Diff #6876)

Bump year.

81 ↗(On Diff #6876)

81-84 and 99-103 (probably something else too). I'm thinking about like following:

// Private and probably more descriptive name.
float GetRatio() const
{
	 return (m_MaxValue - m_MinValue) / (m_CachedActualSize.GetWidth() - m_ButtonSide);
}

void AddValue(float difference)
{
	m_Value = Clamp(m_Value + difference, m_MinValue, m_MaxValue);
	UpdateValue();
}

// Example:
m_Mouse = GetMousePos();
m_IsPressed = true;
float difference = float(m_Mouse.x - GetButtonRect().CenterPoint().x) * GetRatio();
AddValue(difference);
82 ↗(On Diff #6876)

(GetButtonRect().right + GetButtonRect().left) * 0.5 should be replaced with GetButtonRect().CenterPoint().x.

Imarok updated this revision to Diff 7353.Jan 16 2019, 12:15 AM
Imarok marked an inline comment as done.

Deduplicate code by adding two functions

Imarok updated this revision to Diff 7354.Jan 16 2019, 12:19 AM

Use CenterPoint()

Imarok marked 4 inline comments as done.Jan 16 2019, 12:20 AM
Imarok added inline comments.
source/gui/CSlider.cpp
82 ↗(On Diff #6876)

Nice, didn't knew that exists :)

Imarok marked an inline comment as done.Jan 16 2019, 12:20 AM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/gui/CSlider.h
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/974/

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/gui/CSlider.h
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/975/

Imarok updated this revision to Diff 7447.Feb 4 2019, 8:30 PM

Fix license header year

Vulcan added a comment.Feb 4 2019, 8:33 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/1036/

vladislavbelov added inline comments.Feb 16 2019, 5:53 PM
source/gui/CSlider.cpp
75 ↗(On Diff #7447)

Let's replace these lines and similar below by IncrementallyChangeValue.

source/gui/CSlider.h
59 ↗(On Diff #7447)

I suggest to move these functions under the protect section. As we already have similar functions there.

Imarok added inline comments.Mar 3 2019, 3:32 PM
source/gui/CSlider.cpp
75 ↗(On Diff #7447)

Oh, good idea.

source/gui/CSlider.h
59 ↗(On Diff #7447)

Sure

Imarok updated this revision to Diff 7512.Mar 3 2019, 3:45 PM
Imarok marked 2 inline comments as done.

Implement suggested changes

Vulcan added a comment.Mar 3 2019, 3:48 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/1076/display/redirect

elexis added a subscriber: elexis.Mar 3 2019, 4:17 PM
elexis added inline comments.
source/gui/CSlider.cpp
56 ↗(On Diff #7512)

(probably less fragmentation when grouped with UpdateValue)

(float difference could be const to gain compile errors if this function would change the value)

75 ↗(On Diff #7512)

(OT, but this will become step_size if we later go with that fixed-width slider model, right? So actually there shouldn't be rounding issues (other than that one can address rounding issues if there are some; by then))

Imarok added inline comments.Mar 3 2019, 4:20 PM
source/gui/CSlider.cpp
56 ↗(On Diff #7512)

(probably less fragmentation when grouped with UpdateValue)

UpdateValue might get used somewhere else in future.

(float difference could be const to gain compile errors if this function would change the value)

👍

Imarok updated this revision to Diff 7513.Mar 3 2019, 4:41 PM

Adding const

Vulcan added a comment.Mar 3 2019, 4:43 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/1077/display/redirect

vladislavbelov accepted this revision.Mar 25 2019, 8:16 PM

The patch looks good to me.

source/gui/CSlider.h
48 ↗(On Diff #7513)

I'd suggest to add a little description what the function does.

This revision is now accepted and ready to land.Mar 25 2019, 8:16 PM
Imarok added inline comments.Apr 4 2019, 11:33 PM
source/gui/CSlider.h
48 ↗(On Diff #7513)

good idea. I'd propose:

/**
 * Retrieves the current normalized slider position
 * @return normalized position of the slider (ranging from 0 to 1)
 */
vladislavbelov added inline comments.Apr 4 2019, 11:42 PM
source/gui/CSlider.h
48 ↗(On Diff #7513)

I suppose it's not the slider position, but a density ratio or somehow else. Because you don't count m_Value inside GetSliderRatio.

Imarok added inline comments.Apr 5 2019, 12:09 AM
source/gui/CSlider.h
48 ↗(On Diff #7513)

Oh, you are totally right. I was confused... (Getting late)
So what about:

/**
 * Retrieves the ratio between the value of the slider and its actual size in the GUI.
 * @return ratio between the value of the slider and its actual size in the GUI
 */
vladislavbelov added inline comments.Apr 5 2019, 1:00 AM
source/gui/CSlider.h
48 ↗(On Diff #7513)

A bit repeated but looks ok.

elexis added inline comments.Apr 5 2019, 1:44 AM
source/gui/CSlider.h
48 ↗(On Diff #7513)

Why do we need the sentence twice?
Why do we need the sentence twice?

Imarok added inline comments.Apr 5 2019, 9:06 AM
source/gui/CSlider.h
48 ↗(On Diff #7513)

Doxygen format. First is description. Second what it returns.
Im some cases automated parsers use one in other cases they use the other.
I don't see a way to go with only one of both and still applying ro Doxygen format.

elexis added inline comments.Apr 5 2019, 4:57 PM
source/gui/CSlider.h
48 ↗(On Diff #7513)

Im some cases automated parsers use one in other cases they use the other.

But there can exist arbitrarily many, arbitrarily broken or omissive parsers.

Imagine we would fill in every field of doxygen, and then the same information in multiple fields, tens of thousands of lines without information added and possibly conflicting information (duplication going out of sync).

The purpose of doxygen / JSdoc is (1) to not use random custom comment syntax, but a well-defined syntax and (2) to be able to generate HTML that provides an abstract of the most important mechanisms (not necessarily every parameter, return value for instance).

The border between being too verbose isn't too clear sometimes, but this one really seems like an anti-pattern to me, imagine that would be duplicated for every function globally, unpresentable.

We had this discussion about JSdoc before with Philip, but I can't find it on IRC. He said something along the lines that noone can understand the source code from looking only at the JSdoc/doxygen HTML, but that this is read alongside the code.

Imarok added inline comments.Apr 6 2019, 3:27 PM
source/gui/CSlider.h
48 ↗(On Diff #7513)

So you propose to don't add a comment for return?
Or do you want no description?

Stan added a subscriber: Stan.Apr 6 2019, 3:31 PM
Stan added inline comments.
source/gui/CSlider.h
48 ↗(On Diff #7513)

Usually you would remove description :)

vladislavbelov added inline comments.Apr 6 2019, 4:00 PM
source/gui/CSlider.h
48 ↗(On Diff #7513)

Or @return :)

Stan added inline comments.Apr 6 2019, 4:01 PM
source/gui/CSlider.h
48 ↗(On Diff #7513)

No description cause return is a special keyword :) So it's recognized elsewhere.

vladislavbelov added inline comments.Apr 6 2019, 4:05 PM
source/gui/CSlider.h
48 ↗(On Diff #7513)

AFAIK doxygen may add a description without a return information.

elexis added inline comments.Apr 6 2019, 5:42 PM
source/gui/CSlider.h
48 ↗(On Diff #7513)

IMO flip a coin which copy to nuke or check which tools should be supported by the codebase and make it a benefitial pattern (coding convention) before it can become an antipattern. (Such an investigation might also conclude that duplicaion is more important, even when it affects the entire codebase.)

This revision was automatically updated to reflect the committed changes.