Hi Adi,
thanks for doing this. It's a great help for reviewers (that want to translate)!

First off one UI comment: I think the naming text is wrong. It should not read "Enter ... mode" but simply display the current mode. This is consistent with the way the little yellow edit buttons work: You display a current setting and add an edit icon to change it. So if Danilo and Michael concur, I ask you to just display the current mode, e.g. "Reviewer mode (What's this?)".

When trying out the feature, the lack of hover interaction was the first thing that struck me as odd. No underlyning, no changing mouse cursor. At least that's what happens (not) on Firefox. It's easily fixed, though. See below.

The other two main issues are the structure of the JS code and the missing CSS on the help pop-up. Please see my comments about the JS code. The missing CSS should be easily fixed.

I am marking it as "Needs fixing" for me to see what JS code fixes you come up with. Please don't hesitate to talk to me about it as I am not the Great Javascript Expert and might be missing something.

Thank you for your work!

Henning


> === modified file 'lib/canonical/launchpad/javascript/translations/pofile.js'
> --- lib/canonical/launchpad/javascript/translations/pofile.js	2010-03-26 23:26:18 +0000
> +++ lib/canonical/launchpad/javascript/translations/pofile.js	2010-04-16 09:09:12 +0000
> @@ -91,6 +91,90 @@
>    });
>  };
>  
> +var WORKING_MODE_SWITCH_ID = "#translation-switch-working-mode";
> +var WORKING_MODE_SWITCH_HELP_ID = "#translation-switch-working-mode-help";
> +var WORKING_MODE_COOKIE = "translation-working-mode";
> +var WORKING_MODE_REVIEWER = "reviewer";
> +var WORKING_MODE_TRANSLATOR = "translator";

Add a blank line here, please.

> +var getWorkingMode = function () {
> +    var current_mode = Y.Cookie.get(WORKING_MODE_COOKIE);
> +    if (current_mode == WORKING_MODE_TRANSLATOR) {
> +        return WORKING_MODE_TRANSLATOR;
> +    } else {
> +        return WORKING_MODE_REVIEWER;
> +    }
> +};

I do not see the magic that this function does. Why not simply this:

var getWorkingMode = function () {
    retrurn Y.Cookie.get(WORKING_MODE_COOKIE);
}

> +
> +var setTranslatorModeText = function () {
> +    switch_mode_text = Y.one(WORKING_MODE_SWITCH_ID);
> +    if (switch_mode_text === null) {
> +        Y.log("Could not find " + WORKING_MODE_SWITCH_ID);
> +        return;
> +    }
> +    switch_mode_text.set(
> +        'innerHTML',
> +        'Enter reviewer mode');
> +};

I link to avoid "innerHTML" where possible. There is a "text" attribute, so
I think this should work.

    switch_mode_text.text = 'Enter reviewer mode';

> +
> +var setReviewerModeText = function () {
> +    switch_mode_text = Y.one(WORKING_MODE_SWITCH_ID);
> +    if (switch_mode_text === null) {
> +        Y.log("Could not find " + WORKING_MODE_SWITCH_ID);
> +        return;
> +    }
> +    switch_mode_text.set(
> +        'innerHTML',
> +        'Enter translator mode');
> +};

Likewise. And a lot of duplicate code, too. Could that not be parameterized
somehow?

> +
> +
> +var switchWorkingMode = function () {
> +    if (getWorkingMode() == WORKING_MODE_TRANSLATOR) {
> +        setReviewerModeText();
> +        Y.Cookie.set(
> +            WORKING_MODE_COOKIE,
> +            WORKING_MODE_REVIEWER,
> +            {
> +                path: "/"
> +            });
> +    } else {
> +        setTranslatorModeText();
> +        Y.Cookie.set(
> +            WORKING_MODE_COOKIE,
> +            WORKING_MODE_TRANSLATOR,
> +            {
> +                path: "/"
> +            });
> +    }
> +};

Lot's of code duplication here! Please create a function that takes the
working mode as a parameter.

