Merge lp:~adiroiban/launchpad/bug-359180-take-2 into lp:launchpad
| Status: | Merged | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Approved by: | Graham Binns on 2010-03-16 | ||||||||
| Approved revision: | no longer in the source branch. | ||||||||
| Merged at revision: | not available | ||||||||
| Proposed branch: | lp:~adiroiban/launchpad/bug-359180-take-2 | ||||||||
| Merge into: | lp:launchpad | ||||||||
| Diff against target: |
802 lines (+446/-68) 11 files modified
lib/canonical/launchpad/javascript/translations/pofile.js (+306/-2) lib/canonical/launchpad/templates/batchnavigator-navigation-links.pt (+4/-0) lib/lp/translations/browser/pofile.py (+20/-2) lib/lp/translations/browser/tests/pofile-views.txt (+20/-2) lib/lp/translations/browser/translationmessage.py (+24/-0) lib/lp/translations/stories/standalone/xx-pofile-translate-newlines-check.txt (+2/-2) lib/lp/translations/stories/standalone/xx-pofile-translate.txt (+33/-5) lib/lp/translations/templates/currenttranslationmessage-translate-one.pt (+8/-11) lib/lp/translations/templates/pofile-translate.pt (+10/-43) lib/lp/translations/templates/translationmessage-translate.pt (+7/-0) lib/lp/translations/templates/translations-macros.pt (+12/-1) |
||||||||
| To merge this branch: | bzr merge lp:~adiroiban/launchpad/bug-359180-take-2 | ||||||||
| Related bugs: |
|
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Graham Binns (community) | code, js | 2010-02-25 | Approve on 2010-03-16 |
|
Review via email:
|
|||
This proposal supersedes a proposal from 2010-01-20.
Commit Message
Add keybindings on translating pofiles or translation messages.
Description of the Change
= Bug 35180 =
Would be useful if the First - Previous - Next - Last link in the translation interface had a keyboard shortcut for fast accessing them. Don't need to be visible, but at least documented somewhere.
Maybe even the Save & Continue button.
== Proposed Fix ==
Add the following keybindings on +translate page for pofile and translaitonmessage
First field is autofocused.
Shift+Alt+b - Focus first translation field
Shift+Alt+a - First page
Shift+Alt+n - Next page
Shift+Alt+p - Previous page
Shift+Alt+l - Last page
Shift+Alt+s - Save and continue
Shift+Alt+Down - Next field
Shift+Alt+Up - Previous field
Shift+Alt+j - Next field
Shift+Alt+k - Previous field
Shift+Alt+C - Copy original text (both singular and plural)
Shift+Alt+0 - Mark current translation
Shift+Alt+NUMBER - Mark suggestion NUMBER
Shift+Alt+d - Dismiss all suggestions
Shift+Alt+r - Tick "Someone should review this translation"
== Pre-implementation notes ==
I talked with Danilo about keybindng implementation using comments for bug 35180
Previous MPs:
https:/
https:/
Part of this branch was already approved in previous MPs, but I have refactored most of those functions.
== Implementation notes ==
The legacy JS code from canonical/
Those functions from lp.js was rewritten using YUI3. Maybe we can no delete them from lp.js.
I have added a test for Bug #513625, since that bug was produces by some changes in this branch.
== Tests ==
I was not able to produce a reasonable windmill test since it is not trivial to find the current focused node or to see if a node is focused.
This requires adding onFocus and onBlur trigger for all DOM nodes.
./bin/test -t pofile-views
== Demo and QA ==
Log in as admin or as a person with rights on adding translations to a pofile.
Go to a pofile translate page:
https:/
The first field should have focus and you can start translating right away.
Adding a new translation should automatically select the radio button in front of it.
Try the keybindings described in the "Proposed fix" section.
= Launchpad lint =
Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.
Linting changed files:
lib/canonical
== JSLint notices ==
No handlers could be found for logger "bzr"
jslint: No problem found in '/home/
jslint: 1 file to lint.
| Adi Roiban (adiroiban) wrote : | # |
| Paul Hummer (rockstar) wrote : | # |
Please change line 140 of this diff so that the page tests conform with the 80 character line rule. Other than that, thanks for looking into this.
| Adi Roiban (adiroiban) wrote : | # |
Due to bug 513625, the original branch was removed from lp/devel.
I have added a testcase for bug 513625 and I'm resubmitting this branch for review.
| Graham Binns (gmb) wrote : | # |
After talking with Adi, it's clear that the branch is actually an update to an older branch which was landed on devel and then pulled before 10.01 was released due to issue with the keybindings. However, I'm unable to get a sane diff for the changes since then.
Adi, in order to be able to review this branch I need a sane diff for it. At the moment I can't get one because I get a bunch of conflicts when trying to merge it into devel. Here's how you can produce one for me:
1. Branch devel.
2. Merge your original branch into your branch of devel.
3. Merge this branch and paste the diff into the merge proposal.
That should give us just the changes that you've made since your last branch was approved; I'll be happy to review the new diff.
| Adi Roiban (adiroiban) wrote : | # |
Hi,
$ bzr branch devel merge
$ bzr branch lp:~adiroiban/launchpad/bug-359180/ merge-1
$ cd merge
$ bzr merge ../merge-1/
Nothing to do.
The original branch is part of devel... just that a new commit is reverting those changes. There was no "hard rebase" to revert the branch.
I can not get a clean diff.
Is there an option for a filter to diff to only show lines changed be me?
Maybe it will be easier to have a new full review.
| Adi Roiban (adiroiban) wrote : | # |
Hi,
This is the diff.
It is still quite big as the original branch was submitted 2 months ago and was one of my first patch for LP.
Meanwhile I discovered the JavaScript guidelines and looking again at the code I found many places where the previous code could be improved.
Please let me know if there are any other actions I need to do to make this branch ready for review?
=== modified file 'lib/canonical/
--- lib/canonical/
+++ lib/canonical/
@@ -1,7 +1,7 @@
/** Copyright (c) 2009, Canonical Ltd. All rights reserved.
*
* @module lp.pofile
- * @requires event, node
+ * @requires anim, cookie, event-key, event, node
*/
YUI.add(
@@ -42,8 +42,48 @@
};
+var hide_notification = function(node) {
+ var hide_anim = new Y.Anim({
+ node: node,
+ to: { height: 0,
+ marginTop: 0, marginBottom: 0,
+ paddingTop: 0, paddingBottom: 0 }
+ });
+ node.setStyle(
+ hide_anim.
+ hide_anim.on('end', function(e) {
+ node.setStyle(
+ });
+ hide_anim.run();
+}
+
+self.updateNot
+ var notice = Y.one('
+ var balloon = notice.
+ var dismiss_
+ documentation_
+
+ // Check the cookie to see if the user has already dismissed
+ // the notification box for this session.
+ var already_seen = Y.Cookie.
+ if (already_seen) {
+ notice.
+ }
+
+ var cancel_button = notice.one(
+ '.important-
+ // Cancel button starts out hidden. If user has JavaScript,
+ // then we want to show it.
+ cancel_
+ cancel_
+ e.halt();
+ hide_notificati
+ Y.Cookie.
+ });
+};
+
+
self.setFocus = function(field) {
- //Y.log(e.type + ":" + e.keyCode + ': ' + field);
// if there is nofield, do nothing
if (Y.one('#' + field)) {
Y.one('#' + field).focus();
@@ -75,7 +115,7 @@
// The replacement regex strips all tags from the html.
to.
- selectWidget(
+ selectWidgetByI
};
@@ -97,7 +137,6 @@
var singular_select = translation_stem + '_translation_
var translation_
var translation_plural = translation_stem + '_translation_';
- //Y.log(e.type + ":" + e.keyCode + ': ' + singular_select);
// Copy singular text
copyOrigin
@@ -112,43 +151,102 @@
};
-var selectWidget = function(widget) {
- if (Y.one('#' + widget)) {
- Y.one('#' + widget)
| Graham Binns (gmb) wrote : | # |
Hi Adi,
I really like this branch; good job! I'm not entirely happy about the lack of Windmill tests, but I understand what you mean about it being non-trivial to write one for this case. Please file a bug about adding some Windmill tests for this, detailing the difficulties you had. It might be worth mailing the launchpad-dev list to see if anyone has any suggestions.

= Bug 359180 =
This is the follow up for the current commited branch for bug 359180 as it was discovered that Shift+Alt+up and Shift+Alt+Down are used by.
== Proposed fix ==
Use Shift+Alt+j and Shift+Alt+k for navigation.
Add fields in the translations_order only if the user can edit(add, change or suggest) the translation.
== Pre-implementation notes == /code.edge. launchpad. net/~adiroiban/ launchpad/ bug-359180/ +merge/ 16422
Notes can be found on the previous MP:
https:/
== Implementation details ==
There was a small refactorization for getting translations_order and autofocus_html_id.
== Tests ==
I was not able to produce a reasonable windmill test since it is not trivial to find the current focused node or to see if a node is focused.
This requires adding onFocus and onBlur trigger for all DOM nodes.
The test for the view is here:
./bin/test -t pofile-views
== Demo and Q/A == /code.edge. launchpad. net/~adiroiban/ launchpad/ bug-359180/ +merge/ 16422
Demo and Q/A can be found on the previous MP
https:/
Instead of UP and DOWN, use j and k
= Launchpad lint =
Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.
Linting changed files: /launchpad/ javascript/ translations/ pofile. js
lib/canonical
== JSLint notices == adi/launchpad/ lp-branches/bug-359180-take-2/ lib/canonical/ launchpad/ javascript/ translations/ pofile. js'.
No handlers could be found for logger "bzr"
jslint: No problem found in '/home/
jslint: 1 file to lint.