-
Notifications
You must be signed in to change notification settings - Fork 100
Improved supports for in-app/game notifications and toasts #587
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
base: main
Are you sure you want to change the base?
Conversation
36ab19c to
dd22210
Compare
|
Demo from #588: Minicraft.Plus.2023-12-31.02-24-16.-.Trim.mp4 |
|
In-app notifications are usually a result of bad game design. Using this will make the game feel more like a cheap mobile game than an free and open-source sandbox game. Perhaps an icon can be showed insted when a controller is plugged in? |
# Conflicts: # src/client/java/minicraft/item/FishingRodItem.java # src/client/java/minicraft/item/SummonItem.java # src/client/java/minicraft/screen/AchievementsDisplay.java
|
New design for toast notifications Minicraft.Plus.2024-02-09.11-48-17.mp4 |
|
App status top bar (shows automatically for 2 seconds on startup and shown when "9" ( Minicraft.Plus.2024-02-09.17-32-31.mp4 |
|
The new achievement unlocking toast, replacing the original text-only notification. Minicraft.Plus.2024-02-09.19-37-49.mp4 |
|
Updates has been made for this pull request regarding the issues and it is now ready for review again. |
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 changes look great!
Although, app toast and game toast should be merged to just be game toast. Having two is unneccecary.
|
Ok, it is removed. |
|
Can you update this please. |
# Conflicts: # src/client/java/minicraft/core/Renderer.java # src/client/java/minicraft/screen/TutorialDisplayHandler.java
|
Okay, branch updated. |
# Conflicts: # src/client/java/minicraft/entity/furniture/DeathChest.java
|
This is a good addition to the game, but I still think that app toasts and game toasts should be merged. I don't see how they are substantially different. Also, are you still working on this? |
|
I could still fix minor things for my pull requests on this repository, or else they could be closed. |
|
I think at least this PR should be merged |
Makkkkus
left a comment
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.
I still have some problems I wish you would adress.
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 in-app notifications by introducing a new toast system with configurable positioning and upgraded visual styles, and it replaces the legacy notifications with in-game notifications.
- Introduces AppToast with several frame implementations and configurable top/bottom positioning.
- Implements AchievementUnlockToast for achievement notifications, replacing text-only alerts.
- Updates various game logic files to use inGameNotifications and integrates toast management in the core update and render flows.
Reviewed Changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/client/java/minicraft/screen/AppToast.java | New toast implementation supporting multiple frame styles and configurable positioning. |
| src/client/java/minicraft/screen/AchievementsDisplay.java | Added AchievementUnlockToast and updated achievement notification logic. |
| src/client/java/minicraft/level/tile/*.java | Replaced legacy notifications with inGameNotifications across wall, boss, and rock tiles. |
| src/client/java/minicraft/item/*.java | Updated notification calls in item interactions to use inGameNotifications. |
| src/client/java/minicraft/core/Updater.java, Renderer.java, Game.java | Integrated toast management and new in-game notifications with an app status bar. |
| protected int tick = 0; | ||
| protected int animationTick = 0; // 0 to ANIMATION_TIME, ANIMATION_TIME to 0 | ||
|
|
||
| protected static abstract class ToastFrame { |
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.
Please remove the ToastFrame class as it is a single-variable class with no functionality that cannot be done in the Toast class. There is no need to create classes just for the fun of it.
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.
Notice
|
|
||
| public static final AppStatusBar appStatusBar = new AppStatusBar(); | ||
|
|
||
| public static class AppStatusBar { |
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.
Perhaps move the AppStatusBar and it's implementations into it's own file?
And variables are usually declared at the top of the class.
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.
I am not so sure about this since it is only used in Renderer and (ticked by) Updater.
| this.status = status; | ||
| } | ||
| } | ||
| } |
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.
I think AppStatusBar, AppStatusElement and HardwareAccelerationElementStatus are similar enough to to be merged as a single class (similar to how the holding item bar at the bottom of the screen works).
Looking at how HARDWARE_ACCELERATION_STATUS is declared on the parent of the object it references, it would make more sense to create a struct with the data you want each icon to have, and then create instances of that struct when adding a new icon.
Blinking and all of that can be implemented in AppStatusBar, negating the need to call any other class. And System.getProperty("sun.java2d.opengl") doesn't necessarily need to be cached.
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.
I am not sure why you meant they are similar. Anyway, looking at this implementation is not sufficient. As an example, #588 added 2 new icons with more complex implementation details than this one.
Blinking is meaningful only for a particular one icon, thus not responsible for AppStatusBar to implement.
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.
I'm proposing to move all functionality of this into the AppStatusBar class. Bringing in other pull requests into this is irrelevant.
Anything can be implemented in AppStatusBar, it's just a question if you are willing to cooperate and do such. Look at how other similar features are implemented (that you haven't designed).
| protected int tick = 0; | ||
| protected int animationTick = 0; // 0 to ANIMATION_TIME, ANIMATION_TIME to 0 | ||
|
|
||
| protected static abstract class ToastFrame { |
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.
Notice
353e26e to
5fce00e
Compare
| } | ||
|
|
||
| public abstract class AppStatusElement { | ||
| public enum GeneralStatus { |
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 can be a boolean.
Makkkkus
left a comment
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.
I'm very dissapointed with these changes.
|
|
||
| @MagicConstant(intValues = {ACCELERATION_ON, ACCELERATION_OFF}) | ||
| private int status = ACCELERATION_ON; | ||
| public class HardwareAccelerationElementStatus extends AppStatusElement<GeneralStatus> { |
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.
Please remove this class.
| OFF, ON | ||
| } | ||
|
|
||
| public abstract class AppStatusElement<E extends Enum<E>> { |
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.
Please remove this class. It serves no purpose.
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.
Please check further implementation than just a single usage with a minimal usage pattern. This API is designed to be extensible.
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.
We don't know if this will be extended yet. Minimal implementation is fine for most uses.
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.
Still, this is a part of this API design.
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.
If this will not be extended, it is whole pointless to add this API; I would rather remove this feature from this pull request.
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.
I agree
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.
Removed App Status Bar; #588 will be broken.
| * Game Toasts are rendered only in gameplay and always located in the top-left corner without margin. | ||
| * The content of game toasts is implementation-specific. | ||
| */ | ||
| public abstract class GameToast extends Toast { |
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.
Why did you add this class??
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.
To constrain and categorize to have only game toasts and app toasts more exactly. In design, it is not expected to see app toasts being in the list of game toasts, and game toasts are expected to behave similarly, different from app toasts in certain aspects.
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.
We already discussed this. You removed it. And now you're adding it back? Adding it is just a way of writing more code that does less. Adding code for the sake of adding code is pointless.
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.
I have not removed it, but ignored it from the implementation to simplify the structure (in fact it should be specialized for the API). Also, adding design constrains preventing unexpected behaviors is an important thing for designing API.
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.
Good this is a game and not an API then
This feature is reserved for future use. This adds a support for in-app notifications, where toasts in the screen. It is different from in-game notifications, bare text only, as this includes a frame, and the color can be changed. This is important when there are some alerts, warnings and important information needed to notify the users.
I have several ideas about box popups and notifications: (fading is not supported right now)
Position for toasts may be configurable to upper or lower positions (also for each type of toast notification?).
Additionally, achievement unlocking toast is added to replace the text-only notifications regarding unlocking achievements. (https://discord.com/channels/280723930942013440/1141498209634623539)
This improves visual quality of life.