Skip to content
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

GUI (1.21) #121

Open
wants to merge 5 commits into
base: 1.21.1
Choose a base branch
from
Open

GUI (1.21) #121

wants to merge 5 commits into from

Conversation

xllifi
Copy link

@xllifi xllifi commented Dec 11, 2024

This uses SGui to make GUIs. Someone should probably remake safe command to also use SGui.
The GUI:

  • Fully replaces info, list and members commands
  • Provides a GUI for admin and modify commands (opened if no sub-command is specified).
  • Implements functionality of home (tp only), rank promote, rank demote and kick commands in list GUIs.

I'm not sure if I made a good job with UI. I don't really like some icons (primarily in ModifyGui).

I have compiled a node view of the GUI (click to view) A node view of this pull request's features.

For viewing / editing in Obsidian (unzip contents to the root of vault): factions gui node view.zip

Revert to `9505d81` (last 1.21 commit)
Bump Java
Change Initializaiton message to remove Minecraft version
Add SGui dependency
Add `Icons`
Add `AdminGui` and use it in `AdminCommand`
Add `PagedGui`
Add `ListGui` and use it in `ListCommand`
Split `HomeCommand#go` into `HomeCommand#go` and `HomeCommand#execGo` for use in `ListGui`
Multiple QoL changes (additional button actions, some text tweaks)
Add `ModifyGui` and use it in `ModifyCommand`
- Reformatted my code to comply with Project settings
- Finalized command changes
- Finalized GUI
  - All GUI "buttons" now play click sound
  - Finalized Icon names
  - Removed unused Icons
  - `InfoGui` now allows Owners to open `ModifyGui` directly
  - Added `MemberGui`
  - `ModifyGui` now allows to change MOTD, change color, change type and has a "Go back" button.
  - `PagedGui` doesn't use translation keys any more
Copy link
Collaborator

@BlueZeeKing BlueZeeKing left a comment

Choose a reason for hiding this comment

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

I honestly couldn't stop smiling while playing around with this. The attention to detail is honestly incredible. I thought about creating something like this myself, but I never ended up doing it, so I'm super happy someone else is. I just have a few smaller issues with some things.

src/main/java/io/icker/factions/command/HomeCommand.java Outdated Show resolved Hide resolved
src/main/java/io/icker/factions/command/InfoCommand.java Outdated Show resolved Hide resolved
src/main/java/io/icker/factions/command/ModifyCommand.java Outdated Show resolved Hide resolved
src/main/java/io/icker/factions/command/RankCommand.java Outdated Show resolved Hide resolved
build.gradle Show resolved Hide resolved
src/main/java/io/icker/factions/ui/ListGui.java Outdated Show resolved Hide resolved
src/main/java/io/icker/factions/ui/MemberGui.java Outdated Show resolved Hide resolved
Not fixed is only this conversation: ickerio#121 (comment)
@xllifi
Copy link
Author

xllifi commented Dec 15, 2024

I have made changes to implement these requested changes. Only unresolved request is InfoGui.java#L29-L32 because there was no answer when I committed.

@ickerio
Copy link
Owner

ickerio commented Dec 15, 2024

Thanks for the awesome PR and thanks for reviewing Blue. Blue feel free to merge whenever you thinks its ready

Copy link
Collaborator

Choose a reason for hiding this comment

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

Even if these are used as base64, I would represent them in a readable format in the code. Having raw base64 in source code can be seen as suspicious, even though I've checked over these and they're fine.

Copy link
Author

Choose a reason for hiding this comment

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

There's no way that I'm aware of to store these as a more readable format. If you decode the string, it's not even a texture itself - just a link to a PNG file.

Copy link
Collaborator

@PyPylia PyPylia Dec 15, 2024

Choose a reason for hiding this comment

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

You can store it as that json string and encode it to base64 at runtime.

Copy link
Author

Choose a reason for hiding this comment

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

I don't see a point in doing this. It will just use server resources and take up lines of code.

Every Patbox (Patbox is the developer of SGui) mod with a GUI uses base64 strings for player head icons, Fabric Tailor stores skins as base64 strings.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll wait to see the opinion of the other developers then.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I it's not ideal but it seems like the best solution to me

Copy link
Collaborator

Choose a reason for hiding this comment

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

Encoding to base64 at runtime takes essentially no time in return for far better code readability.

@PyPylia
Copy link
Collaborator

PyPylia commented Dec 15, 2024

Have you made it possible for someone to still use the original commands without using the GUI? I personally like having the option of both, maybe with an additional setting under the settings command.

@xllifi
Copy link
Author

xllifi commented Dec 15, 2024

Have you made it possible for someone to still use the original commands without using the GUI? I personally like having the option of both, maybe with an additional setting under the settings command.

I tried to change commands as little as possible. Members, info and list commands are now GUI-only since having both text message and a GUI show up would just look strange and it was out of scope for me to add GUI toggles.

I have no idea how settings are stored and I can't really work on this PR at the moment. I set it so that maintainers can edit, someone more familiar with the system can make a change.

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