After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 377306 - [sunaudiomixer] mixer track labels not localized
[sunaudiomixer] mixer track labels not localized
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other opensolaris
: Normal normal
: 0.10.6
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on: 414645
Blocks:
 
 
Reported: 2006-11-20 06:04 UTC by Takao Fujiwara
Modified: 2007-03-10 07:25 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch for gst-libs/gst/interfaces/mixertrack.h (461 bytes, patch)
2006-11-20 06:06 UTC, Takao Fujiwara
rejected Details | Review
Patch for sys/sunaudio/gstsunaudio.c and sys/sunaudio/gstsunaudiomixertrack.c (961 bytes, patch)
2006-11-20 06:07 UTC, Takao Fujiwara
none Details | Review
Patch for gst-mixer/src/track.c and gst-mixer/src/preferences.c (930 bytes, patch)
2006-11-20 06:08 UTC, Takao Fujiwara
none Details | Review
ja.po for gst-plugins-good (9.00 KB, text/plain)
2006-11-20 06:10 UTC, Takao Fujiwara
  Details
Patch for POTFILES.in (775 bytes, patch)
2006-11-20 06:12 UTC, Takao Fujiwara
committed Details | Review
untranslated_label version for gst-plugins-base gst-plugins-good and gnome-media (3.97 KB, patch)
2006-12-20 09:40 UTC, Takao Fujiwara
none Details | Review
Revised the patch from Comment #12 (6.09 KB, patch)
2007-02-28 13:24 UTC, Takao Fujiwara
none Details | Review
make sunaudiomixertrack use the untranslated-label property if supported (2.69 KB, patch)
2007-03-04 19:04 UTC, Tim-Philipp Müller
committed Details | Review

Description Takao Fujiwara 2006-11-20 06:04:54 UTC
When invoke gnome-volume-control on Solaris, "Volume", "Gain" and "Monitor" are not localized.

gnome-volume-control compare /dev/$dev with the $dev label internally so cannot change the strings simply. I added *l10n_label in GstMixerTrack struct.
I'm attaching the patch. Probably this patch can fix #154054.
Comment 1 Takao Fujiwara 2006-11-20 06:06:33 UTC
Created attachment 76898 [details] [review]
Patch for gst-libs/gst/interfaces/mixertrack.h
Comment 2 Takao Fujiwara 2006-11-20 06:07:49 UTC
Created attachment 76899 [details] [review]
Patch for sys/sunaudio/gstsunaudio.c and sys/sunaudio/gstsunaudiomixertrack.c
Comment 3 Takao Fujiwara 2006-11-20 06:08:48 UTC
Created attachment 76900 [details] [review]
Patch for gst-mixer/src/track.c and gst-mixer/src/preferences.c
Comment 4 Takao Fujiwara 2006-11-20 06:10:52 UTC
Created attachment 76901 [details]
 ja.po for gst-plugins-good
Comment 5 Takao Fujiwara 2006-11-20 06:12:05 UTC
Created attachment 76902 [details] [review]
Patch for POTFILES.in
Comment 6 Tim-Philipp Müller 2006-11-20 12:29:48 UTC
Some comments:

 - GstMixerTrack is public API. You can't just add a new member to
   the structure somewhere in the middle, this breaks ABI compatibility.
   If at all, this needs to be added to the end with the padding reduced
   (however, we don't seem to have any padding in this case, not sure
   if we could still add an extra member or not).

 - _if_ you add a new member like label_l10n and then change
   gnome-volume-control etc. to use it you should really also
   make sure that all other GstMixerTrack users (alsamixer, ossmixer, etc.)
   set this new member, otherwise gnome-volume-control will no longer
   show any labels for alsa/oss users, will it?

 - in your patch to gst-mixer/src/track.c: why is the l10n label used only
   for vertical orientations, but not horizontal ones? (admittedly the
   translation marker in the code for the horizontal label doesn't really
   make a whole lot of sense, but still).

 - where in the code does gnome-volume-control compare mixer track
   labels with device names?


As far as I know alsa mixer track labels are localised by default, so if it is possible at all it would be great if the same could be done for the sun audio mixer tracks (ie. just localising mixertrack->label directly instead of adding a new struct member for this).

Comment 7 Takao Fujiwara 2006-11-30 07:43:43 UTC
Thanks for the comments with details.

> - where in the code does gnome-volume-control compare mixer track
>   labels with device names?

