Make stacked preview annotations generate showkaseMetadata components for each annotation #259

oas004 · 2022-08-28T16:07:08Z

This is the next step in supporting Multipreview annotations. Referencing #255 as the first step.

Here I have added functionality to make a ShowkaseMetadata object for each Preview annotation. Eg.

@Preview(
    name = "small font",
    group = "font scales",
    fontScale = 0.5f
)
@Preview(
    name = "large font",
    group = "font scales",
    fontScale = 1.5f
)
@Composable
fun ComposablePreviewFont() {

}

Before, this would only make one component in Showkase. Now it should make one for each preview and place them in their respective group. Here they would both be placed in the group font scales

Furthermore, I would advice to review this by commit as I had to update the tests because of some arrangement due to a check in the processor. I placed the test update in 8371ec2.

I'm leaving this as a draft PR because I'm having some issues with getting this to work with KAPT.
With KSP, the method roundEnvironment.getElementsAnnotatedWith(PREVIEW_CLASS_NAME) returns all the elements for multi stacked preview. However, with KAPT, it does not seem to work the same way. Is there some other function I should use here? Any suggestions are more than welcome 😄
Added a slack post for anyone interested 😄

showkase-processor/src/main/java/com/airbnb/android/showkase/processor/ShowkaseProcessor.kt

...ase-processor/src/main/java/com/airbnb/android/showkase/processor/models/ShowkaseMetadata.kt

.../src/main/java/com/airbnb/android/showkase/processor/writer/ShowkaseCodegenMetadataWriter.kt

vinaygaba · 2022-08-31T17:12:03Z

.../src/main/java/com/airbnb/android/showkase/processor/writer/ShowkaseCodegenMetadataWriter.kt

+                && showkaseMetadata.componentIndex != null
+                && showkaseMetadata.componentIndex > 0
+            ) {
+                "${showkaseMetadata.showkaseGroup}_${showkaseMetadata.showkaseName}_${showkaseMetadata.componentIndex}"


I know we were hoping for a better name but we can probably append componentIndex for now.

vinaygaba

This is looking in good shape. You can do the ShowkaseComposable support later on as it's not super pressing. I think you are at a point where you can move to the custom annotation stuff we discussed previously. Great job in making such a critical feature happen 👏🏻

There is another test for checking the old behaviour

...testing/src/test/java/com/airbnb/android/showkase_processor_testing/ShowkaseProcessorTest.kt

oas004 · 2022-08-31T20:17:15Z

This is looking in good shape. You can do the ShowkaseComposable support later on as it's not super pressing. I think you are at a point where you can move to the custom annotation stuff we discussed previously. Great job in making such a critical feature happen 👏🏻

Thank you very much @vinaygaba! And thank you so much valuable review 🙏 I tried to add support for stacked @ShowkaseComposable in b109238. I also wrote some docs about these not working with KAPT because of https://youtrack.jetbrains.com/issue/KT-49682 . Furthermore, added tests for only KSP for stacked @Preview and @ShowkaseComposable cases 😃

vinaygaba · 2022-08-31T20:24:31Z

@oas004 Amazing! One final thing you can do is write a test for the ShowkaseBrowser that verifies the following:

Stacked previews show up in the browser
They are being treated as separate previews

Add the test cases here - https://github.com/airbnb/Showkase/blob/master/showkase-browser-testing/src/androidTest/java/com/airbnb/android/showkase_browser_testing/ShowkaseBrowserTest.kt

oas004 · 2022-08-31T20:32:06Z

@oas004 Amazing! One final thing you can do is write a test for the ShowkaseBrowser that verifies the following:

Stacked previews show up in the browser

They are being treated as separate previews

Add the test cases here - https://github.com/airbnb/Showkase/blob/master/showkase-browser-testing/src/androidTest/java/com/airbnb/android/showkase_browser_testing/ShowkaseBrowserTest.kt

That is a very good idea! 🙌 Is there a way for us to bypass it in KAPT on CI? I think if I added those BrowserTests, then it will fail on KAPT every time 🤔

vinaygaba · 2022-08-31T20:42:27Z

@oas004 Oh no, you are right. We currently run these tests for both ksp and kapt and pass the PuseKsp flag from CLI. Is there a way to access this value in the test case somehow? If there is, we can always pass the test when it's kapt and run the test as usual when its ksp. This is how we run these tests - https://github.com/airbnb/Showkase/blob/master/.github/workflows/android.yml#L59

vinaygaba · 2022-08-31T20:46:20Z

@oas004 Can you try this - in the android.yml file, also add -DuseKsp=true and -DuseKsp=false and then try accessing them in the new test cases using System.getProperty("useKsp")

oas004 · 2022-08-31T21:14:01Z

@oas004 Can you try this - in the android.yml file, also add -DuseKsp=true and -DuseKsp=false and then try accessing them in the new test cases using System.getProperty("useKsp")

I think that can work! Will test this out first thing in the morning😊

oas004 · 2022-09-01T13:38:17Z

@vinaygaba Can we re-run the UI Tests? It didn't provide me a log to check if it worked or not 😅

oas004 · 2022-09-01T13:40:51Z

@vinaygaba Can we re-run the UI Tests? It didn't provide me a log to check if it worked or not 😅

it seems like it was failing on the screenshot tests 🤔 I think I might need to take that into account as well somehow 🤔

oas004 · 2022-09-01T17:36:07Z

It seems like the KAPT stage is working on all but one emulator and then fails, so I can't see the others that checks green to verify if the System.getProperty trick works. I can't get it to work locally either though, so maybe I could try to save the variable in the gradle file as well? Or would that just be dumb? 🤔

.github/workflows/android.yml

sample/src/main/java/com/airbnb/android/showkasesample/BasicChip.kt

vinaygaba · 2022-09-02T18:37:09Z

...ting/src/androidTest/java/com/airbnb/android/showkase_browser_testing/ShowkaseBrowserTest.kt

@@ -37,31 +36,54 @@ class ShowcaseBrowserTest {
            }
        )

