i18n: Localize Nautilus .py script#9976
Conversation
swsnr
left a comment
There was a problem hiding this comment.
I'm not a member of the ghostty project, so please excuse me for leaving a review, but as the original author of this script (albeit for Wezterm originally), and being somewhat familiar with xgettext and Python, I'd like to leave two remarks concerning this pull request 🙂
dist/linux/ghostty_nautilus.py
Outdated
| import gettext | ||
|
|
||
| DOMAIN = "com.mitchellh.ghostty" | ||
| share_dir = os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) |
There was a problem hiding this comment.
I think this would be more readable with pathlib:
from pathlib import Path
locale_dir = Path(__file__).absolute().parent.parent.parent / "locale"And does the Ghostty build system even guarantee that the locales always end up at this path, relative to the nautilus extension directory?
There was a problem hiding this comment.
I'll note that you can use .parents[2] instead of .parent.parent.parent =)
There was a problem hiding this comment.
@trag1c Oh, thanks 🙏 I learn a new thing every day 🙂
There was a problem hiding this comment.
@swsnr As far as I can see, the nautilus extension is here
GhosttyResources
try steps.append(b.allocator, &b.addInstallFile(
b.path("dist/linux/ghostty_nautilus.py"),
"share/nautilus-python/extensions/ghostty.py",
).step);
and the locale is relative to share so share/locale/....
During the build process, each locale in `.po` files is compiled
into binary `.mo` files, stored under `share/locale/<LOCALE>/LC_MESSAGES/com.mitchellh.ghostty.mo`.
This can be directly accessed by `libintl`, which provide the various `gettext`
....
The only issue is what I mentioned in the original comment where some other platforms put the locale with a different suffix and then it doesnt work, so it just fallbacks.
There was a problem hiding this comment.
I guess it'll work then. However, I think a comment which explains this rationale would help for future understanding 🙂
src/build/GhosttyI18n.zig
Outdated
| "-", | ||
| }); | ||
| // Silences the "unknown extension" errors | ||
| _ = xgettext.captureStdErr(); |
There was a problem hiding this comment.
I don't think that that's a good idea.
I believe the proper way to handle different types of sources with different flags in xgettext is to use separate xgettext invocations to extract messages into intermediate pot files, and then use a final xgettext invocation to merge the intermediate pot files into the final one.
There was a problem hiding this comment.
Yeah thanks, I have edited it now. Much cleaner this way.
There was a problem hiding this comment.
You're welcome. Can't review the zig code tho, as I'm entirely unfamiliar with the language 🙂
|
@swsnr Even though it seems you are not an official maintainer, I saw emoji reactions by at least a maintainer, so I assume these changes were approved as well from them. I made those changes, lmk and thanks for the review! |
|
|
||
| // Collect to intermediate .pot file | ||
| xgettext.addArg("-o"); | ||
| const gtk_pot = xgettext.addOutputFileArg("gtk.pot"); |
There was a problem hiding this comment.
I'm entirely unfamiliar with zig and its build system, so pardon me if this is a stupid question, but shouldn't you cleanup these temporary POT files after the xgettext_merge down below has finished?
There was a problem hiding this comment.
No, since it would only be output to Zig's output cache (./.zig-cache) and not the final prefix
|
|
||
| // Collect to intermediate .pot file | ||
| xgettext.addArg("-o"); | ||
| const gtk_pot = xgettext.addOutputFileArg("gtk.pot"); |
There was a problem hiding this comment.
No, since it would only be output to Zig's output cache (./.zig-cache) and not the final prefix
|
Should these wait until after #10632 is done, or should we try to sneak these into 1.3? |
|
If it's not too much work for the translators then I think we should try to squeeze these in 1.3. It's partially my fault this PR has stalled for this long |
|
It's a single short string, so I don't expect it to be too hard on the translators; I'm more concerned about the administration nightmare, since:
It's not like it's too difficult though, it's just really easy to get confused with the translations that haven't been updated yet. On the contrary, nothing says we have to get every translation for this in before 1.3, so we could just prioritize the sixteen already updated translations. |
|
#9904 was merged recently which adds two strings, which means we have to deal with the problem anyway. At the moment there's been three PRs which add those two strings (#10788 #10792 #10794). It's probably best to merge this as soon as possible, so that we don't have to deal with the same problem twice and with an extra division. (This needs another merge/rebase since it conflicts with every translation file again apparently…) |
|
@00-kat done! Hope no more new strings are added 😛 |
Closes #9266.
Big Note: I noticed that this worked properly under
NixOS, but on myUbuntuVM it didnt.The reason is in src/build/GhosttyI18n.zig because the locale is expected in the
<lang>_<region>without the encoding suffix.<lang>_<region>_<encoding>If i force it to always trim the encoding it works, but I am guessing its there for a reason ,so maybe some of the maintainer can shed some light in the best way forward, as I am not an expert in how other systems deal with it. Here you see
Open in Ghostty-> Abrir con Ghosttypyfile withruffbut didnt want to drown the changes, so maybe something that could be worth doing so that also ourpyfiles have std formatting.Note
Used AI only for helping me debug where the locales could be and why was it not detected, but no code help whatsoever