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

New viewmodel for every instance of same fragment #145

Closed
Bodo1981 opened this issue Jun 9, 2018 · 16 comments
Closed

New viewmodel for every instance of same fragment #145

Bodo1981 opened this issue Jun 9, 2018 · 16 comments
Labels
Milestone

Comments

@Bodo1981
Copy link

Bodo1981 commented Jun 9, 2018

Describe the bug
I have a fragment which has a personId to show details of a person

class PersonDetailsFragment : Fragment {
   val personId: Int by lazy { intent.extras.getInt("personId") }
   val viewModel: PersonViewModel by viewModel()

   override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        viewModel.personDetailsLiveData.observe(this, Observer<List<Cryptocurrency>> {
           // show person
        })
    }
}

To Reproduce
Steps to reproduce the behavior:

  1. Open a person (PersonFragment)
  2. Click on a friend of this person
  3. It opens a new instance of PersonFragment but gets the viewmodel of the previous PersonFragment
  4. The app crashes with the following exception
java.lang.IllegalArgumentException: Cannot add the same observer with different lifecycles

Expected behavior
If i open a new instance of the same fragment it should also create a new viewmodel.

Koin project used and used version (please complete the following information):
koin-android-architecture v0.9.3

@arnaudgiuliani arnaudgiuliani added the question Usage question label Jun 11, 2018
@arnaudgiuliani
Copy link
Member

Hello,

you are injecting with by inject().

You have to use by viewModel() to inject and bind your viewModel to the lifecycle owner: val viewModel: PersonViewModel by viewModel()

@Bodo1981
Copy link
Author

I am sorry.
In my project i am using by viewModel(). I corrected it in this issue.

This one is working (it is using the android standard implementation:

class HomeFragment : Fragment() {
    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)

        ViewModelProviders.of(this).get(HomeViewModel::class.java).liveData.observe(this,
             Observer<List<Items>> {
              })
}

And this one is not working (using di with koin)

class HomeFragment : Fragment() {
    private val homeViewModel: HomeViewModel by viewModel()

    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)

        homeViewModel.liveData.observe(this,
             Observer<List<Items>> {
              })
}

Steps to reproduce:

  1. App starts with HomeFragment
  2. Navigate to another Fragment
  3. Navigate again to HomeFragment (not going back, but put another HomeFragment on the stack)
  4. Error throws with koin implementation
java.lang.IllegalArgumentException: Cannot add the same observer with different lifecycles

@arnaudgiuliani arnaudgiuliani added type:regression android status:checking currently in analysis - discussion or need more detailed specs and removed question Usage question labels Jun 12, 2018
@arnaudgiuliani
Copy link
Member

Ok, what project did you use? koin-android-architecture v0.9.3 ?

@Bodo1981
Copy link
Author

Yes i am using koin-android-architecture v0.9.3
Thanks for your investigation

@arnaudgiuliani
Copy link
Member

If you can make a very small project that reproduces your case (just a public GitHub project), I would be very nice and help us save time 👍

@Bodo1981
Copy link
Author

Of course... :-)
Here it is: https://github.com/Bodo1981/KoinViewModelCrash

You have to use Android Studio 3.2 Canary 18

When you checkout the repo it is working like intended.

  1. Click "Go Details"
  2. on next fragment click "Go back"
  3. another HomeFragment is created and pushed on the stack -> "Go Details" Button is visible

If you change HomeFragment to this the app crashes after clicking back on second fragment:

  1. Click "Go Details"
  2. on next fragment click "Go back"
  3. another HomeFragment is created and pushed on the stack -> App crashes!
class HomeFragment : Fragment() {

    // not working
    private val homeViewModel: HomeViewModel2 by viewModel()

    override fun onCreateView(inflater: LayoutInflater, container: ViewGroup?, savedInstanceState: Bundle?): View? {
        return inflater.inflate(R.layout.fragment_home, container, false)
    }

    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)

        // working
        //val homeViewModel = ViewModelProviders.of(this).get(HomeViewModel2::class.java)

        homeViewModel.cryptocurrencyLiveData.observe(this,
                                                 Observer<List<Cryptocurrency>> {

                                                 })
    }

    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
        super.onViewCreated(view, savedInstanceState)

        detailsButton.setOnClickListener {
            view.findNavController().navigate(R.id.navToDetails)
        }
    }
}

@arnaudgiuliani
Copy link
Member

Thanks!

@arnaudgiuliani arnaudgiuliani added status:accepted accepted to be developed and removed status:checking currently in analysis - discussion or need more detailed specs labels Jun 16, 2018
@arnaudgiuliani arnaudgiuliani added this to the 1.0.0 milestone Jun 19, 2018
@arnaudgiuliani
Copy link
Member

You have to declare a ViewModel class with viewModel dsl keyword:

val homeModule: Module = applicationContext{
    context("home") {
    bean { CryptocurrencyRepository(get(), get()) }
    viewModel { HomeViewModel(get()) }
    viewModel { HomeViewModel2() }
    }
}

I've added a protection to not allow injection of definition that are not ViewModel 👍 (koin-android-viewmodel version 1.0.0-alpha-23)

@arnaudgiuliani
Copy link
Member

I'll push you a PR to fix your sample.

@Bodo1981
Copy link
Author

Shame on me... I looked everywhere if i use it the correct way but did not see it...

SORRY!

@arnaudgiuliani
Copy link
Member

No problem :)

This was also the moment to reinforce viewmodel injection check ;)

@joelfernandes19
Copy link

@arnaudgiuliani I'm using version 2.0.0-beta-1, and I'm facing the exact same issue as described above. I have declared my ViewModel classes with viewModel dsl keyword.

I'm also using Navigation Component to navigate between fragments. When I navigate from fragment A to B, and then navigate back to A, a new ViewModel is created and the observer is called multiple times.

Is there something that I'm doing wrong?

@dawnchen
Copy link

@arnaudgiuliani I'm using version 2.0.0-beta-1, and I'm facing the exact same issue as described above. I have declared my ViewModel classes with viewModel dsl keyword.

I'm also using Navigation Component to navigate between fragments. When I navigate from fragment A to B, and then navigate back to A, a new ViewModel is created and the observer is called multiple times.

Is there something that I'm doing wrong?

Please double check your ViewModel injection in activity or fragment, should be "by viewModel()" or "by sharedViewModel()", not "by inject()".

@e4basil
Copy link

e4basil commented Jun 17, 2019

@arnaudgiuliani same problem here also I'm using version 2.0.1

@arnaudgiuliani
Copy link
Member

Please reopen a new ticket

@mrizyver
Copy link

mrizyver commented Sep 30, 2019

@joelfernandes19, I had the same issue. When i rotate screen, my activity in onCreate method created a new instance of fragment. When we create a new instance of fragment, it takes a new instance of ViewModel. I just do like that
image

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

No branches or pull requests

6 participants