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

Update some EditGameRulesScreen and GameRules mappings for accuracy and consistency #488

Merged
merged 15 commits into from
Sep 12, 2023

Conversation

Antikyth
Copy link
Contributor

@Antikyth Antikyth commented Sep 6, 2023

This PR changes existing mappings in EditGameRulesScreen and GameRules.

EditGameRulesScreen

  • RuleListWidget -> GameRuleEntryListWidget
    • In line with other widgets extending EntryListWidget.
    • Rule changed to GameRule for consistency: I found it confusing when reading code and seeing game rules variously referred to as rules or game rules.
  • occurrences of name -> message
    • This is to reduce confusion between the "technical name", as seen in GameRules.Key.getName(), and the message shown in GUIs.
    • message chosen based on the name used in buttons.
  • occurrences of description -> tooltip
    • These actually refer to the tooltip.
  • occurrences of ruleName -> description
    • These actually refer to the description.
  • occurrences of rule -> gameRule
    • For consistency, as above.
  • AbstractRuleWidget -> AbstractEntry
    • Entry is for consistency with other classes extending EntryListWidget.
    • Called AbstractEntry rather than AbstractGameRuleEntry because it is also used for categories.
  • NamedRuleWidget -> GameRuleEntry
    • This is what the actual game rule entries extend.
  • RuleCategoryWidget -> CategoryEntry
    • name -> title
      • matches its narration

GameRules

  • added the names for all the game rules and categories: it's just the string names in WHATEVER_THIS_CASE_IS case.
  • Rule -> AbstractGameRule
    • This is an abstract class extended by the boolean and int game rule classes.
  • In general, rule -> game rule
    • For consistency, as previously mentioned.

Category

  • category, getCategory -> translationKey, getTranslationKey
    • As you can imagine, these actually refer to the translation key.

@ix0rai ix0rai added t: refactor proposes a refactor v: snapshot targets a snapshot version of minecraft reviews needed please review this PR s: small PRs with less than 200 lines labels Sep 6, 2023
@Antikyth Antikyth marked this pull request as ready for review September 6, 2023 02:42
Copy link
Member

@ix0rai ix0rai left a comment

Choose a reason for hiding this comment

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

fantastic once again!
the detailed descriptions are amazing to have, I hardly needed to skim the diff to review this :)

Comment on lines +36 to +37
ARG 2 y
ARG 3 x
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that these have been swapped as well. The method's parameters are indeed GuiGraphics graphics, int y, int x. I assume it's a mistake. But they are used within the method, which is how you can tell:

protected void drawMessage(GuiGraphics graphics, int y, int x) {
    if (this.name.size() == 1) {
        graphics.drawText(EditGameRulesScreen.this.client.textRenderer, (OrderedText)this.name.get(0), x, y + 5, 16777215, false);
    } else if (this.name.size() >= 2) {
        graphics.drawText(EditGameRulesScreen.this.client.textRenderer, (OrderedText)this.name.get(0), x, y, 16777215, false);
        graphics.drawText(EditGameRulesScreen.this.client.textRenderer, (OrderedText)this.name.get(1), x, y + 10, 16777215, false);
    }
}

See how the x and y are used in the drawText calls. The y is shifted based on whether there is one or two lines of text.

Comment on lines 70 to +73
METHOD m_evugyeiw (Ljava/util/Map$Entry;)V
ARG 1 entry
ARG 1 categoryMapEntry
METHOD m_ffjznduo (Ljava/util/Map$Entry;)V
ARG 1 entry
ARG 1 gameRuleMapEntry
Copy link
Contributor Author

@Antikyth Antikyth Sep 6, 2023

Choose a reason for hiding this comment

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

This is a lambda function:

map.entrySet()
    .stream()
    .sorted(Map.Entry.comparingByKey())
    .forEach(categoryMapEntry -> {
        this.addEntry(
            EditGameRulesScreen.this.new CategoryEntry(
                Text.translatable(((GameRules.Category) categoryMapEntry.getKey()).getTranslationKey()).formatted(Formatting.BOLD, Formatting.YELLOW)
            )
        );
        ((Map) categoryMapEntry.getValue())
            .entrySet()
            .stream()
            .sorted(Map.Entry.comparingByKey(Comparator.comparing(GameRules.Key::getName)))
            .forEach(gameRuleMapEntry -> this.addEntry((AbstractEntry) gameRuleMapEntry.getValue()));
    });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

map is of type Map<GameRules.Category, Map<GameRules.Key<?>, AbstractEntry>>.
categoryMapEntry is of type Map.Entry<GameRules.Category, Map<GameRules.Key<?>, AbstractEntry>>.
gameRuleMapEntry is of type Map.Entry<GameRules.Key<?>, AbstractEntry>.

@@ -145,7 +145,7 @@ CLASS net/minecraft/unmapped/C_xmldumst net/minecraft/world/GameRules
ARG 1 key
ARG 2 type
CLASS C_bctfwntr Acceptor
METHOD call call (Lnet/minecraft/unmapped/C_xmldumst$C_avotqrag;Lnet/minecraft/unmapped/C_xmldumst$C_iuaedxxw;Lnet/minecraft/unmapped/C_xmldumst$C_mymgluou;)V
METHOD call accept (Lnet/minecraft/unmapped/C_xmldumst$C_avotqrag;Lnet/minecraft/unmapped/C_xmldumst$C_iuaedxxw;Lnet/minecraft/unmapped/C_xmldumst$C_mymgluou;)V
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All methods that call this are named accept, and it is the only thing in the class called Acceptor.

