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

Jump to bottom

Initial work on translations #1728

Merged
merged 3 commits into from Feb 4, 2018
Merged

Initial work on translations #1728

merged 3 commits into from Feb 4, 2018

Conversation

ImMorpheus
Copy link
Contributor

@ImMorpheus ImMorpheus commented Jan 24, 2018 •
edited

This should fix #659

  • tile.doorOak.name
  • tile.cobbleWall.name
  • tile.stone.name
  • tile.diode.name
  • tile.doorBirch.name
  • tile.comparator.name
  • tile.banner.name
  • tile.diode.name
  • tile.banner.name
  • tile.sapling.name
  • tile.doorDarkOak.name
  • tile.comparator.name
  • tile.prismarine.name
  • tile.doorJungle.name
  • tile.doorAcacia.name
  • tile.sponge.name
  • tile.doorSpruce.name
  • tile.stoneSlab2.name
  • tile.concretePowder.name
  • tile.null.name | (BlockPistonMoving) not translatable
  • tile.bed.name
  • tile.concrete.name
  • tile.brewingStand.name
  • tile.null.name | (BlockEndGateway) not translatable
  • tile.skull.name
  • tile.null.name | (BlockEndGateway) not translatable
  • tile.stoneSlab2.name
  • tile.flowerPot.name

Should also fix part of #739
Broken translations:

  • Handtype
  • CraftingRecipe
  • Statistic
  • PopulatorType
  • EntityType
  • PotionEffectType
    EDIT: ignore these, they require SpongeAPI changes. Another PR will follow when this get merged

@ImMorpheus
Copy link
Contributor Author

ImMorpheus commented Jan 24, 2018 •
edited

Regarding some block translations:

Every (Mixin)Block implements BlockType which implements Translatable, however not every Block (I'm referring to mojang code) is translatable (has a translation inside the .lang file).

Example:

[STDOUT]: ========
[STDOUT]: Mojang translation: tile.null.name
[STDOUT]: Translation: tile.null.name
[STDOUT]: Class: class net.minecraft.block.BlockEndPortal
[STDOUT]: ========
[STDOUT]: Mojang translation: tile.bed.name
[STDOUT]: Translation: tile.bed.name
[STDOUT]: Class: class net.minecraft.block.BlockBed
[STDOUT]: ========
[STDOUT]: Mojang translation: tile.concrete.name
[STDOUT]: Translation: tile.concrete.name
[STDOUT]: Class: class net.minecraft.block.BlockColored
[STDOUT]: ========
[STDOUT]: Mojang translation: tile.null.name
[STDOUT]: Translation: tile.null.name
[STDOUT]: Class: class net.minecraft.block.BlockPistonMoving
[STDOUT]: ========
[STDOUT]: Mojang translation: tile.null.name
[STDOUT]: Translation: tile.null.name
[STDOUT]: Class: class net.minecraft.block.BlockEndGateway
[STDOUT]: ========

How should I treat these ?

gabizou
gabizou reviewed Jan 25, 2018
View reviewed changes
src/main/java/org/spongepowered/common/mixin/core/block/MixinBlockBanner.java

@Override
public Translation getTranslation() {
return new SpongeTranslation("item.banner.white.name");
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the translations to be re-created every time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are they worth caching ?
Currently there is no "standard way" of doing the translation. Half of the blocks cache the translation (ex. https://github.com/SpongePowered/SpongeCommon/blob/bleeding/src/main/java/org/spongepowered/common/mixin/core/data/types/MixinBlockPrismarineEnumType.java )
Either way, I will probably fix all of them to be consistent in the next PR. (so, input wanted)

Copy link
Member

Choose a reason for hiding this comment

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

AFAICT either the translations are not used at all or used multiple times (because a plugin actually wants them)

IMO lazy caching is a good way that does not consume that much memory.

@parlough
Copy link
Contributor

Due to not all translations being present and the fact the method does not return an optional. Can we fall back to their id?

@ImMorpheus
Copy link
Contributor Author

That's the current behavior.

@limbo-app limbo-app added status: needs review a code review is needed status: needs testing does this run, does it solve the issue etc labels Jan 30, 2018
@ImMorpheus
Copy link
Contributor Author

Status ? I need this for the catalog PR.

gabizou
gabizou approved these changes Feb 2, 2018
View reviewed changes
Copy link
Member

@gabizou gabizou left a comment

Choose a reason for hiding this comment

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

While this works and functions as expected, a cleanup would be necessary to lazy load the translations as fields. Doesn't cost us much in memory and useful to keep re-using the same object over time.

@gabizou gabizou added status: ready to merge This PR is ready to be merged and removed status: needs review a code review is needed status: needs testing does this run, does it solve the issue etc labels Feb 2, 2018
@parlough parlough merged commit a8d1cf1 into SpongePowered:stable-7 Feb 4, 2018
parlough pushed a commit to parlough/SpongeCommon that referenced this pull request Feb 4, 2018
@ImMorpheus @parlough
Initial work on translations fixes (SpongePowered#1728)
e8dc00d 
* Initial work on translations

* Fix bed, brewindstand, concrete, flowerpot and slabs

* Fix skull and compile error
@parlough parlough mentioned this pull request Feb 4, 2018
Closed
RedNesto pushed a commit to RedNesto/Sponge that referenced this pull request Feb 5, 2018
@ImMorpheus @RedNesto
Initial work on translations fixes (SpongePowered#1728)
b82033d 
* Initial work on translations

* Fix bed, brewindstand, concrete, flowerpot and slabs

* Fix skull and compile error
@ImMorpheus ImMorpheus mentioned this pull request Feb 10, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ST-DDT ST-DDT ST-DDT left review comments

@gabizou gabizou gabizou approved these changes

@kashike kashike kashike approved these changes

Assignees
No one assigned
Labels
status: ready to merge This PR is ready to be merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Some block translations are off
6 participants
@ImMorpheus @parlough @gabizou @ST-DDT @kashike @limbo-app