AtriusX / Waystones

A small plugin that brings long-distance teleportation to Minecraft survival mode!
https://www.spigotmc.org/resources/waystones.93917/
11 stars 7 forks source link

Add internationalization and localization support #15

Closed NewbieOrange closed 3 years ago

NewbieOrange commented 3 years ago

Make all messages / actionbars / item displayname and lore customizable.

Some minor modifications were done to internal data classes to facilitate this.

This PR will close #3.

NewbieOrange commented 3 years ago

Hi @AtriusX, any thoughts?

AtriusX commented 3 years ago

@NewbieOrange Thanks for the interest in the project! I'll take a look over this one shortly, going to get to the smaller PRs you sent first.

AtriusX commented 3 years ago

Added a comment to commit 0abd193

NewbieOrange commented 3 years ago

Does commit 8118eae break expected behaviour?

AtriusX commented 3 years ago

Does commit 8118eae break expected behaviour?

I can't really confirm but I don't see why it would break. Should test it just to be safe though.

AtriusX commented 3 years ago

Will be going offline shortly, so any changes afterward will be looked at in the morning. 👋🏼

AtriusX commented 3 years ago

We still have to add an update to the README file, but otherwise we can try testing things as they are. @Sepshun mind giving this a look?

luna-skye commented 3 years ago

This is taking a while as I'm trying to rather extensively test it, since this is a decent sized system being implemented. So, a few points:

I'm noticing one small inconsistency though, not sure if this is intentional or not but upon changing the localization value through /ws config localization [language], it doesn't reload and take effect immediately, though every other config value type does. You could just run the reload command to and that'd take care of it, but afaik it does this for every value other than localization.

Potentially unrelated to this PR but rather the way we have the WarpState system setup, but no error is being displayed at all on a warp that has been severed/broken.

Other than this, everything else functions incredibly well!

NewbieOrange commented 3 years ago

Yes, I noticed that no message is displayed when the link is severed, a seperated issue tho (not caused by this PR, hopefully).

AtriusX commented 3 years ago

I'm noticing one small inconsistency though, not sure if this is intentional or not but upon changing the localization value through /ws config localization [language], it doesn't reload and take effect immediately, though every other config value type does. You could just run the reload command to and that'd take care of it, but afaik it does this for every value other than localization.

Maybe this has to do with the localization class not being stored within the config itself? A solution to this might be to retrieve the current locale within the get operator of the class and reload if it is different. Otherwise we can just assign the update by moving the storage of the class into the configuration instead of using Locale.

AtriusX commented 3 years ago

@NewbieOrange Any thoughts on updating LocaleParser to store Localization objects instead? This would remove the need to manually reload it since the object would be regenerated each time the option is updated.

NewbieOrange commented 3 years ago

I am thinking about make Property have something like onUpdate() and we can pass localization.reload() as an argument, which the Property will call on update.

AtriusX commented 3 years ago

That could work too actually, try that out if you'd like

AtriusX commented 3 years ago

@NewbieOrange Looking at the code you might just need to apply this to the relevant files?

class Property<T>(
    property: String,
    default: T,
    private val parser: ArgumentParser<T>,
    private val onUpdate: () -> Unit = {}
) {
    // ..........
    operator fun invoke(input: String?): Boolean {
        value = parser.parse(input) ?: return false
        onUpdate()
        return true
    }

And then apply this to the configuration:

val localization: Property<Locale> =
        Property("localization", Locale.ENGLISH, LocaleParser) {
            localization.reload()
        }

From what I can tell, this might also remove the need to explicitly call localization.reload() in the reload command based off how the ConfigManager reloads data. (It might actually call onUpdate automatically as a result.

NewbieOrange commented 3 years ago

I have updated the code as you suggested, was busy working on other stuff.

I don't have time to test whether it work or not yet.

AtriusX commented 3 years ago

That's fine, @Sepshun can test the update for us again. Once we know that this works then we just need an update to the README file I think.

luna-skye commented 3 years ago

Seems to be working perfectly!

AtriusX commented 3 years ago

@NewbieOrange can you add the option to the README file? I think that should finish things off.

NewbieOrange commented 3 years ago

Added the option to README. Note that English is not my first language, feel free to update the description.