Comment on lines 132 to 142
METHOD m_hsodkmym visitBoolean (Lnet/minecraft/unmapped/C_xmldumst$C_iuaedxxw;Lnet/minecraft/unmapped/C_xmldumst$C_mymgluou;)V
COMMENT Visit a boolean rule.
METHOD m_hsodkmym visitBooleanGameRule (Lnet/minecraft/unmapped/C_xmldumst$C_iuaedxxw;Lnet/minecraft/unmapped/C_xmldumst$C_mymgluou;)V
COMMENT Visit a boolean game rule.
COMMENT
COMMENT <p>Note {@link #visit(GameRules.Key, GameRules.Type)} will be called before this method.
ARG 1 key
ARG 2 type
METHOD m_mfrkzoox visit (Lnet/minecraft/unmapped/C_xmldumst$C_iuaedxxw;Lnet/minecraft/unmapped/C_xmldumst$C_mymgluou;)V
METHOD m_mfrkzoox visitGameRule (Lnet/minecraft/unmapped/C_xmldumst$C_iuaedxxw;Lnet/minecraft/unmapped/C_xmldumst$C_mymgluou;)V
ARG 1 key
ARG 2 type
METHOD m_vtezodau visitInt (Lnet/minecraft/unmapped/C_xmldumst$C_iuaedxxw;Lnet/minecraft/unmapped/C_xmldumst$C_mymgluou;)V
COMMENT Visit an integer rule.
METHOD m_vtezodau visitIntGameRule (Lnet/minecraft/unmapped/C_xmldumst$C_iuaedxxw;Lnet/minecraft/unmapped/C_xmldumst$C_mymgluou;)V
COMMENT Visit an integer game rule.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it was confusing that these are named visitInt and visitBoolean when they don't visit ints or booleans but rather IntGameRules and BooleanGameRules.

@Antikyth
Copy link
Contributor Author

Antikyth commented Sep 6, 2023

I already fixed those javadocs. It needs to be rebuilt.

Copy link
Contributor

@EnnuiL EnnuiL left a comment

Choose a reason for hiding this comment

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

After looking more deeply into the CI error, I can see there are still few (one...?) Javadoc reference errors, with an attempted reference to Visitor#visit (renamed to Visitor#visitGameRule) being the cause

@Antikyth
Copy link
Contributor Author

Antikyth commented Sep 6, 2023

After looking more deeply into the CI error, I can see there are still few (one...?) Javadoc reference errors, with an attempted reference to Visitor#visit (renamed to Visitor#visitGameRule) being the cause

Ah, sorry. I see now. It looked a lot like the javadoc I'd already fixed.

@Antikyth
Copy link
Contributor Author

Antikyth commented Sep 6, 2023

Also, I think visit would have been a better name, but because there are ones for visiting booleans and int game rules as well, I think it needs game rule on the end to not be confusing.

mappings/net/minecraft/world/GameRules.mapping Outdated Show resolved Hide resolved
mappings/net/minecraft/world/GameRules.mapping Outdated Show resolved Hide resolved
mappings/net/minecraft/world/GameRules.mapping Outdated Show resolved Hide resolved
@Antikyth Antikyth requested a review from ix0rai September 7, 2023 06:29
Copy link
Contributor

@EnnuiL EnnuiL left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@IotaBread IotaBread added the update-base used to notify github actions that the base branch should be updated label Sep 7, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2023

🚨 Target branch is already set to 23w35a

@github-actions github-actions bot removed the update-base used to notify github actions that the base branch should be updated label Sep 7, 2023
@IotaBread IotaBread added the update-base used to notify github actions that the base branch should be updated label Sep 7, 2023
@github-actions github-actions bot changed the base branch from 23w35a to 1.20.2-pre2 September 7, 2023 22:56
@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2023

🚀 Target branch has been updated to 1.20.2-pre2

@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2023

🚨 Please fix merge conflicts before this can be merged

@github-actions github-actions bot added outdated this pull request hasn't been updated to the latest version or has merge conflicts and removed update-base used to notify github actions that the base branch should be updated labels Sep 7, 2023
@supersaiyansubtlety supersaiyansubtlety removed the reviews needed please review this PR label Sep 8, 2023
@Antikyth
Copy link
Contributor Author

Antikyth commented Sep 8, 2023

Oh boy I love me some merge conflicts /s

@ix0rai ix0rai added final-comment-period is approved and will soon be merged if no issues are raised s: medium PRs with less than 700 lines and more than 200 and removed outdated this pull request hasn't been updated to the latest version or has merge conflicts s: small PRs with less than 200 lines labels Sep 9, 2023
@ix0rai ix0rai merged commit 2531020 into QuiltMC:1.20.2-pre2 Sep 12, 2023
6 checks passed
@Antikyth Antikyth deleted the game-rules branch September 12, 2023 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period is approved and will soon be merged if no issues are raised s: medium PRs with less than 700 lines and more than 200 t: refactor proposes a refactor v: snapshot targets a snapshot version of minecraft
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants