feat: add notification for inventory key binding - #50
Conversation
|
@jdrueckert this can be merged? |
That PR is three years old and in draft mode so I don't think so 😅 |
ea6b783 to
44c4a23
Compare
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 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 |
|
Rebased onto current
Happy to have someone else look it over before merge. |
44c4a23 to
40d8b4b
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Resolve the icon dependency and key-formatting issues, and add focused tests.
Review effort: Lite
Findings: 1
What changed in this PR
Adds an empty-inventory notification displaying the configured inventory key and dismissing when inventory opens.
Changes:
- Adds notification lifecycle handling.
- Formats and displays the inventory key binding.
- Adds the Notifications module dependency.
| File | Summary |
|---|---|
src/main/java/org/terasology/module/inventory/systems/InventoryUIClientSystem.java |
Implements notification display, dismissal, and key-binding formatting. |
module.txt |
Declares the Notifications dependency. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Rebuilt on current develop rather than rebased — the original branch's diff was mostly stale package-rename noise plus one file (InventoryClientSystem.java) with zero real content changes.
- Shows an "Empty Pockets" notification via the Notifications module when the player spawns with an empty inventory; dismissed when they open the inventory.
- Dropped a dead addHUDElement("Inventory:EmptyPocketsNagWidget") call — no such UI asset exists; the Notifications module's shared overlay already renders ShowNotificationEvent without it.
- Added the empty-inventory check in onLocalPlayerInitialized so the notification only shows when the inventory is actually empty.
- getActivationKey: only convert a bound key's display name to a circled letter when it's actually A-Z; a digit or symbol key now renders as-is instead of an unrelated circled Unicode character (review feedback from Copilot).
- module.txt: added Notifications (required) and ModuleTestingEnvironment (optional) dependencies, with version ranges matching what's actually published in Artifactory.
Original feature by jdrueckert, branch feat/add-notification-for-inventory-key.
Co-Authored-By: soloturn <soloturn@gmail.com>
40d8b4b to
648b141
Compare
soloturn
left a comment
soloturn
left a comment
There was a problem hiding this comment.
Rebuilt against current develop, verified with :modules:Inventory:compileJava (BUILD SUCCESSFUL), and fixed the Copilot finding on getActivationKey (non-letter keys were being mangled into unrelated circled Unicode characters — now only A-Z gets that treatment).
No description provided.