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

Migrate main screen to Compose #5799

Merged
merged 104 commits into from
Oct 30, 2024
Merged

Migrate main screen to Compose #5799

merged 104 commits into from
Oct 30, 2024

Conversation

westnordost
Copy link
Member

@westnordost westnordost commented Aug 15, 2024

What is migrated:

  • buttons on main screen
  • main menu
  • the various messages dialogs
  • team mode dialog (it is now wizard-like)
  • overlay selection dropdown
  • edit history sidebar and undo dialog
  • consuming geo URI
  • showing "apply preset" dialog after scanning QR code
  • error handling, i.e. showing an error message or showing a dialog, asking user to report an issue (after crash or after error during upload or download)
Screen-20240822-005327.mp4
toomuch.mp4

To do:

  • see TODOs in code: Some elements of the MainActivity
  • QuestAutoSyncer is currently Android specific. Regarding the internet connection state, I think I already wrote some wrapper?
  • check if MapControls and MainViewModel can be reduced / reorganized. It's growing fast...

I think no such devices were manufactured for over ten years
@westnordost westnordost marked this pull request as ready for review October 25, 2024 19:58
@westnordost
Copy link
Member Author

I had to stop myself from ever-expanding the scope of this. I reviewed the stuff myself and tested most of the things.

@westnordost
Copy link
Member Author

@Helium314 @FloEdelmann @matkoniecz I will soon merge this to master. If you would like to lend a hand in reviewing this before it is merged, you should do it soon.

@westnordost
Copy link
Member Author

westnordost commented Oct 26, 2024

I kind of don't like about this work that it - necessarily - leaves so many open ends, i.e. contact points between the ViewModel+Compose code and the (old) Android Fragments/Activity code. But it is currently not possible really to reduce this while not all things the MainActivity needs to talk to are all re-done in ViewModel+Compose, as everything comes together at the MainActivity.

It is also particularly hard to migrate this properly because there is so much stuff happening specific to Android.
Certainly there may be some parts that could be migrated now, but the longer this PR sits around and is expaned in scope, the more problematic it gets to finally merge it.

Copy link
Member

@FloEdelmann FloEdelmann left a comment

Choose a reason for hiding this comment

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

I had a cursory look over the changes (without running the app or tests) and found nothing that caught my eye.

@westnordost
Copy link
Member Author

westnordost commented Oct 28, 2024

Thanks! I took it out for a test today and found two issues that I'll try to fix today/tomorrow:

1. Need darker shadow on the star counter, the number is hard to see
2. The crosshair position, the pin position and the actually placed position each differ when placing things in e.g. the things overlay

@westnordost
Copy link
Member Author

westnordost commented Oct 28, 2024

3. edit history sidebar: background color of already uploaded edits is ugly
4. undo button doesn't align with first item in edit history sidebar

@westnordost
Copy link
Member Author

westnordost commented Oct 29, 2024

5. when selecting "don't keep activities" in developer options and then tabbing out of StreetComplete, the following things don't seem to work at all anymore: compass, zoom in, zoom out and location button; tapping on overlays, tapping on any pins, display/highlight of selected edit in edit history sidebar

@westnordost westnordost merged commit 2c2d8c5 into master Oct 30, 2024
@FloEdelmann FloEdelmann deleted the compose-edithistory branch October 30, 2024 14:31
@rugk
Copy link
Contributor

rugk commented Dec 2, 2024

The second animation/video with the quest icons sliding up from below the screen but in tilted mode looks kinda uhm… tbh unprofessional /childish.

I guess the actual reason for that is that it is tilted in perspective but it does not account for that in the animation. The animation is just a slide in effect from below, which would maybe work for a flat image, but not the 3d view there. IMHO this reminds me of some silly PowerPoint animations I once did. Not blaming here, just trying to make you aware of that. Especially maybe as the icons are flat and have no "depth", it looks very artificial and not "real" of you get what I mean?
It may be especially surprising if/as it does not follow the tilt from the map aka the map in the video was in 2d top-down view.
Porposed solution would this just be to adhere for the tilt of the image itself aka I guess just zoom in while moving in or so or some real animation I dunno.
Or well just drop the tilt? Would be fine too imho.

Or maybe animate each icon/quest at once and not in "bulk". Like dropping that animation altogether and have the icons "pop in" with some random delay and possibly location (transparency fade effect maybe with zoom), so it looks like what you seem to convey here:"quests pop up everywhere, this is too much"?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants