Rework the translation UI

cool, +1 for this patch. See what Gàbor think about it ;)

Cool... looks much better to me with the colors than the strike through variant 1 year ago...

I am working on l10n integration for http:/drupal.org/project/atr, which does something similar. It might be cool to put this code in there, so it can be used by other projects as well. That way we centralise all text review methods for Drupal.

With strings that have the same word twice, like "Der Pfadpräfix oder die Sprachdomain" versus "Der Pfadpräfix oder die die Sprachdomain", the second "die" doesn't get displayed in the results.

I only tested the patch with the PHP algorithm. According to kkaefer the JS verison doesn't have this problem.

Here is a very quick review

1. Code review tells me there are code style problems in the patch. Eg if() statements without {}
2. Code review tells me you add a full on jQuery 1.3 into the module. Why not depend on jQuery update module?
3. I did not look at query builder changes but I really wonder what "contains" would search in. It should search in any string that is still active and displayed. Looks like it does search in suggestions.
4. I did run some pretty basic tests of the UI and noticed a few issues:

My big picture of this patch, is that its functionality looks amazingly cool. It does a frightening amount of changes introducing new permissions, the diff functionality as well as getting rid of two separate screens, etc. To make sure that all this goes flawlessly, we also need the included automated tests updated, so we can ensure all works as expected. There is very good test coverage for the suggestions / translations / filters, etc.

I'd love to do a quick screencast of how it works and what would be my expectations, so we can get some usability feedback quickly.

More thinking / review:

- While this UI is (IMHO) way better then the previous translation tab UI, it does not replace the defining functionality of the moderate screen. When people import big amounts of bad translations, or a different language version, then the moderate screen lets people mass-decline those strings instead of going through them one by one when multiple suggestions exist for the parent strings at hand.

- I'm seeing stabilized strings in the constants but not on the UI.

- Moving selected strings is inconsistent in how it marks declined strings. When you pick a different string, the string picked before has a line-through, but not the others, even though you actually decline that one as well by picking a different string. When the texts become crossed-through, it gets much harder to review the diffs and make corrections in my selection.

PICTURES:

When picking a different translations, only the previously selected one is crossed-though:

When texts actually became crossed-through, it is hard to view changes:

This looks all very promising for usability... do you plan to keep all suggestions in the UI? I remember of 20 or more suggestions for one string... I believe the counter will go up much more on d.o - nevertheless the gold crown gives is a chance to block, we may have 50 suggestions until something becomes golden :-). Not sure if we are able to decline all others in such a case automatically or manually.

This system still makes it possible to decline suggestions (which was not too intuitive for me judging from the explanation from Konstantin above). The outstanding suggestions are displayed with the active translation. (Also, previously approved but not anymore active translations are not shown either).

I was planning to understand the workflow suggestions better here and make up a video of the suggestions before posting for the larger audience on localize.drupal.org, so people who can obviously not install all the modules to test the new UI can still see how it works. I still plan to do that but many other important stuff got into the way, so fixed other issues this week (news post at http://localize.drupal.org/node/214 ).

Hah, I've actually found via twitter that you have a video at http://i.kkaefer.com/l10n.mov Which shows some of the features I've not even realized you have added :) It also underlines one removed/missing feature through: you cannot copy the source string to quickly start a translation. That would quite good in places where the source has HTML code or many placeholders as shown in your examples. Also, sometimes it is hard to make out what you mean as there is no sound on the video.

Did a quick video and posted feedback request at http://localize.drupal.org/node/258

My quick feedback is as follows:

- still don't know why is l10n_community not dependent on http://drupal.org/project/jquery_update then, instead of including it in the module itself
- managed to understand while testing is that you always add a suggestion (and the feedback in the dsm is misleading in saying "translation"!), even if you are an admin, you need to enter the suggestion *and* pick that to make it appear as the active one (now that I understand it sounds like a logical workflow, but the feedback on submit would be better fixed)
- the marking as stable is not "missing" per say, it has some implementation items, like "mark other people's translations as stable" appearing int he l10n_community_get_permission() function
- noticed that not all placeholders are properly detected, while @url is fine, @drupal-link is not (not sure it was this exact placeholder, but noticed - is treated as a separator, but it is not).

All-in-all, I'm happy with the functionality of the patch. Do you see any possibility of staging its inclusion somehow, or would that be more work then how useful it is? Eg. I'd imagine permission changes can be committed first, but then at a loss of ideas on how could we divide the changes. Reviewing smaller chunks would obviously be easier.

Also, underlining that adapting tests to the changes is mandatory. I'll try to do that later if you do not have resources to work on that.

subscribe

@kkaefer: as I told u in IRC (or on twitter, don't remember) a month ago or so, I can't think of anything I would like to see different in this
that doesn't mean it is perfect as it is and that it is invulnerable to improvement or anything, it just means that the improvement this would provide as it is right now, blows my mind far too hard to be able to perform any sort of proper evaluation

in short: it's sheer awesomeness clouds my judgment

I would prefer only one "more information" link on the page, and that would show/hide the usage info for all strings. Because when I need that info, then I have to click many of these links to find the correct string I want to translate..

Great improvements..

Terrific improvement and thanks for the screencast at localize.drupal.org which made me land here. However, I'm not enough into the translation server stuff yet in order to give some decent feedback. Will give it a try.

Did anyone recently apply the #23 patch against the latest 6.1 branch? I'm having a hard time applying it with eclipse so if anyone has a working version, it would be great if one could share a new patch. I will then apply it on a live server (which is currently up for testing) so anybody can give it a try

Another note: I concur to reuse jquery_update as noted by Gábor in #25.

subscribe
Like Your work!

#605748: Approve Selected + Decline unselected at once in moderation can make Us happy too 8)

This is great!

The UI should have also support for the below features. Some of them are not subject of this ticket but are listed here because of their impact on UI layout...

1. A UI button/icon for creating/overwriting suggestion from source string.
2. A UI button/icon for creating/overwriting suggestion from Google Translate. #608288: Let Google suggest translations is dealing with this but i recall here because of its impact on UI.
3. Branch support. This is now only a proposal in #608488: Translation inheritance but if implemented, then each suggestion must be linked to a branch (defaulting to base) and we must have the ability to setup this through UI. Add here also for UI geometry/layout reason. Problem: This mean that we can have more than one translated item but no more than one per branch (!?)

The issue at #610898: Unable to import translation on http://localize.drupal.org/ notes that not listing suggestions on the browse/translate UI is confusing. This patch helps clean up that confusion as well :)

#608288: Let Google suggest translations is really the most worst idea I have ever read about... I would never use Google as translation service. Have you ever read the damn ugly sh** that came out? I believe not... better translate by hand than using Google translation service.

EDIT: Changed "heard" to "read about" so that no one reads "had"

@hass: can you please pick the right issue to post your followup?

@hass

... Let Google suggest translations is really the most worst idea I have ever heard ....

It's about suggesting not about definitive translations... It's a little bit more than "copy from source". You don't have to use it if you don't like it...

Sry, commented over there...

+1 for general reworking.

But scrolling through this issue, I see the screenshot, and I'd like to say -- little fiddly icons beyond the obvious small set of '+' and 'X' and '?' are hard to understand!
On the UI as it currently stands there is a sort of <-> icon and I have NO IDEA what it means!

The permissions part of this patch was broken out to #220119: Permission to "approve other suggestions only" thanks to @kkaefer for sending a partial patch with only those changes. Postponing on that.

#220119: Permission to "approve other suggestions only" is now committed, so code related to that should be removed from this patch. This unblocks this issue, so people can keep working on it. To actually be able to deploy #220119: Permission to "approve other suggestions only" in a meaningful way on l.d.o, we still need some more changes, but those are orthogonal to a reworked translation UI. Opened an issue for that with an initial patch at #652038: Migrate permissions to og_user_roles, instead of our own code.

@kkaefer: can you pick up working on this with a patch that does not contain the permission changes anymore? :)

@kkaefer: The #40 patch have errors. Eclipse complains about l10n_community.js and ajax.inc. Patch shows they should be deleted... no idea why Eclipse complains. I fixed the patch and removed jquery13.js and added the dependency with jquery_update.

@gabor: Do we need to check with hook_requirement if jquery_update 6.x-2.x is installed or are you fine with the jquery_update dependency only that may also allow to install jquery_update 6.x-1.x?

• Use jQuery Update.module

Replaced jQ13 with jQuery

I've added the images from #8 to CVS so patch testing should be easier. http://drupal.org/cvs?commit=310236

I'd be happy to keep committing parts of this patch until we can reach completion. There are multiple things here that we could stage in smaller chunks to avoid a big patch review.

