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

Decouple :vector variants/build types #6783

Merged
merged 12 commits into from
Aug 11, 2022
Merged

Conversation

ouchadam
Copy link
Contributor

@ouchadam ouchadam commented Aug 9, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

  • Decouples the :vector variant/build type specific logic introducing interfaces, this will allow the variant logic to be lifted out of the vector module. It also means we can avoid including debug related entry points within the main sourceset.

Motivation and context

A handful of refactors to more easily enable #6779 - lifting variant logic to the application module

Screenshots / GIFs

No UI changes

Tests

Smoke test on gplay and fdroid variants

Tested devices

  • Physical
  • Emulator
  • OS version(s): 28


@InstallIn(SingletonComponent::class)
@Module
abstract class DebugModule {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

instead of relying on file overrides with the variants we can provide variant unique modules to the dagger graph and provide different implementations

the variant logic could also be extracted to their own modules in the future if wanted

@@ -187,7 +187,6 @@
android:name="android.support.PARENT_ACTIVITY"
android:value=".features.home.HomeActivity" />
</activity>
<activity android:name=".features.debug.DebugMenuActivity" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this can now safely exist in only the debug sourceset as the debug implementation of DebugNavigator is the only place to reference the activity

@ouchadam ouchadam marked this pull request as ready for review August 9, 2022 12:06
@ouchadam ouchadam requested review from a team and ganfra and removed request for a team August 9, 2022 12:59
@ouchadam
Copy link
Contributor Author

ouchadam commented Aug 9, 2022

it looks like the pre-existing AndroidManifest warnings are being pulled in by danger because the file has changes

@ouchadam ouchadam mentioned this pull request Aug 9, 2022
6 tasks
}

@Provides
fun providesFlipperProxy() = object : FlipperProxy {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we should delete the existing FlipperProxy file in the release variant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah good catch! will do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed 6526cf3

return FDroidGuardServiceStarter(preferences, appContext)
companion object {
@Provides
fun provideGuardServiceStarter(preferences: VectorPreferences, appContext: Context): GuardServiceStarter {
Copy link
Contributor

Choose a reason for hiding this comment

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

I know it is a detail but could we rename the provide methods using the prefix provides all the time to keep consistency in all modules?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good to me!

import im.vector.app.features.settings.legals.FlavourLegals
import javax.inject.Inject

class GoogleFlavorLegals @Inject constructor() : FlavourLegals {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we rename FlavourLegals to FlavorLegals to be consistent in naming?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@mnaturel mnaturel left a comment

Choose a reason for hiding this comment

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

LGTM, I added small comments about naming.

@ouchadam ouchadam enabled auto-merge August 11, 2022 08:50
@ElementBot
Copy link

Warnings
⚠️

vector/src/main/AndroidManifest.xml#L79 - The attribute android:allowBackup is deprecated from Android 12 and higher and may be removed in future versions. Consider adding the attribute android:dataExtractionRules specifying an @xml resource which configures cloud backups and device transfers on Android 12 and higher.

⚠️

vector/src/main/AndroidManifest.xml#L79 - The attribute android:allowBackup is deprecated from Android 12 and higher and may be removed in future versions. Consider adding the attribute android:dataExtractionRules specifying an @xml resource which configures cloud backups and device transfers on Android 12 and higher.

⚠️

vector/src/main/AndroidManifest.xml#L232 - Consider splitting data tag into multiple tags with individual attributes to avoid confusion

⚠️

vector/src/main/AndroidManifest.xml#L232 - Consider splitting data tag into multiple tags with individual attributes to avoid confusion

⚠️

vector/src/main/AndroidManifest.xml#L235 - Consider splitting data tag into multiple tags with individual attributes to avoid confusion

⚠️

vector/src/main/AndroidManifest.xml#L235 - Consider splitting data tag into multiple tags with individual attributes to avoid confusion

⚠️

vector/src/main/AndroidManifest.xml#L289 - Expecting android:screenOrientation="unspecified" or "fullSensor" for this activity so the user can use the application in any orientation and provide a great experience on Chrome OS devices

⚠️

vector/src/main/AndroidManifest.xml#L289 - Expecting android:screenOrientation="unspecified" or "fullSensor" for this activity so the user can use the application in any orientation and provide a great experience on Chrome OS devices

⚠️

vector/src/main/AndroidManifest.xml#L423 - Exported receiver does not require permission

⚠️

vector/src/main/AndroidManifest.xml#L423 - Exported receiver does not require permission

Generated by 🚫 dangerJS against 439224e

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

18.2% 18.2% Coverage
0.0% 0.0% Duplication

@ouchadam ouchadam merged commit 616c16f into develop Aug 11, 2022
@ouchadam ouchadam deleted the feature/adm/decouple-variants branch August 11, 2022 09:59
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

Successfully merging this pull request may close these issues.

3 participants