build: check API surface for Android suitability - #83
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
2ac1b3a to
a64955a
Compare
a64955a to
2f339b1
Compare
Closes #84. UIText's clipboard access was hardwired to java.awt, which #83 could only make crash-safe (LinkageError caught, degrades to a no-op) on platforms without it - not actually give a working clipboard there. This makes the implementation swappable instead: - New ClipboardProvider interface in nui-input's org.terasology.input.device package, alongside the existing KeyboardDevice/MouseDevice device abstractions this mirrors. - AwtClipboardProvider (nui, org.terasology.nui.util) - the extracted, unchanged java.awt implementation, now the default rather than hardcoded. - UIText.setClipboardProvider(ClipboardProvider) lets an application swap the default at startup; getClipboardContents()/setClipboardContents() delegate to whatever's currently set, still catching LinkageError as a defensive fallback regardless of which implementation is active. - LibGDXClipboardProvider (nui-libgdx) wraps libGDX's own cross-platform Gdx.app.getClipboard() - backed by java.awt on desktop, Android's ClipboardManager on Android, and the browser clipboard on GWT/HTML, so it actually works everywhere nui-libgdx runs (which is everywhere DestinationSol runs). Applications call UIText.setClipboardProvider(new LibGDXClipboardProvider()) during startup to opt in. No behavior change for existing desktop consumers that don't call setClipboardProvider - AwtClipboardProvider is still the default. Verified: ./gradlew build succeeds, zero unignored AnimalSniffer violations.
Closes #84. UIText's clipboard access was hardwired to java.awt, which #83 could only make crash-safe (LinkageError caught, degrades to a no-op) on platforms without it - not actually give a working clipboard there. This makes the implementation swappable instead: - New ClipboardProvider interface in nui-input's org.terasology.input.device package, alongside the existing KeyboardDevice/MouseDevice device abstractions this mirrors. - AwtClipboardProvider (nui, org.terasology.nui.util) - the extracted, unchanged java.awt implementation, now the default rather than hardcoded. - UIText.setClipboardProvider(ClipboardProvider) lets an application swap the default at startup; getClipboardContents()/setClipboardContents() delegate to whatever's currently set, still catching LinkageError as a defensive fallback regardless of which implementation is active. - LibGDXClipboardProvider (nui-libgdx) wraps libGDX's own cross-platform Gdx.app.getClipboard() - backed by java.awt on desktop, Android's ClipboardManager on Android, and the browser clipboard on GWT/HTML, so it actually works everywhere nui-libgdx runs (which is everywhere DestinationSol runs). Applications call UIText.setClipboardProvider(new LibGDXClipboardProvider()) during startup to opt in. No behavior change for existing desktop consumers that don't call setClipboardProvider - AwtClipboardProvider is still the default. Verified: ./gradlew build succeeds, zero unignored AnimalSniffer violations.
Closes #84. UIText's clipboard access was hardwired to java.awt, which #83 could only make crash-safe (LinkageError caught, degrades to a no-op) on platforms without it - not actually give a working clipboard there. This makes the implementation swappable instead: - New ClipboardProvider interface in nui-input's org.terasology.input.device package, alongside the existing KeyboardDevice/MouseDevice device abstractions this mirrors. - AwtClipboardProvider (nui, org.terasology.nui.util) - the extracted, unchanged java.awt implementation, now the default rather than hardcoded. - UIText.setClipboardProvider(ClipboardProvider) lets an application swap the default at startup; getClipboardContents()/setClipboardContents() delegate to whatever's currently set, still catching LinkageError as a defensive fallback regardless of which implementation is active. - LibGDXClipboardProvider (nui-libgdx) wraps libGDX's own cross-platform Gdx.app.getClipboard() - backed by java.awt on desktop, Android's ClipboardManager on Android, and the browser clipboard on GWT/HTML, so it actually works everywhere nui-libgdx runs (which is everywhere DestinationSol runs). Applications call UIText.setClipboardProvider(new LibGDXClipboardProvider()) during startup to opt in. No behavior change for existing desktop consumers that don't call setClipboardProvider - AwtClipboardProvider is still the default. Verified: ./gradlew build succeeds, zero unignored AnimalSniffer violations.
|
Since NUI does not explicitly target Android, even though it is used on that platform by Destination Sol, I am not sure if this is an appropriate change. Depending on the Android Gradle Plugin for validation seems excessive and undermines the perspective of NUI as a platform-independent library. |
2d1f50c to
006dfd7
Compare
thank you very much for the feedback @BenjaminAmos ! corrected now, plugin only switched on if android sdk installed and desired. |
|
I would prefer that the |
006dfd7 to
f7e546b
Compare
f7e546b to
c7ea812
Compare
|
@BenjaminAmos agreed, done - dropped |
Closes #84. UIText's clipboard access was hardwired to java.awt, which #83 could only make crash-safe (LinkageError caught, degrades to a no-op) on platforms without it - not actually give a working clipboard there. This makes the implementation swappable instead: - New ClipboardProvider interface in nui-input's org.terasology.input.device package, alongside the existing KeyboardDevice/MouseDevice device abstractions this mirrors. - AwtClipboardProvider (nui, org.terasology.nui.util) - the extracted, unchanged java.awt implementation, now the default rather than hardcoded. - UIText.setClipboardProvider(ClipboardProvider) lets an application swap the default at startup; getClipboardContents()/setClipboardContents() delegate to whatever's currently set, still catching LinkageError as a defensive fallback regardless of which implementation is active. - LibGDXClipboardProvider (nui-libgdx) wraps libGDX's own cross-platform Gdx.app.getClipboard() - backed by java.awt on desktop, Android's ClipboardManager on Android, and the browser clipboard on GWT/HTML, so it actually works everywhere nui-libgdx runs (which is everywhere DestinationSol runs). Applications call UIText.setClipboardProvider(new LibGDXClipboardProvider()) during startup to opt in. No behavior change for existing desktop consumers that don't call setClipboardProvider - AwtClipboardProvider is still the default. Verified: ./gradlew build succeeds, zero unignored AnimalSniffer violations.
Same Android-suitability strategy as gestalt's
gestalt-library-common.gradle.kts: applies the AnimalSniffer plugin plus thegummy-bears-api-24signature set to every subproject, checking NUI's API surface against what's actually available on Android.Why this matters concretely: DestinationSol's
enginemodule already depends onnui,nui-libgdx,nui-gestalt, andnui-reflectdirectly, and DestinationSol targets Android via libGDX — so NUI's API surface needs to actually work there.build.gradle: buildscript classpath for the Android Gradle Plugin and the AnimalSniffer Gradle plugin itself.common.gradleis a plain "apply from" script, so it can't declare plugins via aplugins {}block the way gestalt's precompiled build-logic convention plugin can — the classicapply plugin: 'ru.vyarus.animalsniffer'form needs the classpath supplied from the root project instead.gradle/common.gradle: applies the plugin, adds the signature dependency.Unlike gestalt, NUI wasn't Android-clean — the check initially surfaced 21 real violations across two modules:
ReflectionUtil(6) andObjectLayoutBuilder(2): genuinely fixable by calling through the concreteMethod/Constructortype instead of the unavailableExecutable/Parametertypes — no public API change, no behavior change.UIText's clipboard handling (13):java.awt.Toolkit/Clipboard/DataFlavor/StringSelectiongenuinely don't exist on Android at all — not fixable by calling a different method. Isolated into a private nested class withLinkageErrorcaught by the caller, so copy/paste degrades to a no-op with a logged warning instead of crashing.animalsniffer.ignoreexcludesjava.awt.Toolkit/java.awt.datatransfer.*explicitly for this one real, permanent, documented exception.animalsniffer.ignoreFailuresis gone — the check is fully fatal, zero unignored violations.Beyond the API-signature check: also adds a minimal, non-published
com.android.applicationmodule (nui-android-verify, only included when a local Android SDK is configured) that actually depends onnui/nui-libgdxand references real classes from both — because the AnimalSniffer check only proves "no referencedjava.*symbol is missing from Android," not that the compiled classes actually survive D8 desugaring/dexing/APK assembly. Running it found one more real, previously invisible problem: Android'scheckDuplicateClassestask (Gradle's own resolution has no equivalent — it picks one version per coordinate, not per class) failed on two pre-existing conflicts in the transitive dependency graph:com.google.code.findbugs:jsr305vscom.google.code.findbugs:annotations(both declarejavax.annotation.*) — excluded innui-android-verify; any real Android app depending onnui-libgdxwould hit the same conflict and need the same exclusion.org.reflections:reflections(declared directly bynui/nui-reflect/nui-gestalt5/nui-gestalt7) vs theorg.terasology:reflectionsforkgestalt-modulealready pulls in transitively — fixed at the source: all four now consistently depend on the fork. It's API-identical to upstream (confirmed by diffing its one commit againstronmamo/reflections— it only drops thejavax.servlet/slf4j-simpleoptional deps upstream still carries), so this is a zero-behavior-change fix. Upgrading to current upstream0.10.2instead was considered and rejected — it removesStore.getAll()and changesSubTypesScanner's defaultjava.lang.Object-exclusion behavior, whichTypeRegistryexplicitly relies on (new SubTypesScanner(false)); not a safe mechanical swap.Test plan:
./gradlew build(including tests) succeeds with the AnimalSniffer check active and fully fatal./gradlew :nui-android-verify:assembleDebugsucceeds end-to-end — real unsigned debug APK produceddexdump/string search thatUITextandLibGDXKeyboardDeviceare genuinely present in the resultingclasses*.dex, not just declared as an unused dependency./gradlew build(nolocal.properties) is unaffectedStacked on #82 (Gradle 9.7.1 + Java 17 bump) — AGP 9.3.1 doesn't work under the old 8.2.1 wrapper.
Partially addresses #84 — the real working clipboard for non-AWT platforms is #85, stacked on top of this one.