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

Container -> ScreenHandler #1106

Merged
merged 6 commits into from
Mar 8, 2020

Conversation

Prospector
Copy link
Contributor

@Prospector Prospector commented Feb 9, 2020

This is the culmination of many issues, PRs, and discord chats. While I completely understand those in favor of using the mojang name Menu (I supported it for a long time too!), the opinion against it is too strong. I hope that we can settle on this being more preferable to the existing name, Container, though.

@Runemoro Runemoro added refactor A PR that renames existing names. vote A vote on a name refactor labels Feb 9, 2020
@Prospector Prospector added the snapshot A PR that targets a snapshot version of Minecraft label Feb 9, 2020
@Runemoro Runemoro added discussion and removed snapshot A PR that targets a snapshot version of Minecraft vote A vote on a name refactor labels Feb 9, 2020
@Prospector Prospector added vote A vote on a name refactor discussion and removed discussion labels Feb 9, 2020
@Runemoro Runemoro added snapshot A PR that targets a snapshot version of Minecraft and removed snapshot A PR that targets a snapshot version of Minecraft labels Feb 9, 2020
Copy link
Collaborator

@sfPlayer1 sfPlayer1 left a comment

Choose a reason for hiding this comment

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

Conceptually ok, no in-depth review.

CONVENTIONS.md Outdated Show resolved Hide resolved
Copy link
Contributor

@Earthcomputer Earthcomputer left a comment

Choose a reason for hiding this comment

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

We could keep arguing about this forever, and hence keep being stuck on "container". I'm in favour of this PR - it's time to move onto a better name. Sometimes coming up with a name that perfectly describes function while not being 1000 characters long just isn't possible. ScreenHandler is as good as any that anyone is going to come up with.

Copy link
Contributor

@LambdAurora LambdAurora left a comment

Choose a reason for hiding this comment

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

There's some problems like comments which are missing, but overall it's nice.

@Prospector
Copy link
Contributor Author

I'll fix the comments at the end so they don't get nuked again.

@Prospector Prospector removed the request for review from Juuxel February 9, 2020 22:18
@Juuxel Juuxel added the outdated A PR that targets an outdated branch. label Feb 15, 2020
@Juuxel
Copy link
Member

Juuxel commented Feb 15, 2020

Could you update this to 20w07a?

@liach
Copy link
Contributor

liach commented Feb 17, 2020

Imo this is just a handle, not necessarily a handler; other objects work on this to make screens do stuff; this is just the pathway other objects use to handle screens, hence handle is sufficient.

@Prospector Prospector changed the base branch from 20w06a to 20w09a March 1, 2020 01:39
@Prospector
Copy link
Contributor Author

Ok this is all updated with the comment issue fixed & should be ready.

@Runemoro Runemoro removed outdated A PR that targets an outdated branch. discussion labels Mar 1, 2020
@LambdAurora
Copy link
Contributor

After rechecking a last time comments, there's still some missing (like in Property class).

@Prospector Prospector changed the base branch from 20w09a to 20w10a March 4, 2020 23:46
@Prospector Prospector added the outdated A PR that targets an outdated branch. label Mar 4, 2020
@Prospector Prospector removed the outdated A PR that targets an outdated branch. label Mar 5, 2020
@@ -0,0 +1,9 @@
CLASS net/minecraft/class_1712 net/minecraft/screen/ScreenHandlerListener
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just "screen listener"?

Copy link
Contributor Author

@Prospector Prospector Mar 5, 2020

Choose a reason for hiding this comment

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

Hm, what exactly does this listen for?

Copy link
Contributor

Choose a reason for hiding this comment

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

or ScreenUpdateListener? HandlerListener sounds... extraneous?

Copy link
Contributor

Choose a reason for hiding this comment

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

ScreenHandleCallback as this is like a callback when the screen handling is done

Copy link
Member

@modmuss50 modmuss50 left a comment

Choose a reason for hiding this comment

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

I think this is mostly agreed upon, changes look good. We will need to deprecate fabric api's fabric-containers-v0 and create a new module. I think we need to open an issue/pr about this. This is a good time to apply any changes to that api.

@i509VCB
Copy link
Contributor

i509VCB commented Mar 5, 2020

I mean since almost all mods broke on 10a, I think now may be the best time for that

@Runemoro
Copy link
Contributor

Runemoro commented Mar 8, 2020

@Prospector could you fix conflicts?

@modmuss50
Copy link
Member

Merge conflicts, and then I see no reason not to

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge refactor A PR that renames existing names. vote A vote on a name refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet