Created attachment 76898 [details] [review]
Patch for gst-libs/gst/interfaces/mixertrack.h

Created attachment 76899 [details] [review]
Patch for sys/sunaudio/gstsunaudio.c and sys/sunaudio/gstsunaudiomixertrack.c

Created attachment 76900 [details] [review]
Patch for gst-mixer/src/track.c and gst-mixer/src/preferences.c

Created attachment 76901 [details]
 ja.po for gst-plugins-good

Created attachment 76902 [details] [review]
Patch for POTFILES.in

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).

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?

> > - 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? :)

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.

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.

How is this status?
Could we integrate the patch? 

> 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?

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.

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.

Thanks for your reply.
I understood.

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.

> 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?

Created attachment 83915 [details] [review]
make sunaudiomixertrack use the untranslated-label property if supported

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.

Thanks for your works.
Our Japanese staffs had a office moving last week so I'm busy because of no hardwares.
Please go ahead.