Migrate MoveNodeFragment speech bubble content to Compose (commonMain)#6797
Conversation
MoveNodeFragment's UI is split between Android-specific map interaction (arrow drawing, pin positioning, screen coordinate conversion) and portable speech bubble content (title, description, buttons). Only the latter needs to differ per platform, so extracting it to a shared composable lets the upcoming iOS port reuse the same UI without duplicating the layout logic. MeasureDisplayUnit moves to commonMain because the composable's caller needs it to format distances. The file is pure Kotlin (kotlin.math + kotlinx.serialization) with one exception: String.format is unavailable in Kotlin/Native, so it's replaced with a small formatFixed helper that produces identical output for the 0/1/2 decimal cases this class uses. The Fragment wrapper keeps all its existing responsibilities (arrow drawable, pin marker, IsCloseableBottomSheet/IsMapPositionAware interfaces, survey confirmation, edit submission) and passes a MoveNodeDistanceState to the composable instead of imperatively updating TextViews. No changes to MainActivity or the Listener interface. Addresses streetcomplete#6796
westnordost
left a comment
There was a problem hiding this comment.
Did you test this? From the code, it looks like the OK button might be at a different place now than it was before. Now, it looks like it is in a column below the other content.
You might have noticed that due to the bottom-up method of the migration, the button bars and OK button of all the quest forms have not been migrated yet. (This is due soon, because I am almost done with migrating all the quest form content, currently I am on the note stuff).
Now, MoveNodeFragment does not inherit from the quest forms, so already migrating the button bar and OK button is fine, however, next step would be to generalize the button bar and OK button composables and outsource it into common ui components that can be used for the other places as well (e.g. ButtonBar.kt, FloatingOkButton). Ideally, this could happen right away in this PR.
(Alternatively, it would also be fine to keep the OK button and button bar in the layout XML for now and migrate that later.)
Due to an error in GitHub, I can't comment on MeasureDisplayUnit.kt. In line 43, you seem to have changed a figure space into a normal white space. Could you revert that?
The figure space was accidentally replaced with a regular space when moving the file from androidMain to commonMain.
Reusable animated FAB with check icon, matching the View-based popIn/popOut animations (scale 0.5-1.0, fade, 100ms). Will be used by MoveNodeFragment and eventually all quest form bottom sheets.
Simple Row-based button bar for bottom sheet button panels. Callers add VerticalDivider between buttons when multiple are present.
The OK button was inside the speech bubble content, but it should be a sibling at the SlidingRelativeLayout level (matching the original XML). Also simplifies the Fragment to pass raw Float distance instead of a sealed state class, with the composable handling display logic. Uses the new FloatingOkButton and ButtonBar composables.
|
I had tested this functionally in an emulator, to make sure that everything still worked. You're right, I had inadvertently changed the positioning of the OK button. In my day job, I have built the habit of including before and after screenshots so that I and reviewers have an easier time observing those kinds of changes. I'll do that here. Screenshots
StylingYou'll note that the font family is different, the colour of the body text is different, and the Cancel button text is no longer red, it is now blue. Based on my digging through the issues and codebase, I believe that these styling changes/gaps are project-wide thus far, and that I shouldn't address them in this PR. Please let me know if that's an incorrect decision. I extracted
Done in 983eead. I changed the Fragment to just pass a raw Float distance via
Also done.
I had not realized that those were not normal whitespaces. I have configured my editor to highlight non-standard whitespace now. And I have also learned what a figure space is, so thank you! Again, I am very open to feedback on this. I'm happy to keep editing if there are big or small things that you would like changed. If you're happy with it, then please feel free to tell me what to pick up next, if you have an idea. |
westnordost
left a comment
There was a problem hiding this comment.
Looks good! A few small nitpicky things
I have a few. They are, (for me,) a bit in the "huff... ehh, let's do it later..." zone (Not because they are awkward, but rather because I don't have a Mac machine I like to work and test with). So, ideal for you to pick up:
|
|
Thank you for the detailed suggestions! I can pick up any/all of these, but I think I should start with option 2.ii, the WheelPicker-based DatePicker. I'll open an issue with the details before starting, so you can sanity-check the approach. For the M2 to M3 migration, I agree it'll need to happen eventually, but tackling it now feels like a little too much for my... 4th PR. I don't mind doing grunt work at all, but I'll get settled in a bit with the codebase first. |
1. Consolidate move-node files into a `move_node/` subpackage under `screens/main/bottom_sheet`. 2. Remove my redundant nested Column. 3. Move content padding out of the composable and back into the ComposeView. 4. Apply hint text style to the description text. 5. Rename ic_check_48dp.xml to ic_check_48.xml, following the updated naming convention for icons. 6. Fix ButtonBar KDoc. 7. Fix FloatingOKButton KDoc. 8. Add FloatingOKButton contentDescription.






Summary
Addresses #6796
MoveNodeFragment's speech bubble content (title, description, cancel/OK buttons) into aMoveNodeFormcomposable incommonMainMeasureDisplayUnittocommonMain(replaceString.formatwith the project'sNumberFormatterexpect/actual for locale-aware formatting on both platforms)androidMainkeeps arrow drawable, pin positioning, map interfaces, survey confirmation, and edit submissionApproach
The split follows the bottom-up pattern from PR #6022 (BuildingLevelsForm): portable UI in
commonMain, platform wrapper inandroidMain. The Fragment now passes aMoveNodeDistanceStatesealed interface to the composable instead of imperatively updating TextViews and togglingpopIn/popOuton the OK button.No changes to
MainActivity,MoveNodeFragment.Listener, or DI wiring.Test plan
./gradlew assembleDebugcompiles cleanly./gradlew :app:compileKotlinIosSimulatorArm64compiles cleanly (with fix branches merged)Again, I'm very happy to change the approach or anything here, based on your feedback.