Skip to content

Conversation

@InsertCreativityHere
Copy link
Member

@InsertCreativityHere InsertCreativityHere commented Oct 17, 2025

This PR won't work until the slice-tools snapshots pulls in this fix tonight: zeroc-ice/ice#4561

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 adds a new Glacier2/session demo to the Java demos directory. The demo illustrates how to implement and configure a custom Glacier2 Session Manager and demonstrates the use of default servants.

Key changes:

  • Added a complete Java implementation of the Glacier2/session demo with client and server components
  • Updated the Java README.md to include entries for the new Glacier2 Session demo and corrected the table formatting
  • Fixed minor documentation inconsistencies in the C# version of the same demo

Reviewed Changes

Copilot reviewed 21 out of 23 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
java/README.md Added table entries for the new Glacier2/Session demo and fixed table alignment
java/Glacier2/session/slice/PokeBox.ice Defined Slice interfaces for the Pokemon application (PokeBox and PokeSession)
java/Glacier2/session/settings.gradle.kts Configured Gradle settings with Maven snapshot repositories for nightly builds
java/Glacier2/session/server/src/main/java/com/example/glacier2/session/server/* Implemented server-side components including SessionManager, SharedPokeBox, and storage interfaces
java/Glacier2/session/server/build.gradle.kts Configured server build with Ice and Glacier2 dependencies
java/Glacier2/session/gradlew.bat Added Gradle wrapper script for Windows
java/Glacier2/session/gradlew Added Gradle wrapper script for Unix-like systems
java/Glacier2/session/gradle/wrapper/gradle-wrapper.properties Configured Gradle wrapper to use version 8.12.1
java/Glacier2/session/config.glacier2 Configured Glacier2 router settings including endpoints and session manager
java/Glacier2/session/client/src/main/java/com/example/glacier2/session/client/Client.java Implemented client application demonstrating session creation and Pokemon management
java/Glacier2/session/client/build.gradle.kts Configured client build with Ice and Glacier2 dependencies
java/Glacier2/session/README.md Added comprehensive documentation for the demo including build and run instructions
java/Glacier2/session/.gitignore Added standard Gradle ignore patterns
csharp/Glacier2/Session/Server/IPokeStore.cs Fixed documentation wording from "for" to "of"
csharp/Glacier2/Session/Client/Program.cs Corrected comment to specify "Glacier2 router" instead of "Glacier router"

@InsertCreativityHere InsertCreativityHere marked this pull request as ready for review October 20, 2025 15:35
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

Copilot reviewed 21 out of 23 changed files in this pull request and generated 1 comment.

class InMemoryPokeStore implements PokeStore {
private final Map<String, List<String>> _store = new HashMap<>();

public void saveCollection(String userId, List<String> pokemon) {
Copy link
Member

Choose a reason for hiding this comment

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

Missing @Override in this file?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

{
/// <summary>Saves the Pokémon collection for a specific user.</summary>
/// <param name="userId">The user ID.</param>
/// <param name="userId">The user's ID.</param>
Copy link
Member

@bernardnormier bernardnormier Oct 20, 2025

Choose a reason for hiding this comment

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

I would keep user ID. It's a common term, much more common than user's ID.

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 put it back how it was. The apostrophe made it less grammatically ambiguous though.

/**
* A proxy to the SessionControl object hosted by the Glacier2 router.
* This proxy allows us to control the Glacier2 session, in particular to destroy it.
* In this demo, that's the only per-session state maintained by {@code DefaultPokeSession}.
Copy link
Member

Choose a reason for hiding this comment

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

Should this be @link instead of @code?

Copy link
Member

Choose a reason for hiding this comment

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

Or just maintained by this class. This links to self are not very useful.

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 agree 'this class' is simpler.

import java.util.Map;

/**
* {@code SessionManager} is an Ice servant that implements Slice interface 'Glacier2::SessionManager'.
Copy link
Member

Choose a reason for hiding this comment

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

I'd remove the @code.

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's a Java convention that when you use a class' name in it's own description, it should use code formatting.
We don't follow that convention very well, but that doesn't mean now is a bad time to start.

* @return the saved Pokémon collection, or {@code null} if no collection was saved for {@code userId}.
* The returned list is immutable. Attempting to modify it will throw an UnsupportedOperationException.
*/
List<String> retrieveCollection(String userId);
Copy link
Member

Choose a reason for hiding this comment

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

I guess this a shortcoming of the JDK that doesn't provide an immutable list interface. Here we are documenting the expected behavior of the implementation but we cannot enforce it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's definitely a little lame. I spent some time looking into if there's any kind of enforcement, but there short of passing the internal List wrapper type, there just isn't.

Even worse, when you see a List, you don't even know if its safe to mutate or not. Not a very elegant design.


// Retrieve the Pokémon collection for the user associated with the current session.
@Override
public String[] getInventory(Current current) {
Copy link
Member

Choose a reason for hiding this comment

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

What about mapping the collection to a List in the Slice file and avoid this conversion.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that removed the distracting conversion code. Very nice!

public void caught(String[] pokemon, Current current) {
String userId = getUserId(current);

List<String> newPokemon = new ArrayList<>(Arrays.asList(pokemon));
Copy link
Member

Choose a reason for hiding this comment

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

Here too, we can map the input parameter to a List.

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

Copilot reviewed 21 out of 23 changed files in this pull request and generated no new comments.

@InsertCreativityHere InsertCreativityHere merged commit 8584f68 into zeroc-ice:main Oct 22, 2025
11 of 16 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.

4 participants