Tryton Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(361)

Issue 32881002: sao: Add keyboard shortcuts

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 1 week ago by perilla
Modified:
0 minutes ago
Reviewers:
pokoli, ced, reviewbot
Visibility:
Public.

Description

I think that use alt key instead ctrl key is a less invasive solution in order to not replace native browser shortcuts. According to irc log https://www.tryton.org/~irclog/2016-06-02.log.html#t2016-06-02 11:25 require:https://tryton-rietveld.appspot.com/31931002/ issue6104

Patch Set 1 #

Total comments: 4

Patch Set 2 : Shorcut on tab apply only on last tab. Definition on screen apply to all #

Patch Set 3 : Register shorcuts once on current tab #

Patch Set 4 : Adding global search shortcut #

Total comments: 1

Patch Set 5 : Register shorcuts on document ready #

Patch Set 6 : shortcut definition on separate function in order to reuse when create dialog #

Patch Set 7 : Callbacks only on allowed view types, example (pagedown and pageup only on tree view) #

Patch Set 8 : Evaluating if there is at least one view open, if shortcut is not global #

Patch Set 9 : Support for next and previous tab with alt+pagedown and alt+pageup #

Total comments: 2

Patch Set 10 : Reuse jQuery(document) #

Patch Set 11 : dont use anonymous function #

Patch Set 12 : Added global search on set_shortcuts #

Total comments: 2

Patch Set 13 : Define global shortcuts one by one, and dont use mousetrap inside dropdown menus #

Total comments: 17

Patch Set 14 : update to tip, and apply ced comments #

Total comments: 2

Patch Set 15 : Replace up and down by alt+up and alt+down, remove search for 'a' tag #

Total comments: 8

Patch Set 16 : Add changelog, modal dialog #

Patch Set 17 : Update to tip, remove kbd css #

Total comments: 19

Patch Set 18 : Apply fixes #

Patch Set 19 : Translated global shortcuts #

Total comments: 10

Patch Set 20 : Fix comments #

Patch Set 21 : Close, switch, reload available on all views #

Total comments: 31

Patch Set 22 : Fix comments and optimizations #

Total comments: 23

Patch Set 23 : Fix comments #

Patch Set 24 : Update to tip #

Unified diffs Side-by-side diffs Delta from patch set Stats (+141 lines, -13 lines) Patch
M CHANGELOG View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +1 line, -0 lines 0 comments Download
M bower.json View 1 1 chunk +2 lines, -1 line 0 comments Download
M index.html View 1 1 chunk +1 line, -0 lines 0 comments Download
M src/sao.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 2 chunks +108 lines, -0 lines 0 comments Download
M src/tab.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 2 chunks +18 lines, -1 line 0 comments Download
M src/view/form.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 11 chunks +11 lines, -11 lines 0 comments Download

Messages

Total messages: 78
perilla
2 months, 1 week ago (2016-12-10 04:46:54 UTC) #1
reviewbot
flake8 OK URL: https://codereview.tryton.org/32881002
2 months, 1 week ago (2016-12-10 05:02:14 UTC) #2
ced
Why is moustrap class added? How does it work when there are many tabs open? ...
2 months, 1 week ago (2016-12-10 09:36:20 UTC) #3
perilla
Shorcut on tab apply only on last tab. Definition on screen apply to all
2 months, 1 week ago (2016-12-10 12:59:07 UTC) #4
reviewbot
flake8 OK URL: https://codereview.tryton.org/32881002
2 months, 1 week ago (2016-12-10 13:01:56 UTC) #5
perilla
On 2016/12/10 09:36:20, ced wrote: > Why is moustrap class added? > > How does ...
2 months, 1 week ago (2016-12-10 13:03:21 UTC) #6
ced
On 2016/12/10 13:03:21, perilla wrote: > On 2016/12/10 09:36:20, ced wrote: > > Why is ...
2 months, 1 week ago (2016-12-10 13:14:58 UTC) #7
perilla
Register shorcuts once on current tab
2 months, 1 week ago (2016-12-11 03:11:11 UTC) #8
reviewbot
flake8 OK URL: https://codereview.tryton.org/32881002
2 months, 1 week ago (2016-12-11 03:25:36 UTC) #9
perilla
Adding global search shortcut
2 months, 1 week ago (2016-12-11 05:20:56 UTC) #10
reviewbot
flake8 OK URL: https://codereview.tryton.org/32881002
2 months, 1 week ago (2016-12-11 05:24:30 UTC) #11
ced
I do not think shortcuts should be set on tab creation. Indeed I think they ...
2 months, 1 week ago (2016-12-11 10:57:52 UTC) #12
perilla
Register shorcuts on document ready
2 months, 1 week ago (2016-12-12 03:24:04 UTC) #13
reviewbot
flake8 OK URL: https://codereview.tryton.org/32881002
2 months, 1 week ago (2016-12-12 03:34:44 UTC) #14
perilla
shortcut definition on separate function in order to reuse when create dialog
2 months, 1 week ago (2016-12-12 03:40:01 UTC) #15
reviewbot
flake8 OK URL: https://codereview.tryton.org/32881002
2 months, 1 week ago (2016-12-12 03:53:06 UTC) #16
perilla
Callbacks only on allowed view types, example (pagedown and pageup only on tree view)
2 months, 1 week ago (2016-12-13 13:21:05 UTC) #17
perilla
Evaluating if there is at least one view open, if shortcut is not global
2 months, 1 week ago (2016-12-13 13:30:10 UTC) #18
reviewbot
flake8 OK URL: https://codereview.tryton.org/32881002
2 months, 1 week ago (2016-12-13 13:33:31 UTC) #19
perilla
Support for next and previous tab with alt+pagedown and alt+pageup
2 months, 1 week ago (2016-12-14 10:52:04 UTC) #20
reviewbot
flake8 OK URL: https://codereview.tryton.org/32881002
2 months, 1 week ago (2016-12-14 11:02:46 UTC) #21
pokoli
https://tryton-rietveld.appspot.com/32881002/diff/150001/src/sao.js File src/sao.js (right): https://tryton-rietveld.appspot.com/32881002/diff/150001/src/sao.js#newcode587 src/sao.js:587: jQuery(document).ready(function() { is really required to call aonther time ...
2 months, 1 week ago (2016-12-14 11:24:53 UTC) #22
perilla
Reuse jQuery(document)
2 months, 1 week ago (2016-12-14 12:51:02 UTC) #23
reviewbot
flake8 OK URL: https://codereview.tryton.org/32881002
2 months, 1 week ago (2016-12-14 12:55:33 UTC) #24
perilla
dont use anonymous function
2 months, 1 week ago (2016-12-14 18:06:37 UTC) #25
reviewbot
flake8 OK URL: https://codereview.tryton.org/32881002
2 months, 1 week ago (2016-12-14 18:29:07 UTC) #26
perilla
Added global search on set_shortcuts
2 months, 1 week ago (2016-12-14 20:01:37 UTC) #27
reviewbot
flake8 OK URL: https://codereview.tryton.org/32881002
2 months, 1 week ago (2016-12-14 20:03:01 UTC) #28
pokoli
https://tryton-rietveld.appspot.com/32881002/diff/210001/src/sao.js File src/sao.js (right): https://tryton-rietveld.appspot.com/32881002/diff/210001/src/sao.js#newcode625 src/sao.js:625: //TODO Add global shorcuts (global search, ...) This is ...
2 months, 1 week ago (2016-12-15 08:45:05 UTC) #29
perilla
Define global shortcuts one by one, and dont use mousetrap inside dropdown menus
2 months, 1 week ago (2016-12-15 13:01:17 UTC) #30
reviewbot
flake8 OK URL: https://codereview.tryton.org/32881002
2 months, 1 week ago (2016-12-15 13:01:55 UTC) #31
ced
https://tryton-rietveld.appspot.com/32881002/diff/230001/src/sao.js File src/sao.js (right): https://tryton-rietveld.appspot.com/32881002/diff/230001/src/sao.js#newcode588 src/sao.js:588: .on('ready', set_shortcuts); I think it is better to use ...
1 month, 3 weeks ago (2016-12-26 15:43:32 UTC) #32
perilla
update to tip, and apply ced comments
1 month, 1 week ago (2017-01-08 04:21:16 UTC) #33
perilla
https://tryton-rietveld.appspot.com/32881002/diff/230001/src/sao.js File src/sao.js (right): https://tryton-rietveld.appspot.com/32881002/diff/230001/src/sao.js#newcode588 src/sao.js:588: .on('ready', set_shortcuts); On 2016/12/26 15:43:31, ced wrote: > I ...
1 month, 1 week ago (2017-01-08 04:25:19 UTC) #34
perilla
https://tryton-rietveld.appspot.com/32881002/diff/250001/src/sao.js File src/sao.js (right): https://tryton-rietveld.appspot.com/32881002/diff/250001/src/sao.js#newcode641 src/sao.js:641: current_tab.el.find('a[id="' + func + '"]').click(); find button link on ...
1 month, 1 week ago (2017-01-08 04:31:06 UTC) #35
perilla
1 month, 1 week ago (2017-01-08 04:31:34 UTC) #36
perilla
1 month, 1 week ago (2017-01-08 04:31:59 UTC) #37
reviewbot
flake8 OK URL: https://codereview.tryton.org/32881002
1 month, 1 week ago (2017-01-08 04:45:02 UTC) #38
ced
https://tryton-rietveld.appspot.com/32881002/diff/230001/src/sao.js File src/sao.js (right): https://tryton-rietveld.appspot.com/32881002/diff/230001/src/sao.js#newcode638 src/sao.js:638: document.activeElement.tagName != 'A') { On 2017/01/08 04:25:19, perilla wrote: ...
1 month, 1 week ago (2017-01-08 10:33:05 UTC) #39
perilla
On 2017/01/08 10:33:05, ced wrote: > https://tryton-rietveld.appspot.com/32881002/diff/230001/src/sao.js > File src/sao.js (right): > > https://tryton-rietveld.appspot.com/32881002/diff/230001/src/sao.js#newcode638 > ...
1 month, 1 week ago (2017-01-08 11:47:25 UTC) #40
ced
On 2017/01/08 11:47:25, perilla wrote: > On 2017/01/08 10:33:05, ced wrote: > > https://tryton-rietveld.appspot.com/32881002/diff/230001/src/sao.js > ...
1 month, 1 week ago (2017-01-08 11:56:15 UTC) #41
perilla
Replace up and down by alt+up and alt+down, remove search for 'a' tag
1 month, 1 week ago (2017-01-08 17:42:18 UTC) #42
reviewbot
flake8 OK URL: https://codereview.tryton.org/32881002
1 month, 1 week ago (2017-01-08 17:48:28 UTC) #43
ced
Missing a dialog which explain all the shortcuts. https://tryton-rietveld.appspot.com/32881002/diff/270001/src/sao.js File src/sao.js (right): https://tryton-rietveld.appspot.com/32881002/diff/270001/src/sao.js#newcode588 src/sao.js:588: jQuery(document).ready(function() ...
1 month, 1 week ago (2017-01-10 10:29:22 UTC) #44
ced
On 2017/01/10 10:29:22, ced wrote: > Missing a dialog which explain all the shortcuts. And ...
1 month, 1 week ago (2017-01-10 10:29:46 UTC) #45
perilla
https://tryton-rietveld.appspot.com/32881002/diff/270001/src/sao.js File src/sao.js (right): https://tryton-rietveld.appspot.com/32881002/diff/270001/src/sao.js#newcode588 src/sao.js:588: jQuery(document).ready(function() { On 2017/01/10 10:29:22, ced wrote: > Why ...
1 month ago (2017-01-20 04:53:42 UTC) #46
perilla
Add changelog, modal dialog
1 month ago (2017-01-20 04:54:48 UTC) #47
reviewbot
flake8 OK URL: https://codereview.tryton.org/32881002
1 month ago (2017-01-20 05:11:59 UTC) #48
perilla
Remove kbd css, add missing semicolon
1 month ago (2017-01-20 13:12:05 UTC) #49
reviewbot
flake8 OK URL: https://codereview.tryton.org/32881002
1 month ago (2017-01-20 13:13:51 UTC) #50
perilla
Update to tip, remove kbd css
1 month ago (2017-01-20 14:25:33 UTC) #51
reviewbot
flake8 OK URL: https://codereview.tryton.org/32881002
1 month ago (2017-01-20 14:38:33 UTC) #52
ced
You should write the final commit message. https://tryton-rietveld.appspot.com/32881002/diff/270001/src/sao.js File src/sao.js (right): https://tryton-rietveld.appspot.com/32881002/diff/270001/src/sao.js#newcode620 src/sao.js:620: [Sao.i18n.gettext('Next'), 'next', ...
1 month ago (2017-01-21 00:31:50 UTC) #53
perilla
''
3 weeks, 6 days ago (2017-01-23 21:33:03 UTC) #54
perilla
https://tryton-rietveld.appspot.com/32881002/diff/330001/src/sao.js File src/sao.js (right): https://tryton-rietveld.appspot.com/32881002/diff/330001/src/sao.js#newcode590 src/sao.js:590: }); On 2017/01/21 00:31:49, ced wrote: > I still ...
3 weeks, 6 days ago (2017-01-23 21:36:24 UTC) #55
reviewbot
flake8 OK URL: https://codereview.tryton.org/32881002
3 weeks, 6 days ago (2017-01-23 21:39:03 UTC) #56
ced
https://tryton-rietveld.appspot.com/32881002/diff/330001/src/sao.js File src/sao.js (right): https://tryton-rietveld.appspot.com/32881002/diff/330001/src/sao.js#newcode700 src/sao.js:700: ['Previous tab', 'alt+pageup'], On 2017/01/23 21:36:23, perilla wrote: > ...
3 weeks, 6 days ago (2017-01-23 21:58:15 UTC) #57
perilla
Translated global shortcuts
3 weeks, 6 days ago (2017-01-23 22:28:30 UTC) #58
reviewbot
flake8 OK URL: https://codereview.tryton.org/32881002
3 weeks, 6 days ago (2017-01-23 22:40:27 UTC) #59
pokoli
https://tryton-rietveld.appspot.com/32881002/diff/370001/CHANGELOG File CHANGELOG (right): https://tryton-rietveld.appspot.com/32881002/diff/370001/CHANGELOG#newcode1 CHANGELOG:1: * Add keyboard shortcuts Must be updated as there ...
3 weeks, 3 days ago (2017-01-26 15:19:39 UTC) #60
perilla
Fix comments
3 weeks, 2 days ago (2017-01-27 15:16:36 UTC) #61
perilla
https://tryton-rietveld.appspot.com/32881002/diff/370001/CHANGELOG File CHANGELOG (right): https://tryton-rietveld.appspot.com/32881002/diff/370001/CHANGELOG#newcode1 CHANGELOG:1: * Add keyboard shortcuts On 2017/01/26 15:19:39, pokoli wrote: ...
3 weeks, 2 days ago (2017-01-27 15:47:50 UTC) #62
reviewbot
flake8 OK URL: https://codereview.tryton.org/32881002
3 weeks, 2 days ago (2017-01-27 15:49:00 UTC) #63
perilla
Close, switch, reload available on all views
3 weeks, 2 days ago (2017-01-27 16:00:53 UTC) #64
reviewbot
flake8 OK URL: https://codereview.tryton.org/32881002
3 weeks, 2 days ago (2017-01-27 16:17:16 UTC) #65
ced
The description of this review has a better place on the tracker. Here it should ...
3 weeks, 1 day ago (2017-01-28 23:58:21 UTC) #66
perilla
https://tryton-rietveld.appspot.com/32881002/diff/410001/src/sao.js File src/sao.js (right): https://tryton-rietveld.appspot.com/32881002/diff/410001/src/sao.js#newcode617 src/sao.js:617: [Sao.i18n.gettext('Search'), 'search', 'alt+f', ['tree']], On 2017/01/28 23:58:20, ced wrote: ...
2 weeks, 4 days ago (2017-02-01 14:05:17 UTC) #67
perilla
Fix comments and optimizations
2 weeks, 4 days ago (2017-02-01 17:02:57 UTC) #68
reviewbot
flake8 OK URL: https://codereview.tryton.org/32881002
2 weeks, 4 days ago (2017-02-01 17:17:35 UTC) #69
ced
https://tryton-rietveld.appspot.com/32881002/diff/430001/src/sao.js File src/sao.js (right): https://tryton-rietveld.appspot.com/32881002/diff/430001/src/sao.js#newcode604 src/sao.js:604: // [type, name, id of button or function, shortcut] ...
1 week, 5 days ago (2017-02-07 23:25:49 UTC) #70
ced
https://tryton-rietveld.appspot.com/32881002/diff/430001/src/sao.js File src/sao.js (right): https://tryton-rietveld.appspot.com/32881002/diff/430001/src/sao.js#newcode637 src/sao.js:637: jQuery(document).ready(function() { On 2017/02/07 23:25:49, ced wrote: > I ...
1 week, 5 days ago (2017-02-08 08:51:44 UTC) #71
perilla
Fix comments
1 week, 1 day ago (2017-02-12 04:03:31 UTC) #72
perilla
https://tryton-rietveld.appspot.com/32881002/diff/430001/src/sao.js File src/sao.js (right): https://tryton-rietveld.appspot.com/32881002/diff/430001/src/sao.js#newcode604 src/sao.js:604: // [type, name, id of button or function, shortcut] ...
1 week, 1 day ago (2017-02-12 04:10:06 UTC) #73
reviewbot
flake8 OK URL: https://codereview.tryton.org/32881002
1 week, 1 day ago (2017-02-12 04:14:52 UTC) #74
perilla
Update to tip
6 days ago (2017-02-14 13:05:51 UTC) #75
reviewbot
flake8 OK URL: https://codereview.tryton.org/32881002
5 days, 23 hours ago (2017-02-14 13:39:36 UTC) #76
ced
https://tryton-rietveld.appspot.com/32881002/diff/430001/src/sao.js File src/sao.js (right): https://tryton-rietveld.appspot.com/32881002/diff/430001/src/sao.js#newcode650 src/sao.js:650: e.preventDefault(); On 2017/02/12 04:10:06, perilla wrote: > On 2017/02/07 ...
5 days, 14 hours ago (2017-02-14 22:34:03 UTC) #77
perilla
0 minutes ago (2017-02-20 13:26:42 UTC) #78
https://tryton-rietveld.appspot.com/32881002/diff/430001/src/sao.js
File src/sao.js (right):

https://tryton-rietveld.appspot.com/32881002/diff/430001/src/sao.js#newcode650
src/sao.js:650: e.preventDefault();
On 2017/02/14 22:34:03, ced wrote:
> On 2017/02/12 04:10:06, perilla wrote:
> > On 2017/02/07 23:25:49, ced wrote:
> > > it is really needed?
> > 
> > In case that shorcut is recognized as
> > a native browser browser shortcut.
> 
> But is not managed by mousetrap?

If I remove this line, Firefox don't trigger alt+h.

https://tryton-rietveld.appspot.com/32881002/diff/430001/src/sao.js#newcode654
src/sao.js:654: focused.focus();
On 2017/02/14 22:34:03, ced wrote:
> On 2017/02/12 04:10:06, perilla wrote:
> > On 2017/02/07 23:25:49, ced wrote:
> > > why this focus stuff?
> > 
> > When I am editing a one2many record inside a 
> > form, or a editable tree, when I use alt+s to
> > save changes, only changes outside one2many are
> > changed.
> > 
> > You can see this behaviour on this link.
> > https://vimeo.com/203652638
> 
> 
> OK but I think the correct way to fix it is by implementing the set_value for
> editable Tree. But it should be done in a separate patch.

Acknowledged.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cd18842