Implement customized translation bit on translations

Diving in.

OK -- the first part (being 4 out of 6 points above) is done, plus a suggestion from Gabor: provide a filtering option for the statuses, in the translation screen.

I'll continue this week, but upload what I have now. There's @todo's on basic stuff like thinking about the name of the added column... I'll rethink those when I did the last part of this issue.

Filtering options I added into an existing dropdown. This means that not all theoretical filtering options are possible (like 'untranslated strings plus those entered on the site') but I think that's fine.

Setting to review for naming of constants, naming of UI stuff, etc. But I'll rethink that myself, when done with the rest of the issue. So it's still CNW too.

Other incoming suggestion from Gábor: make the added 'translation status' a filtering option in the .po export.

locale_update_8001() should migrate data from l10n_update. Check for the existence of 'l10n_status' column in the same table and copy the data into the new 'status' field.

Changed the naming of the translations to 'custom' translations. 'Entered on this site' may be confusing if a po-file is uploaded. I've considered 'default', 'original', 'community' and 'custom', 'overridden', 'local'. But chosen the text below to be the most straight forward and least jargon.

• 'Translated strings'
• 'Translated strings, default translation'
• 'Translated strings, custom translation'

If we agree on this naming, we could change the field name 'status' to 'override'. (Not included in the patch)

The translations overview page does not need a 'status' column. The content of this column is 99(.9)% of the time 'default' According to the User Interface guide this should be avoided.

In locale_translate_edit_form_submit() only set the status to LOCALE_TARGET_USER_PROVIDED if the string actually changed. When a site uses multiple languages, the translation page presents multiple translatable fields. Only the translations which were changed should be flagged.

Both, thanks for your comments. Sutharsan, thanks for the code changes with nearly all your comments.

I defer to Sutharsan's experience on naming, and personally like the term 'override' for the database field.

Constant names/strings have been changed to match the 'default/custom' in the proposed description. Note their names are now quite generic, because the 'property name' is not included in the constant names##, and DEFAULT/CUSTOM are pretty generic terms. I believe in this case, that's OK...

Further:

• In the 'export' screen, I got into naming issues again (because even though I like the term "override", it can't be used as a noun that the user understands) - and used "status" again, and "kind of translation".
• "For other kinds, strings will be exported without translation" -> is this the intended behavior? (i.e. all source strings will be in a .po file, even if you select you only want to export the custom translations? It is that way for the language selection...)
• The 'setting correct status on import' (see first code block/comment in #5) is not done yet.

-----

## I'm probably not being clear, however I try to describe that. I mean the construct 'entity.propertyname=value' here maps to 'locale_target.override=value'. The constants LOCALE_TARGET_DEFAULT and LOCALE_TARGET_CUSTOM seem to be giving the 'entity' a value. They don't even name the 'property' which the value actually belongs to. But LOCALE_TARGET_OVERRIDE_DEFAULT and LOCALE_TARGET_OVERRIDE_CUSTOM don't seem clearer to me. Same if we'd put STATUS there.

Excellent work Gábor You couldn't wait untill we did this ;)

@Gábor: you're right in unassigning me - but I could put in some time for a little review & extra change.

(Naming "customized": ah. consistency in is nice. UI works well. Constant names are growing on me, I'm starting to like them.
Dependent checkboxes: ooh, shiny! Wasn't used to those yet, hence me putting the stuff in one.)

--

There should be an extra import mode now - the idea copied from l10n_update, though not the exact implementation:

Basically, if you import something as 'customized' and then later you import newer 'default' translations, you should have the option to not overwrite your customized translations, only the 'default' ones.
I also made it so that it would work the other way around (i.e. importing a newer 'customized' file overwrites only 'customized' strings, never 'default' ones). Not sure if there is a realistic use case for that.

It makes for a horrible description in the import screen, though. I'd like to have another pair of eyes on that.

Also, as Gábor said, still Needs Work for tests.

+++ b/core/includes/gettext.incundefined
@@ -26,8 +26,11 @@
  * @param $mode
  *   Should existing translations be replaced LOCALE_IMPORT_KEEP or
  *   LOCALE_IMPORT_OVERWRITE.
+ * @param $customized
+ *   Whether the strings being imported should be saved as customized.
+ *   Use LOCALE_CUSTOMIZED or LOCALE_NOT_CUSTOMIZED.

When this patch gets re-rolled, the docblocks like this one need some work.

The @param $customized needs the (optional) phrase to start the description as well as a statement of what the default value is. Also, I thought type hinting was recommended for D8 docblocks. Hence, something akin to the below would be more appropriate (wrapped at 80 characters):

 * @param int $customized
 *   (optional) A parameter indicating whether the imported stings should be saved with a customized flag.  The default is LOCALE_NOT_CUSTOMIZED.  Permissible values are LOCALE_CUSTOMIZED and LOCALE_NOT_CUSTOMIZED.   

Other parameters on some of the functions being modified in this patch also are missing the (optional) sting as well as type hinting. I would suggest that they get corrected in this patch as well.

I think people will have trouble figuring out what a "customized translation" vs. a "non-customized translation" is, and so they'll have trouble selecting the right checkboxes. How about keeping a set of radio buttons with better descriptions, something like:

New translations will be added. For strings that already have translations:

() Don't overwrite any existing ones
() Only overwrite built-in translations.
() Only overwrite custom translations.
() Overwrite all existing translations

I think this makes the "safe" option and the "these are the translations I want" options more clear.

I agree with drifter about the import page modifications, and I think a description on the export page should helpful to explain what does "customized translation" stand for.

We don't have to agree yet on what is the best wording. Lets see if we can come up with two of three alternatives, make simple mockups (or write it in code) and try it out with a few people.

Oh. Wow. Sweet :D

Observations:

• Whether or not (1 checkbox + 4) radio buttons vs. (3) checkboxes help guide the user better to the choices they need -- that might be, I don't know. Agree with making mockups for these, and leading those past some extra people.




• But I think people will have a challenge to figure out what this customized vs. non-customized thing is, no matter what you call it. (Plus indeed, we have the issue that the term "built-in" isn't being clear on what it fully implies: also including downloaded community translations, in the future.) I don't see the radio buttons helping there.
  
  
  So yes, this needs good help text to ease the user into the options. Both on the explanation of the difference between custom vs. non-custom, and guiding the user with as little pain as possible, to what they really need




• IMHO, "what they really need" is usually to overwrite non-custom translations - also in the case where the .po file is non-custom. That's much like l10n_update: "non-custom translations are whatever the community gives me, if they decide on updates, I want those too".
  
  
  The code (Gábor's matrix) sets "do not overwrite non-custom" by default, if the imported file is non-custom.
  
  
  So the reason for that is that this is the 'safest' option. Not that it's the 'most commonly used' option, because it isn't. Is that a fair statement to make?




• So the help text would hopefully include something along the lines of "if you don't know/care what all this means and you are just importing a translation file from a source you trust... you want to click the 'Overwrite non-custom' [ checkbox / radio button ] ".




• While I like the 'radical-ness' which fits the rest of what this issue is turning into :)... renaming of the tab wouldn't make things completely clear. 'customizing' can also be done on the "import" tab.

Patch contains no functional changes since patch 31, just a few minor things that caught my eye. I didn't test, only read code.

Bumping the testbot.

#41 and #42 are testbot-breakers ... for some reason, qa.d.o has troubles parsing the test result. Please do not requeue.

I hope to get back later today to debug what the root issue might be.

The patch corrects a column definition. That may explain the broken test. But testing this patch fails locally on the po import test. But importing the same files manually passed. Lets see what the test bot thinks about it.

These are exactly the tests that failed when I ran the tests locally (that's at least something). It looks like the plural formula is being overwritten. But when importing manually all is fine. Test files taken from the code are attached here.

Simpletest did not have the javascript behavior to automatically uncheck 'Include non-customized translations' so.. overwrite_options[not_customized] was always defaulting to 1. Patch forthcoming....

Submitted for testing...

Import

Export (Source Text)

Export (Language Specific)

For the import and export UI, I agree that it's not clear what custom translations are. I think we should add some help text to the list of options: 'Custom translations are translations which haven't been downloaded from a centralized source like localize.drupal.org'.

The word 'Non-customized' could also be replaced with 'default'.

I also think that in the Import UI, the phrase 'Import contains custom translations' should be replaced with 'Import contains customized translations' to make the label consistent with the other options.

These look really good! Definitely a simplified UI improvement with more fine grained options.

Here is a summary of feedback from the admin perspective looking at the current UI:

EXPORT

Source Text only option:

When I go to the export screen, I am looking for a language. Source text only doesn't really make sense as the sane default. Ideally it would be the main language on the site default that would be in here and that would be a non default option.
If this option was selected, it would make sense that it be explained what exactly it would deliver or what a use case might be. Why would I select this?

When Language is selected:

We should assume that most exports will include the entire file, so having the 3 checkboxes visible with the option to generate a blank file seems a bit confusing.
Perhaps an option to view these as more advanced options for people who select the option may be helpful.
"Customize content of file exposes the options" ie. Include customized translations, include non-customized translations, and include untranslated text.

IMPORT

When the language is selected, the three checkboxes don't seem related. The first check box "Import contains custom translations" describes the contents of the file. The 2nd two check boxes are actions that should be taken upon importing.
Because these are different groups, I suggest that we move the import contains custom translations to be nearer to the file field.
Terminiology:
Overwrite existing non customized translations
Overwrite existing customized translations

These options are only one word apart and it may be confusing for people. I don't have a current solution and I do like the 'customized' terminology. I just worry that they are visually similar and require a bit of mind bending to figure out exactly what this would get you.

One idea would be to possibly create summaries for each of the permutations. For example if you selected "import contains custom translation" AND "overwrite existing customized translations", a message would be given such as "You have selected file X to be imported, the updated translations in your file will replace what currently exists on the site."

For the import UI, in order to make it clearer that the 'Import contains custom translations' checkbox is all-or-nothing, I think we should move it to a separate radio button field.

Label: This import consists of custom translations, Checkboxes: Yes/No

Coding style review.

+++ b/core/includes/gettext.inc
@@ -454,15 +467,34 @@ function _locale_import_one_string($op, $value = NULL, $mode = NULL, $lang = NUL
+  if ($lid) {
+    $existing_customized = db_query("SELECT MAX(customized) FROM {locales_target} WHERE lid = :lid AND language = :language", array(':lid' => $lid, ':language' => $langcode))->fetchField();
+	}
+

Looks like a tab there on the last line with the }.

+++ b/core/includes/gettext.inc
@@ -479,25 +511,25 @@ function _locale_import_one_string_db(&$report, $langcode, $context, $source, $t
-      elseif ($mode == LOCALE_IMPORT_OVERWRITE) {
-        // Translation exists, only overwrite if instructed.
+      elseif ($overwrite_options[$existing_customized ? 'customized' : 'not_customized']) { ¶
+				// Translation exists, only overwrite if instructed.
         db_update('locales_target')

More tabs and trailing spaces.

+++ b/core/modules/locale/locale.install
@@ -176,6 +176,12 @@ function locale_schema() {
+        'type' => 'int',
+        'not null' => TRUE,
+        'default' => 0, // LOCALE_NOT_CUSTOMIZED
+        'description' => 'Boolean indicating whether the translation is modified.',
+      ),

Comments should be on a separate line, even for cases like this one. Not sure how the other rules apply here (like having a proper sentence ending with a .)

+++ b/core/modules/locale/locale.install
@@ -485,6 +491,27 @@ function locale_update_8005() {
+    'not null' => TRUE,
+    'default' => 0, // LOCALE_NOT_CUSTOMIZED
+  );

Same here.

+++ b/core/modules/locale/locale.install
@@ -485,6 +491,27 @@ function locale_update_8005() {
+  // If l10n_update module was installed retain its status data, but drop the
+  // old field from the table.
+  if (db_field_exists('locales_target', 'l10n_status')) {
+    db_update('locales_target')
+      ->expression('customized', 'l10n_status')
+      ->execute();
+    db_drop_field('locales_target', 'l10n_status');
+  }

Core *usually* doesn't deal with contrib modules in the upgrade path. Even if stuff is moved into core (e.g. fields in D7).

Just wanted to point this out, I'm fine with keeping it if is ok'd by others.

+++ b/core/modules/locale/locale.pages.inc
@@ -36,13 +37,16 @@ function _locale_translate_seek() {
+      if ($query['customized'] != 'all') {
+        $sql_query->condition('t.customized', $query['customized'], '=');
+      }

The '=' is not necessary, that's the default.

• Spaces corrected.
• Removal of 'l10n_status' column deleted from locale_update_8005(). When l10n_update lands in core, the is more clean-up work to do. It does not belong in this patch.
• I do agree that we write comments on a new line. But what's the alternative here? A comment line within an array? Or comment like 'The below default value is the constant LOCALE_NOT_CUSTOMIZED'. I left it unchanged.
• '=' removed

Still 'needs work' for #54 - #56

Export, Source Text only

When no language is available (system language only) a text is displayed instead of the (one option) select list.

Export, When Language is selected

Language default value changed to the site's default language.
Added a checkbox to hide/display the export options. Or should this be a collapsible fieldset?
Changed 'Include non-customized translations' to 'Include default translations'

Import

In order to explain a concept, it needs to have a name. Instead of 'default translation' and 'customized translation' I looked for a name that give these translations a natural state above the other translations. Base translation and Priority translation is my choise, but I am open to suggestions. I only changed the import and export interface, no name changes in the code yet.

Try again ;)
Still needs work, I need to check the test. But would love feadback on the interface.

Needs usability feedback on using a checkbox or collapsed fieldset to hide/show details of customized export.

The fieldset variant:

I would prefer the option of using a collapsed fieldset. Although the "collapse upon clicking a checkbox" is a possible pattern, it is not one that performs very well with users (it is is magic to them, when a checkbox collapsed into more optionns).

Given that this is functionality, that looks like very experty (not for the 80%) - I would agree with the idea of putting them in a collapsed fieldset..

Test now pass locally.

Just heard about this Issue the first time.

I think it is a really good Idea, but I would suggest to call the "Translation Types" different:

- Custom Translation
- Community Translation

After a day, I realize that my 'priority' naming idea was not so well thought out. We are not used to calling things names in Drupal. Lets revert to 'default' and 'custom(ized)'.

But it made me realize that we are building power user functions into core here. First (second and third) time users of Drupal wil not import and export po's. All they want is "one button". Select a language and the're done. They may not like the translation, but bothering them with export a pot file, grab a translation tool, import a po file is way to far. And we are busy adding options to control yet another layer, the customized translation string. If we want to smoothen the learning curve the least we should do is use sensible defaults and hide them (fieldset). Better would be to move these interface options into a contrib module. But best we could do is move the complete import and export interface to a contrib module. Once we have Localization Update module in core, we should leave import and export to the power users which are best served by contrib.

I agree we need the plumbing, but do we need to add all possible options into the user interface. Yes, we need to add a custom translation to database and the API, yes l.d.o translations will never be 100%, and yes there need an interface to modify the default translation, yes these custom translations need to be treated different from the imported l.d.o translations. But I argue against bells and whistles for inexperienced users. For example the option to control whether imported translations will override the default and/or the custom translation, the options to export which flavor of translation (default, customized, untranslated). Using po files is not for the faint of hart, history proved this and lead us to build l.d.o. I do agree on the need for the underlying mechanisms which we are building in this issue. But to me core is not the place for full featured interfaces for power users.
[edit] some text changes

This issue require focus - there's to much context of translation forms!

There's a different angles to me here:
1) Developer (site builder) - need to translate config, some custom module's provides strings to UI of site's editors
2) Translator (site editor) - need to translate content, layout options, actions (links, buttons, rules) probably some custom UI elements (accordion, slider)

We could use the same element to translate strings and probably separate storage for different kind of strings

With customized bit we gets another dimension in strings' storage

so it makes queries slower and we need different strategies for caching

The UI could be implemented in separate issue, so if we have no time for UI we still have storage, cache and api and contrib project to grow better UI

@gábor: I'm glad to read that we have more common ground, than I initially thought. I have no intention blocking this issue. In fact I continued on this issue again to make another small variation of the interface. It's difficult to find the right terminology.
Should we update the module's help text. And should we anticipate in the text on (automatic) translations update?

For me this new UI looked quite straightforward, but it is better to be on the safe side with being as clear as possible.

Can't really improve this any more. I've tried different layout variations, but none did make it clearer non better layout. The patch changes only the 'priority' wording back to 'default' and 'custom(ized'). I left the help text unchanged as the translation handling is currently in flux and don't know where it will end.

A very quick scan of the UI options from someone to whom even the concept of customized translation sets is completely new.

For me, maybe not wanting to use the feature it means more checkboxes. So in order not to bother me but to provide the people who want to use it with the extra power:

1. Make clever defaults so I do nothing wrong
2. An option that rather hides the extra options and is called "options" or "advanced options" or whatever like in http://drupal.org/node/1445004#comment-5779464 looks good, as it employs UI patterns that appear familiar to me.

Gabor asked for UI feedback.
Instant reactions to the new UI which I understand is rather on the right side of http://drupal.org/node/1445004#comment-5673836
Uh, oh, three checkboxes, which ones do I choose? Somehow fear to do something wrong.
Same with old interface, but if I am a fearsome person I choose "only import new, don't overwrite" and feel this will most probably not break anything.

Now I have three options
* import contains custom
* Overwrite existing customized
* Overwrite existing non-customized

Uh, oh, what?
If I was new to Drupal I would be completely confused and am even now...
I only want to import a translation and have no idea if it is custom or not, but I do not want to break any stuff I already have...

Would advocate to provide clever default settings or somehow name it in a way so I know how to import a regular .po file I got from localize.drupal.org or some official download place.

I guess the people that import customized translations know what they are doing. So these people would be a very small minority, and somehow the entire custom translation thing would be an "advanced option" for me.

To aid the UI evaluation, some screenshots with the #77 patch applied:

Import

Export

For a default import and export settings we need a default scenario. I wrote a few, but note that I have little input from the real world of maintaining multilingual sites.

1. Private translations: I maintain my own translation. I need it for it's tone of voice to match the audience of my websites . It is (re)used on different websites.
2. Unmaintained language: I have a language which is not provided by l.d.o. The translation may or may not be shared with the community at some point in the future. I use this translation on all of my websites.
3. Contributed translation: I have translated this module and contributed the translation to l.d.o But approval seems to be going slow over there and I want to use this translation on my website.
4. Translation deployment: Translations on our websites are updated automatically using Localization Update module. But we modified some strings on the test site. We want to deploy these changes from test to acceptance and to live.

Which scenarios are most likely? On which should we base our default settings.

@Gabor: right, the D7 screen is not that intuitive as well :)
I did not know that plural form bit until now, is this brand new?

The problem I stil see is: I think the user needs to choose one of those three options to import his .po file. The two old options still have me wondering every time if I am doing it right. With the three new ones, imagining I was a user new to drupal I have a very hard time choosing one as I understand them even less.

It is that all three sound a bit intimidating to me. But I guess "Overwrite default translations" would equal the old overwrite option and "Import Strings as custom translations" would be equal to "keep old, only import new"?

Now the challenge would be to name the three new ones in a way the casual user confidently chooses one.
Which one is chosen by default?

At last I think I am getting what this custom translation thing is all about: Someone has changed some translations in his current version of Drupal, and the new system makes sure he can safely import .po files without losing them, while still overwriting default translations?

Going really fancy there might be a mechanism that detects if any custom translations are there and only shows checkboxes / radios relating to that in that case. This would make the interface simpler for people who have not translated any string and just wanna import an innocent .po file.

But this might complicate matters on the programming side.

@eigentor, This customized flag for translations is introduced by this patch to allow the admin to override translations and prevents them from being overwritten by automated translation updates like the Localization Update module. Translations edited via the translation UI will be marked as customized. Translations imported from l.d.o by Localization Update are not marked customized (e.g. become non-customized or default translation). In the same way, manually imported po files from l.d.o should become default translation. Customized translations are the admin's realm, default translations are the system's realm. Customized translations give the admin full control, default translations allow override by automatic updates.

We default to custom strings overwriting default translations but not your existing custom translations.

O.K. this makes totally sense and should match 95% or more of users, and more importantly, the users that do not understand the system in depth.

So make this the default.

Now: should it be chosen by default and the options hidden behind an "options" fieldset or not.

- New users: would gain from that, because they do not need to worry and are not stopped by some fancy options that may make them hesitant and in consequence not import their .po file in fear of breaking something.

- Experienced Drupal users: would click the fieldset, find options, and as they are proficient problem-solvers (else they would never have sticked with Drupal that long) find an option that suits them, click that and are happy

- People that know the old UI and miss the two options to choose from: might go crazy, curse core developers, but in the end also click on the fieldset and find the options.

So that's what I would advocate for: make the option preselected that fits 80-95% of people importing a .po file and hide the confusing options behind a collapsible fieldset.

• Added tests for:
• Import with overriding existing non-customized strings
• Import with overriding existing customized strings
• Export customized translations
• Export untranslated strings
• Hide field set with export options when no languages is selected.
• Wording back to "non-customized"/"customized".

No changes to import default settings yet.

Well don't wanna play the broom invoked by the sorcerers apprentice that you cannot stop anymore :)

If you really wanna nail the UI I guess the only real way is some quick and dirty user testing. And find out who are the main users of this screen. Yeah, sane default is important.
Who does the screen target primarily? This question needs to be answered. I find we still make the mistake in Drupal to target ourselves that already know Drupal too much, when it comes to UI decisions.

So I'm mostly taking the side of the novice.
But if the novice never goes there, this makes for a completely different situation.

Patch modified with #95

Regarding the UI. I know this interface is difficult for Drupal newbies, I don't need a UX test for that. The best thing we can do for this user group is to prevent that they will need these pages in the first place. If they reach the import page, they have managed to downloaded a po file from l.d.o and want to import it into their site (and even reaching this point will be a challenge for them!). And that is exactly what Localization Update in core can help them with by its automatic imports from l.d.o. Even for experienced Drupal users this interface is challenging, but at lease we have a more limited set of scenarios to focus at. Lets get this issue rolling, it is a blocking issue for automated import.

Ok, try again ;)

Removed quotes from around LANGUAGE_SYSTEM.

+++ b/core/includes/gettext.incundefined
@@ -80,13 +84,17 @@ function _locale_import_po($file, $langcode, $mode) {
+ * @param $overwrite_options
+ *   An associative array indicating what data should be overwritten, if any.
+ *   - not_customized: not customized strings should be overwritten.
+ *   - customized: customized strings should be overwritten.
  * @param $lang
  *   Language code.
+ * @param $customized
+ *   Whether the strings being imported should be saved as customized.
+ *   Use LOCALE_CUSTOMIZED or LOCALE_NOT_CUSTOMIZED.

This looks confusing, $customized is just a boolean.
Suppose doc-block should describe deeper what's going to be imported and overridden.

+++ b/core/includes/gettext.incundefined
@@ -395,7 +407,7 @@ function _locale_import_one_string($op, $value = NULL, $mode = NULL, $lang = NUL
-        if (($mode != LOCALE_IMPORT_KEEP) || empty($locale_plurals[$lang]['plurals'])) {
+        if ((array_sum($overwrite_options) > 0) || empty($locale_plurals[$lang]['plurals'])) {

use array_filter() not array_sum()

+++ b/core/includes/gettext.incundefined
@@ -454,15 +467,34 @@ function _locale_import_one_string($op, $value = NULL, $mode = NULL, $lang = NUL
+  $existing_customized = NULL;
+  if ($lid) {
+    $existing_customized = db_query("SELECT MAX(customized) FROM {locales_target} WHERE lid = :lid AND language = :language", array(':lid' => $lid, ':language' => $langcode))->fetchField();
+  }
+

use else to define $existing_customized, or just drop definition before if()

Also MAX() need docs!!! Why ? Do we have many translations for one string in one language?

+++ b/core/includes/gettext.incundefined
@@ -479,25 +511,25 @@ function _locale_import_one_string_db(&$report, $langcode, $context, $source, $t
-      if (!$exists) {
+      if (!isset($existing_customized)) {

seems empty() make more sense

+++ b/core/includes/gettext.incundefined
@@ -479,25 +511,25 @@ function _locale_import_one_string_db(&$report, $langcode, $context, $source, $t
-      elseif ($mode == LOCALE_IMPORT_OVERWRITE) {
+      elseif ($overwrite_options[$existing_customized ? 'customized' : 'not_customized']) {

This kind of arguments is hard to pass from code and harder to read

+++ b/core/includes/gettext.incundefined
@@ -521,13 +553,14 @@ function _locale_import_one_string_db(&$report, $langcode, $context, $source, $t
+  elseif (isset($existing_customized) && $overwrite_options[$existing_customized ? 'customized' : 'not_customized']) {

same

I think this logic should be refactored

+++ b/core/includes/gettext.incundefined
@@ -828,17 +861,67 @@ function _locale_import_parse_quoted($string) {
-function _locale_export_get_strings($language = NULL) {
-  if (isset($language)) {
-    $result = db_query("SELECT s.lid, s.source, s.context, s.location, t.translation FROM {locales_source} s LEFT JOIN {locales_target} t ON s.lid = t.lid AND t.language = :language", array(':language' => $language->langcode));
+function _locale_export_get_strings($language = NULL, $options = array()) {

Are you sure that it's easy to rememeber each of this params?

not_ not- - the underscore hard to remember!

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -34,62 +34,80 @@ function locale_translate_import_form($form, &$form_state) {
+  $form['customized'] = array(
+    '#title' => t('Treat imported strings as custom translations'),
+    '#type' => 'checkbox',
+  );

This option really confusing let's hide it into fieldset

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -34,62 +34,80 @@ function locale_translate_import_form($form, &$form_state) {
+  $form['overwrite_options'] = array(
+    '#type' => 'container',
+    '#tree' => TRUE,
+  );

I think better make this fieldset with title Advanced options

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -34,62 +34,80 @@ function locale_translate_import_form($form, &$form_state) {
+    $customized = $form_state['values']['customized'] ? LOCALE_CUSTOMIZED : LOCALE_NOT_CUSTOMIZED;

This should be passed from form item, no reason for this logic here, use #return_value

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -118,65 +134,75 @@ function locale_translate_export_screen() {
+    $form['content_options'] = array(
+      '#type' => 'fieldset',
+      '#title' => t('Export options'),

content_options?
Advanced [export] options

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -118,65 +134,75 @@ function locale_translate_export_screen() {
+  $language = ($form_state['values']['langcode'] != LANGUAGE_SYSTEM ? language_load($form_state['values']['langcode']) : NULL);

this hard to read - better if() else

+++ b/core/modules/locale/locale.installundefined
@@ -176,6 +176,12 @@ function locale_schema() {
+        'description' => 'Boolean indicating whether the translation is modified.',

s/modified/customized

+++ b/core/modules/locale/locale.installundefined
@@ -485,6 +491,18 @@ function locale_update_8005() {
+function locale_update_8006() {
+  $spec = array(
+    'type' => 'int',
+    'not null' => TRUE,
+    'default' => 0, // LOCALE_NOT_CUSTOMIZED
+  );

add description from schema here

+++ b/core/modules/locale/locale.pages.incundefined
@@ -435,21 +460,23 @@ function locale_translate_edit_form_submit($form, &$form_state) {
+      if (!empty($translation) && ($translation != $value)) {

not need extra braces

+++ b/core/modules/locale/locale.testundefined
@@ -1197,6 +1250,66 @@ EOF;
+   * Helper function that returns a .po file which strings will be marked
+   * as customized.
+   */
+  function getCustomPoFile() {
+    return <<< EOF
+msgid ""
+msgstr ""
+"Project-Id-Version: Drupal 7\\n"

drupal 8 :)

can we re-use already implemented poFiles in tests?

Fixed another array_sum() - it's not good to sum booleans, also array_filter returns array and check for emptiness with count is overhead in function that used in loop is bad idea, I know about #1189184 but this is a really bottle-neck place

Also fixed another Drupal 6 in .po

1445004-opt-105.patch is a bit refactored to reduce a number of queries

I've measured performance for array functions

array_filter 0.91011500358582
array_sum 0.61800193786621

updated to one query

Suppose we need measures on import with one and 2 queries, most of time system has string in both tables so initial import makes less sense. If one query has better statistics we could backport it to D7

I have done performance tests with two scenario's on both patches:

• Import with 0% overlap
• Import with 100% overlap

The first scenario is with are strings which do not have a matching source string in the database. e.g. the intial import of a po file. The second scenario is importing string of with all strings have a matching source string in the database. e.g. import the same po file twice. In real live the initial import of a module will have a low overlap (estimated 0 - 20%) sequential imports of updated module updates and import of new module releases will have a high overlap (estimated 80 - 100%).
#108 is approx 5% slower than #107 on imports with 0% overlap
#108 is approx 15% faster than #107 on imports with 100% overlap
During the live time of a site many more imports will be carried out with a high translation overlap rate. Therefore I advise to use #108.

Forgot to mention, the above tests were carried out without overwriting existing translations. When overwriting existing translations both patches perform equal.

@Sutharsan thanx a lot for testing! 5% slowness on initial import seems depends on mysql settings, less queries should be faster. In case of server on different machine boost should became bigger.

Sorry I'm late to the party :-p I didn't have anything to add to the higher level discussions but should have returned last week...

After a review of the thread I have some minor things (don't worry, you can set to RTBC in no time):

• Consistency of terms to be as non-confusing to the user as possible. I think they may have been left like this inadvertantly when changing them around and around? Language changed:
  • Treat imported strings as customized translations
  • Overwrite existing non-customized translations
  • Overwrite existing customized translations

  Note: The "Translation type" filter dropdown on the search screen still has options "Base translation" and "Customized translation". I didn't change "Base" back (despite inconsistency) because I really don't know if that would be any better.

• Re. bottom remark of #85/#86: you can actually hide checkboxes in certain situations, without making things a four step process. You don't need to read through the .po file, only through the existing translations.
  
• If no language has been added yet, things default to the first in the list. It's a minor thing, but probably better to not preselect any language so newbies won't create an unwanted language accidentally.

Attached is the view of the import screen on a freshly installed site with no languages enabled and no translations imported. Right after importing your first .po file, one of the two now-hidden checkboxes will appear.

...and I've left some other things for a followup issue on purpose, because IANAUIExpert. Feel free to ignore.

I realize that there is no way to explain the import screen fully to everyone without a full man page. But maybe a description with the checkboxes still helps to at least make inexperienced users not feel completely lost?

Or are descriptions that are this large, against UI rules? Probably, huh? (I'm not succeeding in making them any shorter.)

Also IMHO it doesn't hurt to make the first checkbox stand apart from the other two. Because now, users' first impression is "ah, there are 3 on/off options below each other, so 3 options of exactly the same type". Which isn't true. And the fact that the 2nd checkbox does not have a description, helps that.

Also, I see one other thing that might have fallen through the cracks: pixelite's #56 about making the first checkbox into radiobuttons with a Yes/No choice. That would also achieve 'making it stand apart from the other checkboxes'...

Anyway: The attached screenshot is all you need to decide whether you want to continue with #114 or #115.

I very much agree with Gábor and will therefore not comment on #114 - #117.

Filter select list modified to 'Non-customized translation' (other options unchanged: 'All' and 'Customized translation')
Patch based on #108 (1445004-opt-107.patch) as is was the better performing one.

Also, WTH include an empty option if that is not valid?

To prevent an invasive action (creating a wrong language) from taking place if you didn't select anything.

What do you do in the import if there is no language set?

You don't do anything in the import. You throw a validation error, that's what you do. Because of above.

Hey, I'm just answering your questions. As reference for the followup issue.

I'm sorry. I meant to only sweep through the last 50 comments to see how things have evolved, and meant to split out the 'trivial' stuff from the 'non trivial' stuff, that's why I created two patches. Apparently the notion of what is 'trivial' is something I should still get a hold of.

Reviewed Sutharsan's patch.

This is a great feature, and one that is much wanted I'm sure. Committed to 8.x.

I think it warrants a CHANGELOG.txt entry; let's add one?

Moving to Jennifer's queue.

Looks good -- changelog entry has been committed.