Improve SoftwareKeyboard::set_filter_callback

[FenrirWolf]

We don't need to box the user's closure, and we can let the user deal with normal Rust strings while handling CString conversion internally too.

[Meziu]

Wait, we do need to box the user’s closure if it captures variables. This way we’d be losing some functionality. Also, we had opted to use CString in the user API to avoid too many relocations/conversions internally and keep things simple. Both of these were design decisions we had voluntarily implemented in #149.

[FenrirWolf]

Aaah, that's a good point about variable captures. I definitely agree with keeping a boxed closure in that case.

I'm not sure why we would want to push string conversion complexity onto the user when we can handle that part internally though, but if that's how we want to handle things then it's not too huge of a deal.

[FenrirWolf]

I pushed a change reverting back to a boxed closure. I personally think that handling string stuff behind the scenes makes for a better API, but if we prefer keeping things the way they are then I won't be heartbroken if we close this PR instead.

[Meziu]

Well, the main thought behind that decision was “ctru-rs would basically have to re-allocate all of the string’s memory just for a small convenience”. That goes a bit too much over the principle of a “safe yet thin wrapper”. In my opinion, CString has a good API and should be more than enough in most cases. By having the user work with it directly, we can avoid heavy overhead.

[FenrirWolf]

That sounds reasonable enough to me. I'll go ahead and close this in that case.

[FenrirWolf]

Actually wait a sec, I believe there is one change here that's still a reasonable one, and even a more performant one than the existing code.

You see, libctru gives us enough information to pass the user a proper &str instead of a &cstr since the internal_callback receives both a text and text_size parameter. So by just making a string slice with that information instead of recomputing the string length with strlen (which happens when making a &cstr), we're both giving the user a better API and saving some overhead. So if nothing else, I think this change is one we'd want to keep.

So with that being said, I pushed another commit that keeps that change while reverting everything else.

[Meziu]

You see, libctru gives us enough information to pass the user a proper &str instead of a &cstr since the internal_callback receives both a text and text_size parameter. So by just making a string slice with that information instead of recomputing the string length with strlen (which happens when making a &cstr)

Alright, this is more like it! I wonder whether we can fully trust all of the data given to us, but by diving in a bit of unsafe code this might actually turn out better than before. A thing to note is that at that point we’d end up in the “asymmetrical” API problem we encountered in the other isse. In short, it’s a bit weird to pass in a &str and get a CString as output. It should either be cstr -> cstr (either references or owned types is no different) or str -> str.

[FenrirWolf]

I wonder whether we can fully trust all of the data given to us, but by diving in a bit of unsafe code this might actually turn out better than before.

Funny you should mention that. The callback is currently called by swkbdMessageCallback, and it looks like it does indeed pass a proper pointer and length. But once #157 is merged, we'll be the ones in charge of calling it and we can be extra sure that the arguments are right.

As for the asymmetrical issue, I'm not sure it matters very much if the goal is to provide an API that's both as ergonomic and performant as possible. In this case the &str -> CString API is the best way to achieve that. And I'll bet we can improve things further once the other PR is in too, since we could potentially replace the libctru callback with one better suited for our purposes.

[adryzz]

what about a Cow?

[FenrirWolf]

And I'll bet we can improve things further once the other PR is in too

I did some experimenting and it turns out there will indeed be better solutions to this issue once #157 is merged, so I'm fine with closing this PR and submitting a better fix later on.