+    // This will alter now since KSP supports stacked preview annotations and KAPT does not.
+    private val componentSize = if (System.getProperty("useKsp") == "true") {


Can you add a more descriptive comment about why we are doing this. Along with link to the issue

Good idea! Added something in fb87f3c :) Does that look okay? :)

vinaygaba

Very exciting! Thank you for your patience and getting this to the finish line 👏

…e preview annotations

oas004 · 2022-09-02T20:08:21Z

Very exciting! Thank you for your patience and getting this to the finish line 👏

This is supercool! I will start on the actual mutlipreview annotation as soon as possible! 😄 🤩 Thank you so much for your valuable input! 🙏 Feel like I am learning a lot from this! 😃 😍

...ting/src/androidTest/java/com/airbnb/android/showkase_browser_testing/ShowkaseBrowserTest.kt

vinaygaba

Looks like we have everything covered at this point 🥳

Make stacked preview annotations generate showkaseMetadata components

df66b1f

oas004 force-pushed the feature/stacked-preview-annotation branch from 8371ec2 to ba69d50 Compare August 28, 2022 16:15

vinaygaba reviewed Aug 30, 2022

View reviewed changes

showkase-processor/src/main/java/com/airbnb/android/showkase/processor/ShowkaseProcessor.kt Show resolved Hide resolved

vinaygaba reviewed Aug 30, 2022

View reviewed changes

...ase-processor/src/main/java/com/airbnb/android/showkase/processor/models/ShowkaseMetadata.kt Outdated Show resolved Hide resolved

vinaygaba reviewed Aug 30, 2022

View reviewed changes

.../src/main/java/com/airbnb/android/showkase/processor/writer/ShowkaseCodegenMetadataWriter.kt Outdated Show resolved Hide resolved

oas004 added 2 commits August 30, 2022 20:56

Remove distinct name and use index instead

f185cec

Update processor tests as some componentkeys changed

a6b003b

oas004 force-pushed the feature/stacked-preview-annotation branch from ba69d50 to a6b003b Compare August 30, 2022 18:57

vinaygaba mentioned this pull request Aug 31, 2022

Add automatic preview screenshot testing with Paparazzi and Showkase android/nowinandroid#101

Closed

vinaygaba reviewed Aug 31, 2022

View reviewed changes

vinaygaba approved these changes Aug 31, 2022

View reviewed changes

oas004 added 5 commits August 31, 2022 21:07

Update base test to expose modes for processor unit tests

e9c04d0

Update stacked preview processor test to only test ksp

ef2f73e

There is another test for checking the old behaviour

Add note in documentation about KAPT vs KSP support

65fbd96

Add support for multiple stacked ShowkaseComposable annotations

b109238

Add note about KSP support for stacked ShowkaseComposable annotation

bc1c216

vinaygaba reviewed Aug 31, 2022

View reviewed changes

...testing/src/test/java/com/airbnb/android/showkase_processor_testing/ShowkaseProcessorTest.kt Show resolved Hide resolved

oas004 marked this pull request as ready for review August 31, 2022 20:17

Update processor tests component ids

0fdf16c

Add tests for stacked annotations for KSP builds only

d165e7d

oas004 force-pushed the feature/stacked-preview-annotation branch from 2b16ce2 to d165e7d Compare September 2, 2022 10:05

vinaygaba reviewed Sep 2, 2022

View reviewed changes

.github/workflows/android.yml Outdated Show resolved Hide resolved

vinaygaba reviewed Sep 2, 2022

View reviewed changes

sample/src/main/java/com/airbnb/android/showkasesample/BasicChip.kt Show resolved Hide resolved

Add use ksp flag on ci pipeline

e2d2d67

vinaygaba reviewed Sep 2, 2022

View reviewed changes

vinaygaba approved these changes Sep 2, 2022

View reviewed changes

Add a more descriptive comment on why KAPT does not support repeatabl…

fb87f3c

…e preview annotations

Log use ksp value from System property

84315b7

oas004 force-pushed the feature/stacked-preview-annotation branch from 58a6ac2 to 84315b7 Compare September 3, 2022 21:27

Use buildconfig to get if we are running ksp or not

fbf680a

oas004 force-pushed the feature/stacked-preview-annotation branch from 8d57459 to fbf680a Compare September 4, 2022 20:01

vinaygaba reviewed Sep 5, 2022

View reviewed changes

...ting/src/androidTest/java/com/airbnb/android/showkase_browser_testing/ShowkaseBrowserTest.kt Outdated Show resolved Hide resolved

Use buildconfig instead of getProperty on ksp only test

f2003ad

vinaygaba approved these changes Sep 5, 2022

View reviewed changes

vinaygaba merged commit 3dffbac into airbnb:master Sep 5, 2022

oas004 deleted the feature/stacked-preview-annotation branch September 5, 2022 06:56

This was referenced Sep 9, 2022

Add support for Custom Preview Annotations #263

Closed

Add stacked multi preview support for KAPT #264

Open

vinaygaba mentioned this pull request Sep 22, 2022

Support multi-preview annotations (annotations annotated with @Preview) #233

Closed

oas004 mentioned this pull request Feb 12, 2023

Add support for CustomPreviewAnnotations in Showkase 🚿 🌈 #303

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make stacked preview annotations generate showkaseMetadata components for each annotation #259

Make stacked preview annotations generate showkaseMetadata components for each annotation #259

oas004 commented Aug 28, 2022 •

edited

Loading

vinaygaba Aug 31, 2022

vinaygaba left a comment •

edited

Loading

oas004 commented Aug 31, 2022

vinaygaba commented Aug 31, 2022

oas004 commented Aug 31, 2022

vinaygaba commented Aug 31, 2022

vinaygaba commented Aug 31, 2022 •

edited

Loading

oas004 commented Aug 31, 2022

oas004 commented Sep 1, 2022

oas004 commented Sep 1, 2022

oas004 commented Sep 1, 2022

vinaygaba Sep 2, 2022

oas004 Sep 2, 2022

vinaygaba left a comment

oas004 commented Sep 2, 2022

vinaygaba left a comment

Make stacked preview annotations generate showkaseMetadata components for each annotation #259

Make stacked preview annotations generate showkaseMetadata components for each annotation #259

Conversation

oas004 commented Aug 28, 2022 • edited Loading

vinaygaba Aug 31, 2022

Choose a reason for hiding this comment

vinaygaba left a comment • edited Loading

Choose a reason for hiding this comment

oas004 commented Aug 31, 2022

vinaygaba commented Aug 31, 2022

oas004 commented Aug 31, 2022

vinaygaba commented Aug 31, 2022

vinaygaba commented Aug 31, 2022 • edited Loading

oas004 commented Aug 31, 2022

oas004 commented Sep 1, 2022

oas004 commented Sep 1, 2022

oas004 commented Sep 1, 2022

vinaygaba Sep 2, 2022

Choose a reason for hiding this comment

oas004 Sep 2, 2022

Choose a reason for hiding this comment

vinaygaba left a comment

Choose a reason for hiding this comment

oas004 commented Sep 2, 2022

vinaygaba left a comment

Choose a reason for hiding this comment

oas004 commented Aug 28, 2022 •

edited

Loading

vinaygaba left a comment •

edited

Loading

vinaygaba commented Aug 31, 2022 •

edited

Loading