Add support for combination of minimal and autocomplete appearances

[grzesiek2010]

Closes #3736 #3297

What has been done to verify that this works as intended?

I tested select widgets manually.

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

This is what our users requested and what we decided to implement.
When it comes to the implementation I used RecyclerView like in all other selects (excluding those horizontal: LikertWidget/ListWidget/ListMultiWidget), thanks to that we can mix minimal with the same appearances we use in normal selects: columns-pack/columns-n/no-buttons etc.

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?

We need to test all select widgets that use RecyclerView (basicly all select widgets excluding those horizontal: LikertWidget/ListWidget/ListMultiWidget).

This change also enables mixing more appearances and now we can mix minimal widgets not only with autocomplete (what was the purpose of the issue) but also with other appearances we use in normal selects like: no-buttons/columns-pack/columns-n

that's why I said that we need to test all possible selects with all possible appearances. I know it's not easy since we can mix a lot of appearances.

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

Any form with any select widgets.
I'm attaching some that I used:
selectAppearances.zip
selectAppearancesWithMinimal.zip

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

Yes we need to update screenshots.

Before submitting this PR, please make sure you have:

• run ./gradlew checkAll and confirmed all checks still pass OR confirm CircleCI build passes and run ./gradlew connectedDebugAndroidTest locally.
• 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]

@lognaturel @seadowg I need to add some final changes and tests probably but could you play with it for a minute and let me know if there is anything I should change? You can use just the All widgets form.

[grzesiek2010]

@seadowg @lognaturel as I said above probably I need to think of adding some more tests (the more the better) but it would be good to hear from you at least about the UI.

[lognaturel]

Sorry about the delay and hope you had a good week away! This looks great. I hadn't considered that it would allow both minimal and autocomplete to combine with other appearances. It's always been confusing to know what appearances can combine so that's great. The downside is that now there are a lot more select views than before so careful automated tests that will save us @getodk/testers from having to manually try the full cross product of appearances is really important!

I didn't immediately know what to do with the input line that looks disabled. In the case of minimal and autocomplete combined, if that field is the first on a screen, could the field have focus with the keyboard displayed? The full screen dialog would then appear when the user starts typing. That way it would feel like a text field with autocomplete. That's what the original requester described on the forum so it seems there's a precedent for it.

For minimal without autocomplete, I think we should keep the same look as on master. That's what Material Design's exposed dropdown menu does with a ▼ at the right of the text field. We may even want to keep the text Select One Answer. What made you want to remove that?

I think that making a selection should exit the full screen dialog the same way that on master making a selection in a minimal select collapses the spinner. It's particularly inconvenient on a large screen to have to go all the way up to the top to exit.

On Android 5.1, the background color of the full screen dialog is not the same as the background color of the form entry activity. Seems they should match.

