Removed limit for MouseHighlighter Animation Duration and Animation D… #29352
Conversation
…elay. (If user sets these values to 0ms, it will actually be set to 1ms to avoid crashing the app)
|
I made this a draft because I don't know if my hack to avoid the fader not working is allowed or not. When I tested this solution with both Fade Duration and Fade Delay set to 0, it worked as intended (except that it's actually 1ms instead of the displayed 0) |
| @@ -196,7 +196,7 @@ | |||
| <NumberBox | |||
| MinWidth="{StaticResource SettingActionControlMinWidth}" | |||
| LargeChange="100" | |||
| Minimum="100" | |||
| Minimum="0" | |||
There was a problem hiding this comment.
Why not setting this to 1 instead of doing a hack?
There was a problem hiding this comment.
I figured that would lead to an annoying interaction.
If you try to go to 0 and end up on 1 and then increase the duration again using the arrows, you'd end up on 11 or 101.
But that's probably worth it to avoid the hack, right?
There was a problem hiding this comment.
There are two xaml properties for this control you can use to manipulate this behavior: SmallChange and LargeChange. Not sure if it makes sense to adjust them.
There was a problem hiding this comment.
That's my thinking. Then I'd have to add conditionals for those. Which in turn could lead to users getting annoyed because it doesn't work "as it should".
That's my justification for my hack. If we don't want that, then I think I'd prefer to just set the limit to 1 as you suggested.
| @@ -205,7 +205,7 @@ | |||
| <NumberBox | |||
| MinWidth="{StaticResource SettingActionControlMinWidth}" | |||
| LargeChange="100" | |||
| Minimum="100" | |||
| Minimum="0" | |||
|
Why would it crash with zero in the first place? |
I haven't been able to figure that out yet. Looking into it now. |
Okay, so I'm not entirely sure what I'm looking at. This is from _msvc_chrono.hpp
And this is from the MouseHighlighter.cpp file. Does this mean (sorry if my question is dumb) that the duration can't be 0 because we use std::chrono? |
|
"period negative or zero" seems to be the reason. An animation that takes no time, would technically not be an animation. |
That's my interpretation as well. Then the question becomes, should I keep my hack or set the lower limit to 1ms? I can't really see another way of resolving the issue. |
I think the best thing is setting it to 1. If we can't crash on a json value of zero or negative int you can remove the hack. Otherwise we should keep it. |
Removed the hack ;D
Changed the values to 1
|
With the value limited to 1, I don't crash when I test locally. Removed the hack and set the lower limits to 1. If everything looks good and we all agree that this is the best course of action, I believe this one is ready. |
|
I do agree the "101" issue isn't pretty. If it were me, I would go for the work-around in code. But that's just me, or us 3. Maybe another opinion would be good. |
I think it's confusing if for a user if he/she sets 0 and it acts as 1. I would mark it as ready and wait on feedback from the core team. |
|
There is nobody who could possibly detect an animation of one millisecond. |
|
Another option is to have the lower limit set to 1, and add a conditional that makes LargeChange and SmallChange 99 and 9 respectively if it's set to 1. What do you think about such a solution? |
Wayyy too complicated. I suggest to go with your first solution and (in code) treat zero as one. We're dealing with milliseconds here, not seconds... |
True. That would be overengineering a simple problem. Okay. I'll add the workaround again, and then let the core team have the final say. |
Reintroducing workaround
Changed the minimum values for FadeDelayMs and FadeDuration back to 0.
…elay.
(If user sets these values to 0ms, it will actually be set to 1ms to avoid crashing the app)
Summary of the Pull Request
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed