Skip to content

Add common parent class for application classes#16151

Merged
ravishanker merged 3 commits intofeature/dagger-to-hiltfrom
feature/add-basewordpress
Mar 28, 2022
Merged

Add common parent class for application classes#16151
ravishanker merged 3 commits intofeature/dagger-to-hiltfrom
feature/add-basewordpress

Conversation

@irfano
Copy link
Copy Markdown
Member

@irfano irfano commented Mar 22, 2022 •
edited
Loading

We need different application classes for UI test and real app to use Hilt. The main reason for this requirement is that we can't use @Inject annotation in the test application because the test application handles injection differently in Hilt.

WordPress: Abstract superclass application which contains common jobs for real and test applications.
WordPressApp: Real application
WordPressTest: UI test application. The only difference from WordPressApp will be handling injection differently. Difference will be in next PRs.

We need the ability to initialize Hilt whenever we want without @Inject annotation. To provide this, I added WordPress, which contains common stuff for WordPressTest and WordPressApp.

This PR can seem meaningless before future PRs, but it will be useful after adding Hilt.

To test:
Build and run the app. It should be running without an error.

Regression Notes

  1. Potential unintended areas of impact
    Building and tests might be broken.

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    I smoke tested the app, and it's working as expected. Also, tests are working fine.

  3. What automated tests I added (or what prevented me from doing so)
    No tests are added since no new feature is added.

PR submission checklist:

  • I have completed the Regression Notes.
  • 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.

@irfano irfano self-assigned this Mar 22, 2022
@irfano
Copy link
Copy Markdown
Member Author

irfano commented Mar 22, 2022 •
edited
Loading

@ravishanker, I need to update WordPressTest's superclass with BaseWordPress to continue this PR. But after doing that, tests won't work because (WordPress) casts will fail in tests. There are 225 WordPress casting in the code. So, I need to change all these castings to cast BaseWordPress, but it will change a lot of files.

Alternatively, I can use WordPress as a superclass, as I did in #15653. Then I can add another class for real applications. But the important drawback is that I will have to change the application's name in this way, like WordPress -> WordPressProd.

WordPress-Android/WordPress/src/main/AndroidManifest.xml

Line 56 in 3457027

android:name=".WordPress"

I think application name change is a critical change. It could be used for deeplinks or in some external services.

I couldn't find a better solution than changing 225 casting lines from (WordPress) to (BaseWordPress). Do you think I should go this way?

@ravishanker
Copy link
Copy Markdown
Contributor

ravishanker commented Mar 23, 2022

WordPress

@irfano - It is never easy these kind of structural changes. Are these casts only in tests or in the application code? If it is in tests I wouldn't worry about it. In any case, it would be good to socialize this change with the rest of android community. Please post about it to #android-dev slack channel and get some feedback on the proposed change.

@irfano
Copy link
Copy Markdown
Member Author

irfano commented Mar 23, 2022

Are these casts only in tests or in the application code?

No, it's in the application code. Thanks, I'll share this to get feedback.

@irfano irfano changed the title Add BaseWordPress as the superclass of WordPress Add common parent class for application classes Mar 27, 2022
@irfano
Copy link
Copy Markdown
Member Author

irfano commented Mar 27, 2022

After receiving feedback from Slack, I decided to name superclass as WordPress to avoid causing changes on 255 files. We couldn't come up with any drawbacks of changing the application class name from WordPress to WordPressApp.

@irfano irfano requested a review from ravishanker March 27, 2022 21:06
@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Mar 27, 2022

Warnings
⚠️ PR has more than 300 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

@irfano irfano marked this pull request as ready for review March 27, 2022 21:07
irfano
irfano commented Mar 27, 2022
View reviewed changes
WordPress/src/androidTest/java/org/wordpress/android/WordPressTest.kt
import org.wordpress.android.modules.DaggerAppComponentTest

class WordPressTest : WordPress() {
class WordPressTest : WordPressApp() {
Copy link
Copy Markdown
Member Author

@irfano irfano Mar 27, 2022

Choose a reason for hiding this comment

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

For now, WordPressTest is extended from WordPressApp. When Hilt added, WordPressTest will extend WordPress and inject and init AppInitializer differently from WordPressApp.

Copy link
Copy Markdown
Contributor

@ravishanker ravishanker Mar 28, 2022

Choose a reason for hiding this comment

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

Sounds good. I've been able to build and run without error

ravishanker
ravishanker approved these changes Mar 28, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@ravishanker ravishanker left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@ravishanker ravishanker merged commit c54b299 into feature/dagger-to-hilt Mar 28, 2022
@ravishanker ravishanker deleted the feature/add-basewordpress branch March 28, 2022 07:39
@irfano irfano mentioned this pull request Apr 18, 2022
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@ravishanker ravishanker ravishanker approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@irfano irfano

Labels

[Type] Enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@irfano @ravishanker