Moving the Building Levels quest form to Jetpack Compose.

[GaeaKat]

No description provided.

[GaeaKat]

Spotted a couple flaws, fixing

[westnordost]

Hey, thank you for that contribution!

For being new to Compose, you are already doing a lot of things correctly, IMO.

Here's a small pre-review, without having looked at how it looks like in the app:

1. Some formatting could be improved. I think ktlint could help you there, @FloEdelmann is the expert regarding that. Things like space after if, space before {, space around operators like !=, space after , etc

2. For things like buttons (AddBuildingLevelsButton), try to use standard components. Doesn't using Button as the container for the row of text, image, text produce a satisfactory result? The issues with defining your own shapes resembling buttons are amongst other things 1. doesn't look right in dark mode, 2. no tap (ripple) feedback, 3. isn't changed automatically when app-wide theme changes are made (such as using Material 3 instead of Material 2)

3. avoid specifying the font size directly, use styles wherever possible, e.g. style = MaterialTheme.typography.caption or MaterialTheme.typography.body2 (See the Typography class for the available text styles in Material2). Same goes for other styling, e.g. regarding colors (for errors etc).

4. Are you sure about wrapContentSize/wrapContentWidth? I think it is superfluous / not doing what you think it does.

5. don't put compose code into fragments. So there's this levels+roof levels input field? That's one widget, put it into an own file! So there is this form consisting of the mentioned input field plus a row of previous-selection buttons? Put that into yet another file! There's that row of previous-selection buttons? Put that at least into its own function.

6. For the levels+roof levels input, you want to hoist the state, see https://developer.android.com/develop/ui/compose/state#state-hoisting . I.e. the function should take the levels as normal String parameters and instead have a function onValuesChanged or similar to report the changed value(s) to the parent. Basically, how TextField works, too.

[GaeaKat]

re 5: so split it out into sections a bit more? got it.
6. I must admit, I was worried I would end up with like levels:Int, onLevelsChanged: (Int)->Boolean, roofLevels: Int, onRoofLevelsChanged: (Int)->Boolean which now I look at it, doesn't look as bad as I thought

[GaeaKat]

My attempt at using Button lead to this, and it was more time and code removing the default things, then adding clickable to a row, especially as according to docs and testing, using the clickable modifier does the ripple feedback. though using the theme more so stuff like dark mode works is needed.

[westnordost]

Well, you have several paddings there in nested composables (of which some are not necessary, by the way).
That the button is blue, is I think okay, that's the material style after all.

But if it really doesn't work out at all, you should use Surface, see e.g. MapButton.

[GaeaKat]

This is what I end up with after I have trimmed the button down / using Button component.

[GaeaKat]

Converting this back to draft while I work on the issues presented. thankyou for the points! :)

[GaeaKat]

Moved out of draft, ran ktlint over the files I changed now, moved it to it's own file, and moved the row to it's own file. and moved MaterialTheme.color and MaterialTheme.Typography

[westnordost]

So, is this ready to review/merge? Or do you want to review it yourself, first? Some small things like formatting etc. one might notice oneself.

[GaeaKat]

Should be ready I think

[GaeaKat]

Thankyou for the reviewed changes @FloEdelmann you are right that looks much better!

[westnordost]

Overall, it looks pretty good! You did a lot of things in Compose right, right away.

There are a many small things, so I rather worked on these myself then to complain about (and explain) all these. If you like to learn, have a look at what I changed.

Some general things specially for StreetComplete:

• max line width is 100 chars
• previews should be private
• I usually add a short documentation comment for every public composable
• formatting should be consistent: if it fits in one line, fine, if not, expand not like this

Row(modifier = Modifier
    .height(52.dp), verticalAlignment = Alignment.CenterVertically) {

but like this

Row(
    modifier = Modifier.height(52.dp),
    verticalAlignment = Alignment.CenterVertically
) {

Some general things for Compose:

• gotta check if it works well in right-to-left layouts.
  
• for paddings between items in a Row, Column, LazyRow etc., use the *Arrangement parameter (e.g. horizontalArrangement = Arrangement.spacedBy(16.dp))
• public composables should always have a Modifier parameter, for consistency. This parameter should always be passed to the immediate child composable
• The items function in LazyRow and LazyColumn has an overload that allows you to directly pass a List, so that's a little more convenient than with indices
• check out https://developer.android.com/develop/ui/compose/layouts/intrinsic-measurements for e.g. expanding the height of an element to the maximum height within a row. Very handy. The article explains why fillMaxHeight() is often not what one wants

[GaeaKat]

Taking a look at doing those changes now!

[westnordost]

I will look at completing this soon. If you are or currently or planning to do so soon yourself, shout out!

This PR can then serve as a template how a conversion of the UI should look like.

[GaeaKat]

I will look at completing this soon. If you are or currently or planning to do so soon yourself, shout out!

This PR can then serve as a template how a conversion of the UI should look like.

I honestly think I have misunderstood the issue, I tried to convert to TextFieldValue but I get runtime errors stating that a TextFieldValue can't be stored in the state.

java.lang.IllegalArgumentException: MutableState containing TextFieldValue(text='', selection=TextRange(0, 0), composition=null) cannot be saved using the current SaveableStateRegistry. The default implementation only supports types which can be stored inside the Bundle. Please consider implementing a custom Saver for this class and pass it as a stateSaver parameter to rememberSaveable().

[westnordost]

Okay, sorry that it took so long.

After looking at it from all the sides, I understand your last comment and also have to admit that I was wrong: I assumed that always TextFieldValue should be used because only then, the cursor position and selection will work properly when editing the text field. However, it turns out that this is false, and it works just as well when passing a String. Looking into the source code, at BasicTextField.kt line 612, it becomes clear as to why: If a String is passed, the text field will manage a TextFieldValue internally. Albeit, not with rememberSavable, so on configuration change (device rotation etc), the cursor position and selection is lost, but nothing else.

Using TextFieldValue is just ever so slightly better and regarding rememberSavable + TextFieldValue, that could apparently be done with

var value by rememberSaveable(stateSaver = TextFieldValue.Saver) { mutableStateOf(TextFieldValue()) }

but since this part is currently on the edge of Android view code / Compose code, I think this cannot be used. So anyway, current state is fine. This can be changed (or not) when the form Fragment is replaced with compose, too. Until then, ¯\_(ツ)_/¯

I also made a comment earlier that the BuildingLevelsForm could just handle Int?s and reject any non-integer input instead of showing it as an error (i.e. red outline), but in the end, the way it is now is fine, too, because it also handles gracefully a certain edge case when dealing with invalid incoming OSM data: If any input that doesn't result in an Integer would be rejected, trying to correct a value like "1.5" in the input field would not work, because when placing the cursor at the end and then pressing backspace, the resulting string would be "1." - not an integer -, so the input field would reject that change.

So, pretty much the two only changes that matter that I made bumbling around in the source code of your PR are:

• simplify code and fix a bug where the form would accept any garbage input for the roof levels if roof levels are optional (fd841e3)
• replace explicit styling for invalid input with using the isError parameter of the text field (ad5dbe3 , then 2174490) for implicit (Material design) error-styling