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

added getGuiItem method #1879

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

Lory9098
Copy link

@Lory9098 Lory9098 commented Dec 9, 2024

No description provided.

Copy link
Owner

@stefvanschie stefvanschie left a comment

Choose a reason for hiding this comment

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

Thank you for your pull request. While I think a getGuiItem method can be useful, I don't quite agree with the approach taken here.

First, I don't think getGuiItem should be an abstract method in the Pane class. This method doesn't make sense for all panes, as demonstrated by PaginatedPane and MasonryPane that currently throw exceptions. Furthermore, it'd also mean components need to implement this method which is probably unnecessary and raises some questions about the exact implementation. For that reason, I think this method should be implemented only in the OutlinePane, StaticPane, and PatternPane classes.

Secondly, the method should take a Slot parameter, rather than an int so users can specify slots in their preferred way (either via a pair of coordinates or an index). Additionally, the return value is better represented as a regular GuiItem marked as @Nullable, without the Optional. The rest of the codebase doesn't use Optionals anywhere and instead uses annotations to mark whether results can be null. It is better to keep this consistent.

Additionally, some of the implementations appear to not work correctly. While the items in panes are often stored internally as a List, this does not necessarily mean the items are rendered linearly. Panes such as OutlinePane can render the items differently based on gaps, alignment, orientation, etc. PatternPane places items depending on the pattern specified. From a brief look, it seems to me only the implementation of StaticPane is correct currently.

Lastly, it seems that you have accidentally deleted the CI/CD. Please revert that, otherwise it'll be hard to merge your PR without causing interruptions.

If you can address these points, I'd be happy to merge your PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants