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

Add the ability to not depend on the "compose.material" module #497

Open
igordmn opened this issue Mar 14, 2021 · 12 comments
Open

Add the ability to not depend on the "compose.material" module #497

igordmn opened this issue Mar 14, 2021 · 12 comments
Assignees
Labels
desktop discussion Need further discussion to understand if it actually needed enhancement New feature or request gradle Gradle plugin problems multiplatform

Comments

@igordmn
Copy link
Collaborator

igordmn commented Mar 14, 2021

Probably we can make compose.desktop.currentOs dependent on compose.foundation, not on compose.material.

In this case developers who don't want to use material module can write:

implementation(compose.desktop.currentOs)

or

implementation(compose.desktop.currentOs)
implementation(compose.foundation)

or

implementation(compose.desktop.foundation.currentOs)

If material module is needed then developers can write:

implementation(compose.desktop.currentOs)
implementation(compose.material)

or

implementation(compose.desktop.material.currentOs)
@igordmn igordmn self-assigned this Mar 14, 2021
@igordmn igordmn added the gradle Gradle plugin problems label Mar 14, 2021
@olonho
Copy link
Contributor

olonho commented Mar 15, 2021

What's the point for such a change?

@olonho olonho added the discussion Need further discussion to understand if it actually needed label Mar 15, 2021
@igordmn
Copy link
Collaborator Author

igordmn commented Mar 15, 2021

What's the point for such a change?

If someone will write a library-replacement for material module (with a bunch of UI components), there is no easy way to use it without material (because our desktop module depends on material) .

After this change we can use only that library, without material

@jimgoog
Copy link
Collaborator

jimgoog commented Mar 15, 2021

I'm actually a little confused by the whole currentOs thing; why is this needed when using MPP? That is to say, why is it not sufficient to just add a reference to the maven artifact with gradle metadata and then let the platform-specific questions get resolved automatically?

@igordmn
Copy link
Collaborator Author

igordmn commented Mar 15, 2021

maven artifact with gradle metadata

Yes, I am also thought about that! But at the time I tried (Summer 2020) there were some issues.

Maybe I just didn't know MPP plugin very well, maybe MPP plugin didn't support different jvm binary targets.

But we can try again, and if we will need something from MPP plugin, we can communicate with MPP team.

@igordmn igordmn added multiplatform enhancement New feature or request labels Mar 15, 2021
@olonho
Copy link
Contributor

olonho commented Mar 16, 2021

currentOs is to provide different artifacts on different platforms (as Skia is pretty big, and we don't want to pack all possible native libs into single artifact). Do not think MPP will help here.

@igordmn
Copy link
Collaborator Author

igordmn commented Mar 16, 2021

Do not think MPP will help here

Theoretically we can still ship separate artifacts for each platform (mac-arm64, mac-x64, win64, linux64), and make MPP plugin resolve platform automatically:

kotlin {
    android()
    jvm("desktop")
    jvm("macosArm64")
    jvm("windowsX64")

    sourceSets {
        val commonMain by getting {
            dependencies {
                api(compose.material)
            }
        }

        val desktopMain by getting {
            dependsOn(commonMain)
            dependencies {
                api(compose.desktop)
            }
        }

        val macosArm64Main by getting {
            dependsOn(desktopMain)
        }

        val windowsX64Main by getting {
            dependsOn(desktopMain)
        }
    }
}

Compose Gradle plugin task "./gradlew run" will determine current Os and will run appropriate target.

It looks verbose, so we need to think can we reduce the code size.

@igordmn igordmn added this to the 1.0 milestone Nov 2, 2021
@igordmn igordmn removed this from the 1.0 milestone Nov 18, 2021
@igordmn igordmn removed the p:normal label Dec 9, 2022
@Wertual08
Copy link

Are there any workarounds for now to not include compose.material?

@malliaridis
Copy link

@Wertual08 I am using Material 3 and Material 2 was causing many naming conflicts and styling issues when wrongly imported, so I excluded it from every package that includes it (works fine so far, but tested only with Material 3 included). For example:

implementation(compose.desktop) {
  exclude("org.jetbrains.compose.material")
}

@Wertual08
Copy link

@malliaridis Many thanks!

@Omico
Copy link
Contributor

Omico commented Dec 12, 2023

Bought it from #4035,

Maybe it is time to remove DesktopTheme.jvm.kt completely.

Then we can safely remove compose.material.

@PMARZV
Copy link

PMARZV commented Apr 3, 2024

Any ETA for this?? is there any blocker that prevents this from happening???

@igordmn
Copy link
Collaborator Author

igordmn commented Aug 2, 2024

curentOs maybe not needed if #3282 is resolved. compose.desktop module can be removed after this as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
desktop discussion Need further discussion to understand if it actually needed enhancement New feature or request gradle Gradle plugin problems multiplatform
Projects
None yet
Development

No branches or pull requests

9 participants