1. There are the image related portions, which (for the new images at least) should be now covered (see #45).

2. There are white space fixes included in the patch, which should not anymore be required thanks to commits landing lately. If there are any (eg. devel.inc, extractor.inc, moderate.inc, welcome.inc only have whitespace changes in the patch)

3. There is the self-decline portion of the patch, which is also available at #652460: Add permission to allow declining own translations, so we either solve that first or that one will need to apply changes to the UI added in here to conform.

4. There are relatively standalone libs included like jquery.worddiff.js, which would be useful to have some code comments on top (author, year, $Id$, etc.), and since I'd expect little comments to that library, we could probably add it and see issues for it later (eg. how good it is in finding Chinese word boundaries :)

5. There are bookkeeping changes in how strings are saved, which could be added without this patch also (as in part of a breakout of this patch).

There might be other possibly independently applicable parts of this patch that we can get it in to smooth in review and implementation of the changes.

Tried to foster some more interest for helping out with this at http://localize.drupal.org/node/708 a week ago but did not seem to succeed.

Call me in for a code sprint on Drupalcon SF for this issue. One of the days before the conference?
Edit: I know there are still a few months to go, but it can never hurt to call to early

I sincerely hope we can deploy this *way* before Drupalcon SF. (Am I dreaming? :)

I *really* hope we don't need to dream sooo many weeks...

This fallen out of date a bit. Trying to fix hunks so it applies again:

$ patch -p0 < newui_0.patch 
patching file l10n_community/ajax.inc
Reversed (or previously applied) patch detected!  Assume -R? [n] n
Apply anyway? [n] y
Hunk #1 FAILED at 1.
File l10n_community/ajax.inc is not empty after patch, as expected
1 out of 1 hunk FAILED -- saving rejects to file l10n_community/ajax.inc.rej
patching file l10n_community/devel.inc
Reversed (or previously applied) patch detected!  Assume -R? [n] n
Apply anyway? [n] y
Hunk #1 FAILED at 49.
1 out of 1 hunk FAILED -- saving rejects to file l10n_community/devel.inc.rej
patching file l10n_community/editor.css
patching file l10n_community/editor.js
patching file l10n_community/extractor.inc
Reversed (or previously applied) patch detected!  Assume -R? [n] n
Apply anyway? [n] y
Hunk #1 FAILED at 200.
1 out of 1 hunk FAILED -- saving rejects to file l10n_community/extractor.inc.rej
patching file l10n_community/import.inc
patching file l10n_community/jquery.worddiff.js
patching file l10n_community/l10n_community.css
patching file l10n_community/l10n_community.info
patching file l10n_community/l10n_community.js
Reversed (or previously applied) patch detected!  Assume -R? [n] n
Apply anyway? [n] y
Hunk #1 FAILED at 1.
File l10n_community/l10n_community.js is not empty after patch, as expected
1 out of 1 hunk FAILED -- saving rejects to file l10n_community/l10n_community.js.rej
patching file l10n_community/l10n_community.module
Hunk #1 FAILED at 207.
Hunk #2 succeeded at 190 (offset -38 lines).
Hunk #3 FAILED at 200.
Hunk #4 FAILED at 425.
Hunk #5 FAILED at 879.
Hunk #6 FAILED at 901.
Hunk #7 FAILED at 943.
Hunk #8 succeeded at 1071 (offset -102 lines).
Hunk #9 succeeded at 1105 (offset -102 lines).
Hunk #10 succeeded at 1135 (offset -102 lines).
Hunk #11 succeeded at 1257 (offset -102 lines).
Hunk #12 succeeded at 1264 (offset -102 lines).
Hunk #13 succeeded at 1278 (offset -102 lines).
Hunk #14 FAILED at 1314.
7 out of 14 hunks FAILED -- saving rejects to file l10n_community/l10n_community.module.rej
patching file l10n_community/moderate.inc
Hunk #1 FAILED at 23.
Hunk #2 FAILED at 33.
Hunk #3 FAILED at 51.
Hunk #4 FAILED at 97.
Hunk #5 FAILED at 156.
Hunk #6 FAILED at 173.
Hunk #7 FAILED at 208.
Hunk #8 FAILED at 241.
Hunk #9 FAILED at 250.
Hunk #10 FAILED at 278.
Hunk #11 FAILED at 304.
11 out of 11 hunks FAILED -- saving rejects to file l10n_community/moderate.inc.rej
patching file l10n_community/translate.inc
Hunk #1 FAILED at 6.
Hunk #2 FAILED at 31.
Hunk #3 FAILED at 147.
Hunk #4 succeeded at 616 (offset -1 lines).
Hunk #5 FAILED at 639.
Hunk #6 FAILED at 662.
Hunk #7 FAILED at 735.
Hunk #8 FAILED at 759.
Hunk #9 succeeded at 849 with fuzz 2 (offset -72 lines).
7 out of 9 hunks FAILED -- saving rejects to file l10n_community/translate.inc.rej
patching file l10n_community/welcome.inc
Reversed (or previously applied) patch detected!  Assume -R? [n] n
Apply anyway? [n] y
Hunk #1 FAILED at 114.
1 out of 1 hunk FAILED -- saving rejects to file l10n_community/welcome.inc.rej
patching file l10n_remote/l10n_remote.module
Reversed (or previously applied) patch detected!  Assume -R? [n] n
Apply anyway? [n] y
Hunk #1 FAILED at 149.
1 out of 1 hunk FAILED -- saving rejects to file l10n_remote/l10n_remote.module.rej

I've opened a DRUPAL-6--2 branch to make progress with this patch (after being committed in a form very similar to the latest patch).

I've already committed the image removals (and additions) as well as complete file removals and file additions. The whitespace changes were not at all applicable (but tried to apply them one by one). Now processing the exact changes to .info files and the editing UI code. This is going to take more time, not as easy as deleting and adding files.

Localize.drupal.org will use the DRUPAL-6--1 branch for the time being, and once this branch looks like stable enough, we'll be able to switch over. I'll try to refrain from other major changes on this branch until we can switch over, so it is smooth. It would also be great to focus on this branch so we can get work on this actively without holding down other stuff for an extended time.

I'm marking this RTBC, while it is not yet really that close. I'm opening the new branch and committing the patch in the interest of setting up an easier testing / development environment for people to work with. It is far from being ready still!

So this is the meat of the patch that was not applied yet and still needs to be fixed to be able to apply / commit. Working on it.

Reworked (most of) the counter code. Committing this cleaned up version now. Also, the remaining of the patch attached.

Ok, integrated the rest of the patch and committed as well. Looks to be working well on DRUPAL-6--2.

We still need to:

- clean up the code, add comments, etc.
- verify how it works in different permission scenarios (it replaces both the view and edit UI for people)
- align tests, so they work well with the UI as it is in place; they are breaking badly ATM

The dev tarball for 2.x-dev is at http://drupal.org/node/695746

Great! THX!!!

Took considerable time today to walk myself through this code, and got to the middle of the theming part (that is to a decent point). I'm committing this interim "cleanup" patch, which lines up the code more to how the other parts of the module are structured. Will continue reviewing the theming functions, the form submission logic and CSS/JS next.

Thanks everyone - this is one awesome improvement!

Here is some more cleanup and comments as I went forward understanding the parts over the weekend. Committing these as well.

Reviewing the translate page I repeatedly get the JS error: "$("em.l10n-placeholder").live is not a function". Although referenced in css I couldn't find any place where a DOM object is created with that id.

After removing that section from editor.js I got the following JS error: "translation.mouseenter is not a function"

Situation:
Firefox 3.5.7 on Ubuntu 9.10
Logged in as user 1
One project added with the l10n_localpacks connector

Otherwise ans awesome improvement.

I took the opportunity to do some minor cleanup, partly suggested by the coder module, see patch.

@helmo: do you run with jQuery update module (which is a required dependency)?

@Gábor Hojtsy: I had 6.x-1.1 of the jquery_update module.
Just updated to 2.0-dev and that solved it.

Unfortunately it is not possible to specify version level dependency in 6.x, so all we can do is to document it. I've added this to the project page:

Drupal 6.x-2.x is the development branch

You'll need jQuery Update 2.x to make this work properly. Sorry, version information is not possible to specify in .info files, so we can only document this requirement.

From #57:

- clean up the code, add comments, etc.

Now mostly done, I think I'm confident enough in the code now (minus the worddiff algorithm internals I did not take time with, but not worries about :).

- verify how it works in different permission scenarios (it replaces both the view and edit UI for people)

Only minimally done yet. Still needs to be done.

- align tests, so they work well with the UI as it is in place; they are breaking badly ATM

I've been working with this over the weekend. Now fully done at #715606: Update test on 6.x-2.x to reflect current reality.

Looks like this needs some manual verification still, but otherwise shaping up well and is well on its way to deployment :)

Ok, I did client side testing and found some JS misbehaviors. Documented and fixed at #715720: JavaScript actions not always in line with permissions.

Worked a bit on making the "more information" feature more useful. Removed "used in" and renamed link to "show occurrences". Made it possible to be hidden. This is committed over at #715984: Restore ability to hide "more info".

I'm attempting to fold the moderation and translation screens properly at #717112: Fold in moderation page to translation page properly. It is not only about dropping the moderation screen, but making the filters work better with user assumptions given our current UI and making it work in a way that is assumed for a moderation flow.