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.

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

Sweep through UI strings to internationalize #277

Closed
gnprice opened this issue Aug 17, 2023 · 8 comments · Fixed by #1325
Closed

Sweep through UI strings to internationalize #277

gnprice opened this issue Aug 17, 2023 · 8 comments · Fixed by #1325
Assignees
Labels
a-i18n Translation, localization, internationalization

Comments

@gnprice
Copy link
Member

gnprice commented Aug 17, 2023

This is a followup to #275. After we have that basic framework for using translated strings in the UI, we'll want to sweep through our existing code and convert it all to use that framework.

Initially this won't do much of anything from a user's perspective, because we won't have more than a handful of strings translated until we wire things up to a service where people can contribute such translations, #276.

Nevertheless it's useful to do this early, because it sets the pattern so that we naturally follow it in new code, rather than having more code that needs to change in an eventual sweep later. We may also come across places that need a context object and didn't before, or otherwise call for refactoring, and it's convenient to do that sooner so that other changes to the same code are based on the new structure from the beginning.

Other notes:

  • Many of these spots are marked with TODO(i18n), though many others are not.
  • Some spots marked TODO(i18n) are for different needs like formatting dates and times. Those are out of scope of this issue.
  • If some strings are especially difficult to wire up for translation for some reason, we can mark them with TODO(i18n) and also file followup issues, and then leave them out for this issue.
@gnprice gnprice added the a-i18n Translation, localization, internationalization label Aug 17, 2023
@gnprice gnprice added this to the Alpha milestone Aug 17, 2023
sirpengi added a commit to sirpengi/zulip-flutter that referenced this issue Sep 20, 2023
@gnprice gnprice self-assigned this Oct 9, 2023
@gnprice gnprice modified the milestones: Alpha, Beta 2 Nov 8, 2023
@gnprice gnprice modified the milestones: Beta 2, Beta 3 Nov 22, 2023
@Detective-Khalifah

This comment was marked as off-topic.

@gnprice gnprice modified the milestones: B2: Summer 2024, Launch May 9, 2024
@PIG208 PIG208 self-assigned this Dec 12, 2024
@gnprice gnprice modified the milestones: M5: Launch, M5-a: Server 10 Jan 14, 2025
@PIG208 PIG208 linked a pull request Jan 21, 2025 that will close this issue
PIG208 added a commit to PIG208/zulip-flutter that referenced this issue Feb 4, 2025
PIG208 added a commit to PIG208/zulip-flutter that referenced this issue Feb 4, 2025
PIG208 added a commit to PIG208/zulip-flutter that referenced this issue Feb 4, 2025
@gnprice
Copy link
Member Author

gnprice commented Feb 5, 2025

The bulk of this is done now, after #1148. (We've also largely been internationalizing new code as we go, over the past year and a half since this issue was filed.)

There's one known remaining spot to be handled, and then some more sweeping to do to confirm that that's all:
#1148 (comment)

PIG208 added a commit to PIG208/zulip-flutter that referenced this issue Feb 5, 2025
@PIG208
Copy link
Member

PIG208 commented Feb 5, 2025

Opened #1325 for the remaining cases.

Follow-up issues for more complex translations:

#1285 for _errorUnimplemented

InlineSpan _errorUnimplemented(UnimplementedNode node, {required BuildContext context}) {
  final contentTheme = ContentTheme.of(context);
  final errorStyle = contentTheme.textStyleError;
  final errorCodeStyle = contentTheme.textStyleErrorCode;
  // For now this shows error-styled HTML code even in release mode,
  // because release mode isn't yet about general users but developer demos,
  // and we want to keep the demos honest.
  // TODO(#194) think through UX for general release
  // TODO(#1285) translate this
  final htmlNode = node.htmlNode;
  if (htmlNode is dom.Element) {
    return TextSpan(children: [
      TextSpan(text: "(unimplemented:", style: errorStyle),
      TextSpan(text: htmlNode.outerHtml, style: errorCodeStyle),
      TextSpan(text: ")", style: errorStyle),
    ]);
  } else if (htmlNode is dom.Text) {
    return TextSpan(children: [
      TextSpan(text: "(unimplemented: text «", style: errorStyle),
      TextSpan(text: htmlNode.text, style: errorCodeStyle),
      TextSpan(text: "»)", style: errorStyle),
    ]);
  } else {
    return TextSpan(
      text: "(unimplemented: DOM node type ${htmlNode.nodeType})",
      style: errorStyle);
  }
}'

(hmm, maybe we can have "(unimplemented: DOM node type ${htmlNode.nodeType})" translated first?)

and inlineLink('said', url).

    return '${userMention(sender!, silent: true)} ${inlineLink('said', url)}:\n' // TODO(#1285)
      '${wrapWithBacktickFence(content: rawContent, infoString: 'quote')}';

#1284 for notification channel name.

    await _androidHost.createNotificationChannel(NotificationChannel(
      id: kChannelId,
      name: 'Messages', // TODO(#1284)
      importance: NotificationImportance.high,
      lightsEnabled: true,
      soundUrl: defaultSoundUrl,
      vibrationPattern: kVibrationPattern,
    ));

PIG208 added a commit to PIG208/zulip-flutter that referenced this issue Feb 5, 2025
PIG208 added a commit to PIG208/zulip-flutter that referenced this issue Feb 5, 2025
@gnprice
Copy link
Member Author

gnprice commented Feb 5, 2025

Continuing the sweep from #1148 (comment), I found several more beyond what's in #1325:

lib/notifications/display.dart
29:  chime2(resourceName: 'chime2', fileDisplayName: 'Zulip - Low Chime.m4a'),
30:  chime3(resourceName: 'chime3', fileDisplayName: 'Zulip - Chime.m4a'),
31:  chime4(resourceName: 'chime4', fileDisplayName: 'Zulip - High Chime.m4a');

I think these will be tricky to actually handle — but they are user-facing strings communicating with English words ("chime", "low", "high"), so definitely in the category of strings that should in principle be translated. I'll add a TODO(i18n) comment for them.


lib/widgets/about_zulip.dart
46:                  subtitle: Text(_packageInfo?.version ?? '(…)')),

This seems like it should straightforwardly get translated.


lib/widgets/app.dart
184:          title: 'Zulip',

This too. It's potentially a pretty conspicuous string, even:

  /// A one-line description used by the device to identify the app for the user.
  ///
  /// On Android the titles appear above the task manager's app snapshots which are
  /// displayed when the user presses the "recent apps" button. […]

lib/widgets/compose_box.dart
712:      .map((file) => '${file.filename}: ${(file.length / (1 << 20)).toStringAsFixed(1)} MiB')

lib/widgets/lightbox.dart
340:    final hours = value.inHours.toString().padLeft(2, '0');
341:    final minutes = value.inMinutes.remainder(60).toString().padLeft(2, '0');
342:    final seconds = value.inSeconds.remainder(60).toString().padLeft(2, '0');
343:    return '${hours == '00' ? '' : '$hours:'}$minutes:$seconds';

I'll give this a TODO(i18n), like other places we format times and dates.


lib/widgets/login.dart
236:                  hintText: 'your-org.zulipchat.com')),

So that's 6 sites. I also found three spots involving dates and times that had a TODO but not a TODO(i18n) (nor any specific issue instead).

I'll send changes that just add or update comments in 5 of those places: the three just mentioned, and the two above that need a TODO added.

That leaves 4 others that should get fixed before we declare this issue complete.

gnprice added a commit to gnprice/zulip-flutter that referenced this issue Feb 5, 2025
Toward zulip#277.

These may be tricky to actually handle translating, particularly
given the logic in _ensureInitNotificationSounds that looks at the
filenames of existing files.  They're also in a pretty obscure
corner where I expect very few users see them, so not a priority.

But they (a) are user-facing and (b) use English words ("chime",
"low", "high") to communicate, so are squarely in the category of
strings that should in principle get translated.  Mark them as such.
@PIG208
Copy link
Member

PIG208 commented Feb 6, 2025

For this string,

lib/widgets/app.dart
184:          title: 'Zulip',

it looks like we should use onGenerateTitle instead to get it localized?

  /// A one-line description used by the device to identify the app for the user.
  ///
  /// On Android the titles appear above the task manager's app snapshots which are
  /// displayed when the user presses the "recent apps" button.
  /// [...]
  /// To provide a localized title instead, use [onGenerateTitle].
  /// {@endtemplate}
  final String? title;
  /// If non-null this callback function is called to produce the app's
  /// title string, otherwise [title] is used.
  ///
  /// The [onGenerateTitle] `context` parameter includes the [WidgetsApp]'s
  /// [Localizations] widget so that this callback can be used to produce a
  /// localized title.
  ///
  /// This callback function must not return null.
  ///
  /// The [onGenerateTitle] callback is called each time the [WidgetsApp]
  /// rebuilds.
  /// {@endtemplate}
  final GenerateAppTitle? onGenerateTitle;

The API seems straightforward, so it makes sense to include this refactor in the PR.

@PIG208
Copy link
Member

PIG208 commented Feb 6, 2025

Hmm, yesterday I did skim pass some of the strings you mentioned, but because I was looking for the more typical translatable strings, skipped things like the host name. In hindsight there should at least be a TODO or a resolution comment for each.

@gnprice
Copy link
Member Author

gnprice commented Feb 6, 2025

Yeah, onGenerateTitle looks right for handling the title.

And yep, exactly — all of the strings that in principle should be translated (or that look like they should) should get at least a comment. I think for the hostname hint, and the others I listed above but didn't include in #1332, the answer is they should just get translated.

rishichirchi pushed a commit to rishichirchi/zulip-flutter that referenced this issue Feb 7, 2025
Toward zulip#277.

These may be tricky to actually handle translating, particularly
given the logic in _ensureInitNotificationSounds that looks at the
filenames of existing files.  They're also in a pretty obscure
corner where I expect very few users see them, so not a priority.

But they (a) are user-facing and (b) use English words ("chime",
"low", "high") to communicate, so are squarely in the category of
strings that should in principle get translated.  Mark them as such.
@gnprice
Copy link
Member Author

gnprice commented Feb 12, 2025

This was completed by #1325.

@gnprice gnprice closed this as completed Feb 12, 2025
github-actions bot pushed a commit that referenced this issue Feb 12, 2025
Toward #277.

These may be tricky to actually handle translating, particularly
given the logic in _ensureInitNotificationSounds that looks at the
filenames of existing files.  They're also in a pretty obscure
corner where I expect very few users see them, so not a priority.

But they (a) are user-facing and (b) use English words ("chime",
"low", "high") to communicate, so are squarely in the category of
strings that should in principle get translated.  Mark them as such.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-i18n Translation, localization, internationalization
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants