If the user clicks somewhere on the slider, the slider-button immediately moves to the mouse position. This allows changing slider values faster.
Details
- Reviewers
vladislavbelov - Commits
- rP22164: [CSlider] On click move the slider-button to the mouse position
- Trac Tickets
- #2593
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
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/differential/725/display/redirect
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/729/display/redirect
source/gui/CSlider.cpp | ||
---|---|---|
81 ↗ | (On Diff #6876) | About which lines are you talking? 83 and 84? |
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. |
source/gui/CSlider.cpp | ||
---|---|---|
82 ↗ | (On Diff #6876) | Nice, didn't knew that exists :) |
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/
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/1036/
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/1076/display/redirect
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)) |
source/gui/CSlider.cpp | ||
---|---|---|
56 ↗ | (On Diff #7512) |
UpdateValue might get used somewhere else in future.
? |
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/1077/display/redirect
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. |
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) */ |
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. |
source/gui/CSlider.h | ||
---|---|---|
48 ↗ | (On Diff #7513) | Oh, you are totally right. I was confused... (Getting late) /** * 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 */ |
source/gui/CSlider.h | ||
---|---|---|
48 ↗ | (On Diff #7513) | A bit repeated but looks ok. |
source/gui/CSlider.h | ||
---|---|---|
48 ↗ | (On Diff #7513) | Why do we need the sentence twice? |
source/gui/CSlider.h | ||
---|---|---|
48 ↗ | (On Diff #7513) | Doxygen format. First is description. Second what it returns. |
source/gui/CSlider.h | ||
---|---|---|
48 ↗ | (On Diff #7513) |
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. |
source/gui/CSlider.h | ||
---|---|---|
48 ↗ | (On Diff #7513) | So you propose to don't add a comment for return? |
source/gui/CSlider.h | ||
---|---|---|
48 ↗ | (On Diff #7513) | Usually you would remove description :) |
source/gui/CSlider.h | ||
---|---|---|
48 ↗ | (On Diff #7513) | Or @return :) |
source/gui/CSlider.h | ||
---|---|---|
48 ↗ | (On Diff #7513) | No description cause return is a special keyword :) So it's recognized elsewhere. |
source/gui/CSlider.h | ||
---|---|---|
48 ↗ | (On Diff #7513) | AFAIK doxygen may add a description without a return information. |
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.) |