Posts Tagged OnScroll

v1.0 Release Complete

Well this is it guys. My last release for the open source course is finished! It has come at the right time too as this is my 100th blog post. So for this release I ended up doing quite a bit more than I originally planned to. It all kind of became hectic in the end because ted suddenly reviewed my OnScroll patch yesterday, which meant that I immediately had to fix the changes he outlined.

First of all I tackled the files touched patch. Jorendorff has mentioned a few times that it is important to show information in an unobtrusive manner. Now, this wasn’t being accomplished earlier and so I decided to put expand/collapse functionality for the files touched.

Next I tackled a new bug where I only wanted  >5 merges to get expand/collapse functionality. Anything less than that should be left as it is. I’ve got that working now.

As I mentioned earlier, I also tackled the OnScroll patch. Over the course of these last eight months this has been the patch that I’ve worked on the most. In this release I’ve done a major rehaul. The client-side code looks very different now as I had to change it to work with the new hg_templates/pushlog.tmpl. Also, I’ve used a lot of jquery to make the code shorter and simpler. Furthermore, I’ve improved bug functionality even further and got the “To Local” link to work with the new entries that load on scroll. These were the goals that I had originally set out to meet and I’ve accomplished them

Now, as I said earlier ted, djc and Pike all gave me some feedback yesterday regarding my OnScroll patch. Thus, I had to make some last minute changes to my release. First of all I tackled a problem with the server side code that handles merges. Previously I was using a long string delimited by a random separator. Now, I’ve replaced that with a nice and simple dictionary data structure. This took a bit of time as I had to rehaul major parts of my client-side implementation so that it could properly read the data that was now being stored in dictionaries.

Furthermore, I moved all my JavaScript code to an external file since the amount of code was just getting to be too much to handle in script tags. I also changed how I deal with the parity counter and how I retrieve the maximum number of entries in the database. Lastly, I made a change in regards to pushlog queries. Beforehand, my code was causing more entries to load even when a user executed a query. That should not have been happening and now I’ve managed to fix that problem.

The following are the important links from this release:

Finally, the release is done! All the patches have been posted to their respective bug pages. Please view the project page for more information.

, , ,

No Comments

v1.0 Release - Various Changes to Improve the OnScroll Patch

Yesterday, ted reviewed my onScroll patch and and he outlined some changes that I should look to make. Lets dive in right away and have a look at what I have changed according to his recommendations:

Move javascript to an external file

ted recommended that I move all my JavaScript to an external file since the amount of lines was just too much. Having all that code in the script tags is not ideal. Thus, I’ve now moved all my JavaScript code to an external file leaving only the following in hg_templates/pushlog.tmpl:

var start = 0;
if(window.top.location.search.substring(1) == "")
  fillPage({max});
 
$(window).scroll(function() {
  if(start > 0) {
    if($(window).scrollTop() == $(document).height() - $(window).height()) {
      loadEntries(1);
    }
  }
});

Change the parity counter

I’ve also changed how I deal with the parity counter which is responsible for setting up the class of each row. In the pushlog the class value of the rows alternates between ‘parity0′ and ‘parity1′. Beforehand I was using an if/else block to replicate this functionality for the new entries loaded on scroll. However, this has now been changed to the following which is a better implementation:

var row = $(document.createElement('tr'));
row.addClass('parity' + counter % 2);
counter++;

So basically if the counter is an odd value counter % 2 will return 1, otherwise it will return 0. For each row I increment the counter by 1, which means that it alternates between an even and odd value.

Change how max entries is retrieved

ted was wondering why I was doing the following to retrieve the total amount of entries in the database:

<div id="page_header" class="page_header" maxentries="#max#">
start = ($("#page_header").attr("maxentries")) - 10;

I know that the above implementation was a very round about way of doing this. In my earlier implementations I had tried doing what he recommended, which is to just declare var start = {max} but for some odd reason that didn’t work for me initially. I tried this again today and now somehow it works. This had me very confused and frustrated. I thought a bit harder about this oddity and realized that what I had probably tried beforehand was this: var start =’{max}’ which sets start to an empty string. Anyways, I’ve rectified the situation and now I do the following to set the value of the ’start’ variable:

fillPage({max});
function fillPage(max) {
  start = max - 10;
  var vHeight = 0;
 
  if (document.documentElement)
    vHeight = document.documentElement.clientHeight;
  else
    vHeight = document.body.clientHeight;
 
  if (document.body.offsetHeight &lt; vHeight) {
    for(var count = 0, offSetHeight = document.body.offsetHeight; offSetHeight &lt; vHeight; count++)
      offSetHeight += 180;
 
    if(count &gt; 0)
      loadEntries(count);
  }
}

OnScroll should not work with any pushlog queries

ted brought up a very good point in one of his comments on the bug page. He asked what would happen if a user ran a query? Would the pushlog try to load more entries even though the user just wants to see his query data? Well, for my previous implementation the answer was yes. The function fillPage() would get called when the query is loaded and it would try to load more entries. I needed to fix this situation and I did so by adding the following code:

if(window.top.location.search.substring(1) == "")
    fillPage({max});

window.top.location.search.substring(1) will return an empty string if no query has been executed. Thus I can just perform a simple if statement to check whether the pushlog is being loaded normally or is a query being executed. If it happens to be the former then fillPage() is called, otherwise it isn’t.

,

No Comments

v1.0 Release - Changing the Data Structure that Handles Merges for the OnScroll Patch

Today ted unexpectedly reviewed my onScroll patch. Now, suddenly I have a ton of changes that I have to make and my release is due tomorrow (technically today since the clock just hit midnight).

One of the problems that ted focused on was how I passed merge changesets from the server-side to the client-side. I was using a weird and ugly string which is delimited by a random separator. I’ve known all along that this was a very ugly solution. When I had come up with this implementation I didn’t know python very well and thus did not use the dictionary data structure, which is an integral part of the language. I don’t know why I haven’t changed this implementation until now. I guess I decided to give other things more priority because although the solution was ugly, it worked.

Nonetheless, I have made the appropriate changes and the following is what my server-side solution looks like now:

1
2
3
4
5
def getMergeData(ctx, mergeData):
    for cs in ctx.parents():
        mergeData.append({'node': ctx.hex(), 'user': clean(person(cs.user())), 'desc': clean(cs.description())})
        if len(cs.parents()) > 1:
            getMergeData(cs, mergeData)

The client side implementation:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
function loadEntries(iterations) {
  for(var t = 0; t < iterations; t++) {
    var end = start;
 
    $('.loader').html('<img src="static/ajax-loader.gif" align="right" />');
    start -= 20;
    $.get('/json-pushes?startID=' + start + '&endID=' + end, function(responseText) {
      var pushData = JSON.parse(responseText), counter = 0;
      for(var i = end; i > start; i--) {
        var row = $(document.createElement('tr'));
        row.addClass('parity' + counter % 2);
        counter++;
 
        // user column
        $('<td>' + pushData[i].user + '<br /><span class="date">' + pushData[i].formattedDate + '</span></td>')
          .appendTo(row)
          .width(184);  
        // rev column
        $('<td>' + createRevTd(pushData[i].individualChangeset) + '</td>').appendTo(row);
        // author and description column
        $('<td>' + '<strong>' + pushData[i].author + ' &mdash ' + createBuglink(pushData[i].desc) + '</strong>' + '</td>').appendTo(row);
        $('.loader').html("");
        $('.titlePush').append(row);
 
        //Check whether it is a merge changeset or not
        if(pushData[i].mergeData != []) {
          for(var j = 0; j < pushData[i].mergeData.length; j++) {
            if(pushData[i].mergeData[j] != "") {
              var node = pushData[i].mergeData[j]['node'];
              var user = pushData[i].mergeData[j]['user'];
              var desc = pushData[i].mergeData[j]['desc'];
 
              createMergeRows(node, user, desc, row);
 
              // nested lists
              if(pushData[i].mergeData[j] instanceof Array) {
                for(var k = 0; k < pushData[i].mergeData[j].length; k++) {
                  node = pushData[i].mergeData[j][k]['node'];
                  user = pushData[i].mergeData[j][k]['user'];
                  desc = pushData[i].mergeData[j][k]['desc'];
 
                  createMergeRows(node, user, desc, row);
                }
              }
            }
          }
        }
      }    
    });
  }
}

So, as you can see now I’m using a list that holds a bunch of dictionaries. Each dictionary has 3 keys: node, user and desc. I pass the the list, mergeData over the to the client side via JSON. Then on the client side I go through the list and retrieve the values of each key (lines 29-32)

However, you must have noticed that the client-side implementation has a nested for loop (line 38). Ted was wondering why it takes a maze of nested loops to display merges (although now it is down to only one nested loop). The reason I have to do it this way is because sometimes mergeData can contain nested lists. Especially when dealing with large merges that tracemonkey usually has.

So there you have it. I’ve replaced the long and ugly string that previously held merge information with a nice and beautiful dictionary.

, ,

No Comments

v1.0 Release - Further Improving Bug Functionality for the OnScroll Patch

In my earlier blog post I outlined that I wanted to further improve bug functionality for the OnScroll patch. Bugs that were only 5 digits long were not being converted into proper links. For example bug 12345 or b#12345 or 12345. I was only covering bugs that were 6 digits in length. The following is my old bug regexps:

1
2
3
re = new RegExp("[bB][uU][gG]\\s\\d\{6\}");
re2 = new RegExp("b=\\d\{6\}");
re3 = new RegExp("\\d\{6\}\\W");

Now have a look at my new bug regexes:

1
2
3
re = new RegExp('[bB][uU][gG]\s\d\{5,}');
re2 = new RegExp('b=\d{5,}');
re3 = new RegExp('\d{5,}\W');

As you can see the regexp implementation hasn’t changed very much. The only change was that ‘6′ became ‘5,’ and now the 5 digit bugs are converted into bug links properly. Basically ‘5,’ specifies that anything >=5 digits should be converted into bug links. Furthermore, 7 digit bugs will start to appear eventually (if they haven’t already, not sure). This implementation automatically takes care of that situation.

Another thing you will notice in my new implementation is that the regexps no longer use double quotes. Instead, I’m now using single quotes which means that I don’t need to use the double backward slash as an escape character.

, ,

1 Comment

v1.0 Release - Getting the Localize Dates Link Working with the New Entries Loaded On Scroll

One of my goals for this release was getting the localize dates link working with the new entries loaded on scroll. This wasn’t happening beforehand. Only the entries that were displayed with the initial page load worked with the localize dates link.

I examined why this was happening by looking at how the dates are localized. The following is the code for this functionality:

14
15
16
17
18
19
20
21
// add click handler to the localize dates link
$('#localize').show().click(function () {
   $(this).hide();
   $('.date').each(function (i) {
     $(this).text(new Date($(this).text()).toLocaleString());
   });
   return false;
});

As you can see above when the localize date link is clicked the function goes through each element that has a class called ‘date’ and transforms the text within the element to a local date string via toLocalString(). Now my problem was that none of the new entries loaded on scroll have any elements that contain a class called ‘date’. Thus, none of the dates for the new entries got translated into their local versions.

In order to rectify this situation I decided to make the following change within loadEntries():

77
78
79
80
// user column
$('<td>' + pushData[i].user + '<br /><span class="date">' + pushData[i].formattedDate + '</span></td>')
  .appendTo(row)
  .width(184);

So basically now I put the date text within a a span. This span has a class called ‘date’ thus when the To Local link is clicked it will go through all the elements that have the class date, which now includes the new entries. There we have it, problem fixed!

, ,

1 Comment

v1.0 Release - Fixing the OnScroll Patch to Work with the New Version of hg_templates

In my earlier blog post I outlined my plan to update my OnScroll patch to work with the new version of hg_templates, which was updated recently. Now, it has access to the jquery library, which can be used to do more fun stuff. I was using jquery beforehand too but now I’ve decided to take it a step further and take advantage of jquery even more. I believe by doing this I can significantly reduce the number of lines of code and still retain the same functionality.

The Old Code

For comparisons purposes I’ve posted the old code below:

57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
function loadEntries(iterations) 
{
  for(var t = 0; t < iterations; t++) {
    var loader = document.getElementById("loader");
    var end = start;
    var req = new XMLHttpRequest();
 
    loader.innerHTML = '<img src="{url}static/ajax-loader.gif" align="right" />';
    start = start - 20;
    req.open('GET', '/json-pushes?startID=' + start + '&endID=' + end, true);
    req.onreadystatechange = function() {
      if(req.readyState == 4) {
        if(req.status != 404) {
          var pushData = JSON.parse(req.responseText), counter = 0;
          //var counter = 0;
          for(var i = end; i > start; i--) {
            var row = document.createElement("tr");
            if(counter == 0) {
              row.className = "parity0"; 
              counter = 1;
            } else {
                row.className = "parity1";
                counter = 0;
            }  
            var userCol = document.createElement("td");
            var revCol = createRevTd(pushData[i].individualChangeset);
            var bugLink = createBuglink(pushData[i].desc);
            var authDescCol = document.createElement("td");
 
            userCol.width = "184px";
            userCol.innerHTML += pushData[i].user + '<br />' + pushData[i].formattedDate;
            authDescCol.innerHTML += '<strong>' + pushData[i].author + ' &mdash ' + bugLink + '</strong>';
            row.appendChild(userCol);
            row.appendChild(revCol);
            row.appendChild(authDescCol);
            loader.innerHTML = "";
            document.getElementById("titlePush").appendChild(row);
 
            //Check whether it is a merge changeset or not
            if(pushData[i].MergeData != []) {
              for(var j = 0; j < pushData[i].mergeData.length; j++) {
                if(pushData[i].mergeData[j] != "") {
                  var mergeStr = pushData[i].mergeData[j];
                  for(var k = 0; k < pushData[i].mergeData[j].length; k++) {
                    if(pushData[i].mergeData[j] instanceof Array) {
                      var actualMergeStr = mergeStr[k].split('|-|');
                      var mergeRev = actualMergeStr[0];
                      var mergeUser = actualMergeStr[1];
                      var mergeDesc = actualMergeStr[2];
                    }
                    else {
                      var actualMergeStr = mergeStr.split('|-|');
                      var mergeRev = actualMergeStr[0];
                      var mergeUser = actualMergeStr[1];
                      var mergeDesc = actualMergeStr[2];
                      k = pushData[i].mergeData[j].length;
                    }
                    if(mergeDesc != pushData[i].desc) {
                      var mergeRow = document.createElement("tr");
                      var mergeUserCol = document.createElement("td");
                      var mergeRevCol = createRevTd(mergeRev);
                      var merge_bugLink = createBuglink(mergeDesc);
                      var mergeAuthDescCol = document.createElement("td");
 
                      mergeRow.className = row.className;
                      mergeUserCol.width = "184px";
                      mergeAuthDescCol.innerHTML += '<strong>' + mergeUser + ' &mdash ' + merge_bugLink + '</strong>';
                      mergeRow.appendChild(mergeUserCol);
                      mergeRow.appendChild(mergeRevCol);
                      mergeRow.appendChild(mergeAuthDescCol);
                      document.getElementById("titlePush").appendChild(mergeRow);
                    }
                  }
                }
              }
            }
          }    
        }  
      }
    }
    req.send(null);
  }
}
 
function createRevTd(rev) 
{
  var td = document.createElement("td");
  td.innerHTML +=
    '<a href=\"/rev/' +
    rev.substring(0, 12) + 
    '\">' + 
    rev.substring(0, 12) +
    '</a>'; 
 
  return td;
}

The New Code

59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
function loadEntries(iterations) {
  for(var t = 0; t < iterations; t++) {
    var end = start;
 
    $('.loader').html('<img src="{url}static/ajax-loader.gif" align="right" />');
    start -= 20;
    $.get('/json-pushes?startID=' + start + '&endID=' + end, function(responseText) {
      var pushData = JSON.parse(responseText), counter = 0;
      for(var i = end; i > start; i--) {
        var row = $(document.createElement('tr'));
        if(counter == 0) {
          row.addClass('parity0');
          counter = 1;
        } 
        else {
          row.addClass('parity1');
          counter = 0;
        }
        // user column
        $('<td>' + pushData[i].user + '<br /><span class="date">' + pushData[i].formattedDate + '</span></td>')
          .appendTo(row)
          .width(184);  
        // rev column
        $('<td>' + createRevTd(pushData[i].individualChangeset) + '</td>').appendTo(row);
        // author and description column
        $('<td>' + '<strong>' + pushData[i].author + ' &mdash ' + createBuglink(pushData[i].desc) + '</strong>' + '</td>').appendTo(row);
        $('.loader').html("");
        $('.titlePush').append(row);
 
        //Check whether it is a merge changeset or not
        if(pushData[i].MergeData != []) {
          for(var j = 0; j < pushData[i].mergeData.length; j++) {
            if(pushData[i].mergeData[j] != "") {
              var mergeStr = pushData[i].mergeData[j];
              for(var k = 0; k < pushData[i].mergeData[j].length; k++) {
                if(pushData[i].mergeData[j] instanceof Array) {
                  var actualMergeStr = mergeStr[k].split('|-|');
                  var mergeRev = actualMergeStr[0];
                  var mergeUser = actualMergeStr[1];
                  var mergeDesc = actualMergeStr[2];
                }
                else {
                  var actualMergeStr = mergeStr.split('|-|');
                  var mergeRev = actualMergeStr[0];
                  var mergeUser = actualMergeStr[1];
                  var mergeDesc = actualMergeStr[2];
                  k = pushData[i].mergeData[j].length;
                }
                if(mergeDesc != pushData[i].desc) {
                  var mergeRow = $(document.createElement('tr')).addClass(row.attr('class'));
                  $(document.createElement('td')).appendTo(mergeRow).width(184);
                  $('<td>' + createRevTd(mergeRev) + '</td>').appendTo(mergeRow);
                  $('<td>' + '<strong>' + mergeUser + ' &mdash ' + createBuglink(mergeDesc) + '</strong>' + '</td>').appendTo(mergeRow);
                  $('.titlePush').append(mergeRow);
                }
              }
            }
          }
        }
      }    
    });
  }
}
 
function createRevTd(rev) {
  return '<a href=\"/rev/' +
    rev.substring(0, 12) + 
    '\">' + 
    rev.substring(0, 12) +
    '</a>';
}

As you can see above is the new code. Lots of changes to look at it. First of all the JavaScript code has been shifted within $document.ready(). The major changes have occurred in loadEntries(). The jquery library makes it extremely simple to retrieve an object by its class or id name. Instead of having to use document.getElementbyId() I can use $(’<class_name>’). I’ve used this way extensively throughout my new implementation. Furthermore, I don’t need to use document.createElement() anymore either. I can just use the  $(’<html>’) syntax to create an element (e.g. line 84). Another great little change is how jquery creates an xmlHttpRequest(). The syntax is so much simpler. Only a simple $.get(url, function(responseText) {} ) is needed (line 65). Another little quirk that makes jquery awesome can be seen on lines 78-80. I can create the new element, append it to another element and change its attributes all in one go.

One important thing to note, however, is that some things didn’t change much at all. Such as the string manipulation functionality found at lines 88-107. All in all the end result of all these changes is that I have manged to reduce the total number of lines by 19. On top of that I have also added 2 extra pieces of functionality (I’ll be discussing them in future posts) without having to add any additional lines of code. A very good result, in my opinion.

, ,

No Comments

v0.8 Release - Fix for the Merge Changeset Problem

In my previous blog post I had outlined the problem with merge changesets for my patch (bug 459727). I’ve come up with a fix for the issue which seems to work. As I had mentioned previously I would move my code to a function so that I can employ recursion to access the merges within the merges. The following is the what the code looks like currently:

def getMergeData(ctx, mergeData):
  for cs in ctx.parents():
    mergeData.append(hex(cs.node()) + '|-|' + clean(person(cs.user())) + '|-|' + clean(cs.description()))
    if len(cs.parents()) &gt; 1:
      getMergeData(cs, mergeData)
 
def pushes_worker(query, repo):
    """Given a PushlogQuery, return a data structure mapping push IDs
    to a map of data about the push."""
    pushes = {}
    for id, user, date, node in query.entries:
        mergeData = []
        ctx = repo.changectx(node)
        if len(ctx.parents()) &gt; 1:
          getMergeData(ctx, mergeData)
        if id in pushes:
            # we get the pushes in reverse order
            pushes[id]['changesets'].insert(0, node)
            pushes[id]['mergeData'].insert(0, mergeData)
        else:
            pushes[id] = {'user': user,
                          'date': date,
                          'changesets': [node],
                          'formattedDate': util.datestr(localdate(date)),
                          'individualChangeset': hex(ctx.node()),
                          'author': clean(person(ctx.user())),
                          'desc': clean(ctx.description()),
                          'mergeData': mergeData,
                          'id': id
                          }
    return pushes

So getMergeData() is where all the functionality now resides. The problem of merges within merges is taken care of by the recursion functionality in getMergeData(). The function takes in ctx and mergeData after checking that there are parents available. Then a for loop is performed to add all the pushes to mergeData. Next a check is performed to see whether the current push is a merge as well and if it is then getMergeData() is called again to add data to the variable mergeData. This change now adds in the missing pushes from some of the larger merge changesets. Beforehand, with the previous code many of the pushes were missing but now with this new functionality they are being shown.

I’ll be putting up a new patch for hgpoller very soon. Unfortunately, ted can’t review my patch until the pushlog is fixed to work with hg 1.1, which will hopefully happen before the end of the semester (April 24th).

, ,

No Comments

v0.8 Release - Problems with Displaying Merge Changesets

In my goals blog post I outlined that I wanted to fix this problem with displaying merge changesets. This is the only major problem remaining with my patch for bug 459727. I had talked to about in a previous post and at that time I thought that the problem was not with my code. However, it seems that I was mistaken. I had a little chat with ted and he seems to think that my problem is that I have merges within merges. This is why the right amount of pushes are not showing up.

I’ve been thinking how I can I solve this problem. I’m gonna have to alter my server side code. Currently it looks like the following:

def pushes_worker(query, repo):
    """Given a PushlogQuery, return a data structure mapping push IDs
    to a map of data about the push."""
    pushes = {}
    for id, user, date, node in query.entries:
        mergeData = []
        ctx = repo.changectx(node)
        if len(ctx.parents()) > 1:
          for cs in ctx.parents():
            mergeData.append(hex(cs.node()) + '|-|' + clean(person(cs.user())) + '|-|' + clean(cs.description()))
        if id in pushes:
            # we get the pushes in reverse order
            pushes[id]['changesets'].insert(0, node)
            pushes[id]['mergeData'].insert(0, mergeData)
        else:
            pushes[id] = {'user': user,
                          'date': date,
                          'changesets': [node],
                          'formattedDate': util.datestr(localdate(date)),
                          'individualChangeset': hex(ctx.node()),
                          'author': clean(person(ctx.user())),
                          'desc': clean(ctx.description()),
                          'mergeData': mergeData,
                          'id': id
                          }
    return pushes

I’ll probably be moving this functionality into a function and then hopefully use some recursion to access the merges within the merges. I don’t think I will need to change any of my server side code though, which is a relief.

I think I have it in mind how I want to implement this. I don’t forsee any problems at the moment. Watch out for another post detailing my fix for this issue.

,

No Comments

v0.8 Release - Improving the Coding Style for the OnScroll Patch

This post is concerning the patch for bug 459727. For this release I decided that I wanted to make some adjustments to my coding style to make it more consistent with the rest of the code. What do I mean by coding style? Well, things like variable names, indentation, new lines, commenting etc. The goal here is to improve code readability.

Braces

OK, first lets examine one of the small functions I’ve implemented in the hg_templates/pushlog.tmpl file:

function fillPage() {
  start = ($("div").attr("id")) - 10;
  var vHeight = 0;
  if (document.documentElement) {
      vHeight = document.documentElement.clientHeight;
  } else {
      vHeight = document.body.clientHeight
    }
 
  if (document.body.offsetHeight &lt; vHeight) {
    for(var count = 0, offSetHeight = document.body.offsetHeight;
        offSetHeight &lt; vHeight;
        count++, offSetHeight += 180);
 
    if(count &gt; 0)
      loadEntries(count);
  }
}

Now, lets look at one of the functions that was already present beforehand:

function localizeDates()
{
  var dates = document.getElementsByClassName("date");
  for (var i=0; i<dates.length; i++) {
    dates[i].textContent = new Date(dates[i].textContent).toLocaleString();
  }
  document.getElementById("localize").style.display = 'none';
  return false;
}

As you can see there’s a bit of a difference in the way the braces are handled. In my function I always keep the braces on the same line but in the other function the brace for the function name is on a new line while the rest of the braces are on the same line. A very peculiar coding style, if you ask me. Then again this may be some sort of coding convention but I haven’t really seen it implemented anywhere else before. I didn’t even notice this subtle difference until recently, which is why I didn’t make this change earlier. So, I’m going to be consistent and use the already implemented way of handling braces.

Variable Names

There doesn’t seem to be any type of convention being used, as far as I can tell, regarding variable names. The key is to just use short but logical variable names to increase readability. I’ve decided to change some of my variable names being used:

Old Name New Name Reasoning
trScroll row This is the actual row that gets added to the end of the table so it’s clearer to give it the name row. Nice and simple
pushCheckins req Since it is an xmlHttpRequest object I’ll give it the name req, pretty conventional
tdScrollUser userCol Since the variable is a column that will hold user information it will be appropriately called userCol
tdScrollChangeset revCol Since the variable is a column that will hold changeset information it will be appropriately called revCol
tdScrollAuthorDesc authDescCol This column will contain both the author name and the description and thus an appropriate name is chosen
trScrollMerge mergeRow Since it is a row that contains a merge push the new name seems more logical
tdScroll_MergeUser mergeUserCol Same reasoning as the userCol change above
tdScroll_MergeC mergeRevCol Same reasoning as the revCol change above
tdScroll_MergeAuthorDesc mergeAuthDescCol Same reasoning as the authDescCol change above
createRevLink(str) createRevTd(rev) This change was recommended by Justin Wood (Callek). The reasoning is that the attribute should indicate what to pass in, and the function name should indicate what it returns
mergeC mergeRev mergeC doesn’t make much sense. What does C stand for? Using mergeRev is definitely more logical


Spacing

One thing that I’ve always found a bit confusing is why or how to space out your code within a function or a method. Some people prefer to put in no spaces while others fall in love with them. Is there some sort of convention for this? Maybe there is but I’m not aware of any. Currently, with this patch’s code I’ve gone space-happy. There’s too much spacing in some places and it doesn’t look good. I’m going to be reducing the spacing wherever possible.

Variable Declarations

One thing I noticed was that I wasn’t putting all the variable declarations together, one after another. That made my code look very untidy. I’ve rectified that situation and now all the declarations at the very start of a code block.

Most of the changes have occurred with loadEntries(). The following is what my code now looks like:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
function loadEntries(iterations) 
{
  for(var t = 0; t < iterations; t++) {
    var loader = document.getElementById("loader");
    var end = start;
    var req = new XMLHttpRequest();
 
    loader.innerHTML = '<img src="{url}static/ajax-loader.gif" align="right" />';
    start = start - 20;
    req.open('GET', '/json-pushes?startID=' + start + '&endID=' + end, true);
    req.onreadystatechange = function() {
      if(req.readyState == 4) {
        if(req.status != 404) {
          var pushData = JSON.parse(req.responseText), counter = 0;
          //var counter = 0;
          for(var i = end; i > start; i--) {
            var row = document.createElement("tr");
            if(counter == 0) {
              row.className = "parity0"; 
              counter = 1;
            } else {
                row.className = "parity1";
                counter = 0;
            }  
            var userCol = document.createElement("td");
            var revCol = createRevTd(pushData[i].individualChangeset);
            var bugLink = createBuglink(pushData[i].desc);
            var authDescCol = document.createElement("td");
 
            userCol.width = "184px";
            userCol.innerHTML += pushData[i].user + '<br />' + pushData[i].formattedDate;
            authDescCol.innerHTML += '<strong>' + pushData[i].author + ' &mdash ' + bugLink + '</strong>';
            row.appendChild(userCol);
            row.appendChild(revCol);
            row.appendChild(authDescCol);
            loader.innerHTML = "";
            document.getElementById("titlePush").appendChild(row);
 
            //Check whether it is a merge changeset or not
            if(pushData[i].MergeData != []) {
              for(var j = 0; j < pushData[i].mergeData.length; j++) {
                if(pushData[i].mergeData[j] != "") {
                  var mergeStr = pushData[i].mergeData[j];
                  for(var k = 0; k < pushData[i].mergeData[j].length; k++) {
                    if(pushData[i].mergeData[j] instanceof Array) {
                      var actualMergeStr = mergeStr[k].split('|-|');
                      var mergeRev = actualMergeStr[0];
                      var mergeUser = actualMergeStr[1];
                      var mergeDesc = actualMergeStr[2];
                    }
                    else {
                      var actualMergeStr = mergeStr.split('|-|');
                      var mergeRev = actualMergeStr[0];
                      var mergeUser = actualMergeStr[1];
                      var mergeDesc = actualMergeStr[2];
                      k = pushData[i].mergeData[j].length;
                    }
                    if(mergeDesc != pushData[i].desc) {
                      var mergeRow = document.createElement("tr");
                      var mergeUserCol = document.createElement("td");
                      var mergeRevCol = createRevTd(mergeRev);
                      var merge_bugLink = createBuglink(mergeDesc);
                      var mergeAuthDescCol = document.createElement("td");
 
                      mergeRow.className = row.className;
                      mergeUserCol.width = "184px";
                      mergeAuthDescCol.innerHTML += '<strong>' + mergeUser + ' &mdash ' + merge_bugLink + '</strong>';
                      mergeRow.appendChild(mergeUserCol);
                      mergeRow.appendChild(mergeRevCol);
                      mergeRow.appendChild(mergeAuthDescCol);
                      document.getElementById("titlePush").appendChild(mergeRow);
                    }
                  }
                }
              }
            }
          }    
        }  
      }
    }
    req.send(null);
  }
}

,

4 Comments

v0.8 Release Goals

Here we are, another release in the making. I’m going to change things up slightly this time around. I’m gonna work on a new bug just to mix things up. Admittedly the bug isn’t something very big but I just wanted to work on something new. Along with taking up a new feature I’m going to be improving my onScroll bug even further. I wanted to have it done and over with but there is still room for improvement. I’ve decided to look at this situation from another perspective. I mean this is what incremental development is all about, making small but significant changes and putting usable code out there. I think that is the goal of this entire course, releasing constantly and making improvements as you go. I want to stay true to that philosophy. Anyway, the following are my goals:

  • Implement an intuitive UI for the changeset query. Apparently many people don’t know that this query exists because currently there is no UI for it.
  • Fixing the repeating bug with merge changesets. I know I had said that it wasn’t a bug with my code but it seems I was wrong. I need to re-examine some of my server side code. Ted has given me some feed back to get me on the right track.
  • Coding style changes are needed with the onScroll patch to make it consistent with the rest of the code. I’ll be looking to change the variable names too. Currently the code isn’t very clean and readable it seems. Also, I’ll be looking to reduce even more code (and retain functionality), if possible.

, ,

No Comments