Prevent upper alarm limit from being set below lower limit in the frontend

[rohanpurohit]

This PR fixes #373

• disables alarm upper limit to go below the lower limit
• disables alarm lower limit to go above the upper limit

[rohanpurohit]

@ethanjli if all good then maybe we can push this to 0.11?

[ethanjli]

Observations from testing:

• When I have SpO2 upper alarm limit at 90 and I rapidly click the "increment" button for the lower limit, I'm rarely able to set the lower limit to 91 (but the button is disabled afterwards) - but I'm unable to reproduce this consistently. I suspect there's a slight delay between when the value reaches 90 and when the button is disabled. I think we need an additional check in the onClick button handlers to sanitize the values to enforce that lower <= upper.
• I am always able to use the rotary encoder to set the lower alarm limit above the upper alarm limit. I believe having the check in onClick would also enforce lower <= upper for the rotary encoder, but I'm not sure.

Do you think we should address these issues in this PR, or in a future PR?

[rohanpurohit]

Observations from testing:

• When I have SpO2 upper alarm limit at 90 and I rapidly click the "increment" button for the lower limit, I'm rarely able to set the lower limit to 91 (but the button is disabled afterwards) - but I'm unable to reproduce this consistently. I suspect there's a slight delay between when the value reaches 90 and when the button is disabled. I think we need an additional check in the onClick button handlers to sanitize the values to enforce that lower <= upper.
• I am always able to use the rotary encoder to set the lower alarm limit above the upper alarm limit. I believe having the check in onClick would also enforce lower <= upper for the rotary encoder, but I'm not sure.

Do you think we should address these issues in this PR, or in a future PR?

I will try to fix the rotary one in this PR, and the other one depending on how often I can reproduce it, I'll either do in this or future PR 👍🏽

[rohanpurohit]

• When I have SpO2 upper alarm limit at 90 and I rapidly click the "increment" button for the lower limit, I'm rarely able to set the lower limit to 91 (but the button is disabled afterwards) - but I'm unable to reproduce this consistently. I suspect there's a slight delay between when the value reaches 90 and when the button is disabled. I think we need an additional check in the onClick button handlers to sanitize the values to enforce that lower <= upper.

Although, I could not reproduce this, I have made a change to onClick callback, can you try to reproduce with the latest changes @ethanjli ? thanks!

• I am always able to use the rotary encoder to set the lower alarm limit above the upper alarm limit. I believe having the check in onClick would also enforce lower <= upper for the rotary encoder, but I'm not sure.

So with the current change there is a regression, it will stop changing once it reaches the disabled limit but it won't change the opposite way unless clicked by mouse/touch, you could maybe test this behavior and let me know if we should revert the change and fix properly in a future PR! just to give an example (decrement the upper limit <= to lower limit, it won't go further down, try incrementing the upper limit via rotary encoder -> that fails, click the increment button then rotary works)

[ethanjli]

This regression is introduced by automatically disabling the Rotary Encoder Controller when the value reaches a limit, which isn't the approach I had in mind. Assuming you tried simply adding a check in the onClick button handlers to sanitize the values to enforce that lower <= upper (i.e. clamping the input to onClick to be within valid ranges such that lower <= upper), which I think should prevent the rotary encoder from breaking lower <= upper, what issues did you encounter with that approach? We already enforce this kind of check for the outer limits on the sliders (e.g. 0 - 100 for SpO2 alarms) - currently how does the frontend prevent values from being adjusted outside those limits by the rotary encoder?

[rohanpurohit]

This regression is introduced by automatically disabling the Rotary Encoder Controller when the value reaches a limit, which isn't the approach I had in mind. Assuming you tried simply adding a check in the onClick button handlers to sanitize the values to enforce that lower <= upper (i.e. clamping the input to onClick to be within valid ranges such that lower <= upper), which I think should prevent the rotary encoder from breaking lower <= upper, what issues did you encounter with that approach? We already enforce this kind of check for the outer limits on the sliders (e.g. 0 - 100 for SpO2 alarms) - currently how does the frontend prevent values from being adjusted outside those limits by the rotary encoder?

Ya it was just a temporary approach, mainly we will have to disable change in one direction(where it's disabled) and pass onClick with changed value in the other, i think it gets tricky there, definitely something I did not think about is, RotaryEncoderController should be aware of which direction is blocked, so passing just disableChange boolean is not right.

[rohanpurohit]

1. This project is licensed under Apache License v2.0 for any software, and Solderpad Hardware License v2.1 for any hardware - do you agree that your contributions to this project will be under these licenses, too? Yes.
2. Were any of these contributions also part of work you did for an employer or a client? No.
3. Does this work include, or is it based on, any third-party work which you did not create? No.