gnome-media/gst-mixer/src/track.c

  for (i = 0; !found && pix[i].label != NULL; i++) {

    gchar *label_l = g_strdup (track->label);

    if (g_strrstr (label_l, pix[i].label) != NULL) {
      str = pix[i].pixmap;
      found = TRUE;
    }

    g_free (label_l);
  }

The track->label is the gst-plugins-good and pix[i].label is gst-mixer.
Do you think we also should i18n pix[i].label?
Comment 8 Tim-Philipp Müller 2006-12-11 15:13:10 UTC
> > - where in the code does gnome-volume-control compare mixer track
> >   labels with device names?
> 
> gnome-media/gst-mixer/src/track.c: (snip)

Ah, I see. The code there just tries to guess an icon for the track from the mixer label. This already fails with alsamixer now if the label is translated and doesn't match; the patch to translate the alsamixer labels in GStreamer was committed by the primary gnome-volume-control maintainer as well, so I'm sure it's not a problem, and it only affects the icon anyway.

 
> The track->label is the gst-plugins-good and pix[i].label is gst-mixer.
> Do you think we also should i18n pix[i].label?

No, I don't think that's a good idea (since chances are that the translation in gnome-volume-control doesn't match the one in the GStreamer mixer implementation later on).

IMHO, we should always translate track->label as we do now in alsamixer already and think about a way to provide the non-translated info to gnome-volume-control in some form (ie. possibly adding a track->untranslated_label or so, if that's possible without breaking ABI compatibility, otherwise we could always add a gst_mixer_track_get_untranslated_device() or _get_icon_hint() or whatever function).

On a side note, I don't entirely understand the point of the fill_labels() function in gstsunaudiomixertrack.c (cases[x].given is never used, and why not just move the array of N_()'ed strings into gst_sunaudiomixer_track_new() directly?) - am I missing something? :)

Comment 9 Takao Fujiwara 2006-12-20 09:40:30 UTC
Created attachment 78671 [details] [review]
untranslated_label version for gst-plugins-base gst-plugins-good and gnome-media

I tried one of your suggestions.
This patch + patch id=76902 works fine with me.
Comment 10 Takao Fujiwara 2006-12-20 10:02:54 UTC
Thanks for the suggestion.
I created the patch for one of your suggestion.

When we continue to use track->label as the translated label, gome-volume-control removes the tags internally:

gnome-media/gst-mixer/src/preferences.c
gchar *
get_gconf_key (GstMixer *mixer, GstMixerTrack *track)
{
...
  for (i = 0; dev[i] != '\0'; i++) {
    if (g_ascii_isalnum (dev[i]))
      res[pos++] = dev[i];
  }
...
  for (i = 0; label[i] != '\0'; i++) {
    if (g_ascii_isalnum (label[i]))
      res[pos++] = label[i];
  }
..
}

The g_ascii_isalnum removes multibyte labels so this patch(id=78671) is slightly different from the previous one.

Could you review this patch?

> otherwise we could always add a gst_mixer_track_get_untranslated_device() or _get_icon_hint() or whatever function).

Yes, it also can be implemented.
I think the gst_mixer_track_get_untranslated_device would be:

char *
gst_mixer_track_get_untranslated_device (char *locale_label)
{
  char *retval = locale_label;

  if (!locale_label)
    return NULL;

  if (!labels)
    fill_labels ();

  for (i = 0; i < MIXER_DEVICES; i++) {
    if (!strcmp (_(case[i].wanted), locale_label)) {
      retval = case[i].wanted;
      break;
    }
  }

  return retval;
}

What do you think? Personaly I'm fun to add the untranslated_label.

> On a side note, I don't entirely understand the point of the fill_labels()
function in gstsunaudiomixertrack.c

You're right. Probably it can be removed. However since I don't provide the original source file, I'm not sure what is intended in this part.
Comment 11 Takao Fujiwara 2007-02-21 15:07:24 UTC
How is this status?
Could we integrate the patch? 
Comment 12 Tim-Philipp Müller 2007-02-21 16:06:27 UTC
> How is this status?
> Could we integrate the patch? 

Not in the current form, since it breaks ABI, see my previous comments.

I was meaning to commit the bit that makes the track name translatable though.

That leaves the issue of how to expose the untranslatable name. Not sure what's best here. I can think of two options: either

 1) we add setters/getters and store the untranslated
    string privately somewhere (ie. not in the struct), or

 2) or we instead add an optional get_type_hint() method to the
    mixer interface structure to get an icon/type hint enum, so
    we'd basically move the code from the volume control to guess
    a suitable icon into GstMixer and return an enum for this.

