Conversation
|
Attention: The translation is incomplete, as some lines do not exist in Phosh |
|
Left some initial comments, I'd maybe spin out the phosh related parts into a separate MR as I assume they can be applied right away. |
|
|
||
| fn main() -> anyhow::Result<()> { | ||
| let _ = gettextrs::bindtextdomain("phosh", "/usr/share/locale"); | ||
| gettextrs::textdomain("phosh")?; |
There was a problem hiding this comment.
? correct here as that would mean early return if gettext can't be initialized.
There was a problem hiding this comment.
I would prefer the app start up with default English strings rather than not start up at all 😅
So how about:
| gettextrs::textdomain("phosh")?; | |
| if let Err(err) = gettextrs::textdomain("phosh") { | |
| log::warn("onoes no i18n"...); | |
| } |
(and move this init after the following logger lines so that the log works properly :)
| Request::CreateSession { .. } => anyhow::Ok(Response::AuthMessage { | ||
| auth_message_type: Secret, | ||
| auth_message: "Password:".into(), | ||
| auth_message: gettextrs::gettext("Password:").into(), |
There was a problem hiding this comment.
If you add new translations you need to bind another gettext domain (phrog) as otherwise the lookups will be done in phosh only (and then use dgettext("phrog", …) to look things up in the correct gettext domain.
Phosh currently uses the gettext domain for lookups but I we could switch to using gi18n-lib.h everywhere to fix the phosh gettext domain - that said using dgettext here right away doesn't have any downsides as far as I know.
There was a problem hiding this comment.
The dgettext approach sounds great.
So I think this boils down to:
| auth_message: gettextrs::gettext("Password:").into(), | |
| auth_message: gettextrs::dgettext("phrog", "Password:").into(), |
(but also note that I think clippy is complaining about this .into())
| pub fn init() -> anyhow::Result<()> { | ||
| gio::resources_register_include!("phrog.gresource").context("failed to register resources.")?; | ||
|
|
||
| let _ = gettextrs::bindtextdomain("phosh", "/usr/share/locale"); |
There was a problem hiding this comment.
In other projects we take that path from meson but idk what's @samcday 's preference is here.
There was a problem hiding this comment.
Let's just hardcode for now since this is what distros are gonna use anyway. Maybe @hustlerone could wander past here and let us know whether this would be an issue with Nix packaging. If so I'm sure (or hoping, at least) that we can still easily solve this with Cargo / build.rs and not need to bring in the Meson bazooka.
|
@codex review |
samcday
added
the
ci-ok
|
Codex Review: Didn't find any major issues. Swish! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
samcday
left a comment
samcday
left a comment
There was a problem hiding this comment.
Thanks for this contribution @x1z53 ... And apologies that I had not already done this minimum groundwork to support the rich existing translations from Phosh.
As Guido noted, we should bind the default domain as you've done to ensure Phosh works as expected, and then just make carefully scoped dgettext("phrog", "foo...") calls for phrog-specific bits.
As for how we juggle the translations for phrog itself, let's defer on any of that in this PR and get this landed as soon as you've fixed up the requested changes. If you want to then follow-up with some extra bits to support potfiles and their wranglin', that'd be swell 🙏
|
|
||
| fn main() -> anyhow::Result<()> { | ||
| let _ = gettextrs::bindtextdomain("phosh", "/usr/share/locale"); | ||
| gettextrs::textdomain("phosh")?; |
There was a problem hiding this comment.
I would prefer the app start up with default English strings rather than not start up at all 😅
So how about:
| gettextrs::textdomain("phosh")?; | |
| if let Err(err) = gettextrs::textdomain("phosh") { | |
| log::warn("onoes no i18n"...); | |
| } |
(and move this init after the following logger lines so that the log works properly :)
| Request::CreateSession { .. } => anyhow::Ok(Response::AuthMessage { | ||
| auth_message_type: Secret, | ||
| auth_message: "Password:".into(), | ||
| auth_message: gettextrs::gettext("Password:").into(), |
There was a problem hiding this comment.
The dgettext approach sounds great.
So I think this boils down to:
| auth_message: gettextrs::gettext("Password:").into(), | |
| auth_message: gettextrs::dgettext("phrog", "Password:").into(), |
(but also note that I think clippy is complaining about this .into())
| pub fn init() -> anyhow::Result<()> { | ||
| gio::resources_register_include!("phrog.gresource").context("failed to register resources.")?; | ||
|
|
||
| let _ = gettextrs::bindtextdomain("phosh", "/usr/share/locale"); |
There was a problem hiding this comment.
Let's just hardcode for now since this is what distros are gonna use anyway. Maybe @hustlerone could wander past here and let us know whether this would be an issue with Nix packaging. If so I'm sure (or hoping, at least) that we can still easily solve this with Cargo / build.rs and not need to bring in the Meson bazooka.
3 participants
No description provided.