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

Fixed clicking on the min value when there is no value yet #6524

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

grzesiek2010
Copy link
Member

@grzesiek2010 grzesiek2010 commented Nov 20, 2024

Closes #6504

Why is this the best possible solution? Were any other approaches considered?

The issue arises because all our question types support a "no answer" state, whereas sliders lack this concept. To address this, we adjusted the slider's appearance but couldn't set its value outside the specified range (or provide no answer at all). For example, if the range was 1–10, the value defaulted to 1 (the minimum) when no answer was provided. As a result, clicking the minimum value didn't trigger a click event since consecutive clicks on the same value don't generate multiple value-changed events.

To resolve this, I implemented a separate interface to listen for touch events and trigger the missing event when no answer has been provided yet.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

As mentioned above, the issue occurred when clicking on the minimum value with no answer provided. Please test this scenario as well as clicking on other values to ensure everything works correctly.

Do we need any specific form for testing your changes? If so, please attach one.

No.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No.

Before submitting this PR, please make sure you have:

  • added or modified tests for any new or changed behavior
  • run ./gradlew connectedAndroidTest (or ./gradlew testLab) and confirmed all checks still pass
  • added a comment above any new strings describing it for translators
  • added any new strings with date formatting to DateFormatsTest
  • verified that any code or assets from external sources are properly credited in comments and/or in the about file.
  • verified that any new UI elements use theme colors. UI Components Style guidelines

@grzesiek2010 grzesiek2010 marked this pull request as ready for review November 20, 2024 20:20
Copy link
Member

@seadowg seadowg left a comment

Choose a reason for hiding this comment

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

I don't think I understand the implementation. I can remove the enable/disable calls and no tests fail, so maybe I'm missing some edge cases that need tests?

import android.view.MotionEvent
import com.google.android.material.slider.Slider

fun Slider.clickOnStep(step: Float) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this should probably live in testshared instead as it's not the kind of thing that I feel should be androidx.test:core (which is what androidtest is really there for).

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@Test
public void changingSliderValueToTheMinOneWhenSliderHasNoValue_setsTheValue() {
RangeDecimalWidget widget = createWidget(promptWithQuestionDefAndAnswer(rangeQuestion, null));
widget.slider.measure(100, 10);
Copy link
Member

Choose a reason for hiding this comment

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

We could add a helper for the measure/layout boilerplate to SliderExt as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

It turns out widget.slider.measure(100, 10); is not needed so I removed it.

@grzesiek2010
Copy link
Member Author

I don't think I understand the implementation.

Let me explain:

  1. When the range question type has no value yet, we programmatically make the slider appear as though it has no value. However, since the slider doesn't inherently support this concept, its value is set to the minimum value under the hood (set but not displayed).
  2. If you try to click on the minimum value, the onValueChanged event is not triggered because it assumes no change has occurred.
  3. This was the issue: if you first clicked on any value other than the minimum, subsequent clicks on the minimum value would trigger the event since it now recognized a change in value.
  4. To resolve this, I added a custom listener to detect clicks on the slider when it is in a "disabled" state (i.e., no value set yet).
  5. This listener operates via an OnTouchListener in a way that ensures it is triggered only when necessary. Specifically:
    • If you click on a value other than the minimum, onValueChanged is already called, and the slider is marked as enabled, so the additional listener is not invoked.
    • If you click on the minimum value, onValueChanged is not called, leaving the slider in a "disabled" state. In this case, the custom listener detects the interaction and handles it appropriately.

I can remove the enable/disable calls and no tests fail, so maybe I'm missing some edge cases that need tests?

This is because I initially added tests only to cover the bug. However, the state is also used to prevent updating the value twice. As mentioned above, if the slider is already enabled, there’s no need to call the listener. I added new tests to cover that.

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.

Impossible to tap the first point on the axis in the range widget
2 participants