> +
> +/**
> + * Initialize the current working mode and attach the node event for
> + * switching between modes.
> + */
> +var initializeWorkingMode = function () {
> +
> +    var working_mode_switch = Y.one(WORKING_MODE_SWITCH_ID);
> +    // Initialize only if user is a reviewer and the switch is on the page.
> +    if (working_mode_switch !== null) {
> +        working_mode_switch.addClass("sprite edit js-action");
> +
> +        var working_mode_switch_help = Y.one(WORKING_MODE_SWITCH_HELP_ID); 
> +        if (working_mode_switch_help !== null) {
> +            var what_is_this = (
> +                '(<a href="/+help/working-modes.html" target="help">' +
> +                'What\'s this?</a>)');
> +            working_mode_switch_help.set("innerHTML", what_is_this);

Ouch, innerHTML again ... I think this whole link should be in the template
with the <li> tag being invisible by default. All you'd have to do here is
remove a class to make it visible.

> +        }
> +        if (getWorkingMode() == WORKING_MODE_TRANSLATOR) {
> +            setTranslatorModeText();
> +        } else {
> +            setReviewerModeText();
> +        }
> +        Y.on("click", switchWorkingMode, WORKING_MODE_SWITCH_ID);
> +    }
> +};
> +
>  
>  self.setFocus = function(field) {
>      // if there is nofield, do nothing
> @@ -180,7 +264,7 @@
>  };
>  
>  
> -var selectTranslation = function(e, widget_id) {
> +var selectTranslation = function(e, field) {
>      // Don't select when tabbing, navigating and simply pressing
>      // enter to submit the form.
>      // Looks like this is not needed for Epiphany and Chromium
> @@ -190,7 +274,24 @@
>          (e.shiftKey && e.altKey && e.keyCode == 75)) {
>              return;
>      }
> -    selectWidgetByID(widget_id);
> +
> +    var translation_select_id = field + '_select';
> +    selectWidgetByID(translation_select_id);
> +
> +    var working_mode_switch = Y.one(WORKING_MODE_SWITCH_ID);
> +    // Initialize only if user is a reviewer and the switch is on the page.

Why do you keep checking that the switch is on the page? Will this code run
on pages were it is not

> +    if (working_mode_switch !== null &&
> +        getWorkingMode() == WORKING_MODE_TRANSLATOR) {
> +        var translation_field = Y.one('#' + field);
> +        if (translation_field !== null &&
> +            translation_field.get('value') === '') {
> +            var html_parts = field.split('_');
> +            var force_suggetion = (
> +                html_parts[0] + '_' + html_parts[1] +
> +                '_force_suggestion');
> +            selectWidgetByID(force_suggetion);
> +        }
> +    }
>  };
>  
>  
> @@ -277,15 +378,15 @@
>          //  * msgset_2_pt_BR_translation_0_new_select
>          var html_parts = fields[key].split('_');
>          var msgset_id = html_parts[0] + '_' + html_parts[1];
> -        var translation_stem = fields[key].replace(/_translation_(\d)+_new/,"");
> -        var translation_select_id = fields[key] + '_select';
> +        var translation_stem = fields[key].replace(
> +            /_translation_(\d)+_new/,"");

translation_stem is not used anywhere. Do we really need the assignement?

>  
>          Y.on(
>              'change', selectTranslation,
> -            '#' + fields[key], Y, translation_select_id);
> +            '#' + fields[key], Y, fields[key]);
>          Y.on(
>              'keypress', selectTranslation,
> -            '#' + fields[key], Y, translation_select_id);
> +            '#' + fields[key], Y, fields[key]);
>  
>          // Set next field and copy text for all but last field
>          // (last is Save & Continue button)
> @@ -363,7 +464,8 @@
>      fields.push('save_and_continue_button');
>  
>      initializeGlobalKeyBindings(fields);
> -    initializeFieldsKeyBindings(fields); 
> +    initializeFieldsKeyBindings(fields);
> +    initializeWorkingMode();
>  };
>  
>  }, "0.1", {"requires": ["event", "event-key", "node", "cookie", "anim"]});
> 
> === added file 'lib/lp/translations/help/working-modes.html'
> --- lib/lp/translations/help/working-modes.html	1970-01-01 00:00:00 +0000
> +++ lib/lp/translations/help/working-modes.html	2010-04-16 09:09:12 +0000
> @@ -0,0 +1,56 @@
> +<html>
> +  <head>
> +    <title>Translation working modes</title>
> +    <link rel="stylesheet" type="text/css"
> +          href="/+icing/yui/current/cssreset/reset.css" />
> +    <link rel="stylesheet" type="text/css"
> +          href="/+icing/yui/current/cssfonts/fonts.css" />
> +    <link rel="stylesheet" type="text/css"
> +          href="/+icing/yui/current/cssbase/base.css" />
> +  </head>

I think some LP-specific style sheet must be missing here. Please look at the
other help files. When I open the pop-up it looks CSS-free....

[...]
> === modified file 'lib/lp/translations/stories/standalone/xx-translation-access-display.txt'
> --- lib/lp/translations/stories/standalone/xx-translation-access-display.txt	2009-07-02 17:16:50 +0000
> +++ lib/lp/translations/stories/standalone/xx-translation-access-display.txt	2010-04-16 09:09:12 +0000
> @@ -22,6 +22,14 @@
>      >>> print_tag(admin_browser.contents, 'translation-access')
>      You have full access to this translation.
>  
> +Users with full access will also have an option to switch between translator
> +and reviewer working mode.
> +
> +    >>> switch_working_mode = find_tag_by_id(
> +    ...     admin_browser.contents,
> +    ...     'translation-switch-working-mode')
> +    >>> switch_working_mode is not None
> +    True

Leave out the "not" and test for "False", please, because ...

>  
>  == Displaying translation groups and reviewers ==
>  
> @@ -106,6 +114,15 @@
>      Your suggestions will be held for review by the managers of this
>      translation.
>  
> +Users without full access will not have an option to switch between translator
> +and reviewer working mode.
> +
> +    >>> switch_working_mode = find_tag_by_id(
> +    ...     user_browser.contents,
> +    ...     'translation-switch-working-mode')
> +    >>> switch_working_mode is None
> +    True

... then the difference to here is much clearer.

Or use "not" in both cases. Maybe "True" is easier associated with "the tag
is visible", so this could be the better solution.

> +
>  If Evolution's translation is set to Closed mode, Joe will not be able
>  to submit suggestions.
>  
> @@ -118,6 +135,15 @@
>      >>> print_tag(user_browser.contents, 'translation-access')
>      This template can be translated only by its managers.
>  
> +If users are not allowed to submit suggestions, they will also not have an
> +option to switch between translator and reviewer working mode.
> +
> +    >>> switch_working_mode = find_tag_by_id(
> +    ...     user_browser.contents,
> +    ...     'translation-switch-working-mode')
> +    >>> switch_working_mode is None
> +    True

Make sure this is consistent with whatever you decided earlier.

> +
>  There is a special case where Joe visits a translation into a language
>  that isn't covered by the translation group: Joe is told he cannot enter
>  translations, and invited to contact the translation group about setting
> @@ -138,7 +164,8 @@
>  
>  Finally, if there is no translation group at all and the permissions do
>  not allow Joe to translate, the page shows that the translation is
> -closed.
> +closed and no option to switch between translator and reviewer working mode
> +will be displayed.
>  
>      >>> evolution.translationpermission = TranslationPermission.CLOSED
>      >>> evolution.translationgroup = None
> @@ -147,3 +174,9 @@
>      ...     'evolution/trunk/+pots/evolution-2.2/es/+translate')
>      >>> print_tag(user_browser.contents, 'translation-access')
>      This translation is not open for changes.
> +
> +    >>> switch_working_mode = find_tag_by_id(
> +    ...     user_browser.contents,
> +    ...     'translation-switch-working-mode')
> +    >>> switch_working_mode is None
> +    True

And here.

> 
> === modified file 'lib/lp/translations/templates/translations-macros.pt'
> --- lib/lp/translations/templates/translations-macros.pt	2010-03-24 18:36:55 +0000
> +++ lib/lp/translations/templates/translations-macros.pt	2010-04-16 09:09:12 +0000
> @@ -126,6 +126,10 @@
>              tal:define="link navigation_menu/details"
>              tal:condition="link/enabled"
>              tal:content="structure link/render"></li>
> +        <li tal:condition="view/user_is_official_translator">

As mentioned earlier, I'd add a CSS class here that makes it invisible and
have the JS remove it.

> +          <a id="translation-switch-working-mode"></a>

Pleaes add a dummy href="#" attribute to the tag so that it gets 'hover'
events. Otherwise it won't be underlined and the mouse cursor does not change.

> +          <span id="translation-switch-working-mode-help" />

Now you can simply put the help link in here.