The font size for the select choice should match the font size of text in a text field (and be dynamic in the same way, I didn't check that). It looks smaller on my device.

I get a SelectOneMinimalWidget.recyclerViewAdapter leak on Android 5.1.

Other than that, the full screen dialog looks good to me both with and without autocomplete.

[grzesiek2010]

I didn't immediately know what to do with the input line that looks disabled. In the case of minimal and autocomplete combined, if that field is the first on a screen, could the field have focus with the keyboard displayed? The full screen dialog would then appear when the user starts typing

Theoretically it is possible but then if you select for example answer option1 it should be displayed in that box (after closing the dialog) so if a user opens the dialog again they could expect only that option1 to be displayed because that's the phrase from the search box? it's weird... maybe we shouldn't display answers in the box but below?

For minimal without autocomplete, I think we should keep the same look as on master. That's what Material Design's exposed dropdown menu does with a ▼ at the right of the text field. We may even want to keep the text Select One Answer. What made you want to remove that?

I somehow missed it being too focused on the new minimal + autocomplete. I agree that if we have just minimal that search box doesn't make sense. To be clear you just want to have something like a spinner button but not the old spinner widget? I mean it will look like a spinner but it will open the same dialog I use for autocomplete right because it doesn't make sense to keep those old widgets for minimal appearances without autocomplete? What about select multi? it had a normal button.

I think that making a selection should exit the full screen dialog the same way that on master making a selection in a minimal select collapses the spinner. It's particularly inconvenient on a large screen to have to go all the way up to the top to exit.

I agree. The dialog should be closed in if we use select_one.

The font size for the select choice should match the font size of text in a text field (and be dynamic in the same way, I didn't check that). It looks smaller on my device.

The font size of every single option is dynamic if that's what you are asking for.

[lognaturel]

@grzesiek2010 and I had a brief chat about this and agreed to fake a spinner look for the inline field (basically add ▼ at the right) for all selects with minimal appearance regardless of whether they autocomplete or not. We also agreed to put the selected value(s) in the text field, even in the case of multiple selects. That increases consistency across the board.

if you select for example answer option1 it should be displayed in that box (after closing the dialog) so if a user opens the dialog again they could expect only that option1 to be displayed because that's the phrase from the search box

Agreed this is totally weird and I hadn't thought through the case where something was already selected. Having considered it more, I think that adding the ▼ at the right will likely make it clear enough that the field is tappable. So basically there would be no difference between minimal and minimal autocomplete within the form. It's only after tapping the text field that there would be a difference. For minimal autocomplete, there would be a search field with focus. For minimal there would just be the list of items. How does that sound, @grzesiek2010? Might be good for you to take a quick look too, @seadowg, since you did the initial sketches. The two questions we're trying to answer are "how do we make it obvious that for a select with the minimal appearance, tapping the field will have some effect" and "should there be any different behavior inline in the form filling view if the autocomplete appearance combines with `minimal."

[grzesiek2010]

Having a box that would look like spinner for both minimal and minimal autocomplete appearances seems right to me but yeah let's hear from @seadowg

[seadowg]

@grzesiek2010 @lognaturel looking at it feels to me like a fake Spinner for just minimal and a fake EditText (with focus where appropriate) for minimal autocomplete makes sense. I think in both cases we should use the real Android views but just override the click behaviour. On the EditText we could have it open the selection screen when anything is typed so it works properly when focused I think?

Maybe just using a Spinner for both is a good first step though and then if we feel like we want to make it slicker later we can?

[grzesiek2010]

So basically you don't agree with what we proposed above (having spinner for both cases) @seadowg? My problem with that edittext is like:

• it is not intuitive
• I don't think that we need to have different widget layouts for minimal vs minimal autocomplete, displaying the search box in the dialog would be enough from a user perspective
• as I said above displaying answers in such edittext is confusing

So I would be in favour of having spinners in all cases.

[seadowg]

So basically you don't agree with what we proposed above (having spinner for both cases) @seadowg?

No I think it works. Sorry I think I was being confusing! I was trying to explain that my ideal would be having the minimal autocomplete look like a standard text widget but when you click on it or type it opens the select screen (like in the mockup). I get that this is probably awkward to implement - it would require making sure you'd handled click typing listeners on the EditText and could smoothly pass the keys typed from one view to the other.

Given all that I'm happy to go with a Spinner for both.

[lognaturel]

@seadowg it’s not about implementation. We just can’t figure out reasonable behavior when something is already selected. The behavior is clear and good when nothing is selected: the text field has focus if it’s the first question on the screen and then if you start typing it opens the dialog and filters the list according to what you’ve typed. But what if you have a fruit list and you’ve already selected “apple”? The edit text will have “apple” in it, right? So then where should the cursor be if the field has focus? What should happen when you start typing?

One new idea I just had is that if there’s already a selection the contents of the edit text could be selected when it gets focus. That way, when you start typing it would start a new search.

[grzesiek2010]

Do you like this? I do

[seadowg]

@grzesiek2010 I like that! Is that just the standard Spinner? I think if possible it'd be nice to avoid any custom views.

@lognaturel in my head this all works but maybe I'm crazy. If something is selected it would the cursor would work just like it would with a standard text field and if you start typing (while focused) or click on it the selection screen would pop up and you would continue there. Again though as I've been thinking about it seems really awkward to implement this smoothly! I guess I'd thought about it like a text field that you can only populate with specific answers. Anyway I think the direction we're going in is the pragmatic one so enough about my mad scientist approach.

[lognaturel]

if you start typing (while focused) or click on it the selection screen would pop up

Let’s say the default or previously selected value is “apple” and you want to change it to “banana.” The question is the only one on the screen. If it were a regular edit text, we’d give it focus and put the cursor at the end. But like @grzesiek2010 mentioned if that were the case and you typed “b” then you’d end up with a filter of “appleb” and an empty filtered list in the dialog. Selecting the whole edit text content so it gets replaced when the user starts typing makes some amount of sense for select ones. It doesn’t really work for select multiples where you might want to change a subset of choices (Edit: I don't know, maybe it still makes sense? Then what you're typing is clearly the filter query, not the full value of the field). I think we’d need to have a really precise behavior description for both those cases to consider something other than forcing the user to tap to get to choices in all cases.

[grzesiek2010]

Is that just the standard Spinner?

it is just a text field + image view: https://github.com/getodk/collect/pull/3964/files#diff-731643aa399535acb9f8639380ec98cc

I have also some concerns about possible ways of closing the dialog/accepting selected answers, for now it is possible to close the dialog clicking on the back arrow (from the toolbar) or clicking the device back button. In both cases selected answers are saved (accepted). Do you find it confusing and think that maybe we shouldn't accept new answers when the dialog is closed by the device back button (treat it as cancel button)? On the other hand that back arrow also looks like a way to click cancel maybe it should be ✔️ ?

[seadowg]

it is just a text field + image view: https://github.com/getodk/collect/pull/3964/files#diff-731643aa399535acb9f8639380ec98cc

Hmmm I think it would be nice to avoid custom UI if possible (as much as I actually like what you've created more than the Android Spinner style). Was there a specific reason to not use an actual Spinner?

[grzesiek2010]

Hmmm I think it would be nice to avoid custom UI if possible (as much as I actually like what you've created more than the Android Spinner style). Was there a specific reason to not use an actual Spinner?

A few reasons:

1. The original spinner looks worse to me.
2. Adding a spinner just to have its layout not functionality (because of course we have our own dialog) is a bit strange
3. It would be more difficult to maintain, it would require an adapter or something that fakes it and changing answers would be more difficult compared to the edit text i use where I just call setText().

[lognaturel]

for now it is possible to close the dialog clicking on the back arrow (from the toolbar) or clicking the device back button. In both cases selected answers are saved (accepted)

This sounds right to me. In the case of a select one, I think it's the only sensible option. Tapping an option selects it and closes the dialog. The back buttons just leaves whatever selection there already might be.

In the case of select multiples, it's a little less clear. But I think that if all ways of exiting the dialog commit the current selections, it's at least consistent and easy to reason about.

[seadowg]

It would be more difficult to maintain, it would require an adapter or something that fakes it and changing answers would be more difficult compared to the edit text i use where I just call setText().

Great point. I think I'm convinced using the custom spinner is less of a maintenance risk than using a real one and faking its setup 👍.

[grzesiek2010]

@lognaturel I implemented improvements and fixes I wanted, would be great if you could take another look during my night/your day at the UI. If what I have implemented is ok tomorrow I'll work on tests, more tests...

[lognaturel]

I get an NPE when trying to tap a minimal select field with your latest commit. I think context may be accessed prematurely but don't have time for a deep dive right now. The previous commit seems to work as expected.

Although the minimal select looks good, it's not consistent with the rest of the question types we offer. I think this might be some of what @seadowg was getting at too. Does it match some material design guideline I haven't noticed? When I look at this page I see that selects look like text fields except for the downward triangle.

I see a leak of SelectMultiMinimalWidget.adapter (maybe that was fixed by your latest commit).

The background color of the full screen dialog doesn't match the FormEntryActivity.

I notice many of the select one appearances don't immediately select an option on tap. I think that should be consistent. Is there a reason it isn't?

It looks like we talked past each other for the select behavior! I was thinking we would treat it like settings full screen dialogs. Look at settings for any of the Google apps and you'll see that they're committed immediately and either the arrow or the hardware back button just returns to wherever the user was before. This is what makes sense to me and I find it really confusing that the hardware back button doesn't commit. If you're not comfortable with that, then I think we should follow the guidelines here. That is, we'd have an X at the upper left and SAVE at the upper right. The hardware back button would close without saving, the same behavior as the X. That would make it clear to me that there's only one path to save and everything else cancels. For this to work you'd need to clearly combine the search box clearing icon (currently x) with the search bar. I have a slight preference for the simper model with immediate commit but it's not very strong.

[grzesiek2010]

I get an NPE when trying to tap a minimal select field with your latest commit. I think context may be accessed prematurely but don't have time for a deep dive right now. The previous commit seems to work as expected.

hmmm can't reproduce now... I tried three devices.

Although the minimal select looks good, it's not consistent with the rest of the question types we offer.

I thought this is what we agreed on... I don't know, this can't be even super consistent because in all the other cases we display choices in a widget and in all the other widgets that display dialogs we use buttons. So how do you even imagine the consistency?

I notice many of the select one appearances don't immediately select an option on tap. I think that should be consistent. Is there a reason it isn't?

what do you mean? Is there any change in behavior that I implemented?

I see a leak of SelectMultiMinimalWidget.adapter (maybe that was fixed by your latest commit).

In canary leak or is it maybe a crash?

The background color of the full screen dialog doesn't match the FormEntryActivity.

is that bad? all our dialog (at least full screen ones) use that color for example the one that asks for identity which also is a part of filling a form.

and either the arrow or the hardware back button just returns to wherever the user was before. This is what makes sense to me and I find it really confusing that the hardware back button doesn't commit.

ok sorry I misunderstood our previous conversation. i added the code back and now answers are saved even even if you click device back button.

[lognaturel]

Still get the crash in Android 5.1. I thought it was because Fragment lifecycle methods changed slightly at some point and there's no onAttach(context) in Android 5.1 but I get it on my Android 9 emulator too.

    java.lang.NullPointerException: Attempt to invoke virtual method 'android.content.res.Resources android.content.Context.getResources()' on a null object reference
        at android.view.ViewConfiguration.get(ViewConfiguration.java:384)
        at android.view.View.<init>(View.java:3834)
        at org.odk.collect.android.adapters.AbstractSelectListAdapter.setUpNoButtonsView(AbstractSelectListAdapter.java:156)
        at org.odk.collect.android.adapters.AbstractSelectListAdapter$ViewHolder.bind(AbstractSelectListAdapter.java:257)
        at org.odk.collect.android.adapters.SelectOneListAdapter$ViewHolder.bind(SelectOneListAdapter.java:110)
        at org.odk.collect.android.adapters.AbstractSelectListAdapter.onBindViewHolder(AbstractSelectListAdapter.java:104)
        at org.odk.collect.android.adapters.AbstractSelectListAdapter.onBindViewHolder(AbstractSelectListAdapter.java:67)
        at androidx.recyclerview.widget.RecyclerView$Adapter.onBindViewHolder(RecyclerView.java:7065)
        at androidx.recyclerview.widget.RecyclerView$Adapter.bindViewHolder(RecyclerView.java:7107)
        at androidx.recyclerview.widget.RecyclerView$Recycler.tryBindViewHolderByDeadline(RecyclerView.java:6012)
        at androidx.recyclerview.widget.RecyclerView$Recycler.tryGetViewHolderForPositionByDeadline(RecyclerView.java:6279)
        at androidx.recyclerview.widget.RecyclerView$Recycler.getViewForPosition(RecyclerView.java:6118)
        at androidx.recyclerview.widget.RecyclerView$Recycler.getViewForPosition(RecyclerView.java:6114)
        at com.google.android.flexbox.FlexboxLayoutManager.getFlexItemAt(FlexboxLayoutManager.java:456)
        at com.google.android.flexbox.FlexboxLayoutManager.getReorderedFlexItemAt(FlexboxLayoutManager.java:474)

I thought this is what we agreed on... I don't know, this can't be even super consistent

I should have reacted to that thread previously. I agree that it's not going to be exactly the same as any other question type. Maybe the thing that bothers me most is the contrasting background because it really makes the field stand out in a field list whereas I think it should look more like question types based on edit texts. I find that the padding is also quite tight around the text when compared to Material Design examples. But all that said, I don't think this is too important for now. Like I said, I agree it looks good, and I think that if we make cosmetic changes later on it won't be confusing as long as the behavior is the same. For now, how about you just try it in a field list with a string question and see if there's anything you can do to make things look more balanced. Then we can come back to it later.

what do you mean? Is there any change in behavior that I implemented?

I am not able to get it running right now but if I remember correctly, select one with columns does what I expect: when I tap a choice, the full screen dialog closes and my choice is committed. However, I think it's all the select one columns-pack no-button appearances that don't do what I expect: I select a choice and nothing happens. I then have to tap to commit the choice and exit the dialog.

In canary leak or is it maybe a crash?

Reported by Leak Canary.

is that bad? all our dialog (at least full screen ones) use that color for example the one that asks for identity which also is a part of filling a form.

Ok, that's reasonable, then!

i added the code back and now answers are saved even even if you click device back button.

Cool. And does it feel ok to you? Again, I'm trying to be consistent with other apps I've used but if you have examples that point to alternatives, I'm certainly open to those.

[grzesiek2010]

Still get the crash in Android 5.1. I thought it was because Fragment lifecycle methods changed slightly at some point and there's no onAttach(context) in Android 5.1 but I get it on my Android 9 emulator too.

ah ok I got it, fixed.

I am not able to get it running right now but if I remember correctly, select one with columns does what I expect: when I tap a choice, the full screen dialog closes and my choice is committed. However, I think it's all the select one columns-pack no-button appearances that don't do what I expect: I select a choice and nothing happens. I then have to tap to commit the choice and exit the dialog.

ok I see, fixed.

Cool. And does it feel ok to you? Again, I'm trying to be consistent with other apps I've used but if you have examples that point to alternatives, I'm certainly open to those.

that seems fine to me and it was the way I implemented it initially so I unnecessarily thought to much about it.

[kkrawczyk123]

Tested with success!
Verified on Android 5.1, 7.1 and 10.

Tested scenarios:

• light/dark mode
• moving forward/ backward
• jump from hierarchy
• minimize app
• screen rotation
• remove response
• read only questions
• RTL language
• different types of select one widget
• different types of select multi widget
• select one/multi with quick appearance
• select one/multi with autocomplete
• selects with icons
• external itemsets
• cascasing external
• fieldlist
• select mininal
• search and select
• gridwidget
• gridwidget with autoplay: issue visible on master too and reported in Regression on grid widget with autoplay  #4043
• select with autoplay
• selects with columns-pack appearance
• don't keep activities
• regression check on all widgets

great work @grzesiek2010!

[mmarciniak90]

I also confirm successful tests on Android versions: 7.0, 8.1, 9.0
@grzesiek2010 👍 no bugs in such a complex PR!!

@getodk-bot unlabel "needs testing"
@getodk-bot label "behavior verified"

[grzesiek2010]

no bugs in such a complex PR!!

wow I can finish my career satisfied then 😄