Depends on whether the untranslated name is useful for anything else, I guess.

Do others have opinions?
Comment 13 Takao Fujiwara 2007-02-22 13:30:10 UTC
OK, I understand either the additional translated_label or untranslated_label is not an option because of ABI.

However I may not understand your suggestion exactly. Please let me confirm the possible way.

> 1) we add setters/getters and store the untranslated
>    string privately somewhere (ie. not in the struct), or

If I correctly understand, it seems gst-mixer will do like:

gst_mixer_set_label (track, "Volume");
gui_label = _("Volume");

If yes, how to determine the track is the "Volume"?
As you know, the current gst-mixer compares the track->label to determine the icon.
Could you give me advices?

> 2) or we instead add an optional get_type_hint() method to the
>    mixer interface structure to get an icon/type hint enum, so

In this sugggestion, which do you expect track->label is C(English) or localized as the specification?
If track->label is English, I think we need an additional API, e.g. 

gui_label = gst_mixer_get_translation (track->label);

If track->label is localized, I think we also need the English label because g_ascii_isalnum() does not work for multibyte chars as I mentioned in the previous comment.

I guess the another way is gst-mixer use the icon hint instead of track->label.

What do you think?

Thanks.
Comment 14 Tim-Philipp Müller 2007-02-22 13:57:47 UTC
track->label will contain the translated label, that's how it is with alsa (FWIW, that change was made by the author of gst-mixer/gnome-volume-control) and how it should be IMHO.

gst-mixer/gnome-volume-control shouldn't try to interpret track->label to get an icon (admittedly that's the best they can do for now though, but it's still not really right).

In my case 1), gst-mixer would need to call a yet-to-be-added gst_mixer_track_get_untranslated_label() or something like that.

In my case 2), gst-mixer would call a yet-to-be-added gst_mixer_track_get_type_hint() or something like that.
Comment 15 Takao Fujiwara 2007-02-22 14:44:44 UTC
Thanks for your reply.
I understood.
Comment 16 Takao Fujiwara 2007-02-28 13:24:18 UTC
Created attachment 83540 [details] [review]
Revised the patch from Comment #12 

OK, I updated the patch.
Could you review the attachment?

I added gst_mixer_track_set_untranslated_label() and gst_mixer_track_get_untranslated_label().

I'm not sure if the icon hint function is needed.

Thanks.
Comment 17 Tim-Philipp Müller 2007-03-04 19:02:53 UTC
> OK, I updated the patch.
> Could you review the attachment?
> 
> I added gst_mixer_track_set_untranslated_label() and
> gst_mixer_track_get_untranslated_label().
> 
> I'm not sure if the icon hint function is needed.

On second thought, it seems best to just add a new property for this to GstMixerTrack instead of setter/getter API (I didn't notice we had properties for the other things already). The advantage of this would be that applications can easily check whether the property exists or not and don't need to bump their -base requirements for something trivial like this.

I've filed this as bug #414645, including a patch.

Will also include a patch against sunaudiomixer for this. Could you test it please?
Comment 18 Tim-Philipp Müller 2007-03-04 19:04:15 UTC
Created attachment 83915 [details] [review]
make sunaudiomixertrack use the untranslated-label property if supported
Comment 19 Tim-Philipp Müller 2007-03-09 19:55:29 UTC
Committed with (untested) unit test:

 2007-03-09  Tim-Philipp Müller  <tim at centricular dot net>

        * sys/sunaudio/gstsunaudio.c: (plugin_init):
        * sys/sunaudio/gstsunaudiomixertrack.c:
        (gst_sunaudiomixer_track_new):
          Actually translate sunaudio mixer track labels instead of just
          marking the strings as translatable (#377306); clean up weird
          label string mapping code that serves no apparent purpose. Also
          set the 'untranslated-label' property when creating mixer tracks
          if the GstMixerTrack base class supports this.

        * tests/check/Makefile.am:
        * tests/check/elements/.cvsignore:
        * tests/check/elements/sunaudio.c: (GST_START_TEST),
        (sunaudio_suite):
          Very minimalistic unit test for sunaudiomixer element (compiles, but not
          actually tested on a system where sunaudiomixer is available).


2007-03-09  Tim-Philipp Müller  <tim at centricular dot net>

        * po/LINGUAS:
        * po/ja.po:
          Add ja.po file from #377306.

Comment 20 Takao Fujiwara 2007-03-10 07:25:09 UTC
Thanks for your works.
Our Japanese staffs had a office moving last week so I'm busy because of no hardwares.
Please go ahead.