Skip to content

Revised snippets for consistency. #579

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

Merged
merged 6 commits into from
Aug 8, 2025
Merged

Revised snippets for consistency. #579

merged 6 commits into from
Aug 8, 2025

Conversation

JonEckenrode
Copy link
Contributor

Revised snippets for consistency and correctness.

@asolovay
Copy link
Contributor

asolovay commented Aug 5, 2025

Did you try compiling this code? I just tried on my machine and I got an error:

/usr/local/google/home/asolovay/AndroidStudioProjects/snippets/misc/src/androidTest/java/com/example/snippets/DeviceCompatibilityModeTestJavaSnippets.java:37: error: cannot find symbol
                    assertFalse(isLetterboxed(activity));
                                ^
  symbol:   method isLetterboxed(DeviceCompatibilityModeTestJavaSnippets.MainActivity)
  location: class DeviceCompatibilityModeTestJavaSnippets

I think that's because isLetterboxed() is now an internal method in the new MainActivity class.

Just a reminder, the checks don't currently run against code in androidTest, so you can't count on Github to tell you the code compiles.

@JonEckenrode
Copy link
Contributor Author

JonEckenrode commented Aug 5, 2025

Thanks, Andrew, for finding this. Try it now. Builds locally.

@asolovay
Copy link
Contributor

asolovay commented Aug 6, 2025

Thanks! Looks fine to me, the code compiles for me too. (I don't think I have privileges to approve a pull request here, but Alex can do that, I assume?)

Just checking -- this last commit is a change to the code in the marked region (the code that shows up on the DAC page). Do you need to change the other Java snippet to match? (the snippet where you show how to define isLetterboxed()?)

@JonEckenrode
Copy link
Contributor Author

Not sure what you mean, Andrew, The class that defines is letterboxed() is a helper class. It's not a snippet that's pulled from GitHub. But that class is added by this PR. There was just a method before.

@asolovay
Copy link
Contributor

asolovay commented Aug 6, 2025

What I mean is, in the latest commit, you've changed assertFalse(isLetterboxed(activity)); to assertFalse(activity.isLetterboxed(activity));. That line of code is shown on the dac page, it's in the android_device_compatibility_mode_assert_isLetterboxed_java region. So I just wanted to confirm that you don't need to make some other corresponding change to the android_device_compatibility_mode_isLetterboxed_java code snippet, which tells the reader how to define their isLetterboxed() method.

But since you have an SME reviewing this CL, I'll let the two of you work that out!

@JonEckenrode
Copy link
Contributor Author

JonEckenrode commented Aug 6, 2025

I see what you mean now. Good point. The android_device_compatibility_mode_isLetterboxed_java snippet is good as is. But that snippet is why the assert snippet uses AppCompatActivity. Thanks for your thoroughness.

…lityModeTestJavaSnippets.java

Co-authored-by: Alex Vanyo <[email protected]>
@JonEckenrode JonEckenrode requested a review from alexvanyo August 7, 2025 22:22
…lityModeTestJavaSnippets.java

Co-authored-by: Alex Vanyo <[email protected]>
@JonEckenrode JonEckenrode requested a review from alexvanyo August 8, 2025 17:33
@JonEckenrode JonEckenrode merged commit f643874 into main Aug 8, 2025
5 checks passed
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