Skip to content

WP Stories integration phase 1 - lint fixes not coming from Stories source code#11920

Closed
mzorz wants to merge 3 commits intofeature/wp-stories-integration-phase1from
feature/wp-stories-integration-phase1-other-lint
Closed

WP Stories integration phase 1 - lint fixes not coming from Stories source code#11920
mzorz wants to merge 3 commits intofeature/wp-stories-integration-phase1from
feature/wp-stories-integration-phase1-other-lint

Conversation

@mzorz
Copy link
Copy Markdown
Contributor

@mzorz mzorz commented May 14, 2020 •
edited
Loading

This PR builds on top of #11864

Note: the instructions for building are in the base PR #11864.

This PR fixes a lot of lint errors that appeared only after adding the portkey-android repo as a git submodule in #11864, and that are not strictly related to the submodule being added, but are all part of the code available in the base develop branch. I have been trying to understand why these issues are reported by the linter only after adding the submodule (and these errors do not appear in the submodule's code), so after struggling for a couple of days and verifying all the libraries used are the same versions, etc as in the target codebase, decided to go ahead and fix the issues with this PR.
In this sense, while this PR needs to be in the chain of a feature branch for development, is not strictly related to the functionality that the feature branch is going to end up having, but only fixes problems in the target codebase to give the new submodule a proper welcome.

