[WIP] Keyboard shortcuts for expand & collapse diff

 






 

 


  Review Request #10951
  —
  Created March 12, 2020 and updated 

 


















 

  

   Information

  





 
 

  hxqlin
 









 
 

  Review Board
 









 
 

  master
 









 
 

  
 









 
 

  10925
 









 
 

  a4627d4...
 






 




 

  

   Reviewers

  





 
 

  reviewboard
 









 
 

  
 






 








 





  


 
 

  
This patch adds keyboard shortcuts to expand & collapse the diff in a review

request and in a diff fragment in a review. Expand is triggered by the e 

key and collapse is triggered by the w key (the c key may seem more 

natural, but it's taken already by another shortcut so w was chosen for its 

proximity to the e key for ease of use between them).


For the diff viewer, expand and collapse buttons are now tracked as anchors

in RB.DiffViewerPageView. This allows us to find the next button on the 

page from the currently selected anchor and trigger it when a keyboard

shortcut is used. The e and w keys are added to the existing key

bindings mixin object to track those key presses and trigger the right 

button.


For diff fragments, there can only be one set of expand buttons and one 

collapse button in any given fragment. If the fragment is focused, the e

and w keys will trigger their respective expand and collapse buttons. This

is implemented by using the key bindings mixin in RB.DiffFragmentView. The 

shortcut to expand the fragment defaults to expanding the diff by 20 lines,

which is achieved by adding a modifier to desired expand button.

 








  


 
 

  
Manual testing & unit tests for the e and w key events in both the diff

fragment and diff viewer.

 







 









 





  








 






 
 
 

  

   

    

     
    

    
    
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
    

   

   

    
     

      Description
      From
      Last Updated
     

    
    



     

      
       
       
W291 trailing whitespace

      		
      
       reviewbotreviewbot
      
      
       
      
     




     

      
       
       
Can you update this to describe what this means? "Next" from what viewpoint? Same below.

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Arguments should align.

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Arguments should align.

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Blank line between a statement and a block.

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Function doc summaries must be a single unwrapped line. Always good to expand on meanings in subsequent paragraphs.

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Querying based on attributes is slow, and that's very important here, because such a query could end up taking an …

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
This can be: label = `'${key}'`;

      		
      
       chipx86chipx86
      
      
       
      
     


    
   

  

 







 






  

 



















 




 




 














 



 

  

   




hxqlin




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  








 
Change Summary:

 
Implement keyboard shortcuts in diff fragments





 



  

   
Commit:

   
    
-ff882a74102785befef7974d3701cb615ac294aa
+ae16814debfad9675f35e5b3fc73f8db50dc09ad

   		
  




  

   
Diff:

   
    
 Revision 2 (+87 -18)
Show changes
 
    reviewboard/reviews/templatetags/reviewtags.py 
    reviewboard/static/rb/css/pages/diffviewer.less 
    reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js 
    reviewboard/static/rb/js/views/diffFragmentView.es6.js 
    reviewboard/templates/diffviewer/diff_file_fragment.html 
    reviewboard/templates/diffviewer/expand_link.html 
    reviewboard/templates/reviews/diff_comment_fragment.html 
    reviewboard/templates/reviews/expand_link.html 
  1 more
 

   		
  



 









 

  
Checks run (1 failed, 1 succeeded)



  

 flake8
 failed.
 





  

 JSHint
 passed.
 





 







 
flake8

 


 















  

 
















 



 

  

   




hxqlin




   

   

  

 



 















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Can you update this to describe what this means? "Next" from what viewpoint?
    

    Same below.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Arguments should align.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Arguments should align.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Blank line between a statement and a block.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    6. 






  6. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Function doc summaries must be a single unwrapped line.
    

    Always good to expand on meanings in subsequent paragraphs.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    7. 






  7. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Querying based on attributes is slow, and that's very important here, because such a query could end up taking an actual noticeable amount of time for larger diffs. It'd be better to specifically look for and cache any such thing up-front, if we can, or to at least narrow down first by something more specific.
    

    Classes are also faster to query.
    

    Also, how is this finding the "next" one? This looks like it'd find all of them and click all of them. (Same with others.)
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      https://reviews.reviewboard.org/r/10951/#comment50645 - updated the selector to use a class instead of an attribute, but the reasoning is the same
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      Sorry this is a bit hard to follow - this is the primary comment I meant to reply to. I'll re-explain everything here. So after all the updated changes, here's why this would only click one button. You said that there will only be one collapse button or set of expand buttons in a diff fragment, so if we look for buttons within a diff fragment, we know we'll have selected a button corresponding to the current fragment. We also only trigger the shortcut if a particular diff fragment is focused so the buttons won't be triggered in the wrong fragment.
      

      For expand buttons, there will be four buttons in a diff fragment in total but we only want to trigger one of them for the shortcut. I added a new modifier class -is-default to the expand button that I chose to be the default for this shortcut, which is the one that will expand the diff by 20 lines. I chose this one since I thought it would be the most common use case out of the expand buttons, but feel free to correct me. So when we look for an expand button with that modifier within a diff fragment, we'll only find one. 
      

      For the collapse button, there will only be one of them in a diff fragment if there's one at all. Again, since we only trigger the shortcut for the current fragment if it's focused and we search for the button within the focused fragment, it will only find the collapse button for the current fragment if it exists and trigger it or do nothing at all.
      

      Hope that made more sense!
      
 
      

      3. 


 
    


    

 
    

    8. 






  8. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    This can be:
    

    

    label = `'${key}'`;

    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    9. 




  9. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    10. 




  

 
















 





 

  

   




hxqlin




   

   





  

 



 

  


   

   

    
 
hxqlin


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      I think you accidentally posted a new review, instead of replying to mine, so I'm not sure exactly what everything pertains to. When you comment in the diff viewer, you're creating a new review, not replying.
      
 
      

      2. 


 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 

   
    
    
    





    
    I added a comment at the bit of code that adds this [data-default-expand-btn="true"] attribute to the button explaining how this button will be the only one. The reasoning behind how these will only affect one button is because you said there will only be one collapse button or set of expand buttons in a diff fragment, so finding and clicking one within the element will only affect the current fragment.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/static/rb/js/views/tests/diffFragmentViewTests.es6.js
      


       (Diff revision 3)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    I forgot to leave some comments regarding this function - it's actually copied over from diffViewerPageViewTests.es6.js. Would it be better to pull this out into a util file somewhere? I wasn't sure if that was the right move or where to put it.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      It's fine to duplicate it right now. It would be ideal for us to have some common function someplace, but that'd be a more standalone task, and it isn't worth spending time on right now.
      
 
      

      2. 


 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/templates/diffviewer/diff_file_fragment.html
      


       (Diff revision 3)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    part of the dependent change
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 

   
    
    
    





    
    part of the dependent change
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    6. 






  6. 
 
    
  
  
    
  
 

   
    
    
    





    
    part of the dependent change
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    7. 






  7. 
 
    
  
  
    
  
 

   
    
    
    





    
    This is the only expand button that will have the data-default-expand-btn="true" in a particular diff fragment, so it will be the only button that's affected by the keyboard shortcut.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    8. 




  8. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    9. 




  

 
















 



 

  

   




hxqlin




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  








 
Change Summary:

 
Fix issues from review and update description.





 



  

   
Description:

   
    
     
This patch adds keyboard shortcuts to expand & collapse the diff in a review
 ~   request. Expand is triggered by the e key and collapse is triggered by the w
 ~   key (the c key may seem more natural, but it's taken already by another shortcut
 ~   so w was chosen for its proximity to the e key for ease of use between them).
   ~ request and in a diff fragment in a review. Expand is triggered by the e 
   ~ key and collapse is triggered by the w key (the c key may seem more 
   ~ natural, but it's taken already by another shortcut so w was chosen for its 
   + proximity to the e key for ease of use between them).
     

 ~   
This change is built on top of the changes in https://reviews.reviewboard.org/r/10925/,
 ~   which make the collapse diff button tabbable.
   ~ 
For the diff viewer, expand and collapse buttons are now tracked as anchors
   ~ in RB.DiffViewerPageView. This allows us to find the next button on the 
   + page from the currently selected anchor and trigger it when a keyboard
   + shortcut is used. The e and w keys are added to the existing key
   + bindings mixin object to track those key presses and trigger the right 
   + button.
   + 

   + 
For diff fragments, there can only be one set of expand buttons and one 
   + collapse button in any given fragment. If the fragment is focused, the e
   + and w keys will trigger their respective expand and collapse buttons. This
   + is implemented by using the key bindings mixin in RB.DiffFragmentView. The 
   + shortcut to expand the fragment defaults to expanding the diff by 20 lines,
   + which is achieved by adding a modifier to desired expand button.

   		
  




  

   
Testing Done:

   
    
 ~   
Manual testing
   ~ 
Manual testing & unit tests for the e and w key events in both the diff
   + fragment and diff viewer.

   		
  




  

   
Commit:

   
    
-b3b0589377ddddd40fc0571df4543d70be6f96d8
+a4627d4e91fcfef95358e9af7b7da926c82b7680

   		
  




  

   
Diff:

   
    
 Revision 4 (+135 -19)
Show changes
 
    reviewboard/reviews/templatetags/reviewtags.py 
    reviewboard/static/rb/css/pages/diffviewer.less 
    reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js 
    reviewboard/static/rb/js/pages/views/tests/diffViewerPageViewTests.es6.js 
    reviewboard/static/rb/js/views/diffFragmentView.es6.js 
    reviewboard/static/rb/js/views/tests/diffFragmentViewTests.es6.js 
    reviewboard/templates/diffviewer/diff_file_fragment.html 
    reviewboard/templates/diffviewer/expand_link.html 
  3 more
 

   		
  



 









 

  
Checks run (2 succeeded)



  

 flake8
 passed.
 





  

 JSHint
 passed.
 





 















  

 









     

    

   

  

  

   
   Loading...