Implemented the new counter widget #6306

grzesiek2010 · 2024-08-05T12:28:36Z

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

The new widget is implemented in the same way as the existing widgets, so there is nothing significant to discuss here. The only additional consideration while working on this issue was the potential use of Jetpack Compose. To implement the layout for this new widget using XML, I had to create the layout file and three separate background XML files. This example highlights the complexity of using XML for layouts. Although I am not an expert in Jetpack Compose, I believe it would simplify the process considerably.

However, I propose we create a separate issue to discuss adopting Jetpack Compose and setting it up, as it might require more detailed consideration.

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?

This adds a new question type. It's an isolated change so we can focus on that new question type. The existing question types should not be at risk.

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

I used this one: counter.xlsx

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

Yes: getodk/docs#1843

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

alyblenkin · 2024-08-06T16:32:29Z

Looking good! A few notes:

I think it should start at zero so that it's more obvious that this is a counter
The number text should be slightly bigger so it's easier to see. I could be wrong but it looks a lot smaller than they other text inputs. I'd imagine people will be looking up counting something and then referencing down every once and a while. Haptic feedback would be a cool addition later on!
It looks like there is a thicker line on the inside of the buttons

Button contrast

Dark theme: are the buttons the same colour as the main menu buttons?
Light theme: the grey I used is too light. The colour you have for the outside border has better contrast, so we can use that for the fill as well. Hopefully shades of grey will be easier once we update the colour palette.

collect_app/src/main/java/org/odk/collect/android/widgets/CounterWidget.kt

collect_app/src/test/java/org/odk/collect/android/widgets/CounterWidgetTest.kt

grzesiek2010 · 2024-08-06T20:33:56Z

I think it should start at zero so that it's more obvious that this is a counter

This would be the only question type that does not support no-answers. We support no-answers even in range widgets, and the reason we display some question types using dialogs (like dates, ranking etc.) was also to allow leaving such a question unanswered. I think it's important to make it consistent with other question types.

The number text should be slightly bigger so it's easier to see. I could be wrong but it looks a lot smaller than they other text inputs. I'd imagine people will be looking up counting something and then referencing down every once and a while. Haptic feedback would be a cool addition later on!

Ok, I will try to increase the size.

It looks like there is a thicker line on the inside of the buttons

Yes, this happens because every element in that layout (the minus button, the plus button, and the value text) has its own border, resulting in doubled adjacent lines. I didn't think it would be noticeable, but I can try to improve it.

Dark theme: are the buttons the same colour as the main menu buttons?

It's the same (colorSurfaceContainerLow) in both light and dark themes.

Light theme: the grey I used is too light. The colour you have for the outside border has better contrast, so we can use that for the fill as well. Hopefully shades of grey will be easier once we update the colour palette.

You mean I should use the same color for the fill and the border?

alyblenkin · 2024-08-06T21:34:03Z

This would be the only question type that does not support no-answers. We support no-answers even in range widgets, and the reason we display some question types using dialogs (like dates, ranking etc.) was also to allow leaving such a question unanswered. I think it's important to make it consistent with other question types.

Ah interesting point. My thinking was zero could be considered no-answer, but zero could be an answer.

It's the same (colorSurfaceContainerLow) in both light and dark themes.

ah okay!

You mean I should use the same color for the fill and the border?

yup, I think we should see if that helps with the contrast.

grzesiek2010 · 2024-08-07T13:28:51Z

@alyblenkin

yup, I think we should see if that helps with the contrast.

Done. I've also increased the font size from 20sp to 25sp. Please let me know if it looks better.

alyblenkin

Looks great

collect_app/src/main/java/org/odk/collect/android/widgets/CounterWidget.kt

seadowg

#6306 (comment)

Just that at the moment, but I'll have a proper last look later.

seadowg · 2024-08-15T08:39:24Z

collect_app/src/main/res/layout/counter_widget.xml

+        android:layout_height="0dp"
+        android:gravity="center"
+        android:background="@drawable/counter_value_background"
+        android:textSize="25sp"


@grzesiek2010 @alyblenkin let's work out a Material 3 text appearance for this to use instead of a custom text size. If there isn't one that feels right, we can tweak one of the appearances to suit us better.

Use disparate text sizes like this will cause us problems down the line when we change themes.

Sounds good.

textAppearanceHeadlineSmall looks nice.

seadowg · 2024-08-15T08:40:45Z

collect_app/src/main/res/drawable/counter_minus_button_background.xml

+    <item>
+        <shape android:shape="rectangle">
+            <corners
+                android:topLeftRadius="10dp"


For this and the plus button, is there a shape appearance theme attribute we could use instead of a hardcoded one?

I'm not sure what would work best but the extended FAB or segmented buttons have a similar design.

dialogCornerRadius looks like a good fit here.

alyblenkin

Looks great! I hope we add haptic feedback one day :)

