cpp: Add set_external_translator() to register a custom translator

[ubruhin]

This intends to implement #3715 for C++: Registering a custom translator from C++, using the set_external_translator() function which has been added recently. For this, I extended the CMake setup to allow enabling the "tr" feature.

Concept Discussion

However, this PR is intended as a base for discussions, it's not a finished implementation. I'd like to clarify first which approach you like to follow.

The most important question is basically how the C++ API should look like. I can imagine 3 different approaches:

1. Provide a virtual Translator base class (interface) which users can derive from, and implement translate() and ntranslate() exactly the same way as in the tr crate. This seems the most obvious solution to me, but it involves a lot of complexity/boilerplate to finally pass a C++ translator object via cbindgen to Slint. The ownership/lifetime needs to be considered carefully for this approach, and I think we have to pass the object around as a struct with function pointers (IMHO complex, unreadable & error-prone).
2. Allow registering just two callback functions translate() and ntranslate() to Slint. A translator object exists only on Rust side, but not on C++ side. It's more primitive, but reduces complexity & boilerplate a lot. Just a bit cumbersome that 2 independent callbacks need to be implemented.
3. Same as variant 2, but with a single callback function rather than two separate. Whether it's a singular or plural form will be distinguished by the passed arguments. Most primitive way, but also the simplest with the least amount of code.

To be honest, even though variant 1 would be the cleanest from a conceptual point of view, I don't see it being worth adding so much complexity and lots of "stupid code" just to pass an interface through the C++/Rust bridge. Variant 2 I also consider as slightly cumbersome, as in practice the implementation of the 2 functions will be largely identical. So for now, I gave variant 3 a try, which is the implementation of this PR. And indeed I am actually surprised how simple and straightforward it is :) There's basically no complexity at all, it's dead-simple code. I think C++ developers are used to work with callbacks, so I don't mind the primitive concept compared to variant 1.

But I wonder what is your opinion? Any other ideas? Did I miss an easy way to implement variant 1?

I tested the current implementation with LibrePCB and so far it seems to work perfectly fine.

Open Questions

Note that the current design still has some open questions which I would try to figure out after the basic concept is clear:

• Currently it's not possible to unregister a callback. I couldn't figure out yet how to make cbindgen accepting to pass nullptr as callback.
• Currently the parameter const uint64_t* n is used to distinguish between singular and plural form. I understand this can be considered as ugly :) But the question is, what would be an acceptable way? std::optional<uint64_t> n? bool is_plural?, ...?
• Do the argument types of the callback function make sense? Slice<uint8_t> for the untranslated strings, SharedString* for returning the translated string. Maybe not the nicest interface, but I think we should choose something that doesn't involve unnecessary string copies (though this is probably not the case at the moment due to converting SharedString to Cow).

ToDo

ToDo after the concept is clear:

• Update documentation?
• Add some tests?
• Just realized that set_translator() should probably be renamed to set_external_translator() for consistency with the Rust function

[tronical]

Thanks for tackling this :)

My feeling is that a C++ class is the cleanest way to handle this. Yes, it means some glue code in Slint itself, but we need some translating glue anyway as I'd rather not have slint::private_api in the types of the public API :)

I think separating singular and plural handling into two functions makes for a cleaner interface overall, so I think the user needs to supply two functions.

If those two functions are just plain freestanding functions, then indeed we could just take two function pointers and be done with it. However, I think we should make it possible to let implementations not require global state, so the function pointers become - in reality - closures and we'd store them as std::function, right?

In that case, I see a tradeoff between two std::functions where the contained closures capture the same state versus a C++ class with two virtual functions and where this captures the join state (i.e. access to translation catalogues).

So my conclusion is that the nicest API for our users would be a C+ class with (pure) virtual functions, where the Slint API accepts an instance.

What do you think?

[ubruhin]

Alright, I think that sounds good. It also allows to wrap the cbindgen API on C++ side to provide a nicer API to the user (e.g. std::string_view rather than a Slice<uint8_t>). The main question then is who takes ownership of the translator object, the user or Slint? Maybe we should wrap the translator in a std::shared_ptr, which is hold by Slint as long as the translator is registered?

From the user point of view I could imagine an API like this:

#include <slint.h>
#include <memory>
#include <string_view>

class MyTranslator : public slint::Translator {
  public:
    MyTranslator(/* custom parameters */) {}

    slint::SharedString translate(std::string_view string, 
                                  std::string_view context) override {
      return slint::SharedString("Foo");
    }

    slint::SharedString ntranslate(uint64_t n,
                                   std::string_view singular,
                                   std::string_view plural,
                                   std::string_view context) override {
      return slint::SharedString("Bar");
    }
};

// Registering (Slint will hold the shared_ptr)
auto translator = std::make_shared<MyTranslator>(/* custom arguments */);
slint::set_external_translator(translator);

// Unregistering (Slint releases the shared_ptr)
slint::set_external_translator(nullptr);

Though I am not sure if developers of embedded devices (e.g. ESP32) will be happy to use std::shared_ptr. An alternative would be to pass the object as a raw pointer to Slint, documenting that Slint does not take ownership of the translator object, i.e. the user has to ensure object lifetime until the translator is unregistered.

[ogoffart]

Thanks for working on this.

slint::set_external_translator should take a std::unique_ptr<slint::Translator> or a std::shared_ptr<slint::Translator> (I'm unsure if there is a need to make it shared, imho unique_ptr is fine)

I think there is no need for SLINT_FEATURE_TR feature, we could just enable tr crate unconditionally, this is a tiny crate. But if we really want to gate this behind a feature, it should get another name.

[ubruhin]

I'm unsure if there is a need to make it shared, imho unique_ptr is fine

My idea was that the user might still need to access the translation object even after passing it to Slint (e.g. for switching to another language at runtime, or whatever). That would be more cumbersome with unique_ptr.

But I'm also fine with unique_ptr, no strong opinion here.

I'll try to implement the translator with a C++ base class soon.

[autofix-ci]

Hi! I'm autofix.ci, a bot that automatically fixes trivial issues such as code formatting in pull requests.

I would like to apply some automated changes to this pull request, but it looks like I don't have the necessary permissions to do so. To get this pull request into a mergeable state, please do one of the following two things:

1. Allow edits by maintainers for your pull request, and then re-trigger CI (for example by pushing a new commit).
2. Manually fix the issues identified for your pull request (see the GitHub Actions output for details on what I would like to change).

[ubruhin]

Alright, I implemented it now with a pure virtual base class slint::Translator, and an instance of it can be passed as a std::unique_ptr to slint::set_external_translator(). So far, this seems to work fine for LibrePCB.

Example:

struct MyTranslator : public slint::Translator {
  slint::SharedString translate(std::string_view string,
                                std::string_view context) const override {
    return slint::SharedString("Singular String");
  }
  slint::SharedString ntranslate(uint64_t n,
                                 std::string_view singular,
                                 std::string_view plural,
                                 std::string_view context) const override {
    return slint::SharedString("Plural String");
  }
};

slint::set_external_translator(std::make_unique<MyTranslator>());

Ownership of the translator object is transfered to Slint and the object gets destroyed from the Drop implementation of the translator object on Rust side. I verified that this works (C++ destructor is called at program exit and when another translator is registered).

I think there is no need for SLINT_FEATURE_TR feature, we could just enable tr crate unconditionally, this is a tiny crate.

Done.

Some open questions:

• To clarify thread safety, I documented that the methods on the Translator object will be called from the thread which the Slint event loop is running in. I hope that is right?
• On Rust side I had to add unsafe impl Send and unsafe impl Sync to CppTranslator. I'm not an expert in this area, but I suspect that should be fine?
• Are the code snippets added to the right place or should they be moved to somewhere else?
• The translated strings are passed as slint::SharedString from C++ to Rust. Then in Rust they are converted to Cow<str> with Cow::Owned(out.into()). I suspect this avoids any unnecessary allocation/deep-copy of the translated string, is that right? I expect/hope the allocation will be made only once, and no unnecessary deep copy occurs.
• The current API does not allow returning a translated string without a heap allocation at all. So, small embedded devices which have the strings as const char* in flash memory will still have one "unnecessary" heap allocation/deep-copy from flash to RAM. I suspect this is fine, i.e. there is no intention to support zero-copy translations?
• I marked the translate methods as const, but I have no strong opinion whether that makes sense or not.

[ogoffart]

To clarify thread safety, I documented that the methods on the Translator object will be called from the thread which the Slint event loop is running in. I hope that is right?

Yes, that's correct, in principle.
But...

On Rust side I had to add unsafe impl Send and unsafe impl Sync to CppTranslator.

That's because the tr::Translator trait requires Send and Sync because tr assume that it can be called from multiple trait even if in Slint we won't do that.
(And this is maybe a design mistake in the tr crate)
But I guess it is Ok for now.

Are the code snippets added to the right place or should they be moved to somewhere else?

Yes, i'd say so.

The translated strings are passed as slint::SharedString from C++ to Rust. Then in Rust they are converted to Cow with Cow::Owned(out.into()). I suspect this avoids any unnecessary allocation/deep-copy of the translated string, is that right? I expect/hope the allocation will be made only once, and no unnecessary deep copy occurs.

Unfortunately here the situation is quite bad and we do more allocations than required.

Cow::Owned(out.into()) will always allocate to convert from a slint::SharedString to a std::String.

We need to return an owned string, and the tr::Translator trait uses type from the std lib which cannot be used in C++.
What we could do is Not use tr::Translator in i-slint-core, just re-implement a trait ourselve that use SharedString or something different.

The current API does not allow returning a translated string without a heap allocation at all. So, small embedded devices which have the strings as const char* in flash memory will still have one "unnecessary" heap allocation/deep-copy from flash to RAM. I suspect this is fine, i.e. there is no intention to support zero-copy translations?

Yes, that's another allocation.
In order to help with the problem, i wanted to have SharedString to be able to point to a &'static str without allocation. But this hasn't been implemented.

The thing is that with the current signature, since we take a string_view in, and a SharedString out, we always need to allocate a SharedString, even if just returning the input.
If we take a SharedString as input, we could just return that. But the problem is that the input is passed by &str so we don't have access to the original SharedString

Quite inefficient.
I wonder if we just ignore the problem for now or if there is a better way to solve the problem.

I marked the translate methods as const, but I have no strong opinion whether that makes sense or not.

I guess that make sense, as translations shouldn't modify the object. Conceptually.

---

So overall i think the design of the Translator API currently in this PR is what makes the most sense right now.
Although that API pevent zero-copy/allocation translations, but i wonder how else to do it without making it overly complicated.

As for the name set_external_translator, i know it matches the internal rust function, but I wonder if we really need external in the name. Isn't just set_translator enough.

One remaining piece are tests

• see api/cpp/tests/window.cpp for an example
• Or tests/cases/translations/bundle.slint

We also need to specify what happens when using both bundle translations and a translator.

[ubruhin]

Thanks for the review!

Yeah I agree the use of the tr trait causes some inefficiency. It might be worth to define an internal trait for a translator, which uses the native string types as used in translations.rs. There could still be a proxy translator then which implements the tr trait for that internal translator, if needed (e.g. for gettext support). However, I think this refactoring would be out of scope for me due to lack of time.

In order to help with the problem, i wanted to have SharedString to be able to point to a &'static str without allocation.

Ah that would be nice for embedded devices indeed.

The thing is that with the current signature, since we take a string_view in, and a SharedString out, we always need to allocate a SharedString, even if just returning the input.

That's true, but I'd say it's not a big problem. The reason for registering a translator is to translate texts into another language. So for every language other than the source language, the returned string is expected to be different from the source string, it's rather an exception if the input will be returned as-is.

So overall i think the design of the Translator API currently in this PR is what makes the most sense right now.
Although that API pevent zero-copy/allocation translations, but i wonder how else to do it without making it overly complicated.

One thing I'm thinking about, if it would be better/possible to wrap a Rust Cow<str> in C++, to allow allocating the string from C++ and returning it to Rust without requiring an additional deep-copy 🤔

As for the name set_external_translator, i know it matches the internal rust function, but I wonder if we really need external in the name. Isn't just set_translator enough.

Agree, I will rename.

One remaining piece are tests

Yes, that's why the PR is still a draft :) I'll have a look at tests when the API is more or less clear.

Btw, if I understand correctly, CI for ESP32 now fails because of no_std, which might be caused by enabling the tr feature unconditionally? I wonder if the tr feature has a hard dependency to std 🤔 Would be a bit bad, as translations would be useful also in no_std environments IMO.

[ubruhin]

Another idea: Would it be reasonable to gate the new public API with #if defined(SLINT_FEATURE_EXPERIMENTAL)? Of course it would be nicer to agree on a stable API now, but this could be a temporary solution to already make this feature available without committing to a stable API yet. The define SLINT_FEATURE_EXPERIMENTAL is already available and used to gate the testing API.

[ogoffart]

Would it be reasonable to gate the new public API with #if defined(SLINT_FEATURE_EXPERIMENTAL)?

Yes, that will certainly make it easier to press the "Merge" button :-P

[ogoffart]

Btw, if I understand correctly, CI for ESP32 now fails because of no_std, which might be caused by enabling the tr feature unconditionally? I wonder if the tr feature has a hard dependency to std 🤔 Would be a bit bad, as translations would be useful also in no_std environments IMO.

Yes, tr doesn't need std in principle, so one could fix that in the tr crate, but that's another reason to not use the Translator trait from tr.

But we could disable the set_translator on the freestanding build maybe.