ModCoderPack / MCPBot-Issues

Issue tracker for MCPBot (https://bitbucket.org/ProfMobius/mcpbot) and MCP mappings.
http://mcpbot.bspk.rs
80 stars 11 forks source link

GUI Class Cleanups #741

Closed Gegy closed 4 years ago

Gegy commented 6 years ago

Motivation

Over time the GUI class mapping has become a huge mess, lacking consistency and in some cases having nonsensical names. 1.13 is a great time to rework this structure, not only because so many other mappings are being reevaluated, but also because GUIs had some revision on the Vanilla side.

Examples

ScreenChatOptions, GuiScreenOptionsSounds, GuiVideoSettings

All menus for changing a specific type of option, yet completely different naming schemes.

GuiScreenServerList

This class is actually the "Direct Connect" screen, and has no list whatsoever.

GuiChat & GuiNewChat

GuiNewChat handles chat history and GuiChat handles new messages. If anything these are swapped around.

Changes

Result

* all from the reference of net.minecraft.client.gui

Generic Components

Overlays/HUD

Generic In-game Screens

Inventory/Container Screens

Generic Menu Screens

Resource Pack Screens

Server Screens

World Screens

Option Screens

Generic Misc Screens

Conclusion

These are a huge amount of changes and definitely encourage a discussion around these renames.

Pokechu22 commented 6 years ago

I support all of the screen and overlay renames, those are definitely good.

The rest I'm less sure about, though.

I think the component names convey a level of unification that isn't present in the actual code; just implementing IGuiEventListener doesn't give you the ability to do any rendering, only to handle input; a list of IGuiEventListener isn't useful for a complete GUI. Also, non-gui classes implement it, specifically Minecraft implements IGuiEventListenerDeferred.

I like the rename to use Forwarding for IGuiEventListenerDeferred, though I don't want the component part of it. I'm less sure about GuiEventHandler. I don't think Panel is the right term; in AWT Panel is something else and it's Container that contains other components (and is subclassed by Panel, ScrollPane, Window, etc).

I have no opinion on Gui to GuiRenderer. That class is just somewhat silly and probably should be static methods (possibly with static imports) instead.

I don't think the rename from GuiScreen to AbstractScreen is helpful. Outside of it being a fair amount of work to update around, I feel like it's not an implementation detail (which I partially associate wtih abstract classes); for instance it's used as a specific parameter in a ton of methods. That rename seems similar to changing Enchantment to AbstractEnchantment just because Enchantment happens to be an abstract class.

I've already made my statement about the Component suffix, but some other thoughts:

Gegy commented 6 years ago

@Pokechu22

I can definitely agree with a lot of what you are saying; the main challenge now is coming up with more suitable names.


bs2609 commented 5 years ago

As a brief note, I think it's important for the "Inventory/Container Screens" to have consistent names with the associated Inventory/Container classes. This can of course be achieved by renaming either class (or both), but the end result should match.

kashike commented 4 years ago

Issues regarding class and package names are now handled in the MCPConfig repository.