dbemke · 2024-08-22T13:10:56Z

Tested with Success!

Verified on a device with Android 10

Verified Cases:

counter in dark and light mode
counter with don't keep activities enabled (minimizing the app, rotating)
removing answers
counters on a field-list
counter and last saved
landscape and portrait view
RTL

WKobus · 2024-08-22T13:12:36Z

Tested with Success

Verified on device with Android 14

grzesiek2010 marked this pull request as ready for review August 5, 2024 15:02

grzesiek2010 requested review from seadowg and alyblenkin August 5, 2024 15:02

seadowg requested changes Aug 6, 2024

View reviewed changes

alyblenkin approved these changes Aug 7, 2024

View reviewed changes

grzesiek2010 mentioned this pull request Aug 8, 2024

Add tests for widgets to assert that answers are not displayed anymore after removing them #6319

Open

grzesiek2010 requested a review from seadowg August 8, 2024 13:49

seadowg requested changes Aug 8, 2024

View reviewed changes

collect_app/src/main/java/org/odk/collect/android/widgets/CounterWidget.kt Outdated Show resolved Hide resolved

grzesiek2010 requested a review from seadowg August 8, 2024 17:10

seadowg requested changes Aug 9, 2024

View reviewed changes

grzesiek2010 requested a review from seadowg August 14, 2024 14:51

seadowg requested changes Aug 15, 2024

View reviewed changes

grzesiek2010 added 12 commits August 20, 2024 23:09

Implemented the new counter widget

5c093b0

Added tests

561286f

Naming improvements

6de395e

Converted WidgetFactoryTest to kotlin

3de5478

Added new tests

6ff368d

Added missing tests for widgetValueChanged

13d6bc5

Improved tracking the value

c5c803d

Changed the fill color

d8f4ead

Increased the font size

e645621

Created a const for max value

614a66d

Use textAppearanceHeadlineSmall to display value

5dccd46

Use dialogCornerRadius to set corners of the counter

7997eb1

grzesiek2010 force-pushed the COLLECT-6137 branch from 57de8e2 to 7997eb1 Compare August 20, 2024 21:09

grzesiek2010 requested a review from seadowg August 20, 2024 21:11

grzesiek2010 requested a review from alyblenkin August 20, 2024 21:11

alyblenkin approved these changes Aug 20, 2024

View reviewed changes

seadowg approved these changes Aug 21, 2024

View reviewed changes

seadowg added the needs testing label Aug 21, 2024

seadowg merged commit 6d2e872 into getodk:master Aug 21, 2024
6 checks passed

WKobus added behavior verified and removed needs testing labels Aug 22, 2024

grzesiek2010 mentioned this pull request Aug 22, 2024

Fixed the counter widget in rtl languages #6370

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented the new counter widget #6306

Implemented the new counter widget #6306

grzesiek2010 commented Aug 5, 2024 •

edited

Loading

alyblenkin commented Aug 6, 2024

grzesiek2010 commented Aug 6, 2024 •

edited

Loading

alyblenkin commented Aug 6, 2024 •

edited

Loading

grzesiek2010 commented Aug 7, 2024

alyblenkin left a comment

seadowg left a comment

seadowg Aug 15, 2024

alyblenkin Aug 15, 2024

grzesiek2010 Aug 20, 2024

seadowg Aug 15, 2024

alyblenkin Aug 15, 2024

grzesiek2010 Aug 20, 2024

alyblenkin left a comment

dbemke commented Aug 22, 2024

WKobus commented Aug 22, 2024

Implemented the new counter widget #6306

Implemented the new counter widget #6306

Conversation

grzesiek2010 commented Aug 5, 2024 • edited Loading

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

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?

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

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

Before submitting this PR, please make sure you have:

alyblenkin commented Aug 6, 2024

grzesiek2010 commented Aug 6, 2024 • edited Loading

alyblenkin commented Aug 6, 2024 • edited Loading

grzesiek2010 commented Aug 7, 2024

alyblenkin left a comment

Choose a reason for hiding this comment

seadowg left a comment

Choose a reason for hiding this comment

seadowg Aug 15, 2024

Choose a reason for hiding this comment

alyblenkin Aug 15, 2024

Choose a reason for hiding this comment

grzesiek2010 Aug 20, 2024

Choose a reason for hiding this comment

seadowg Aug 15, 2024

Choose a reason for hiding this comment

alyblenkin Aug 15, 2024

Choose a reason for hiding this comment

grzesiek2010 Aug 20, 2024

Choose a reason for hiding this comment

alyblenkin left a comment

Choose a reason for hiding this comment

dbemke commented Aug 22, 2024

WKobus commented Aug 22, 2024

grzesiek2010 commented Aug 5, 2024 •

edited

Loading

grzesiek2010 commented Aug 6, 2024 •

edited

Loading

alyblenkin commented Aug 6, 2024 •

edited

Loading