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

Stop accessing non-public fields in Fragment #318

Closed
mbarta opened this issue Jul 5, 2018 · 25 comments
Closed

Stop accessing non-public fields in Fragment #318

mbarta opened this issue Jul 5, 2018 · 25 comments

Comments

@mbarta
Copy link

mbarta commented Jul 5, 2018

After updating the support library of my project to the new AndroidX scheme, the app started crashing after calling this method: BackstackAccessor.isFragmentOnBackStack(Fragment())
Crash:
java.lang.IllegalAccessError: Method 'boolean androidx.fragment.app.Fragment.isInBackStack()' is inaccessible to class 'androidx.core.app.BackstackAccessor'

After some investigation, I found that the issue is this file: https://github.com/sockeqwe/mosby/blob/master/utils-fragment/src/main/java/android/support/v4/app/BackstackAccessor.java which is accessing a non-public field by "faking" the package.

As well as being a bad practice, this stops working after migrating to AndroidX due to the change of package names.

Ideally this should be reverted to the commented-out version of the code still existing in the linked file.

@sockeqwe
Copy link
Owner

sockeqwe commented Jul 5, 2018

I discussed with Adam Powell some time ago, the method isInBackStack() should actually become public.

stops working after migrating to AndroidX due to the change of package names.

that is true unfortunately but there is no easy way around this. The plan is the following: Once androidx is a stable release I will pump version of Mosby to Mosby 4.0 and Mosby 4 will be compatible with androidx only. Open to discuss this and happy for any feedback.

@drampelt
Copy link

drampelt commented Jul 9, 2018

As a temporary workaround until then for those who want to continue using AndroidX, you can exclude the utils-fragment module dependency in your build.gradle:

compile 'com.hannesdorfmann.mosby3:mvi:3.1.0' {
    exclude group: 'com.hannesdorfmann.mosby3', module: 'utils-fragment'
}

and then define your own androidx.core.app.BackstackAccessor class:

package androidx.core.app;

import androidx.fragment.app.Fragment;

public class BackstackAccessor {
    public static boolean isFragmentOnBackStack(Fragment fragment) {
        return false;
    }
}

@tommus
Copy link

tommus commented Sep 23, 2018

AndroidX is now stable 1.0.0. Even more, Google wrote that the Support 28.0.0 is the last version that supports android.support package. It is time to move on.

@mradzinski
Copy link

mradzinski commented Oct 23, 2018

@sockeqwe Hannes, does this affect users of Mosby with Conductor? I guess it'll not since Conductor handles its own backstack and lifecycle which Mosby makes use of it, but I'll soon enough have to migrate to AndroidX for a mammoth of a project and don't want this to be an issue 😕

@sockeqwe
Copy link
Owner

I have to double check it but Conductor should not be a problem. It doesn't use BackstackAccessor at all.

@guyca
Copy link
Contributor

guyca commented Oct 26, 2018

@sockeqwe
Would you consider using this workaround in mosby:

public static boolean isInBackStack(final Fragment fragment) {
    try {
        return fragment.isInBackStack();
    } catch (IllegalAccessError e) {
        return isInBackStackAndroidX(fragment);
    }
}

/**
* Hacky workaround because Fragment#isInBackStack is inaccessible with AndroidX
*/
private static boolean isInBackStackAndroidX(final Fragment fragment) {
    final StringWriter writer = new StringWriter();
    fragment.dump("", null, new PrintWriter(writer), null);
    final String dump = writer.toString();
    return !dump.contains("mBackStackNesting=0");
}

Another possible solution is to keep current implementation and introduce a BackStackAccessorProvider which will let users use a different logic for determining if a fragment is in the back stack or not.

@sockeqwe
Copy link
Owner

sockeqwe commented Oct 26, 2018 via email

@guyca
Copy link
Contributor

guyca commented Oct 26, 2018

Thanks @sockeqwe, submitted a pr.

Edit 1: submitted a new pr

Edit 2: I published my fork which is compatible with AndroidX on jitpack. If anyone is interested you can use it until the PR is merged: implementation 'com.github.guyca:mosby:3.1.6'

@sockeqwe
Copy link
Owner

Thabks a lot. I m going to publish a new release at the end of this week containing this. This makes it useable with jetifier. However I plan to make a 4.0 release that is fully backed on androidx soon (3.x sticks with old support library)

@MaximPestryakov
Copy link

@sockeqwe When do you plan to release 3.1.1?

@sockeqwe
Copy link
Owner

sockeqwe commented Nov 6, 2018 via email

@Jacks0N23
Copy link

@sockeqwe still no new release version ((

@sockeqwe
Copy link
Owner

sockeqwe commented Nov 8, 2018

I have some problems with gradle uploading files of 3.1.1 release to different locations instead of just one single location. I asked that question on stackoverflow, if someone knows how to fix it, here is the link to stackoverflow:
https://stackoverflow.com/questions/53200255/gradle-uploadarchives-runs-in-parallel-causing-multiple-staging-repositories-on

Meanwhile I have at least published a new 3.1.1-SNAPSHOT containing this stuf or you can use jitpack

allprojects {
		repositories {
			...
			maven { url 'https://jitpack.io' }
		}
}

dependencies {
	        implementation 'com.github.sockeqwe:mosby:345efbb0c0'
}

@tommus
Copy link

tommus commented Nov 9, 2018

I've read and responded to Your SO issue. TLDR; make sure You have no org.gradle.parallel=true in your project root gradle.properties file. Based on Your local Gradle configuration, using command line parameter to disable above mentioned paralellism might not work properly. It is worth to give a try.

@sockeqwe
Copy link
Owner

sockeqwe commented Nov 9, 2018

Thanks for answering, I have that set, doesn't help unfortunately. Will try it on a different machine over the weekend.

@dmytroivanovv
Copy link

Im still waiting

@sockeqwe
Copy link
Owner

sockeqwe commented Nov 11, 2018 via email

@Jeff11
Copy link

Jeff11 commented Dec 11, 2018

Unfortunately, the build fails with this exception, because of duplicate BackstackAccessor.
Warning: Exception while processing task java.io.IOException:
Can't write [/home/me/projects/myappAndroid/myapp/build/intermediates/transforms/proguard/prodmyapp/release/0.jar]
(Can't read [/home/me/projects/myappAndroid/myapp/build/intermediates/transforms/RealmTransformer/prodmyapp/release/0(;;;;;;;**.class)]
(Can't read [androidx]
(Can't read [core]
(Can't read [app]
(Can't read [BackstackAccessor.class]
(Duplicate jar entry [androidx/core/app/c.class]))))))

@leslieam
Copy link

leslieam commented Dec 13, 2018

Looking forward to this issue solved. I'm getting the same issue.
java.lang.IllegalAccessError: Method 'boolean androidx.fragment.app.Fragment.isInBackStack()' is inaccessible to class 'androidx.core.app.BackstackAccessor'

@LukasStancikas
Copy link

Yes error similar to @Jeff11 persists in 3.1.1-SNAPSHOT version using gradle dependency 'Program type already present: androidx.core.app.BackstackAccessor'

@sockeqwe
Copy link
Owner

I just released 3.1.1 (not snapshot)

@LukasStancikas is your code available somewhere to reproduce this issue? I guess jetifier is doing to much "smart" things ending up having BackstackAccessor somehow twice ... Do you use Mosby in app and in a library that uses Mosby as well?

@laalto
Copy link
Contributor

laalto commented Dec 18, 2018

In our AndroidX-enabled app the 3.1.1 does not work out of the box, BackstackAccessor crashes with NoSuchMethodError and not with IllegalAccessError as handled in the code. The configuration I was testing has proguard optimizations enabled so it might be part of the issue here.

My workaround:

I believe this could be fixed in the library by catching NoSuchMethodError as well.

@terrakok
Copy link

terrakok commented Jan 6, 2019

Hello @sockeqwe !
BackstackAccessor used for checking that we should persist presenter on screen rotation because isRemoving == true for fragments inside backstack on screen rotation. Right?

But we can check it other way: android calls onSaveInstanceState for this fragments before screen rotation.

abstract class BaseFragment : AppCompatFragment() {

    private var instanceStateSaved: Boolean = false

    //This is android, baby!
    private fun isRealRemoving(): Boolean =
        (isRemoving && !instanceStateSaved) //because isRemoving == true for fragment in backstack on screen rotation
            || ((parentFragment as? BaseFragment)?.isRealRemoving() ?: false)

    //It will be valid only for 'onDestroy()' method
    private fun needDestroyPresenter(): Boolean =
        when {
            activity?.isChangingConfigurations == true -> false
            activity?.isFinishing == true -> true
            else -> isRealRemoving()
        }

    override fun onResume() {
        super.onResume()
        instanceStateSaved = false
    }

    override fun onSaveInstanceState(outState: Bundle) {
        super.onSaveInstanceState(outState)
        instanceStateSaved = true
    }

    override fun onDestroy() {
        super.onDestroy()
        if (needDestroyPresenter()) {
            //destroy presenter
        }
    }
}

I think @guyca code is dirty hack 😉

@sontn-fabbi
Copy link

Thank for compile 'com.hannesdorfmann.mosby3:mvp:3.1.1' // Plain MVP
Issue was resolved for me.

@laalto
Copy link
Contributor

laalto commented Jan 23, 2020

With androidx.fragment 1.2.0 the @guyca dump() hack does not work anymore. There's a bug in FragmentManager trying to dump a detached fragment. Reported it as https://issuetracker.google.com/issues/148189412

As a workaround, changed the hack to another, reflection-based one:

private static boolean isInBackStackAndroidX120(final Fragment fragment) {
        try {
            Field backStackNestingField = Fragment.class.getDeclaredField("mBackStackNesting");
            backStackNestingField.setAccessible(true);
            int backStackNesting = backStackNestingField.getInt(fragment);
            return backStackNesting > 0;
        } catch (NoSuchFieldException | IllegalAccessException e) {
            throw new RuntimeException(e);
        }
    }
}

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

No branches or pull requests