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

Initial work on translations #1728

Merged
merged 3 commits into from Feb 4, 2018

Conversation

Projects
None yet
6 participants
@ImMorpheus
Copy link
Contributor

ImMorpheus commented Jan 24, 2018

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 ImMorpheus requested a review from gabizou as a code owner Jan 24, 2018

@ImMorpheus

This comment has been minimized.

Copy link
Contributor

ImMorpheus commented Jan 24, 2018

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 ?


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

This comment has been minimized.

@gabizou

gabizou Jan 25, 2018

Member

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

This comment has been minimized.

@ImMorpheus

ImMorpheus Jan 25, 2018

Contributor

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)

This comment has been minimized.

@ST-DDT

ST-DDT Jan 25, 2018

Contributor

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

This comment has been minimized.

Copy link
Member

parlough commented Jan 25, 2018

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

This comment has been minimized.

Copy link
Contributor

ImMorpheus commented Jan 25, 2018

That's the current behavior.

@ImMorpheus ImMorpheus force-pushed the ImMorpheus:translations branch from 41aa853 to 9dd8024 Jan 25, 2018

@ImMorpheus

This comment has been minimized.

Copy link
Contributor

ImMorpheus commented Feb 2, 2018

Status ? I need this for the catalog PR.

@gabizou

gabizou approved these changes Feb 2, 2018

Copy link
Member

gabizou left a comment

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.

@kashike

kashike approved these changes Feb 2, 2018

@parlough parlough merged commit a8d1cf1 into SpongePowered:stable-7 Feb 4, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

parlough added a commit to parlough/SpongeCommon that referenced this pull request Feb 4, 2018

Initial work on translations fixes (SpongePowered#1728)
* Initial work on translations

* Fix bed, brewindstand, concrete, flowerpot and slabs

* Fix skull and compile error

RedNesto added a commit to RedNesto/SpongeCommon that referenced this pull request Feb 5, 2018

Initial work on translations fixes (SpongePowered#1728)
* Initial work on translations

* Fix bed, brewindstand, concrete, flowerpot and slabs

* Fix skull and compile error

@ImMorpheus ImMorpheus referenced this pull request Feb 10, 2018

Open

Fix CatalogTypes #1792

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