-
-
Notifications
You must be signed in to change notification settings - Fork 0
Enhance Collection GUI to display parcel items and delivery information #153
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
Conversation
Summary of ChangesHello @Jakubk15, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on significantly improving the user experience within the parcel management system's graphical user interface. By integrating and displaying crucial information such as the contents of a parcel and its delivery status (including estimated arrival times or completion dates), users gain a much clearer and more comprehensive overview of their parcels. This enhancement involves both front-end GUI modifications and back-end service integration, alongside new configuration options for greater customization. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request enhances the parcel collection GUI by displaying parcel contents and delivery information. The changes are well-implemented, leveraging asynchronous data fetching to maintain server performance. New configuration options have been added to support these UI enhancements.
My review includes a few suggestions to improve the code's maintainability and correctness. I've pointed out an issue with using the system's default timezone which could lead to inconsistencies, some dead code that should be removed, and a FIXME comment that should be moved to an issue tracker. I've also suggested a small refactoring to improve code clarity.
| private CompletableFuture<GuiItem> createParcelItemAsync(Parcel parcel, ConfigItem item) { | ||
| CompletableFuture<List<String>> loreFuture = PlaceholderUtil.replaceParcelPlaceholdersAsync( | ||
| parcel, item.lore(), this.guiManager | ||
| ); | ||
| CompletableFuture<Optional<Delivery>> deliveryFuture = this.guiManager.getDelivery(parcel.uuid()); | ||
|
|
||
| return loreFuture.thenCombine(deliveryFuture, (processedLore, deliveryOptional) -> { | ||
| PaperItemBuilder parcelItem = item.toBuilder(); | ||
|
|
||
| List<String> loreWithArrival = new ArrayList<>(processedLore); | ||
|
|
||
| if (deliveryOptional.isPresent()) { | ||
| Delivery delivery = deliveryOptional.get(); | ||
| Instant arrivalTime = delivery.deliveryTimestamp(); | ||
| Instant now = Instant.now(); | ||
|
|
||
| if (arrivalTime.isAfter(now)) { | ||
| Duration remaining = Duration.between(now, arrivalTime); | ||
| String formattedDuration = DurationUtil.format(remaining); | ||
| String formattedDate = DATE_FORMATTER.format(arrivalTime.atZone(ZoneId.systemDefault())); | ||
|
|
||
| String arrivingLine = this.guiSettings.parcelArrivingLine | ||
| .replace("{DURATION}", formattedDuration) | ||
| .replace("{DATE}", formattedDate); | ||
|
|
||
| loreWithArrival.add(arrivingLine); | ||
| } else if (arrivalTime.isBefore(now)) { // not supported rn, because deliveries are deleted on arrival, so the if is always false | ||
| String arrivedLine = this.guiSettings.parcelArrivedLine | ||
| .replace("{DATE}", DATE_FORMATTER.format(arrivalTime.atZone(ZoneId.systemDefault()))); | ||
| loreWithArrival.add(arrivedLine); | ||
| } | ||
| } | ||
|
|
||
| List<Component> newLore = loreWithArrival.stream() | ||
| .map(line -> resetItalic(this.miniMessage.deserialize(line))) | ||
| .toList(); | ||
|
|
||
| parcelItem.lore(newLore); | ||
| parcelItem.name(resetItalic(this.miniMessage.deserialize(item.name().replace("{NAME}", parcel.name())))); | ||
|
|
||
| return parcelItem.asGuiItem(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method has a couple of issues that should be addressed:
-
Inconsistent Timezone: The use of
ZoneId.systemDefault()on lines 141 and 150 can lead to inconsistent date and time formatting across different server environments. It's better to use a fixed timezone like UTC (ZoneId.of("UTC")) or make it configurable inPluginConfig.javato ensure all players see consistent times. -
Dead Code: The
else ifblock on lines 148-152 is commented as unreachable code. This dead code should be removed to avoid confusion for future maintainers. If it's for a future feature, it should be added when that feature is implemented.
src/main/java/com/eternalcode/parcellockers/command/debug/DebugCommand.java
Outdated
Show resolved
Hide resolved
| CompletableFuture<List<String>> loreFuture = PlaceholderUtil.replaceParcelPlaceholdersAsync(parcel, parcelItem.lore(), this.guiManager); | ||
| CompletableFuture<List<ItemStack>> contentFuture = this.guiManager.getParcelContent(parcel.uuid()) | ||
| .thenApply(optional -> optional.map(content -> content.items()).orElse(List.of())); | ||
|
|
||
| return loreFuture.thenCombine(contentFuture, (processedLore, items) -> () -> { | ||
| ConfigItem item = parcelItem.clone(); | ||
| item.name(item.name().replace("{NAME}", parcel.name())); | ||
|
|
||
| List<String> loreWithItems = new ArrayList<>(processedLore); | ||
| if (!items.isEmpty()) { | ||
| loreWithItems.add(this.guiSettings.parcelItemsCollectionGui); | ||
| for (ItemStack itemStack : items) { | ||
| loreWithItems.add(this.guiSettings.parcelItemCollectionFormat | ||
| .replace("{ITEM}", MaterialUtil.format(itemStack.getType())) | ||
| .replace("{AMOUNT}", Integer.toString(itemStack.getAmount())) | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| return PlaceholderUtil.replaceParcelPlaceholdersAsync(parcel, parcelItem.lore(), this.guiManager) | ||
| .thenApply(processedLore -> () -> { | ||
| ConfigItem item = parcelItem.clone(); | ||
| item.name(item.name().replace("{NAME}", parcel.name())); | ||
| item.lore(processedLore); | ||
| item.glow(true); | ||
| item.lore(loreWithItems); | ||
| item.glow(true); | ||
|
|
||
| return item.toGuiItem(event -> { | ||
| this.guiManager.collectParcel(player, parcel); | ||
| refresher.removeItemBySlot(event.getSlot()); | ||
| }); | ||
| return item.toGuiItem(event -> { | ||
| this.guiManager.collectParcel(player, parcel); | ||
| refresher.removeItemBySlot(event.getSlot()); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The createParcelItemAsync method has grown in complexity. To improve readability and maintainability, consider these refactorings:
- On line 121, use a method reference for conciseness:
optional.map(ParcelContent::items). - Extract the logic for adding item details to the lore (lines 127-136) into a separate private helper method. This will make
createParcelItemAsyncshorter and more focused.
Here's an example of how you could refactor:
private CompletableFuture<Supplier<GuiItem>> createParcelItemAsync(
Parcel parcel,
ConfigItem parcelItem,
Player player,
PaginatedGuiRefresher refresher
) {
CompletableFuture<List<String>> loreFuture = PlaceholderUtil.replaceParcelPlaceholdersAsync(parcel, parcelItem.lore(), this.guiManager);
CompletableFuture<List<ItemStack>> contentFuture = this.guiManager.getParcelContent(parcel.uuid())
.thenApply(optional -> optional.map(ParcelContent::items).orElse(List.of()));
return loreFuture.thenCombine(contentFuture, (processedLore, items) -> () -> {
ConfigItem item = parcelItem.clone();
item.name(item.name().replace("{NAME}", parcel.name()));
List<String> loreWithItems = new ArrayList<>(processedLore);
addItemsToLore(loreWithItems, items);
item.lore(loreWithItems);
item.glow(true);
return item.toGuiItem(event -> {
this.guiManager.collectParcel(player, parcel);
refresher.removeItemBySlot(event.getSlot());
});
});
}
private void addItemsToLore(List<String> lore, List<ItemStack> items) {
if (items.isEmpty()) {
return;
}
lore.add(this.guiSettings.parcelItemsCollectionGui);
items.forEach(itemStack -> lore.add(this.guiSettings.parcelItemCollectionFormat
.replace("{ITEM}", MaterialUtil.format(itemStack.getType()))
.replace("{AMOUNT}", String.valueOf(itemStack.getAmount()))
));
}There was a problem hiding this 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 enhances the parcel management system by renaming the IN_PROGRESS status to SENT, adding arrival time and item display functionality to GUIs, and introducing a new utility class for duration formatting.
Key changes:
- Renamed
ParcelStatus.IN_PROGRESStoParcelStatus.SENTfor improved clarity - Enhanced Collection GUI to display parcel items with formatted material names and quantities
- Added delivery information display in Parcel List GUI showing arrival time and remaining duration
- Introduced
DurationUtilfor consistent duration formatting across the application
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/com/eternalcode/parcellockers/parcel/ParcelStatus.java | Renamed enum value from IN_PROGRESS to SENT |
| src/main/java/com/eternalcode/parcellockers/util/DurationUtil.java | New utility class for formatting durations with custom pattern |
| src/main/java/com/eternalcode/parcellockers/util/InventoryUtil.java | Added private constructor to prevent instantiation |
| src/main/java/com/eternalcode/parcellockers/gui/implementation/remote/ParcelListGui.java | Enhanced to fetch and display delivery arrival information with formatted dates and durations |
| src/main/java/com/eternalcode/parcellockers/gui/implementation/locker/CollectionGui.java | Enhanced to fetch and display parcel contents with item names and amounts |
| src/main/java/com/eternalcode/parcellockers/gui/GuiManager.java | Added methods to retrieve parcel content and delivery information |
| src/main/java/com/eternalcode/parcellockers/delivery/DeliveryManager.java | Added get method to retrieve deliveries with cache support |
| src/main/java/com/eternalcode/parcellockers/configuration/implementation/PluginConfig.java | Added configuration for arrival/arrived lines and item display format, changed corner item color |
| src/main/java/com/eternalcode/parcellockers/gui/implementation/locker/SendingGuiState.java | Updated default status to SENT |
| src/test/java/com/eternalcode/parcellockers/database/ParcelRepositoryIntegrationTest.java | Updated test to use ParcelStatus.SENT |
| src/main/java/com/eternalcode/parcellockers/ParcelLockers.java | Updated GuiManager constructor call with new dependencies |
| src/main/java/com/eternalcode/parcellockers/command/debug/DebugCommand.java | Added FIXME comment about using Registry API |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/main/java/com/eternalcode/parcellockers/gui/implementation/remote/ParcelListGui.java
Show resolved
Hide resolved
| public static String format(Duration duration) { | ||
| if (duration.compareTo(ONE_SECOND) < 0) { | ||
| return "0s"; | ||
| } | ||
| return reformat(DURATION.format(duration)); | ||
| } | ||
|
|
||
|
|
||
| private static String reformat(String input) { | ||
| return REFORMAT_PATTERN.matcher(input).replaceAll(REFORMAT_REPLACEMENT).trim(); | ||
| } | ||
| } |
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new DurationUtil class lacks test coverage. Consider adding unit tests to verify the format method behaves correctly for various Duration inputs, including edge cases like zero duration, very short durations (less than 1 second), and durations with multiple time units (days, hours, minutes, seconds).
| public CompletableFuture<Optional<Delivery>> get(UUID parcel) { | ||
| Delivery cached = this.deliveryCache.getIfPresent(parcel); | ||
| if (cached != null) { | ||
| return CompletableFuture.completedFuture(Optional.of(cached)); | ||
| } | ||
| return this.deliveryRepository.fetch(parcel).thenApply(optional -> { | ||
| optional.ifPresent(delivery -> this.deliveryCache.put(parcel, delivery)); | ||
| return optional; | ||
| }); | ||
| } |
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new get method in DeliveryManager lacks test coverage. Consider adding tests to verify correct behavior when fetching deliveries from cache versus the repository, and when deliveries are not found.
…put, fix race condition when parcel would be sent and items would still remain in item storage
…Manager. Use SoundEventKeys instead of legacy Sound references
No description provided.