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

Compiler Warnings as Errors - WordPress Module - FragmentManager #17253

Open
Tracked by #17173
ParaskP7 opened this issue Oct 4, 2022 · 10 comments · Fixed by #19490 or #19568 · May be fixed by #19876
Open
Tracked by #17173

Compiler Warnings as Errors - WordPress Module - FragmentManager #17253

ParaskP7 opened this issue Oct 4, 2022 · 10 comments · Fixed by #19490 or #19568 · May be fixed by #19876

Comments

@ParaskP7
Copy link
Contributor

ParaskP7 commented Oct 4, 2022

Parent #17173

This issue is about resolving the getFragmentManager(...) and requireFragmentManager() compile warnings for the WordPress module, and possibly, for the lib modules as well.

PS: Some of those warnings are already deprecated (see here).


  • This getFragmentManager(...) method is deprecated. This has been removed in favor of getParentFragmentManager() which throws an IllegalStateException if the FragmentManager is null. Check if isAdded returns false to determine if the FragmentManager is null.
  • This requireFragmentManager(...) method is deprecated. This has been renamed to getParentFragmentManager() to make it clear that you are accessing the FragmentManager that contains this Fragment and not the FragmentManager associated with child Fragments.

For more info see:

@peril-wordpress-mobile
Copy link

Fails
🚫

Please add a type label to this issue. e.g. '[Type] Enhancement'

Generated by 🚫 dangerJS

@07jasjeet
Copy link
Contributor

Opened a pull request before assignment. Guilty as charged but can I get this one now?

@ParaskP7
Copy link
Contributor Author

ParaskP7 commented Oct 31, 2023

👋 @07jasjeet and thank you so much for your contribution, I'll be code reviewing the PR you opened today, actually, @antonis self-assigned for being a reviewer himself (many thanks)! 🥇 🙏 💯

By the way, would you like me to assign this issue to you? 🤔


Fixes #7253 (Sub-ticket)
and #17246 partially.

Mind the fact that there are lots of references to getFragmentManager(...) and requireFragmentManager() on both, the Kotlin and Java side, and thus the work on this might end-up being quite significant with lots of testing needed to verify correctness. As such, I think this #19471 PR doesn't fix all FragmentManager, does it? 🤔

What I mean by that is that by adding Fixes #7253, when this #19471 PR gets merged it will also close this issue, but I don't think it should, wdyt? 🤔

@07jasjeet
Copy link
Contributor

Hello @ParaskP7, yes please do assign me this issue.

As for the number of references, we can divide the work into multiple PRs for simplicity, in the current #19471 PR, only the deprecations mentioned here were considered and I wasn't sure if I should move forward with more changes.

Since you have mentioned those as well, surely I will move forward to fix those. But in what manner (span of PRs and code sections), we can discuss. (on Slack maybe?).

As for the last question, no it shouldn't. I think #19471 closes this ticket partially as well if we don't consider the scope to be limited by these analytics mentioned in the ticket.

@ParaskP7
Copy link
Contributor Author

👋 @07jasjeet !

yes please do assign me this issue.

Done! 🌟

As for the number of references, we can divide the work into multiple PRs for simplicity, in the current #19471 PR, only the deprecations mentioned here were considered and I wasn't sure if I should move forward with more changes.

Awesome, and yea, I was thinking that's the case, thanks for confirming! 🙏

Since you have mentioned those as well, surely I will move forward to fix those.

Great, that would help so much! 💯

But in what manner (span of PRs and code sections), we can discuss. (on Slack maybe?).

Definitely, that would be ideal. I think it might be better if we discuss here, just to have everything in one place and help others refer to these discussions later on by just visiting this issue, wdty? 🤔

FYI: About the span of PRs and code sections, take a look at this #18906 issue and how I handle this kind of work to get some inspiration on how you could also use this issue to do something similar (all feedback welcome). Having said that, I would be very comfortable for you to split this work anyway you feel like, that is, after you doing some analysis on the full spectrum of what needs to be done.

PS: I am also NOT against Slack if you prefer that instead.

As for the last question, no it shouldn't. I think #19471 closes this ticket partially as well if we don't consider the scope to be limited by these analytics mentioned in the ticket.

Cool, we agree then, let's just update the PR to mention that too Fixes #7253 partially (Sub-ticket). Plus, apologies for not making this clear earlier while creating this issue. Still, this doesn't mean that the work you did is not moving the needle forward, it actually does and thank YOU so much for that, as they say, well began is half done! 🥇

@07jasjeet
Copy link
Contributor

07jasjeet commented Oct 31, 2023

FYI: About the span of PRs and code sections, take a look at this #18906 issue and how I handle this kind of work to get some inspiration on how you could also use this issue to do something similar (all feedback welcome).

In the above mentioned example issue, the only thing I can do is link PRs to this ticket. Editing issue is an access right only available to the one who opened the issue in the first place. So, I think linking PRs is the way to go for now. I will label all the PRs as "Part-X" to maintain structure and add content to PR description on what changes did take place moving forward.

PS: I am also NOT against Slack if you prefer that instead.

GitHub for formal discussions and slack (pinging is easy) for any dev-issue I get into. If required, I will document Slack chat here as well.

Edit: Although I could mention occurrences in issue comments but that doesn't seem the best way. Let me know what you think.

@ParaskP7
Copy link
Contributor Author

👋 @07jasjeet !

In the above mentioned example issue, the only thing I can do is link PRs to this ticket. Editing issue is an access right only available to the one who opened the issue in the first place. So, I think linking PRs is the way to go for now.

You are right, but also, actually, I was thinking of your creating a master comment instead of editing the PR description and then we use that as our go-to task list, wdyt? 🤔

I will label all the PRs as "Part-X" to maintain structure and add content to PR description on what changes did take place moving forward.

👍

GitHub for formal discussions and slack (pinging is easy) for any dev-issue I get into. If required, I will document Slack chat here as well.

👍

Edit: Although I could mention occurrences in issue comments but that doesn't seem the best way. Let me know what you think.

💯 As I suggested above, I actually think that creating a master comment and updating it accordingly, just like you would have done if you had access to edit the PR discussion could work for us. Wdyt? 🤔

If you like that idea, we can try that as an experiment, see how it feels and then take it from there, continue or update the process. 😊

@07jasjeet
Copy link
Contributor

07jasjeet commented Oct 31, 2023

Master Task list

Part-1 covers deprecations mentioned in this issue.

1. getFragmentManager() Replacement Task list

Part-2

Part-3

Part-4

2. requireFragmentManager() Replacement Task list

Part-2

Note: Tentative work division, may change.

@antonis
Copy link

antonis commented Nov 7, 2023

Reopening autoclosed issue since till we handle the pending parts #17253 (comment)

@antonis
Copy link

antonis commented Nov 22, 2023

Reopening autoclosed issue since till we handle the pending parts #17253 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment