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

Added the ability to change the text: Click to Unlock. As well as level and cost. #3977

Closed
wants to merge 9 commits into from

Conversation

svdermant
Copy link

@svdermant svdermant commented Sep 20, 2023

Description

Because there was no possibility to translate a specific text. I installed IDE Eclipse and imported the repo and made a Maven project from it.

Proposed changes

In the file SurvivalSlimefunGuide.java I changed certain things in line 292.
I also added 3 values ​​to Messages.yml

Related Issues (if applicable)

no Issues was found

Checklist

  • I have fully tested the proposed changes and promise that they will not break everything into chaos.
  • I have also tested the proposed changes in combination with various popular addons and can confirm my changes do not break them.
  • I have made sure that the proposed changes do not break compatibility across the supported Minecraft versions (1.16.* - 1.20.*).
  • I followed the existing code standards and didn't mess up the formatting.

…lock - and Levels also Cost

Code Changes a little bit with low knowhow it should be worked
Few Translations variables for Slimefun
guide.click-unlock
guide.cost
guide.level
@svdermant svdermant requested a review from a team as a code owner September 20, 2023 02:05
@github-actions
Copy link
Contributor

Pro Tip!
You can help us label your Pull Requests by using the following branch naming convention next time you create a pull request. ❤️

Branch naming convention Label
feature/** 🎈 Feature
fix/** ✨ Fix
chore/** 🧹 Chores
api/** 🔧 API
performance/** 💡 Performance Optimization
compatibility/** 🤝 Compatibility

If your changes do not fall into any of these categories, don't worry. You can just ignore this message in that case! 👀

@svdermant svdermant changed the title Added the ability to change the text: Click to Unlock. As well as level and cost. fix/** Added the ability to change the text: Click to Unlock. As well as level and cost. Sep 20, 2023
@svdermant svdermant changed the title fix/** Added the ability to change the text: Click to Unlock. As well as level and cost. Added the ability to change the text: Click to Unlock. As well as level and cost. Sep 20, 2023
messages.yml Outdated Show resolved Hide resolved
@@ -289,7 +289,7 @@ private void displaySlimefunItem(ChestMenu menu, ItemGroup itemGroup, Player p,
menu.addItem(index, new CustomItemStack(ChestMenuUtils.getNoPermissionItem(), sfitem.getItemName(), message.toArray(new String[0])));
menu.addMenuClickHandler(index, ChestMenuUtils.getEmptyClickHandler());
} else if (isSurvivalMode() && research != null && !profile.hasUnlocked(research)) {
menu.addItem(index, new CustomItemStack(ChestMenuUtils.getNotResearchedItem(), ChatColor.WHITE + ItemUtils.getItemName(sfitem.getItem()), "&4&l" + Slimefun.getLocalization().getMessage(p, "guide.locked"), "", "&a> Click to unlock", "", "&7Cost: &b" + research.getCost() + " Level(s)"));
menu.addItem(index, new CustomItemStack(ChestMenuUtils.getNotResearchedItem(), ChatColor.WHITE + ItemUtils.getItemName(sfitem.getItem()), "&4&l" + Slimefun.getLocalization().getMessage(p, "guide.locked"), "", "&a>" + Slimefun.getLocalization().getMessage(p, "guide.click-unlock"), "", "&7" + Slimefun.getLocalization().getMessage(p, "guide.Cost") + research.getCost() + Slimefun.getLocalization().getMessage(p, "guide.Level")));
Copy link
Contributor

Choose a reason for hiding this comment

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

the message key should be lowercase and use hyphens (-) to connect words.
And I don't think string concatenation is good here, better use placeholder.

Copy link
Author

@svdermant svdermant Sep 20, 2023

Choose a reason for hiding this comment

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

No i should not be Lowercase it works with this setup! :)
After all, I tested everything before the request!

Copy link
Contributor

Choose a reason for hiding this comment

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

The key should be lowercase, or it will break the naming convention.
Also, consider using a placeholder instead of concatenating 3 parts for the cost line.

Copy link
Author

@svdermant svdermant Sep 20, 2023

Choose a reason for hiding this comment

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

Hmmm did you mean "getMessage" or "getMessage(p, "guide.Cost")"
Should it be named getMessage(p, "guide.cost") ?

The red line shows the original.
Below is my slightly changed line

No placeholder was used there either. Do I know any placeholders that are marked with %? correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it should be "guide.cost" the whole key should be lowercase.

As for the formatting and placeholders look at the javadocs or some examples for the String#format method to find how it works.

Copy link
Author

Choose a reason for hiding this comment

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

I don't think I need to read about it because the original code, i.e. the red line, doesn't use a placeholder either. It's actually a function query called research.getCost() its not a placeholder so please explain to me exactly what you mean by placeholders?

@svdermant svdermant requested a review from ybw0014 September 20, 2023 03:53
@JustAHuman-xD
Copy link
Contributor

JustAHuman-xD commented Sep 20, 2023

You can find the right messages.yml in src->main->resources->language->en

@svdermant
Copy link
Author

i hope thats now my taken changes are correct. i'm new in this scene and i do my best to understand the code structure.

@variananora variananora added the 🎈 Feature This Pull Request adds a new feature. label Sep 20, 2023
@@ -44,6 +44,9 @@ placeholderapi:

guide:
locked: 'LOCKED'
click-unlock: You Must Unlock This!
cost: 'Cost: '
level: ' Level(s)'
Copy link
Member

Choose a reason for hiding this comment

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

I personally dislike the space here, I think the language file should not include this space and move the space to the code. Not sure how other feels about this one. Any suggestion on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree

Copy link
Author

@svdermant svdermant Sep 20, 2023

Choose a reason for hiding this comment

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

without the spaces the languagewords is combined so in the lore stands without them cost1Level(s)
So i add the Spaces for it.

i dont know how to add spaces in the code by "&7" + Slimefun.getLocalization().getMessage(p, "guide.cost") + research.getCost() + Slimefun.getLocalization().getMessage(p, "guide.level"))) in line 292 at SurvivalSlimefunGuide.java

so i add spaces in the message key to solve this. Even if it's probably wrong.
I'm still pretty new.

Copy link
Author

Choose a reason for hiding this comment

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

on my local machine i removed for testing the spaces in the message keywords... the result is watching the picture.
2023-09-20_17 27 26

To solve this i add spaces ...

Or for better understand how can i add spaces in the code add line 293 at SurvivalSlimefunGuide.java

@SchnTgaiSpock
Copy link
Contributor

The "Level(s)" message should be "%level% Level(s)" and then format it like the other messages

@@ -289,7 +290,7 @@ private void displaySlimefunItem(ChestMenu menu, ItemGroup itemGroup, Player p,
menu.addItem(index, new CustomItemStack(ChestMenuUtils.getNoPermissionItem(), sfitem.getItemName(), message.toArray(new String[0])));
menu.addMenuClickHandler(index, ChestMenuUtils.getEmptyClickHandler());
} else if (isSurvivalMode() && research != null && !profile.hasUnlocked(research)) {
menu.addItem(index, new CustomItemStack(ChestMenuUtils.getNotResearchedItem(), ChatColor.WHITE + ItemUtils.getItemName(sfitem.getItem()), "&4&l" + Slimefun.getLocalization().getMessage(p, "guide.locked"), "", "&a> Click to unlock", "", "&7Cost: &b" + research.getCost() + " Level(s)"));
menu.addItem(index, new CustomItemStack(ChestMenuUtils.getNotResearchedItem(), ChatColor.WHITE + ItemUtils.getItemName(sfitem.getItem()), "&4&l" + Slimefun.getLocalization().getMessage(p, "guide.locked"), "", "&a>" + Slimefun.getLocalization().getMessage(p, "guide.click-unlock"), "", "&7" + Slimefun.getLocalization().getMessage(p, "guide.cost") + research.getCost() + Slimefun.getLocalization().getMessage(p, "guide.level")));
Copy link
Contributor

Choose a reason for hiding this comment

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

add space before guide.click-unlock would better

Copy link
Author

Choose a reason for hiding this comment

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

that is what i whant to know how can i add spaces in the code?

Copy link
Contributor

Choose a reason for hiding this comment

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

"&a> " ...

Copy link
Author

Choose a reason for hiding this comment

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

That workes with the space in the " " string.. but how can i add spaces in functions with the given code:
menu.addItem(index, new CustomItemStack(ChestMenuUtils.getNotResearchedItem(), ChatColor.WHITE + ItemUtils.getItemName(sfitem.getItem()), "&4&l" + Slimefun.getLocalization().getMessage(p, "guide.locked"), "", "&a> " + Slimefun.getLocalization().getMessage(p, "guide.click-unlock"), "", "&7" + Slimefun.getLocalization().getMessage(p, "guide.cost") + research.getCost() + Slimefun.getLocalization().getMessage(p, "guide.level")));

It Looks ingame so.:
2023-09-20_20 41 49

Also the hint from SchnTgaiSpock with the %level% dont work..

@@ -44,6 +44,9 @@ placeholderapi:

guide:
locked: 'LOCKED'
click-unlock: You Must Unlock This!
Copy link
Contributor

Choose a reason for hiding this comment

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

Better keep the original Click to unlock

@svdermant
Copy link
Author

The "Level(s)" message should be "%level% Level(s)" and then format it like the other messages

This format does not work ... i get this ingame...
2023-09-20_20 16 47

@J3fftw1 J3fftw1 added the 🎯 Needs testing This Issue needs to be tested by our team to see if it can be reproduced. label Sep 20, 2023
…sage Keys - Thanks to Idra

Adding Spaces to the code for Message Keys - Thanks to Idra
…k to Unlock

 Remove Spaces and add the Orginal Text for Click to Unlock
Copy link
Contributor

@iTwins iTwins left a comment

Choose a reason for hiding this comment

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

These suggestions fix everything (tested them already).

Comment on lines +47 to +49
click-unlock: Click to unlock
cost: 'Cost:'
level: 'Level(s)'
Copy link
Contributor

@iTwins iTwins Sep 21, 2023

Choose a reason for hiding this comment

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

Suggested change
click-unlock: Click to unlock
cost: 'Cost:'
level: 'Level(s)'
click-unlock:
- '&4&lLOCKED'
- ''
- '&a> Click to unlock'
- ''
- '&7Cost: &b%cost%'
unlock-currency:
levels: 'Level(s)'

@@ -289,7 +290,7 @@ private void displaySlimefunItem(ChestMenu menu, ItemGroup itemGroup, Player p,
menu.addItem(index, new CustomItemStack(ChestMenuUtils.getNoPermissionItem(), sfitem.getItemName(), message.toArray(new String[0])));
menu.addMenuClickHandler(index, ChestMenuUtils.getEmptyClickHandler());
} else if (isSurvivalMode() && research != null && !profile.hasUnlocked(research)) {
menu.addItem(index, new CustomItemStack(ChestMenuUtils.getNotResearchedItem(), ChatColor.WHITE + ItemUtils.getItemName(sfitem.getItem()), "&4&l" + Slimefun.getLocalization().getMessage(p, "guide.locked"), "", "&a> Click to unlock", "", "&7Cost: &b" + research.getCost() + " Level(s)"));
menu.addItem(index, new CustomItemStack(ChestMenuUtils.getNotResearchedItem(), ChatColor.WHITE + ItemUtils.getItemName(sfitem.getItem()), "&4&l" + Slimefun.getLocalization().getMessage(p, "guide.locked"), "", "&a> " + Slimefun.getLocalization().getMessage(p, "guide.click-unlock"), "", "&7" + Slimefun.getLocalization().getMessage(p, "guide.cost") + " " + research.getCost() + " " + Slimefun.getLocalization().getMessage(p, "guide.level")));
Copy link
Contributor

@iTwins iTwins Sep 21, 2023

Choose a reason for hiding this comment

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

Suggested change
menu.addItem(index, new CustomItemStack(ChestMenuUtils.getNotResearchedItem(), ChatColor.WHITE + ItemUtils.getItemName(sfitem.getItem()), "&4&l" + Slimefun.getLocalization().getMessage(p, "guide.locked"), "", "&a> " + Slimefun.getLocalization().getMessage(p, "guide.click-unlock"), "", "&7" + Slimefun.getLocalization().getMessage(p, "guide.cost") + " " + research.getCost() + " " + Slimefun.getLocalization().getMessage(p, "guide.level")));
menu.addItem(index, new CustomItemStack(ChestMenuUtils.getNotResearchedItem().getType(), ChatColor.WHITE + ItemUtils.getItemName(sfitem.getItem()), Slimefun.getLocalization().getMessages(p, "guide.click-unlock", str -> str.replace("%cost%", research.getCost() + Slimefun.getLocalization().getMessage(p, "guide.unlock-currency.levels")))));

Copy link
Contributor

Choose a reason for hiding this comment

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

what if we can use other ways to unlock in the future? i mean this PR: #3942

Copy link
Contributor

Choose a reason for hiding this comment

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

This would also create merge conflicts with that pr, so we could also make it translatable in that pr and close this one.

Copy link
Contributor

Choose a reason for hiding this comment

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

That’s out of scope tho. Creating merge conflicts isn’t the end of the world. And for bigger PRs like that it would be halted by extensive testing anyways

Copy link
Contributor

Choose a reason for hiding this comment

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

Then I guess we don't have to worry about other unlock methods in this pr and just have the other pr handle the changes to that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if the line "Cost: (condition)" should be separated.

Copy link
Member

Choose a reason for hiding this comment

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

I would agree with that.

Putting these under a single key is more annoying to untangle later.

Copy link
Contributor

Choose a reason for hiding this comment

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

True. I edited the suggestion. Is this better?

Copy link
Contributor

Choose a reason for hiding this comment

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

not too sure how crowdin works but wouldn't those two empty lines show up as empty strings to translate? wouldnt break anything; just a little confusing

@github-actions
Copy link
Contributor

Slimefun preview build

A Slimefun preview build is available for testing!
Commit: 837590ef

https://preview-builds.walshy.dev/download/Slimefun/3977/837590ef

Note: This is not a supported build and is only here for the purposes of testing.
Do not run this on a live server and do not report bugs anywhere but this PR!

@Boomer-1
Copy link

i was going to test this but i couldn't ultimately see what was happening, and it seems to be a change of language, so someone who can read multiple languages would be better off testing

@J3fftw1 J3fftw1 added the Stale Marks a PR as stale. label Dec 31, 2023
@J3fftw1
Copy link
Contributor

J3fftw1 commented Dec 31, 2023

Marking this as stale

@J3fftw1
Copy link
Contributor

J3fftw1 commented Feb 25, 2024

Closing this as stale

@J3fftw1 J3fftw1 closed this Feb 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎈 Feature This Pull Request adds a new feature. 🎯 Needs testing This Issue needs to be tested by our team to see if it can be reproduced. Stale Marks a PR as stale.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants