Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update add-on i18n for tinygettext #6453

Closed
wants to merge 3 commits into from
Closed

Update add-on i18n for tinygettext #6453

wants to merge 3 commits into from

Conversation

bunnybot
Copy link

NordfrieseMirrored from Codeberg
Created on Fri May 17 13:15:06 CEST 2024 by Benedikt Straub (Nordfriese)

Type of change
Follow-up to #6411
Counterpart to wl/wl_addons_server#88 (currently served on alpha)

New behavior
We now get add-on translations as PO files from the server. We can now have a cleaner directory structure, and since we control our own cache now, we no longer need the hack of adding the i18n version to the filename.

Possible regressions
Add-on translations, and add-on translation upgrades

@bunnybot bunnybot added this to the v1.3 milestone May 17, 2024
@bunnybot bunnybot self-assigned this May 17, 2024
@bunnybot
Copy link
Author

Assigned to Nordfriese

@bunnybot bunnybot added internationalization Translation system, string fixes, RTL support addon Problems and requests related to add-ons ci:success CI checks succeeded labels May 17, 2024
@bunnybot bunnybot mentioned this pull request May 19, 2024
Open
@bunnybot bunnybot added ci:success CI checks succeeded and removed ci:success CI checks succeeded labels May 19, 2024
hessenfarmer
hessenfarmer reviewed May 20, 2024
View reviewed changes
Copy link
Contributor

@hessenfarmer hessenfarmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM however needs testing see inline comment

src/ui_fsmenu/addons/manager.cc
g_fs->fs_unlink(mo);
}
if (g_fs->file_exists(new_locale_path)) {
g_fs->fs_unlink(new_locale_path);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we had problems in the past and still have imhgo with unlink on windows. Is there an addon with which I could test this from the server.

@bunnybot
Copy link
Author

NordfrieseMirrored from Codeberg
On Mon May 20 13:16:34 CEST 2024, Benedikt Straub (Nordfriese) wrote:

You can use the Alpha server --addon_server_port=7377
Not all add-ons are functional in the Alpha database, but at least Foreign Planet and Frisians Economy Ultra both have working translations there and should be 100% translated in German.

@hessenfarmer
Copy link
Contributor

NordfrieseMirrored from Codeberg On Mon May 20 13:16:34 CEST 2024, Benedikt Straub (Nordfriese) wrote:

You can use the Alpha server --addon_server_port=7377 Not all add-ons are functional in the Alpha database, but at least Foreign Planet and Frisians Economy Ultra both have working translations there and should be 100% translated in German.

ok. so to properly test this I need to use the alpha, and download the frisians Ultra, afterwads I need to change the version manually and redownload it to have the files in question existing. correct?

@bunnybot
Copy link
Author

NordfrieseMirrored from Codeberg
On Mon May 20 13:36:01 CEST 2024, Benedikt Straub (Nordfriese) wrote:

Yes, just edit the addons_i18n_versions file after downloading to set an older version number and restart Widelands for the change to take effect; the add-ons manager should now detect that a newer translations version is available and offer to upgrade.

@hessenfarmer
Copy link
Contributor

ok der File Handle und unlink funktioniert auch unter Windows.

@hessenfarmer
Copy link
Contributor

vielleicht nur ne Randnotiz aber ich hatte jede Menge could not translate: (0 oder 1) messages im log.
Wir sollten diesen Part und den Server Part aber wohl zusammen comitten, damit es kompatibel bleibt.
Wie sollen wir denn mit den Mo Leiochen umgehen, die in den Addon Verzeichnissen noch existieren?

@bunnybot bunnybot added the review:approve The pull request has been approved. label May 20, 2024
@hessenfarmer
Copy link
Contributor

@bunnybot update

@bunnybot
Copy link
Author

Successfully updated the branch as requested in comment 2120367618 at commit 57e3dd3.

@bunnybot bunnybot added ci:success CI checks succeeded and removed ci:success CI checks succeeded labels May 20, 2024
@bunnybot
Copy link
Author

NordfrieseMirrored from Codeberg
On Mon May 20 20:57:52 CEST 2024, Benedikt Straub (Nordfriese) wrote:

Counterpart is deployed on the live system (and alpha serves the website maps integration PR again).

<@>bunnybot merge

The "could not translate" messages are in master too now, but only with --verbose output enabled. Those come from tinygettext, we'll have to look into silencing some of it (maybe only print them if the existing verbose_i18n flag is on).

IMHO we should just keep the MO files around for now, so if users switch between v1.2 and master they don't get their translations autodeleted every time. It's just a few kB so it doesn't really matter much, but after v1.3 we could maybe add a simple cleanup scanner.

@bunnybot bunnybot closed this May 20, 2024
@bunnybot bunnybot deleted the mirror/Nordfriese/widelands/tinygettext-addons branch May 20, 2024 19:03
Noordfrees added a commit to Noordfrees/widelands that referenced this pull request May 20, 2024
@Noordfrees
Update add-on i18n for tinygettext (CB widelands#4815 / GH widelands#…
0551a17 
…6453)

Co-authored-by: Benedikt Straub <benedikt-straub@web.de>
Co-committed-by: Benedikt Straub <benedikt-straub@web.de>
@hessenfarmer
Copy link
Contributor

IMHO we should just keep the MO files around for now, so if users switch between v1.2 and master they don't get their translations autodeleted every time. It's just a few kB so it doesn't really matter much, but after v1.3 we could maybe add a simple cleanup scanner.

I had exactly the same in mind, but just wanted to be sure that we do not forget planning for it.
Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hessenfarmer hessenfarmer hessenfarmer approved these changes

Assignees

@bunnybot bunnybot

Labels
addon Problems and requests related to add-ons ci:success CI checks succeeded internationalization Translation system, string fixes, RTL support review:approve The pull request has been approved.
Projects
None yet
Milestone
v1.3
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@bunnybot @hessenfarmer @Noordfrees