This adds a textgroup parameter everywhere so other modules can add strings to the list and have them in the right text group (other than 'default').

The patch is simple though it needs to add the field everywhere:
- Textgroup parameter for l10_client_add_string_to_page()
- This makes the strings array indexed by textgroup and source
- Adds this new field in _l10n_client_dom_strings()
- Two lines of javascript to have the value submitted.

My use case is i18n module (which adds strings to l10n client's list) but for different text groups. With this one, l10n client can work with i18n (i18nstrings) to translate user defined strings. This is related to: #287210: Some new features ported from 5.x version

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Jose Reyero’s picture

Status: Active » Needs review
mathieu’s picture

Patch still worked, but didn't apply cleanly anymore.. so here's a new version for l10_client-6.x-1.7.

j0nathan’s picture

subscribing...

neurovation.kiwi’s picture

FileSize
6.4 KB

Hi folks,

just added another change - cause there was some problem with passing on the real textgroup instead of the default 'default'.

enjoy!

jhedstrom’s picture

The patch in #4 doesn't apply cleanly (and doesn't apply completely on my machine). Attached is the same patch, rerolled against the latest version.

I think this patch still needs work because it has some pretty serious usability issues for translators. The main issue being that non-default sources are listed twice, without any visual indicator as to which text group they are in. If the translator only translates one of them, the translations for the given string are in a hit-and-miss sort of state.

neomenlo’s picture

Is there a reason why the groups have to be separated?

I'd like to see them all in the same list, to keep things very simple.

aufumy’s picture

in case of collision of words, such as 'associate' noun or verb.

Also view http://drupal.org/node/307338

miro_dietiker’s picture

Status: Needs review » Reviewed & tested by the community

Tested this in production and checked code. Looks pretty neat.

miro_dietiker’s picture

Status: Reviewed & tested by the community » Needs review

Sorry, this RTBC was wrong.
I've tested a lot and all patches don't apply cleanly anymore.

So i've built a new version which is tested and working in production anymore.
This time against -dev with updated code.

I can't see any double text string output. Everything is clean in my UI - as long as you apply the i18n text group patch:
#304434: i18nstrings l10n_client support.

Once fixed this will bring us much further in translation workflows. Please review!

robby.smith’s picture

@miro_dietiker - sorry, you mention you've built a new version..did you forget to upload the patch? or has this issue been moved to #304434: i18nstrings l10n_client support
thanks

miro_dietiker’s picture

sorry.. thought i've uploaded the patch.
here it is. hopefully still the right version.

maijs’s picture

Nope, still can't apply #11 to 6.x-dev version.

$ patch -p0 < l10n_client_i18n_textgroup_2.patch 
patching file l10n_client.module
Hunk #8 FAILED at 443.
1 out of 8 hunks FAILED -- saving rejects to file l10n_client.module.rej
patching file l10n_client.js
dawehner’s picture

This is a new version, which applies cleanly.

Before i reviewed the code, and it looked very fine. So this could be made rtbc by someone else.

dawehner’s picture

This is a new version, which applies cleanly.

Before i reviewed the code, and it looked very fine. So this could be made rtbc by someone else.

miro_dietiker’s picture

Status: Needs review » Reviewed & tested by the community

Well, thx, looks fine to me. Minor changes happened and many people already tested the base patches.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Great, committed this in. I've fixed one issue I found though. In this code segment:

      if (!array_key_exists($data->source, $strings['default'])) {
        $strings[$data->textgroup][$data->source] = (empty($data->translation) ? TRUE : $data->translation);
      }

I think the first 'default' was an issue since we'd have checked an array which we are not working on then :) So modified to check the same array:

      if (!array_key_exists($data->source, $strings[$data->textgroup])) {
        $strings[$data->textgroup][$data->source] = (empty($data->translation) ? TRUE : $data->translation);
      }
miro_dietiker’s picture

Glad to see such progress here. Sure you're right gabor. I'm sure this will fix some of our issues we still had!

Right updated to this version which seems to work smooth.
(Only missing per-user-disable feature of l10n_client which is in issue queue now...)

Thank you!

Gábor Hojtsy’s picture

Yes, I'll look at per-user disable next :)

Gábor Hojtsy’s picture

Version: 6.x-1.x-dev » 7.x-1.x-dev
Status: Fixed » Patch (to be ported)

Needs to be ported / committed to 7.x as well.

dawehner’s picture

PS: It helps if you post the exact patch you commited.

Currently the d7 version seams to be broken(the js interface);

This patch does only gives out the stuff as needed

dawehner’s picture

Status: Patch (to be ported) » Needs review

update status

Gábor Hojtsy’s picture

Status: Needs review » Needs work

As you've already noted, this includes #773812: Fix warnings which was just committed. So we can remove that. Its not part of the port. Also, comparing the D6 and D7 changeset, looks like you removed this line but provided no replacement?

$strings = l10_client_add_string_to_page();

Thanks for working on this!

kiprasm’s picture

Lol, i was working on the same thing (support for textgroups other then the hardcoded 'default') about a week ago (and for the same reason as well - im using i18nstrings), used it throughout the week, and was just about to post the changed files here, but i see this is already done :)

Hopefully this will be merged into the l10n_client 6.x stable version soon, so i will be able to compare it to my version, just out of pure interest :)

UPDATE:
Another lol - now i see it has already been commited just about a week ago :D

Gábor Hojtsy’s picture

@kiprasm: yes, it was committed to the Drupal 6 version and released over a week ago in 6.x-1.8: http://drupal.org/node/872024

Gábor Hojtsy’s picture

Status: Needs work » Fixed
FileSize
7.44 KB

Worked on this one and ported the functionality entirely to Drupal 7. Drupal 7 does not have automated IDs for hidden fields anymore, so I needed to add a class. Going to backport that to Drupal 6, since that might cause bugs like #940142: CCK translation creates strings in the "Built in interface" text group, incompatible with translation_table and other methods.

Gábor Hojtsy’s picture

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.