Summary Table Auto-Update

Review Request #2726 — Created Dec. 1, 2011 and submitted

Information

Review Board

Reviewers

Summary table auto-update

Update the status of the rows within the summary table and update the counters.

Fixes:

Added an anchor selector to the find statement.
Refactored string concatenation.
Fixed comment style.
Removed white space.
Merged with master/origin.
Changed update method to use gCommentIssueManager.
Changed native DOM transformations to jQuery.
Added base value for parseInt. 
Changed jQuery .html() to .text() where relevant.
Ubuntu 10.04 (Linux): Chrome 14.
Description From Last Updated

You're still just calling updateIssueSummaryTable here. I think instead, you should register a callback via the gCommentIssueManager.registerCallback function. You'll need …

mike_conleymike_conley

Space after these comment slashes

mike_conleymike_conley

Instead of using innerHTML, we can use jQuery's .html() instead. While we're at it, we should probably through that string …

mike_conleymike_conley

Same as above

mike_conleymike_conley

Same as above, re: .html(), and parseInt.

mike_conleymike_conley

parseInt should take '10' as the second parameter. This is the base it'll convert to. Browsers are fairly tolerant about …

chipx86chipx86

I imagine you want to use text(). Only use html() If you actually want to deal with the raw HTML …

chipx86chipx86
chipx86
  1. I committed the parent change. Can you update this? Also, if you can show any screenshots, that'd help.
    1. Update r2726 or r2709?
  2. 
      
mike_conley
  1. Yazan:
    
    Nice work - just one question below.
    
    -Mike
  2. reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 1)
     
     
     
    Hm - I sort of figured we'd just register another callback, instead of inserting this new function.
    
    Is there a particular reason you did it this way instead?
    1. I can't say I'm exactly sure what you're asking here. I inserted this function here to avoid javascript racing events. I figured placing in the success clause of the "requestState" function seemed logical, as you would only want to update the table when the new state is set.
      
      Also, I'm not sure what Christian has asked me to update, any ideas?
    2. Yazan:
      
      I gotcha.  I remember when I did the issue code, I knew we wanted to have this table somehow, and that it'd need to react to issue status changes, etc.  That's why I threw in the gCommentIssueManager - it allows you to register callbacks to be fired when a comment state changes.  I figured that for each comment with an issue, we'd want to monitor state changes.
      
      That's how the current issues work, anyhow.  See lines 746-747 in reviews.js to see where they register their callback.
      
      If there's a good reason to have this new function, that's totally cool - but if not,  we might as well use the infrastructure that's there.
      
      -Mike
    3. Mike:
      
      I don't fully understand how "callbacks" work. I wanted to capture the older state of the issue before updating. Also, this function updates things that are not handled by the other functions, such as table row classes and counters. 
    4. Yazan:
      
      So the idea is that we have this global object, gCommentIssueManager, which has functions in it that allow us to register "callback" functions to be called when comments with a particular id are updated.  The callback is a function that takes a single parameter, which is the new state that the comment issue has transitioned to.
      
      Suppose I want some widget to be notified when a comment with an issue, with ID=251 has it's issue state changed.  I would do:
      
      var myCallback = function(aIssueStatus) {
        alert("Hey, sweet, comment with ID = 251 changed it's status to: " + aIssueStatus);
      }
      
      gCommentIssueManager.registerCallback(251, myCallback);
      
      If / when the comment issue's state get's updated (like if I'm the reviewee, and I change the comment issue from open to resolved), then myCallback will automatically get called by the gCommentIssueManager.
      
      Does that make more sense?
      
      -Mike
  3. 
      
ME
ME
mike_conley
  1. Hey Yazan:
    
    Hm, there appears to be some extra stuff in this diff that I don't think you added...
    
    Secondly, while you're a step closer to what I was asking for, I was hoping you would use the built in callback functionality of the gCommentIssueManager instead of hard-wiring your updateIssueSummaryTable function call.  Email me if you need any further assistance on that.
    
    Thanks,
    
    -Mike
  2. reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 3)
     
     
     
    Show all issues
    You're still just calling updateIssueSummaryTable here.
    
    I think instead, you should register a callback via the gCommentIssueManager.registerCallback function.
    
    You'll need to expand the notifyCallbacks helper function on line 162 to pass not only the new issue status, but the old one as well.
    1. I'm only trying to merge master into my branch for now, ran into a little trouble there. Talking to Christian on IRC on fixing my merge. The refactoring and will happen after I merge. I have not changed my code, at all, yet.
  3. 
      
ME
mike_conley
  1. Yazan:
    
    Hrm, there still seems to be some fragments of non-related changes in this diff...
    
    -Mike
    1. I can't get rid of those for some reason, any suggestions?
  2. 
      
ME
ME
mike_conley
  1. Yazan:
    
    Looking good, and real close.  Just a couple of smaller nits I found on this pass.
    
    Thanks for your work!
    
    -Mike
  2. Show all issues
    Space after these comment slashes
  3. Show all issues
    Instead of using innerHTML, we can use jQuery's .html() instead.
    
    While we're at it, we should probably through that string through parseInt - that way, if for some reason the contents of the counter are NOT numeric, parseInt will give us back something we can still decrement.  It might not be the correct number, but it won't halt the script.
  4. Show all issues
    Same as above
  5. Show all issues
    Same as above, re: .html(), and parseInt.
  6. reviewboard/templates/reviews/review_detail.html (Diff revision 6)
     
     
     
     
     
     
    We might want to replace these  's with some CSS styling instead...  
    1. I added a new class "counter" and played around with the margins to compensate for the  's.
  7. 
      
ME
chipx86
  1. Two last small things. Looking good though!
  2. Show all issues
    parseInt should take '10' as the second parameter. This is the base it'll convert to. Browsers are fairly tolerant about leaving it out, but it should be there.
  3. Show all issues
    I imagine you want to use text(). Only use html() If you actually want to deal with the raw HTML code (setting or getting).
  4. 
      
ME
david
  1. Ship It!
  2. 
      
ME
Review request changed
Status:
Completed
Change Summary:
Pushed to master (56b812e). Thanks!