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

Update AndroidX Fragment to 1.3.4 #6394

Merged
merged 6 commits into from
Jun 18, 2021
Merged

Update AndroidX Fragment to 1.3.4 #6394

merged 6 commits into from
Jun 18, 2021

Conversation

TacoTheDank
Copy link
Member

@TacoTheDank TacoTheDank commented May 29, 2021

What is it?

  • Bugfix (user facing)
  • Feature (user facing)
  • Codebase improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

  • Updated AndroidX Fragment to 1.3.4 (changelog)
  • Fix most deprecations from pre-1.3.0 Fragment versions
  • Fix some of the onActivityResult deprecations

Fixes the following issue(s)

  • Comes with whatever bugfixes were made between 1.2.4 and 1.3.4 (which are quite a lot). I highly recommend thoroughly reading the changelog.

APK testing

On the website the APK can be found by going to the "Checks" tab below the title and then on "artifacts" on the right.

Due diligence

Notes

There are other deprecations that come with this upgrade that will need fixing at some point:

  • ViewPager1 is now officially deprecated (YEAH BOIIIIII)

I didn't do other deprecations like onRequestPermissionsResult and the other onActivityResult ones because I'm not too confident and I want to keep this one smaller. I'll do them in other PRs.

@TacoTheDank
Copy link
Member Author

Also, I'm not that great at naming things, so feel free to tell me if you have better names in mind for the objects and functions and I'll rename them.

@AudricV AudricV added the codequality Improvements to the codebase to improve the code quality label May 29, 2021
Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

I've read the changelog, but I do not know the architecture of fragments well enough to understand the changelog and your changes thoroughly.

(btw, do you happen to have some up-to-date and complete guide to fragments? I couldn't find good-quality material about it)

Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

LGTM, thanks :-D

@Stypox Stypox merged commit 74ad488 into TeamNewPipe:dev Jun 18, 2021
@TobiGr
Copy link
Member

TobiGr commented Jun 18, 2021

Did someone test this on KitKat? The app crashes constantly after trying to use the search.

stracktrace: search ``` 06-18 15:07:55.443 5829-5829/org.schabi.newpipe.debug E/ACRA: ACRA caught a NullPointerException for org.schabi.newpipe.debug java.lang.NullPointerException at org.schabi.newpipe.fragments.list.search.SearchFragment.onCreateOptionsMenu(SearchFragment.java:430) at androidx.fragment.app.Fragment.performCreateOptionsMenu(Fragment.java:3100) at androidx.fragment.app.FragmentManager.dispatchCreateOptionsMenu(FragmentManager.java:3181) at androidx.fragment.app.FragmentController.dispatchCreateOptionsMenu(FragmentController.java:391) at androidx.fragment.app.FragmentActivity.onCreatePanelMenu(FragmentActivity.java:288) at androidx.appcompat.view.WindowCallbackWrapper.onCreatePanelMenu(WindowCallbackWrapper.java:94) at androidx.appcompat.app.AppCompatDelegateImpl$AppCompatWindowCallback.onCreatePanelMenu(AppCompatDelegateImpl.java:3070) at androidx.appcompat.view.WindowCallbackWrapper.onCreatePanelMenu(WindowCallbackWrapper.java:94) at androidx.appcompat.app.ToolbarActionBar.populateOptionsMenu(ToolbarActionBar.java:456) at androidx.appcompat.app.ToolbarActionBar$1.run(ToolbarActionBar.java:57) at android.view.Choreographer$CallbackRecord.run(Choreographer.java:761) at android.view.Choreographer.doCallbacks(Choreographer.java:574) at android.view.Choreographer.doFrame(Choreographer.java:543) at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:747) at android.os.Handler.handleCallback(Handler.java:733) at android.os.Handler.dispatchMessage(Handler.java:95) at android.os.Looper.loop(Looper.java:136) at android.app.ActivityThread.main(ActivityThread.java:5017) at java.lang.reflect.Method.invokeNative(Native Method) at java.lang.reflect.Method.invoke(Method.java:515) at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:779) at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:595) at dalvik.system.NativeStart.main(Native Method) ```

@TacoTheDank
Copy link
Member Author

TacoTheDank commented Jun 19, 2021

@TobiGr Are you sure that's related to this PR specifically? I didn't touch anything related to the search function...

EDIT: Actually the NPE might have to do with new nullabilities that may have been introduced between 1.2.4 and 1.3.4.

@TacoTheDank TacoTheDank deleted the androidx-fragment-134 branch June 19, 2021 21:04
@opusforlife2
Copy link
Collaborator

Seems like it: #6522. It specifically refers to your PR.

@TacoTheDank
Copy link
Member Author

@opusforlife2 He may just be referring to 0.21.5. As of right now, Stypox's merge of my PR is the latest commit. Guess we'll have to wait for a response 🤔

@Douile
Copy link
Contributor

Douile commented Jun 19, 2021

Just to clear things up, it looks like #6522 is caused in some way by this PR: when building on commit 6e377dd the crash no longer occurs. lmk if you need more details about build or whatever.

@Douile
Copy link
Contributor

Douile commented Jun 19, 2021

I don't really understand this stuff and I don't see any mentions of changes to Fragment lifecycle here (https://developer.android.com/guide/fragments/lifecycle) but I noticed in SearchFragment.java service is only set in onResume. Copying the exact code from onResume (

try {
    service = NewPipe.getService(serviceId);
} catch (final Exception e) {
    ErrorActivity.reportUiErrorInSnackbar(this,
            "Getting service for id " + serviceId, e);
}

) to onCreate means the crash (#6522) in SearchFragment no longer occurs. idk whether we still need it in onResume or whether this could cause problems in other fragments so I will leave that to you guys.

@Douile Douile mentioned this pull request Jun 19, 2021
4 tasks
Douile added a commit to Douile/NewPipe that referenced this pull request Jun 22, 2021
…ragment

It seems due to TeamNewPipe#6394 updating the FragmentX library there was a
change to the order of lifecycle calls, as such onResume() was no longer
before onCreateOptionsMenu() creating a null pointer exception when
using service in onCreateOptionsMenu() as it is only set in onResume().

By moving the initialization of service to onStart() which still happens
before onCreateOptionsMenu() this crash can be avoided. This commit also
adds a check for a null service to prevent future crashes for similar
issues.
This was referenced Jun 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
codequality Improvements to the codebase to improve the code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants