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

[refactor] Use DevicesPreviews annotation #166

Conversation

mhmd-android
Copy link
Contributor

@mhmd-android mhmd-android commented Oct 29, 2023

Removed individual @Preview functions for different device types and configurations and used the @DevicesPreviews function

#123 close

@Kaaveh Kaaveh self-requested a review October 29, 2023 09:17
@Kaaveh Kaaveh added enhancement New feature or request hacktoberfest labels Oct 29, 2023
@Kaaveh
Copy link
Owner

Kaaveh commented Oct 29, 2023

Great job, @mhmd-android! Could you please provide a screenshot of the new previews in Android Studio?

@mhmd-android
Copy link
Contributor Author

mhmd-android commented Oct 29, 2023

@Kaaveh, Yes sure, but I don't know Why my Android Studio doesn't show the preview, this problem is only in this preview
Anyway, previews are like this:

2023-10-29_133017

@Kaaveh
Copy link
Owner

Kaaveh commented Oct 29, 2023

@mhmd-android I faced this issue before, so I made it a GitHub issue. Can you investigate the root cause?

@mhmd-android
Copy link
Contributor Author

mhmd-android commented Oct 29, 2023

@Kaaveh, As long as I remember, I had this problem and I didn't understand what the problem is, but if you want to merge this PR and after that make it new an issue, if I succeed, I will definitely solve it.

@Kaaveh
Copy link
Owner

Kaaveh commented Oct 29, 2023

I reviewed the code and noticed that WindowsSizeClass requires the screen size as a parameter. Can you modify it to retrieve the screen size automatically?

Screenshot 2023-10-29 at 5 23 58 PM

@mhmd-android
Copy link
Contributor Author

mhmd-android commented Oct 29, 2023

I think it cannot be handled with just DevicesPreviews annotation because we have different previews, each of which receives a different width and height, so, do you have any opinions?

@Kaaveh
Copy link
Owner

Kaaveh commented Oct 29, 2023

Unfortunately, no. However, I think Jetpack Compose has made it easier to preview multiple screens in production.
@mhmd-android

@mhmd-android
Copy link
Contributor Author

mhmd-android commented Oct 29, 2023

@Kaaveh
I think this is why we used several previews in the previous code because the value of WindowSizeClass is different in each preview. That DevicesPreviews works when we don't have such a dependency

@Kaaveh
Copy link
Owner

Kaaveh commented Oct 29, 2023

Right, so let's close this PR. Thanks for your time ✌🏻

@Kaaveh Kaaveh closed this Oct 29, 2023
@mhmd-android
Copy link
Contributor Author

Thanks, dear @Kaaveh, And as the last issue, I should point out that we can't use code generation?

@mhmd-android mhmd-android deleted the refactor/use-devicePreviews-annotation branch October 29, 2023 17:19
@Kaaveh
Copy link
Owner

Kaaveh commented Oct 29, 2023

Thanks, dear @Kaaveh, And as the last issue, I should point out that we can't use code generation?

What do you mean exactly?

@mhmd-android
Copy link
Contributor Author

mhmd-android commented Oct 29, 2023

For example, let's write code generation (of course, I have to point out that I have not done this so far) that the annotation takes two inputs as width and height, and then the code is generated for us in the body of ComposeNewsAppPreview as an annotation processor. And then for each size, we can do it like this. In fact, DevicesPreviews turns into an annotation processor.

Something like this code:

@AutoService(Processor::class)
@SupportedSourceVersion(SourceVersion.RELEASE_8)
class DevicesPreviewsProcessor : AbstractProcessor() {
    override fun getSupportedAnnotationTypes(): MutableSet<String> {
        return mutableSetOf(DevicesPreviews::class.java.name)
    }

    override fun process(annotations: MutableSet<out TypeElement>?, roundEnv: RoundEnvironment?): Boolean {
        val generatedCode = StringBuilder()

        roundEnv?.getElementsAnnotatedWith(DevicesPreviews::class.java)?.forEach { element ->
            val annotation = element.getAnnotation(DevicesPreviews::class.java)
            val width = annotation.width
            val height = annotation.height

            generatedCode.appendLine(
                """
                @Composable
                fun ${element.simpleName}() {
                    ComposeNewsTheme {
                        ComposeNewsApp(
                            windowSize = WindowSizeClass.calculateFromSize(DpSize($width.dp, $height.dp)),
                            displayFeatures = persistentListOf(),
                            uiState = MainContract.State(),
                            closeDetailScreen = {},
                        )
                    }
                }
                """
            )
        }
        return true
    }
}

And finally, be used in this way:

@DevicesPreviews(width = 400, height = 900)
@Composable
fun ComposeNewsAppPreview() {
    Generated.ComposeNewsAppPreview()
}

Of course, the problem here is that this annotation processor only produces this part of the code:

               @Composable
                fun ${element.simpleName}() {
                    ComposeNewsTheme {
                        ComposeNewsApp(
                            windowSize = WindowSizeClass.calculateFromSize(DpSize($width.dp, $height.dp)),
                            displayFeatures = persistentListOf(),
                            uiState = MainContract.State(),
                            closeDetailScreen = {},
                        )
                    }
                }

and I think we cannot use it anywhere else.

Anyway, I said, keep this in mind, if you think this is a suitable solution, it should be researched and finally used.

@Kaaveh
Copy link
Owner

Kaaveh commented Oct 30, 2023

Great idea! Would you like to work on this in the future? There is no rush.

@mhmd-android
Copy link
Contributor Author

Yes, I would, of course

@Kaaveh
Copy link
Owner

Kaaveh commented Oct 30, 2023

Yes, I would, of course

Could you please comment on #169 to assign it to you?

@mhmd-android
Copy link
Contributor Author

Yes, I would, of course

Could you please comment on #169 to assign it to you?

Done 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants