Add a keyboard shortcut central registry for page views.

 






 

 


  Review Request #10913
  —
  Created Feb. 24, 2020 and updated 

 




















 

  

   Information

  





 
 

  LittleGrey
 









 
 

  Review Board
 









 
 

  master
 









 
 

  
 









 
 

  
 









 
 

  10929
 






 




 

  

   Reviewers

  





 
 

  reviewboard
 









 
 

  
 






 








 





  


 
 

  
Created an object which can be a central registry for keyboard

shortcuts on the current page. This new object keyBindings is 

owned by RB.Pageview. Subclasses of RB.PageView (e.g., diffViewerPageView 

and dataGridPageView can extend this new object to have their own

keyboard shortcuts.


Each key set can map to two things: callback mapping to a callback

function and help (temporary mapping to a str text, can be a foundation 

of future features).

 








  


 
 

  
The old keyboard shortcuts on diffViewerPageView are still working.


Tested on Chrome and Firefox.


Tested in dataGridPageView by adding this dummy code segment:


...

keyBindings: _.extend({
    'a': {
        callback: 'testAlert',
    }
}, RB.PageView.prototype.keyBindings),

testAlert() {
    alert("This is a test alert.");
},
...


 







 









 





  

 


 
 

  



 

  
   


    Summary
    ID

    Author

   

  
  

   


    

     
Add a keyboard shortcut central registry for page views.


    		
    3dec5902e36f9da5eec25993418c4e88c667f80a

    Jay

   


   


    

     
Remove one extra  space.


    		
    3d82ba3525b1f48711372952ec94389951542d70

    Jay

   


   


    

     
Add missing semicolon.


    		
    5acf1f77a465eb609535fade44747d64bbd5e00e

    Jay

   


   


    

     
Remove console.log and change some wording.


    		
    e63693dc2281dd08feaf4957f3ab57717c50a342

    Jay

   


   


    

     
Use extend instead of overwriting.


    		
    699106b138a1736c497fdb1ed23c5cbdd20d5605

    Jay

   


   


    

     
Made keyBindings comments more clear.


    		
    30dcd5a21dd08fdcad24a2d9a7e83bcb52292214

    Jay

   



  
 




 













 






 
 
 

  

   

    

     
    

    
    
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
    

   

   

    
     

      Description
      From
      Last Updated
     

    
    



     

      
       
       
just to clarify how the keyboard shortcuts work, would pressing any key in for example 'aAKP<m' trigger the shortcut? also …

      		
      
       hxqlinhxqlin
      
      
       
      
     




     

      
       
       
Col: 61 Missing semicolon.

      		
      
       reviewbotreviewbot
      
      
       
      
     




     

      
       
       
Could change the wording from The dictionary to A dictionary

      		
      
       bui1bui1
      
      
       
      
     




     

      
       
       
use shortcut instead of shortcuts

      		
      
       bui1bui1
      
      
       
      
     




     

      
       
       
Try This key selects the previous comment. instead

      		
      
       bui1bui1
      
      
       
      
     




     

      
       
       
Try This key selects the next file. instead

      		
      
       bui1bui1
      
      
       
      
     




     

      
       
       
Try This key selects the previous diff. instead

      		
      
       bui1bui1
      
      
       
      
     




     

      
       
       
Try This key selects the next diff. instead

      		
      
       bui1bui1
      
      
       
      
     




     

      
       
       
Try This key selects the previous comment. instead

      		
      
       bui1bui1
      
      
       
      
     




     

      
       
       
Try This key selects the next comment. instead

      		
      
       bui1bui1
      
      
       
      
     




     

      
       
       
I'm not totally sure what this commands does on my first read through. I think changing the wording for this …

      		
      
       bui1bui1
      
      
       
      
     




     

      
       
       
Try This key creates a new comment. instead

      		
      
       bui1bui1
      
      
       
      
     




     

      
       
       
Could remove this console.log statement

      		
      
       bui1bui1
      
      
       
      
     




     

      
       
       
i think we usually use extend as opposed to expand

      		
      
       hxqlinhxqlin
      
      
       
      
     




     

      
       
       
not sure but i think defining keyBindings like this will overwrite the parent object; it's not a problem atm since …

      		
      
       hxqlinhxqlin
      
      
       
      
     




     

      
       
       
i would prefer doc instead of help since it feels more specific but this is up to interpretation

      		
      
       hxqlinhxqlin
      
      
       
      
     




     

      
       
       
Nit: "help texts" -> "help text" We should also indicate what fields are expected in the dictionary. What is the …

      		
      
       mike_conleymike_conley
      
      
       
      
     




     

      
       
       
I wonder if we should be more explicit in encouraging subclasses to extend this, by saying "Subclasses should extend this …

      		
      
       mike_conleymike_conley
      
      
       
      
     




     

      
       
       
"&" -> "and"

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
It would be great if this could document each key that's expected by an entry, using a format similar to …

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
"consisting"

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
For each of these, can you wrap the string with gettext()? That'll prepare it for localization.

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
We should also leave off the "This key" part. Just like: "Select the previous file." "Select the next diff." "Re-center …

      		
      
       chipx86chipx86
      
      
       
      
     


    
   

  

 







 






  

 



















 




 

  


   

   

    
 
Checks run (1 failed, 1 succeeded)


    
   


  

  

  





 

 flake8
 passed.
 





 

 JSHint
 failed.
 
















 
JSHint

 


 







  

 





 














 



 

  

   




LittleGrey




   

   

  

 



 















 





 

  

   




bui1




   

   







  

 



 

  


   

   

    
 
bui1


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Good work! I'm excited to see other pages extend their own custom keyboard shortcuts from this in the future. Just some small things that could help with wording
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Could change the wording from The dictionary to A dictionary
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    use shortcut instead of shortcuts
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Try This key selects the previous comment. instead
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Try This key selects the next file. instead
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    6. 






  6. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Try This key selects the previous diff. instead
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    7. 






  7. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Try This key selects the next diff. instead
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    8. 






  8. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Try This key selects the previous comment. instead
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    9. 






  9. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Try This key selects the next comment. instead
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    10. 






  10. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    I'm not totally sure what this commands does on my first read through. I think changing the wording for this will help clear up confusion with users
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    11. 






  11. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Try This key creates a new comment. instead
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    12. 






  12. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Could remove this console.log statement
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    13. 




  13. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    14. 




  

 
















 



 

  

   




LittleGrey




   

   

  

 



 















 





 

  

   




hxqlin




   

   







  

 



 

  


   

   

    
 
hxqlin


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 







  2. 
 
    
  
  
    
  
   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    just to clarify how the keyboard shortcuts work, would pressing any key in for example 'aAKP<m' trigger the shortcut? 
    

    also i think it would be helpful to link to any mockups or additional work related to this review request because it's not super clear from this work alone what you're trying to accomplish.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Yes, the shortcut will be triggered by pressing any key in aAKP<m. This project is not to accomplish something, it's only for building a foundation for page views so they can have shortcuts easily. I will attach my works related to this one in the future.
      
 
      

      2. 


 
    


    

 
    

    3. 















  3. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    i think we usually use extend as opposed to expand
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    not sure but i think defining keyBindings like this will overwrite the parent object; it's not a problem atm since the parent object is empty, but if we end up having global key bindings they won't work on these pages because they'll be overwritten. i think you want something like this:
    

    keyBindings: _.extend({
    ...
    }, RB.PageView.prototype.keyBindings),

    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    i would prefer doc instead of help since it feels more specific but this is up to interpretation
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    6. 




  6. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    7. 




  

 
















 



 

  

   




LittleGrey




   

   

  

 



 















 





 

  

   




mike_conley




   

   







  

 



 

  


   

   

    
 
mike_conley


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Hi Xiaohui,
    

    This looks like a great approach to me! Just a few minor suggestions.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Nit: "help texts" -> "help text"
    

    We should also indicate what fields are expected in the dictionary. What is the key supposed to be? What can the values be?
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    I wonder if we should be more explicit in encouraging subclasses to extend this, by saying "Subclasses should extend this instead of replacing it".
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  4. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 




  

 
















 



 

  

   




LittleGrey




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  











 



  

   
Commits:

   
    





 

  
   


    

    Summary
    ID

    Author

   

  
  

   


    

    

     
Add a keyboard shortcut central registry for page views.


    		
    3dec5902e36f9da5eec25993418c4e88c667f80a

    Jay

   


   


    

    

     
Remove one extra  space.


    		
    3d82ba3525b1f48711372952ec94389951542d70

    Jay

   


   


    

    

     
Add missing semicolon.


    		
    5acf1f77a465eb609535fade44747d64bbd5e00e

    Jay

   


   


    

    

     
Remove console.log and change some wording.


    		
    e63693dc2281dd08feaf4957f3ab57717c50a342

    Jay

   


   


    

    

     
Use extend instead of overwriting.


    		
    699106b138a1736c497fdb1ed23c5cbdd20d5605

    Jay

   




   


    

    

     
Add a keyboard shortcut central registry for page views.


    		
    3dec5902e36f9da5eec25993418c4e88c667f80a

    Jay

   


   


    

    

     
Remove one extra  space.


    		
    3d82ba3525b1f48711372952ec94389951542d70

    Jay

   


   


    

    

     
Add missing semicolon.


    		
    5acf1f77a465eb609535fade44747d64bbd5e00e

    Jay

   


   


    

    

     
Remove console.log and change some wording.


    		
    e63693dc2281dd08feaf4957f3ab57717c50a342

    Jay

   


   


    

    

     
Use extend instead of overwriting.


    		
    699106b138a1736c497fdb1ed23c5cbdd20d5605

    Jay

   


   


    

    

     
Made keyBindings comments more clear.


    		
    30dcd5a21dd08fdcad24a2d9a7e83bcb52292214

    Jay

   



  
 







   		
  




  

   
Diff:

   
    
 Revision 5 (+129 -49)
Show changes
 
    reviewboard/static/rb/js/pages/views/basePageView.es6.js 
    reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js 
    reviewboard/static/rb/js/utils/keyBindingUtils.es6.js 
 

   		
  



 









 

  
Checks run (2 succeeded)



  

 flake8
 passed.
 





  

 JSHint
 passed.
 





 















  

 
















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    "&" -> "and"
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/static/rb/js/pages/views/basePageView.es6.js
      


       (Diff revision 5)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    It would be great if this could document each key that's expected by an entry, using a format similar to our "Args".
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    "consisting"
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    For each of these, can you wrap the string with gettext()? That'll prepare it for localization.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    6. 




  6. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    7. 




  

 
















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    We should also leave off the "This key" part. Just like:
    

      

    • "Select the previous file."
      • 

    • "Select the next diff."
      • 

    • "Re-center the selected diff chunk."
      • 

    • "Create a comment."
      • 

    

    etc.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 









     

    

   

  

  

   
   Loading...