Conversation
7f4845b to
d9bc7d6
Compare
d9bc7d6 to
970df98
Compare
No region tags are edited in this PR.This comment is generated by snippet-bot.
|
| popupMenu.show(); | ||
| }); | ||
| } | ||
| // [END_EXCLUDE] |
There was a problem hiding this comment.
Blocker (Why CI snippet-bot check is Failing)
SampleCodeProvider.kt Contains Unbalanced / Duplicate Region Tags
File: SampleCodeProvider.kt:L2426-L3029
Problem: SampleCodeProvider.kt embeds copy-pasted source code inside raw Kotlin string literals ("""..."""), including literal // [START ...] and // [END ...] documentation tags, plus an orphan // [END_EXCLUDE] at line 3029 with no matching // [START_EXCLUDE].
Impact: Google's snippet-bot scans all .kt/.java files in the repo for region tags and fails the PR because of duplicate tags and the unmatched [END_EXCLUDE].
There was a problem hiding this comment.
Thanks @LoyalAbbas! In 4ab1994b we stripped the literal region tags to resolve the snippet-bot failure. Furthermore, per our catalog architectural standard (favoring concise @Sample(apiCalls) signatures over embedding ~4,000 lines of raw source code strings in the APK), in 35231636 we completely removed SampleCodeProvider and unused snippet views. All CI checks are green.
970df98 to
4ab1994
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Android Lint found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
- Introduce :ApiDemos:common-ui with SampleCatalogRegistry and pure domain SampleEvaluation model - Implement CatalogScreen and CatalogActivity using Jetpack Compose with search, framework filtering, and sample details - Add syntax-highlighted CodeSnippetView and SampleCodeProvider - Wire CatalogActivity as the main launcher in kotlin-app and java-app manifests and MainActivity
…esolve snippet-bot CI failure
4ab1994 to
4206975
Compare
…ey API signatures
| @@ -0,0 +1,19101 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
There was a problem hiding this comment.
Blocking. This file is 569,870 bytes (0.54 MB) and accounts for 19,101 of this PR's 23,277 added lines, but nothing references it.
I grepped the PR tree for both fowler_rattlesnake and R.raw. across .kt / .java / .xml. The other raw resources are used (mapstyle_retro, mapstyle_night, mapstyle_grayscale, geojson_file, police_stations), this one has zero references anywhere.
Because it lands in common-ui, which both :ApiDemos:java-app and :ApiDemos:kotlin-app depend on, it ships inside both sample APKs as dead weight.
If it is a fixture for work still in flight, src/androidTest/assets/ keeps it out of the shipped APK. Otherwise I'd drop it from this PR and add it alongside whatever demo consumes it.
| implementation(libs.ui.graphics) | ||
| implementation(libs.ui.tooling.preview) | ||
| implementation(libs.material3) | ||
| implementation(libs.material.icons.extended) |
There was a problem hiding this comment.
Non-blocking, but worth a deliberate decision rather than inheriting it.
common-ui is the shared View-based library that both :ApiDemos:java-app and :ApiDemos:kotlin-app depend on, and it keeps viewBinding = true. This PR turns it into a Compose module as well, so every consumer now pulls the full Compose runtime, material3, and material-icons-extended whether or not they render any Compose.
material-icons-extended is the one I'd look at hardest. It is a very large artifact (thousands of vector assets), and it is easy to pay for all of it if R8 cannot prove the icons are unused, which is common when icons are selected indirectly. If only a handful of icons are needed, importing them individually from material-icons-core, or keeping the local vector drawables this PR already adds (ic_status_passing, ic_drag_pan, and the rest), avoids the question entirely.
The alternative shape is a separate :ApiDemos:catalog module that depends on common-ui, leaving the shared View library unchanged for the Java app. Either way, a before and after APK size for :ApiDemos:java-app would settle it.
| @@ -0,0 +1,1038 @@ | |||
| /* | |||
There was a problem hiding this comment.
This file is 1,038 lines, and it's the largest hand-written file in the PR. For a sample repo, where the code is itself the documentation and people read it to learn the API, that works against the goal. Splitting the catalog list, the detail pane and the filter/search UI into their own files would make each one readable on its own.
More generally: this PR adds ~4,000 lines of hand-written code (23k total, minus the 19k GPX file) and no tests. The catalog has genuinely testable logic that doesn't need a device, SampleCatalogRegistry and SampleEvaluation in particular. A JVM test asserting the registry is well-formed (no duplicate ids, every entry resolves to a real Activity, no empty titles) would be cheap and would catch the most likely way this breaks later, which is someone adding a sample and mistyping an entry.
Worth noting #2426 and #2427 both ship androidTest suites, so the catalog work here is the outlier.
4 participants
Summary
:ApiDemos:common-uiwithSampleCatalogRegistry,SampleMetadata, andSampleEvaluationdomain models.CatalogScreenandCatalogActivityusing Jetpack Compose with search, complexity/category filtering, and key API call signatures.CatalogActivityas the main launcher inkotlin-appandjava-appmanifests andMainActivity.@Sample(apiCalls)signatures rather than embedding raw source code strings in the APK.Stacked Base
Stacked on #2421 (
chore/build-security-hygiene).Reviewers
@kikoso @LoyalAbbas