The issues that appear here were of 2 kinds:

  • UseRequireInsteadOfGet, in order to actually fix this we'd need to change most of the context!! and activity!! declarations, which I consider this is a nice to have to make code nicer but not really a behavior change, hence I added the flag as a warning class in the lint.xml configuration in 531d608.
  • use lifeCycleOwner in all observe() calls where a Fragment or Activity was being added for ViewModel changes listening. I believe this change needed to be done as it was the source of many other issues that have been otherwise fixed as they manifested themselves (see for example this one here Changing lifecycle owner to be the fragment instead of the activity. #10954 by @develric, I've seen other comments by @planarvoid on other PRs but couldn't find their reference again). This is fixed in a889ee4.

For the second one, I found this SO answer here to be useful https://stackoverflow.com/questions/59521691/use-viewlifecycleowner-as-the-lifecycleowner

From the documentation of Fragment's getLifecycleOwner source code itself it seems this should be the way to go anyway, and is safe to change to the recommended way:

 * Get a {@link LifecycleOwner} that represents the {@link #getView() Fragment's View}
* lifecycle. In most cases, this mirrors the lifecycle of the Fragment itself, but in cases
* of {@link FragmentTransaction#detach(Fragment) detached} Fragments, the lifecycle of the

To test:

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@mzorz mzorz added this to the 15.2 milestone May 14, 2020
@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile Bot commented May 14, 2020 •
edited
Loading

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile Bot commented May 14, 2020

Messages
📖

This PR contains changes in the subtree libs/login/. It is your responsibility to ensure these changes are merged back into wordpress-mobile/WordPress-Login-Flow-Android. Follow these handy steps!
WARNING: Make sure your git version is 2.19.x or lower - there is currently a bug in later versions that will corrupt the subtree history!

  1. cd WordPress-Android
  2. git checkout feature/wp-stories-integration-phase1-other-lint
  3. git subtree push --prefix=libs/login/ https://github.com/wordpress-mobile/WordPress-Login-Flow-Android.git merge/WordPress-Android/11920
  4. Browse to https://github.com/wordpress-mobile/WordPress-Login-Flow-Android/pull/new/merge/WordPress-Android/11920 and open a new PR.

Generated by 🚫 dangerJS

@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile Bot commented May 14, 2020 •
edited
Loading

You can test the changes on this Pull Request by downloading the APK here.

@mzorz mzorz marked this pull request as ready for review May 14, 2020 17:19
This was referenced May 14, 2020
Closed
Merged
@0nko 0nko self-requested a review May 15, 2020 07:47
@0nko 0nko self-assigned this May 15, 2020
0nko
0nko reviewed May 15, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@0nko 0nko left a comment

Choose a reason for hiding this comment

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

Hey @mzorz. The code itself looks good but I was unable to test it because I can't get it to build. I'm getting this error:

ERROR: Unable to resolve dependency for ':WordPress@wasabiDebugUnitTest/compileClasspath': Could not resolve project :libs:portkey-android:stories.

Am I missing some step?

@malinajirka
Copy link
Copy Markdown
Contributor

malinajirka commented May 15, 2020

Thanks @mzorz!

Unfortunately, I'm not able to build the app ("Could not resolve project :libs:portkey-android:stories"). However, I reviewed the code and tested the app on the CircleCI build and it LGTM.

I believe this change needed to be done as it was the source of many other issues that have been otherwise fixed as they manifested themselves (see for example this one here #10954 by @develric, I've seen other comments by @planarvoid on other PRs but couldn't find their reference again). This is fixed in a889ee4.

I think the issues you pointed out are related to a bit different issue. The mentioned issues are dealing with the difference between having the observer Activity or Fragment. Whereas this PR is dealing with the difference between having as the observer Fragment or FragmentLifeCyclerOwner. Anyway, I agree this is a good change as it fixes some issues when dealing with fragment transactions and LiveData.

As I mentioned on the p2 post I think we should also go through the release notes of fragments v1.2.0 and make sure none of the changes are breaking changes.

@mzorz
Copy link
Copy Markdown
Contributor Author

mzorz commented May 15, 2020

Hey @mzorz. The code itself looks good but I was unable to test it because I can't get it to build. > I'm getting this error:

ERROR: Unable to resolve dependency for > ':WordPress@wasabiDebugUnitTest/compileClasspath': Could not resolve project :libs:portkey-> android:stories.
Am I missing some step?

@0nko The instructions are in the base PR, should have made that clear 😅 - look here please #11864 - thanks for checking!

@mzorz
Copy link
Copy Markdown
Contributor Author

mzorz commented May 15, 2020

Thanks for taking the time to check this @malinajirka !

The mentioned issues are dealing with the difference between having the observer Activity or Fragment. Whereas this PR is dealing with the difference between having as the observer Fragment or FragmentLifeCyclerOwner.

I think I wanted to say it's about getting the right lifecycleOwner, which is obtained through the getLifecycleOwner in a Fragment (which as you say is true, all the changes in this PR only touch on Fragments) but it's also applicable to Activity (both Fragment or Activity can be lifecycleOwners). Was trying to show how using getActivity() or this directly in the case of the referenced PR is a common source of problems, that can be solved by encapsulating the decision using getLifecycleOwner().

Anyway, I agree this is a good change as it fixes some issues when dealing with fragment transactions and LiveData.

Right, agreed 👍🏻

As I mentioned on the p2 post I think we should also go through the release notes of fragments v1.2.0 and make sure none of the changes are breaking changes.

As commented elsewhere as well, it'd be great if we could isolate this change on the target develop codebase alone if at all possible, but also happy to do it here in the context of this Stories feature branch chained PRs if it turns out to be the case the only things that need to upgrade belong to this functionality (I don't think so - or couldn't see it with my own eyes only, so I very much appreciate your help here 🙏🏻🙌🏻❤️)

@mzorz
Copy link
Copy Markdown
Contributor Author

mzorz commented May 15, 2020

Going to move this one to target develop soon as discussed, so, switching back to draft for the time being 👍

@mzorz mzorz marked this pull request as draft May 15, 2020 14:39
@mzorz mzorz mentioned this pull request May 15, 2020
Merged
9 tasks
@mzorz
Copy link
Copy Markdown
Contributor Author

mzorz commented May 15, 2020

Closing this one in favor of #11934 which targets develop, thank you!

@mzorz mzorz closed this May 15, 2020
@mzorz mzorz deleted the feature/wp-stories-integration-phase1-other-lint branch May 15, 2020 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@0nko 0nko 0nko left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@0nko 0nko

Labels

None yet

Projects

None yet

Milestone

15.2

Development

Successfully merging this pull request may close these issues.

3 participants

@mzorz @malinajirka @0nko