> +        </li>
>        </ul>
>  </metal:nav-pofile-subpages>
>  
> 
> === modified file 'lib/lp/translations/windmill/tests/test_pofile_translate.py'
> --- lib/lp/translations/windmill/tests/test_pofile_translate.py	2010-04-11 20:07:25 +0000
> +++ lib/lp/translations/windmill/tests/test_pofile_translate.py	2010-04-16 09:09:12 +0000
> @@ -81,3 +81,93 @@
>                                         '_translation_0_new_select')
>          self._check_translation_autoselect(
>              start_url, new_translation_id, new_translation_select_id)
> +
> +
> +class POFileTranslatorAndReviewerWorkingMode(WindmillTestCase):
> +    """Tests for page behaviour in reviewer or translator mode."""
> +
> +    layer = TranslationsWindmillLayer
> +    suite_name = 'POFile Translate'
> +
> +    test_user = lpuser.TRANSLATIONS_ADMIN
> +
> +    switch_working_mode = u'translation-switch-working-mode'
> +    force_suggestion = u'msgset_1_force_suggestion'
> +    new_translation = u'msgset_1_pt_BR_translation_0_new'
> +    js_code = ("lookupNode({id: '%s'}).innerHTML.search('translator') > 0" %
> +            switch_working_mode)
> +
> +    def test_pofile_reviewer_mode(self):
> +        """Test for reviewer mode.
> +
> +        Adding new translations will not force them as suggestions.
> +        """
> +
> +        self.client.open(
> +            url='http://translations.launchpad.dev:8085/'
> +            'evolution/trunk/+pots/evolution-2.2/pt_BR/1/+translate')
> +        self.client.waits.forPageLoad(timeout=constants.PAGE_LOAD)
> +        self.test_user.ensure_login(self.client)
> +
> +        self._ensureTranslationMode(reviewer=True)
> +
> +        self.client.waits.forElement(
> +            id=self.force_suggestion, timeout=constants.FOR_ELEMENT)
> +        self.client.type(text=u'New translation', id=self.new_translation)
> +        self.client.asserts.assertNotChecked(id=self.force_suggestion)
> +
> +    def test_pofile_translator_mode(self):
> +        """Test for translator mode.
> +
> +        Adding new translations will not force them as suggestions.

Look, a stray "not" in here ... ;)

> +        """
> +
> +        self.client.open(
> +            url='http://translations.launchpad.dev:8085'
> +            '/evolution/trunk/+pots/evolution-2.2/pt_BR/1/+translate')
> +        self.client.waits.forPageLoad(timeout=constants.PAGE_LOAD)
> +        self.test_user.ensure_login(self.client)
> +
> +        self._ensureTranslationMode(translator=True)
> +
> +        self.client.waits.forElement(
> +            id=self.force_suggestion, timeout=constants.FOR_ELEMENT)
> +        self.client.type(text=u'New translation', id=self.new_translation)
> +        self.client.asserts.assertChecked(id=self.force_suggestion)
> +
> +        # The new translation will be forced only if the previous new
> +        # translation field is empty. Othewise the force suggestion checkbox
> +        # will remain unchecked.
> +        self.client.click(id=self.force_suggestion)
> +        self.client.keyPress(
> +            id=self.new_translation,
> +            options='a,true,false,false,false,false')

What does this do?

> +        self.client.asserts.assertNotChecked(id=self.force_suggestion)
> +
> +    def _ensureTranslationMode(self, reviewer=False, translator=False):
> +        """Ensure the specified mode is currently selected."""
> +
> +        if (reviewer is translator):
> +            raise AssertionError("You must specify a single working mode.")
> +
> +        self.client.waits.forElement(
> +            id=self.switch_working_mode, timeout=constants.FOR_ELEMENT)
> +
> +        current_is_reviewer = self.client.execJS(js=self.js_code)['output']
> +        need_to_switch_mode = False
> +        if reviewer and not current_is_reviewer:
> +            need_to_switch_mode = True
> +        if translator and current_is_reviewer:
> +            need_to_switch_mode = True
> +        if need_to_switch_mode:
> +            self.client.click(id=self.switch_working_mode)
> +        else:
> +            return
> +
> +        # We check that the mode was changed.
> +        current_is_reviewer = self.client.execJS(js=self.js_code)['output']
> +
> +        if reviewer and not current_is_reviewer:
> +            raise AssertionError("Could not set reviewer mode.")

Why is this needed? Is there no self.client.assert in Windmill? I might
be showing my ignorance here, though ... ;)

> +        if translator and current_is_reviewer:
> +            raise AssertionError("Could not set reviewer mode.")