sitegeist / Sitegeist.LostInTranslation

Automatic Translations for Neos via DeeplApi
GNU General Public License v3.0
9 stars 9 forks source link

FEATURE: Implement strategies and add 'sync' strategy #3

Closed gradinarufelix closed 2 years ago

gradinarufelix commented 2 years ago

This PR adds a setting to switch between different strategies.

Strategy "once"

Behaves like the current version of the package, translating the node once on adoption.

Strategy "sync"

It translates and syncs the node according to a translation mapping setting every time the node in the source language is updated.

Further comments

We could actually elevate this from general setting level to a node setting level, like:

'Vendor.Site:Document.MyPage':
  options:
    translation: 'none' | 'once' | 'sync'
mficzel commented 2 years ago

Would love to see the strategy adjustable for each language via options.deeplStrategy in the dimension configuration.

Also the renaming of translateOnAdoption to translate is breaking. It makes sense but I would prefer a not so generic name that will likely collide with other packages. maybe automaticTranslation would do.

gradinarufelix commented 2 years ago

Got it. But then I'd also suggest a more generic name here:

Neos:
  ContentRepository:
    contentDimensions:
      'language':
        presets:
          'de':
            label: 'German'
            values: ['de']
            uriSegment: 'de'
            options:
              deeplLanguage: 'de'
              translationStrategy: ~ | 'once' | 'sync'
              translateTo:
                - 'it'
                - 'en'

Or should we reverse it like this?

Neos:
  ContentRepository:
    contentDimensions:
      'language':
        presets:
          'de':
            label: 'German'
            values: ['de']
            uriSegment: 'de'
            options:
              deeplLanguage: 'de'
              translationStrategy: ~ | 'once' | 'sync'
              translateFrom: 'en'

I'd also add a configuration checkup in initializeObject() in order to avoid someone creating a bad recursion.

mficzel commented 2 years ago

Hmm translate to makes sense for sync but translatefrom for new translations … currently they translate from the dimension that was used to create the translation … not sure

mficzel commented 2 years ago

Maybe a sane first step would be to translate from the default language on sync

We can always add the config later but getting rid of it is hard.

gradinarufelix commented 2 years ago

Thanks for the input, I already implemented it. I also added a general setting for node types options. automaticTranslation which is true by default and allows to turn off automatic translation for a certain node type in general. This would spare some lines of code in that case.

mficzel commented 2 years ago

@gradinarufelix i pushed some changes. Mainly to separate $presetIdentifiers like "en_UK from languages like "en". Also i limited sync to Nodes published to the live workspace as it feels safer that way at least for now.

Can you check on the adjustments shortly.

gradinarufelix commented 2 years ago

Adjustments look good to me. I also came across a bug and solved it: in sync mode, we also want to remove the node variants if the source node was removed. I moved this logic away from the general translateNode method because this is only valid for the sync mode.

mficzel commented 2 years ago

@gradinarufelix adjusted one last thing.

'Neos.Neos:Node':
  options:
      automaticTranslation: true

This will not only add this to Neos.Neos:Document and Neos.Neos:Content but also to Neos.Neos:ContentCollection which imho makes sense