Skip to content

Conversation

@pepone
Copy link
Member

@pepone pepone commented Jun 5, 2025

Fix #515

@pepone pepone requested a review from Copilot June 5, 2025 17:13

This comment was marked as outdated.

@pepone pepone marked this pull request as ready for review June 6, 2025 09:53
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces an Android demo for the Ice Greeter application. It adds the necessary Slice definitions, resource files (layouts, themes, drawables, and XML configurations), application logic in Kotlin, and Gradle build configuration to support the demo.

  • Added a Slice file defining the Greeter interface
  • Created resource files and layouts for the Android UI
  • Implemented MainActivity and application initialization to integrate with the Ice framework

Reviewed Changes

Copilot reviewed 38 out of 38 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
java/Ice/greeter-android/app/src/main/slice/Greeter.ice Added Slice definition for the Greeter interface
java/Ice/greeter-android/app/src/main/res/xml/data_extraction_rules.xml Added sample data extraction rules for Android backup configuration
java/Ice/greeter-android/app/src/main/res/xml/backup_rules.xml Added sample backup rules file
java/Ice/greeter-android/app/src/main/res/values/themes.xml Defined light theme styling
java/Ice/greeter-android/app/src/main/res/values/strings.xml Added app name string resource
java/Ice/greeter-android/app/src/main/res/values/colors.xml Defined color resources
java/Ice/greeter-android/app/src/main/res/values-night/themes.xml Defined dark theme styling
java/Ice/greeter-android/app/src/main/res/mipmap-anydpi/ic_launcher_round.xml, ic_launcher.xml Added launcher icon vector assets
java/Ice/greeter-android/app/src/main/res/layout/activity_main.xml Created the main activity layout for user input and greeter response
java/Ice/greeter-android/app/src/main/res/drawable/ic_launcher_foreground.xml, ic_launcher_background.xml Added vector drawable assets for the launcher icons
java/Ice/greeter-android/app/src/main/java/com/example/ice/androidgreeter/MainActivity.kt Implemented the main activity with UI interaction and Ice proxy calls
java/Ice/greeter-android/app/src/main/java/com/example/ice/androidgreeter/App.kt Added Application class to initialize and provide the Ice Communicator
java/Ice/greeter-android/app/src/main/AndroidManifest.xml Configured application manifest with permissions, theme, and activity declarations
java/Ice/greeter-android/app/proguard-rules.pro Included ProGuard rules for build optimization
java/Ice/greeter-android/app/build.gradle.kts Set up Gradle build configuration including dependencies and plugins
java/Ice/greeter-android/app/.gitignore, java/Ice/greeter-android/.gitignore Specified files to be ignored by version control

android:layout_width="match_parent"
android:layout_height="wrap_content"
android:text="10.0.2.2"
android:inputType="textUri" />
Copy link

Copilot AI Jun 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Consider using a more appropriate inputType for IP addresses. For example, using 'numberDecimal' or a plain 'text' input may better suit IP address input.

Suggested change
android:inputType="textUri" />
android:inputType="numberDecimal" />

Copilot uses AI. Check for mistakes.
alias(libs.plugins.android.application)
alias(libs.plugins.kotlin.android)
// Apply the Slice-tools plugin to enable Slice compilation.
id("com.zeroc.ice.slice-tools") version "3.8.+"
Copy link

Copilot AI Jun 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a dynamic version pattern ("3.8.+") may lead to non-deterministic builds. Consider specifying a fixed version number for reproducible builds.

Suggested change
id("com.zeroc.ice.slice-tools") version "3.8.+"
id("com.zeroc.ice.slice-tools") version "3.8.5"

Copilot uses AI. Check for mistakes.
android:text=""
android:textAppearance="?android:attr/textAppearanceMedium"
android:padding="16dp"
android:textColor="@android:color/black"
Copy link

Copilot AI Jun 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a hard-coded text color may lead to accessibility issues in dark mode. Consider referencing a theme attribute to ensure proper contrast in different modes.

Suggested change
android:textColor="@android:color/black"
android:textColor="?android:attr/textColorPrimary"

Copilot uses AI. Check for mistakes.
@@ -0,0 +1,38 @@
// Copyright (c) ZeroC, Inc.

package com.example.ice.androidgreeter
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice for the demo name and package name to be consistent: either greeter android or android greeter, not demo=greeter-android and package=androidgreeter.

I prefer "Android Greeter".

@@ -0,0 +1,83 @@
package com.example.ice.androidgreeter
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing copyright line

// The proxy is recreated for each request to ensure the latest IP address is used.
val greeter = GreeterPrx.createProxy(communicator, "greeter:tcp -h $ip -p 4061")

// Send the greetAsync request on a background thread and wait for the response
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct?

The Ice async API does not block the calling thread so it should not be necessary to send this request "on a background thread".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Android you can not start any Network IO in the UI thread, this includes Asynchronous Network IO.

See https://developer.android.com/reference/android/os/NetworkOnMainThreadException

In 3.7 we worked around that with the Background IO executor but it was removed in 3.8 (We should mention in the CHANGELOG).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@externl externl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I didn't test in a simulator. What does the UX look like?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this our standard .gitignore?

* through [getCommunicator]. If initialization fails, the application will crash immediately.
*/
class App : Application() {
private var communicator: Communicator? = null
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this communicator destroyed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is not, application remains in the background until is killed.

The Android app life cycle is a bit complex I think it is not worth to enter into these details for a simple greeter.
https://developer.android.com/guide/components/activities/activity-lifecycle

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pointed to the wrong doc, the communicator lives in the application and is independently of the activity life cycle. And activity can be destroyed because the UI change orientation, but this shouldn't trigger a communicator destroy.

There is Application.onTerminate but is only useful for emulators

https://developer.android.com/reference/android/app/Application#onTerminate()

val greetButton = findViewById<Button>(R.id.greetButton)
val greetResponse = findViewById<TextView>(R.id.greetResponse)

// Retrieve the Ice communicator from the application class
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In all other demos, we end most comments with a period.

@pepone pepone merged commit c823eb7 into zeroc-ice:main Jun 10, 2025
10 of 11 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.

Create Android Version of Ice/greeter

3 participants