Thanks for improving this! I was wrong in some instances and you explained well. Good. ;)
There are merge conflicts in your branch. I based this review on a diff against a branch where I resolved those as thought it would be right. Please merge devel and resolve those, too. I'll leave this as "needs fixing" just because of that. All other changes are good.
Please see my final comments below and incorporate them.
Thanks,
Henning
> === modified file 'lib/canonical/launchpad/javascript/translations/pofile.js'
> --- lib/canonical/launchpad/javascript/translations/pofile.js 2010-04-13 12:58:36 +0000
> +++ lib/canonical/launchpad/javascript/translations/pofile.js 2010-04-28 10:07:46 +0000
> @@ -92,6 +92,52 @@
> });
> };
>
> +var WORKING_MODE_SWITCH_ID = "#translation-switch-working-mode";
> +var WORKING_MODE_CONTAINER_ID = "#translation-switch-working-mode-container";
> +var WORKING_MODE_COOKIE = "translation-working-mode";
> +var WORKING_MODE_REVIEWER = "reviewer";
> +var WORKING_MODE_TRANSLATOR = "translator";
> +
> +var getWorkingMode = function () {
Sanatizing the cookie seems like a good idea but please add a short comment
about it.
> + var current_mode = Y.Cookie.get(WORKING_MODE_COOKIE);
> + if (current_mode == WORKING_MODE_TRANSLATOR) {
> + return WORKING_MODE_TRANSLATOR;
> + } else {
> + return WORKING_MODE_REVIEWER;
> + }
> +};
> +
> +var setWorkingMode = function (mode, text) {
> + Y.one(WORKING_MODE_SWITCH_ID).set('innerHTML', text);
> + Y.Cookie.set(WORKING_MODE_COOKIE, mode, {path: "/"});
> +};
I like the way you refactored this, very nice. ;) Let me just add one more
twist to it, if you don't mind.
I think 'text' does not need to be passed in but can be selected inside this
function. This way the actual strings are only defined here and not in two
places like you have it now.
var setWorkingMode = function (mode) {
if(mode == WORKING_MODE_TRANSLATOR) {
text = 'Translator mode';
} else {
text = 'Reviewer mode';
}
Y.one(WORKING_MODE_SWITCH_ID).set('innerHTML', text);
Y.Cookie.set(WORKING_MODE_COOKIE, mode, {path: "/"});
};
> +
One new line too many.
> +
> +var switchWorkingMode = function () {
> + if (getWorkingMode() == WORKING_MODE_TRANSLATOR) {
> + setWorkingMode(WORKING_MODE_REVIEWER, 'Reviewer mode');
> + } else {
> + setWorkingMode(WORKING_MODE_TRANSLATOR, 'Translator mode');
> + }
> +};
So, this becomes
var switchWorkingMode = function () {
if (getWorkingMode() == WORKING_MODE_TRANSLATOR) {
setWorkingMode(WORKING_MODE_REVIEWER);
} else {
setWorkingMode(WORKING_MODE_TRANSLATOR);
}
};
> +
> +/**
> + * Initialize the current working mode and attach the node event for
> + * switching between modes.
> + */
> +var initializeWorkingMode = function () {
> +
> + Y.one(WORKING_MODE_CONTAINER_ID).removeClass('unseen');
This container is still conditional in TAL, don't you need to check for
its presence here? I think I was wrong to ask you why you keep checking
for it. This looks like a place where it should be done, I guess.
> + if (getWorkingMode() == WORKING_MODE_TRANSLATOR) {
> + Y.one(WORKING_MODE_SWITCH_ID).set(
> + 'innerHTML', 'Translator mode');
> + } else {
> + Y.one(WORKING_MODE_SWITCH_ID).set(
> + 'innerHTML', 'Reviewer mode');
> + }
And this simply:
setWorkingMode(getWorkingMode());
Or is there a problem with the cookie being set at this stage?
> + Y.on("click", switchWorkingMode, WORKING_MODE_SWITCH_ID);
> +};
> +
>
> var setFocus = function(field) {
> // if there is nofield, do nothing
> @@ -193,6 +239,21 @@
> }
> 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.
"if user is a reviewer" sounds wrong. "if working in translator mode"?
> + if (working_mode_switch !== null &&
The check is approriate here because TAL might not display it at all.
> + 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 = (
Missing 's' here, 'force_suggestion'.
> + html_parts[0] + '_' + html_parts[1] +
> + '_force_suggestion');
> + selectWidgetByID(force_suggetion);
Here, too.
> + }
> + }
> };
>
>
> @@ -279,7 +340,8 @@
> // * 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_stem = fields[key].replace(
> + /_translation_(\d)+_new/,"");
>
> Y.on(
> 'change', selectTranslation,
> @@ -312,7 +374,7 @@
> },
> '#' + fields[key], 'down:82+shift+alt', Y, msgset_id);
>
> - // Shift+Alt+d - Toggle dismiss all translations
> + // Shifqt+Alt+d - Toggle dismiss all translations
Good catch!
> Y.on(
> 'key', function(e, stem) {
> toggleWidget(stem + '_dismiss');
> @@ -360,7 +422,8 @@
> fields.push('save_and_continue_button');
>
> initializeGlobalKeyBindings(fields);
> - initializeFieldsKeyBindings(fields);
> + initializeFieldsKeyBindings(fields);
> + initializeWorkingMode();
> };
>
>
>
> === modified file 'lib/lp/translations/help/imported-upload.html'
> === modified file 'lib/lp/translations/help/permissions-policies.html'
Thanks for fixing these two! ;)
> === 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-28 09:32:00 +0000
> @@ -0,0 +1,56 @@
> +
> +
> + Translation working modes
> + + href="/+icing/yui/cssreset/reset.css" />
> + + href="/+icing/yui/cssfonts/fonts.css" />
> + + href="/+icing/yui/cssbase/base.css" />
> +
Yes, looking much better!
[...]
>
> === 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-28 09:32:00 +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
>
[...]
> +
> + >>> switch_working_mode = find_tag_by_id(
> + ... user_browser.contents,
> + ... 'translation-switch-working-mode')
> + >>> switch_working_mode is None
> + True
Inconsistency! Please invert this.
>
> === 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-28 09:32:00 +0000
> @@ -126,6 +126,16 @@
> tal:define="link navigation_menu/details"
> tal:condition="link/enabled"
> tal:content="structure link/render">
> +