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

Use ViewModels (master) #5530

Open
westnordost opened this issue Mar 9, 2024 · 10 comments
Open

Use ViewModels (master) #5530

westnordost opened this issue Mar 9, 2024 · 10 comments
Labels
code cleanup iOS necessary for iOS port

Comments

@westnordost
Copy link
Member

westnordost commented Mar 9, 2024

This is the master ticket for refactoring to include view models. View models should be used for Fragments and Activities that either have a state that transcends the view lifecycle or access the data layer.

Contributing

As this is a larger undertaking which can be implemented step by step - a Fragment at a time - this ticket shall serve as a master ticket. Whenever you would like to contribute, select a Fragment, post that you are now working on that one here and later post a PR when you are done.

How to

How to find Fragments and Activities that should have view models

Search for activities and fragments that...

  1. have onSaveInstanceState - it means it saves its state manually. This state should be in the ViewModel
  2. by inject() - it means that dependencies are injected which are likely part of the data layer. Data access should be managed by the ViewModel
  3. NOT to be done for now: have something like parentFragment as? Listener ?: activity as? Listener - this pattern is used for communication between fragments. This can be replaced with shared view models, but when using Compose, probably only the top-level fragments will remain as fragments anyway. So, wait with that until using Compose

How to implement ViewModels

ViewModel documentation

If you are like me and learn best by example / by copying. Look in the code for classes named *ViewModel and *ViewModelImpl. For example EditStatisticsViewModelImpl.

This will also give you a good idea in which style ViewModels should be written and what code should go into the ViewModel and what should remain the the View (i.e. the Fragment).
The ViewModels job is to supply the view with the data it needs with a convenient interface and data structure, hold this state and also offer an interface to manipulate the data, if applicable.

Advantages

We want to refactor the code to use ViewModels for the following reasons:

  • For the planned iOS version, we will migrate (step by step) to Compose Multiplatform as UI framework. This means that use of Android-specific Fragments and Activities must eliminated. A step towards this goal is to separate state and data access from these into platform independent code

  • For keeping (view) state that transcends the view lifecycle it is more convenient, as it does not need to be saved and manually re-created

  • It provides a better separation between view logic and view data. Quite necessary when using Compose as UI framework and at least highly recommended otherwise

  • It will likely simplify communication between fragments

  • It's quite standard to use this pattern and recommended by Android architectural design guidelines. The more standard things are used in StreetComplete, the easier it is for new contributors to join

(This ticket supsersedes #5070 which also included a lot of discussion.)

@westnordost
Copy link
Member Author

I added view models for

  • LoginFragment + OAuthFragment (merged them into one)
  • AchievementsFragment
  • LinksFragment

Now, the profile screen all has viewmodels.

@westnordost
Copy link
Member Author

Added viewmodels for everything in the settings screen, the about screen and some controls on the main screen.

@westnordost
Copy link
Member Author

and edit history, which spawned a crictical bug that has been fixed in v57.3

@westnordost westnordost moved this from Todo to In Progress in iOS Port Apr 29, 2024
@trymnilsen
Copy link

I have some time available in December, and I'd like to contribute to this. I did a quick search for onSaveInstanceState and its seems like its mostly the map, the different forms/answer screens and location permission fragment that is left? What do you think would be the best way to contribute to the iOS port?

@westnordost
Copy link
Member Author

On view models, mainly the map and every single quest form (and other forms) would need one. Currently, all this logic is in the quest form Fragments. All these fragments inherit from a base class which contains all the core logic, e.g. what should happen when the user taps the OK button (solves the quest). Now, I didn't think this through yet, but I think there should probably be a base class for all the quest view models. Other than only start with a very basic ViewModel for the AbstractQuestAnswerFragment (etc.) there is not really a good limit on where to wrap up a PR without it growing very large.

The viewmodel for map, though, shouldn't have such a big scope.

However, if you are looking for the best way to contribute to the iOS port, take a look at #5421 (comment) (all the way down, to "Contributing"). At the moment, I think it would be easier to contribute by migrating more UI to Compose simply because it's easier to cut this pie into small pieces.

@PRIMA-RT

This comment was marked as off-topic.

@Doomsdayrs

This comment was marked as off-topic.

@PRIMA-RT

This comment was marked as off-topic.

@Doomsdayrs

This comment was marked as off-topic.

@westnordost

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code cleanup iOS necessary for iOS port
Projects
Status: In Progress
Development

No branches or pull requests

4 participants