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.

Sign up for GitHub

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

Jump to bottom

ScreenHandler -> Menu #1165

Closed
wants to merge 13 commits into from
Closed

ScreenHandler -> Menu #1165

wants to merge 13 commits into from

Conversation

Juuxel
Copy link
Member

@Juuxel Juuxel commented Mar 9, 2020 •
edited
Loading

Replaces ScreenHandler with Menu, as seen in various registry and logging strings.

The name Menu is shown in unobfuscated names such as the createMenu method or the minecraft:menu registry (which Yarn incorrectly calls SCREEN_HANDLER).

This PR is the successor of #846. Also includes parts of #1166.

@Juuxel Juuxel added refactor A PR that renames existing names. snapshot A PR that targets a snapshot version of Minecraft labels Mar 9, 2020
@jaskarth
Copy link
Contributor

jaskarth commented Mar 9, 2020

how do i concern emote on github

Devan-Kerman
Devan-Kerman reviewed Mar 9, 2020
View reviewed changes
Copy link
Contributor

@Devan-Kerman Devan-Kerman left a comment

Choose a reason for hiding this comment

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

Why

liach
liach suggested changes Mar 9, 2020
View reviewed changes
Copy link
Contributor

@liach liach left a comment

Choose a reason for hiding this comment

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

Quite a few places where you've reverted handler to container.

mappings/net/minecraft/client/gui/screen/ingame/AnvilScreen.mapping Outdated Show resolved Hide resolved
mappings/net/minecraft/client/gui/screen/ingame/BeaconScreen.mapping Outdated Show resolved Hide resolved
mappings/net/minecraft/client/gui/screen/ingame/BrewingStandScreen.mapping Outdated Show resolved Hide resolved
mappings/net/minecraft/client/gui/screen/ingame/CartographyTableScreen.mapping Outdated Show resolved Hide resolved
mappings/net/minecraft/client/gui/screen/ingame/CraftingTableScreen.mapping Outdated Show resolved Hide resolved
mappings/net/minecraft/menu/NameableMenuFactory.mapping Outdated Show resolved Hide resolved
mappings/net/minecraft/menu/SmithingTableMenu.mapping Outdated Show resolved Hide resolved
mappings/net/minecraft/network/packet/c2s/play/ConfirmGuiActionC2SPacket.mapping Show resolved Hide resolved
mappings/net/minecraft/network/packet/s2c/play/ConfirmGuiActionS2CPacket.mapping Outdated Show resolved Hide resolved
mappings/net/minecraft/recipe/book/RecipeBook.mapping Outdated Show resolved Hide resolved
modmuss50
modmuss50 requested changes Mar 9, 2020
View reviewed changes
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 dont see this being merged in, we just spent months coming to an agreement on a replacement to Container.

We won't be changing names like this every day.

#1106 Was mostly agreed upon to not be bad, it had no outstanding reviews.

@florensie
Copy link
Contributor

Phew. I was just about to say I'd go commit die if it were to get merged.

Juuxel and others added 2 commits March 9, 2020 17:16
@Juuxel
Remove pointless/incorrect changes, slot cleanup
8682daf
@Juuxel @liach
Apply suggestions from code review
a1fb6d7 
Co-Authored-By: liach <7806504+liach@users.noreply.github.com>
@Juuxel
Copy link
Member Author

Juuxel commented Mar 9, 2020

#1106 Was mostly agreed upon to not be bad

It creates a bunch of nonsense names like ScreenHandler createMenu(); or Registry<...> SCREEN_HANDLER = create("menu").

@modmuss50
Copy link
Member

We were aware of those 2 places, and agreed menu wasn’t the right for us.

@Juuxel
NameableMenuFactory.getTitle() -> getDisplayName()
e8003c7 
It's the same method as Nameable.getDisplayName() which is also used
by entities such as players. (I don't think players have a title? :P)
@liach liach mentioned this pull request Mar 9, 2020
Merged
@Juuxel Juuxel changed the base branch from 20w10a to 20w11a March 11, 2020 18:29
modmuss50
modmuss50 requested changes Mar 17, 2020
View reviewed changes
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.

Just so you know, I dont see this been merged. A lot of discussion happened over a long period of time and it was clear the Menu was not the right choice for yarn.

Feel free to maintain your own fork that has these changes, but we wont be making large yarn changes every other day driving mod devs nuts.

@Juuxel
Copy link
Member Author

Juuxel commented Mar 17, 2020

Yes, I know this most likely won't ever get merged. I will still maintain this for my own use, though, as ScreenHandler isn't the best name ever. I wish we'd gone with ScreenController, but what can I do ¯\_(ツ)_/¯

@liach
Copy link
Contributor

liach commented Mar 17, 2020

@Juuxel so can I close this pr so I am less concerned with the number of open prs in yarn... I want things clean

@liach liach added vote A vote on a name refactor outdated A PR that targets an outdated branch. labels Mar 17, 2020
@Juuxel Juuxel changed the base branch from 20w11a to 20w12a March 18, 2020 20:04
@Juuxel Juuxel removed the outdated A PR that targets an outdated branch. label Mar 18, 2020
@Runemoro Runemoro mentioned this pull request Mar 20, 2020
Closed
Yoghurt4C
Yoghurt4C reviewed Mar 20, 2020
View reviewed changes
Copy link

@Yoghurt4C Yoghurt4C left a comment

Choose a reason for hiding this comment

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

based

@YanisBft YanisBft added the outdated A PR that targets an outdated branch. label Mar 25, 2020
@Juuxel Juuxel changed the base branch from 20w12a to 20w13a March 25, 2020 18:26
@Juuxel Juuxel removed the outdated A PR that targets an outdated branch. label Mar 25, 2020
@Juuxel Juuxel changed the base branch from 20w13a to 20w13b March 27, 2020 13:57
@YanisBft YanisBft added the outdated A PR that targets an outdated branch. label Apr 2, 2020
@Juuxel Juuxel changed the base branch from 20w13b to 20w14a April 3, 2020 19:17
@Juuxel Juuxel removed the outdated A PR that targets an outdated branch. label Apr 3, 2020
@Juuxel Juuxel changed the base branch from 20w14a to 20w15a April 9, 2020 05:36
@Juuxel Juuxel closed this Apr 9, 2020
@valoeghese
Copy link
Contributor

screenhandler bad

@shedaniel
Copy link
Contributor

i have to defend from 2 words names, back to container bois

@Juuxel Juuxel mentioned this pull request Oct 23, 2020
Closed
@valoeghese
Copy link
Contributor

screenhandler bad

@kanpov
Copy link

kanpov commented Jul 16, 2022 •
edited
Loading

this is forever going to be a part of history of blockgame

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Yoghurt4C Yoghurt4C Yoghurt4C approved these changes

@Devan-Kerman Devan-Kerman Devan-Kerman left review comments

@modmuss50 modmuss50 modmuss50 requested changes

@liach liach liach approved these changes

@florensie florensie florensie approved these changes

@falseresync falseresync falseresync approved these changes

Assignees
No one assigned
Labels
refactor A PR that renames existing names. snapshot A PR that targets a snapshot version of Minecraft vote A vote on a name refactor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet