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

Implemented overrideToFallbackLanguage flag and fallbackLanguage; #14

Conversation

Antonijo2307
Copy link
Contributor

Add functionality that enables user to configure Swiftl18n to use fallback language in case there is no translation for searched key.

Productive task:
https://app.productive.io/1-infinum/task/8092739

@Antonijo2307
Copy link
Contributor Author

The problem I see is that there is no way to determine for certain that there is no translation for a specific key, there are no constraint on key naming so it can easily be called the same as translation. I don't know if you all had some idea on how to resolve this issue, cause our translation tables after they are generated by polyglot already have filled empty translations with key values.
Maybe we should leave those values empty and if empty return key values or fallback value? Or constraint key naming to some recognisable format like key_some_key_neme... What do you think?

@Truba
Copy link
Member

Truba commented May 24, 2024

@jabou
@Antonijo2307 also had a good comment. Take for example:
translationKey: "ok"
english translation: "ok"
croatian translation: "dobro"
If user set's english as preferred language, and croatian as a fallback they would get "dobro" as a translation.

@jabou
Copy link
Member

jabou commented May 27, 2024

@jabou
@Antonijo2307 also had a good comment. Take for example:
translationKey: "ok"
english translation: "ok"
croatian translation: "dobro"
If user set's english as preferred language, and croatian as a fallback they would get "dobro" as a translation.

@Truba @Antonijo2307

Maybe we can check if the translation has _? In your case, we can assume that the "ok" key is translated as the value key does not contain "_".
In the case of checking the _, I would add some "smarter" logic than just checking if the value contains _ as this will not cover the translation, e.g., "Fill empty lines: _" (e.g., hint/subtitle).
We can separate text by _, and if there are more than 1 or 2 elements in the array, -> not translated. Or check if we have _ and " " (spaces) -> translated. Something like that should cover cases without _.

@Antonijo2307
Copy link
Contributor Author

Antonijo2307 commented May 28, 2024

@jabou && @Truba
Or we can leave them empty during translation table generation, and then just handle empty string, if empty use key or default translation, depending on the overrideToFallbackLanguage flag value. It should be easy to implement, older project can choose to stay as they are or just do "polyglot pull" again to clean translation tables.

@jabou
Copy link
Member

jabou commented May 28, 2024

@Antonijo2307 This will break debugging; the main purpose of showing the key instead of the translation is for QA (and the client) to easily see what translations are missing on the screen.
However, I'm all ears if you can combine those stuff somehow.

@Antonijo2307
Copy link
Contributor Author

Antonijo2307 commented May 28, 2024

@jabou
No breaking of debug:

if translation.isEmpty {
    if overrideToFallbackLanguage {
        translation = falbackLanguageTranslation
    } else {
        translation = key_value
    }
}

so it will be the same result as you defined it in this task, if you set overrideToFallbackLanguage = false then you will see keys in places that do not have translations...

SwiftI18n/Classes/Main/I18nManager.swift Outdated Show resolved Hide resolved
SwiftI18n/Classes/Main/I18nManager.swift Outdated Show resolved Hide resolved
@Antonijo2307
Copy link
Contributor Author

@jabou I don't see how it can break someones implementation, if someone defined his own block than this one will just not be called anyways. The solution with predefined localizationPerformingBlockWithFallbackLanguage block does the same thing, if I define block "localizationPerformingBlockWithFallbackLanguage" on manager and I set that one from my project than I can't use my custom block if I used one before or if I use my custom block than I can't use localizationPerformingBlockWithFallbackLanguage block, right?. So it doesn't really solve the problem. As @Truba said, the problem can be resolved locally per project, and anyone who has custom block implemented will have to deal with it by updating code in their custom localisation block, we can make some instructions on how to do it. But I think most users use this predefined localisation block, and if they did not define documentation language it will behave the same as before, so again no broken implementation.

So I can ditch the lazy part by declaring fallbackLanguage as static var, manager is singletone with private constructor after all, and do something like this:

public var localizationPerformingBlock: (_ key: String, _ language: String)->(String) = { (key, language) in

        let translation = NSLocalizedString(key, tableName: language, comment: "")
        
        guard translation == key else {
            return translation
        }

        guard let fallbackLanguage = I18nManager.fallbackLanguage else {
            return translation
        }
        
        return NSLocalizedString(key, tableName: fallbackLanguage, comment: "")
    }

What do you think? Do you have some usecase to demonstrate breaking of functionality that I didn't account for?

@goranbrl
Copy link
Member

goranbrl commented Oct 8, 2024

@jabou @Truba @Antonijo2307 what's the status of this PR?

@Antonijo2307
Copy link
Contributor Author

@goranbrl We are still trying to work out some implementation details. @jabou said he will take a look at my questions as soon as he gets a chance.

@jabou
Copy link
Member

jabou commented Oct 24, 2024

@Antonijo2307 I checked the proposed implementation, and it still breaks the logic. As long as localizationPerformingBlock is a variable that anyone can change, our logic will stop working.

Have you checked maybe if something like this will work:
Return original logic of localizationPerformingBlock:

    public var localizationPerformingBlock: (_ key: String, _ language: String)->(String) = { (key, language) in
        return NSLocalizedString(key, tableName: language, comment: "")
    }

and then move your logic into the localizedString(forKey:):

    func localizedString(forKey key: String) -> String {
        let translation = localizationPerformingBlock(key, language)

        guard translation == key else {
            return translation
        }

        guard let fallbackLanguage else {
            return translation
        }

        return localizationPerformingBlock(key, fallbackLanguage)
    }

In this case, if the user sets fallbackLanguage (you can remove it from static -> public var fallbackLanguage: String?), the user's logic for localizationPerformingBlock will still stay the same, except it will be called twice if we have fallbackLanguage and primary language translation is missing + user does not change returned key back to the block. If they do change it (basically they implemented some similar idea that we did in here), that translation will just be returned, and fallbackLanguage will be ignored.

If they have some custom logic within localizationPerformingBlock, by adding the fallbackLanguage, the will still keep the logic that we want to accomplish.
In your current proposal, if user set the fallbackLanguage and implements their own localizationPerformingBlock, that property will not do anything and it will look like a bug in the lib.

If you go with the proposed approach, please update readme and document fallbackLanguage. As a note, let's add how that property works and how it's managed by localizationPerformingBlock. I.e., if someone overrides translation key that represents the missing translation, that fallbackLanguage will not be taken into the count.

@Antonijo2307
Copy link
Contributor Author

Ok I have updated the code. Now the block is free for implementation and internally LocText doesn't call it directly. This solves most of the problems from above.
Problems that are not solved are the ones where key is just one word, like:
ok, done, save...
All these examples will have the same translations in english, and if english is not fallback language, they will be translated
I don't see any solution but to standardise key naming to lets say key_[something]. I don't know if we can do this but that is the only solution I can see. @Truba @jabou what do you think?

@jabou
Copy link
Member

jabou commented Oct 25, 2024

@Antonijo2307

but to standardise key naming to lets say key_[something]

I would agree, but that should not be part of this lib. We can write that limitation when fallback translation is used in the readme (documentation) with the proposed solution.

SwiftI18n/Classes/Main/I18nManager.swift Outdated Show resolved Hide resolved
SwiftI18n/Classes/Main/I18nManager.swift Show resolved Hide resolved
SwiftI18n/Classes/Main/I18nManager.swift Outdated Show resolved Hide resolved
SwiftI18n/Classes/Views/SwiftUI/LocText.swift Show resolved Hide resolved
SwiftI18n/Classes/Main/I18nManager.swift Outdated Show resolved Hide resolved
@Antonijo2307 Antonijo2307 changed the base branch from master to feature/translation-fallback/fallback_language October 28, 2024 14:32
@Antonijo2307 Antonijo2307 merged commit 60a7c16 into feature/translation-fallback/fallback_language Oct 28, 2024
1 check passed
@Antonijo2307 Antonijo2307 deleted the feature/translation-fallback/1088-Default-language-fallback-translation branch October 28, 2024 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants