Last Comment Bug 459727 - pushloghtml should show more than 10 entries at a time
: pushloghtml should show more than 10 entries at a time
Status: REOPENED
[kanban:engops:https://mozilla.kanban...
:
Product: Developer Services
Classification: Other
Component: Mercurial: Pushlog (show other bugs)
: other
: All All
: -- enhancement
: ---
Assigned To: Sid Kalra
: Hal Wine [:hwine] (use NI)
Mentors:
http://hg.mozilla.org/mozilla-central...
Depends on: 1117021
Blocks:
  Show dependency treegraph
 
Reported: 2008-10-13 14:23 PDT by Sid Kalra
Modified: 2015-11-04 15:16 PST (History)
15 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch for pushlog to load more entries OnScroll (5.36 KB, patch)
2008-11-16 12:35 PST, Sid Kalra
no flags Details | Diff | Splinter Review
Patch for templates to load more entries OnScroll (62.69 KB, patch)
2008-11-16 12:36 PST, Sid Kalra
no flags Details | Diff | Splinter Review
Loader GIF for templates (2.55 KB, image/gif)
2008-11-16 12:39 PST, Sid Kalra
no flags Details
Patch for hgpoller to load more entries onScroll (5.15 KB, patch)
2008-12-03 16:58 PST, Sid Kalra
no flags Details | Diff | Splinter Review
Patch for hg_templates to load more entries onScroll (62.84 KB, patch)
2008-12-03 17:00 PST, Sid Kalra
no flags Details | Diff | Splinter Review
Patch for hgpoller to load more entries onScroll (3.87 KB, patch)
2008-12-07 09:42 PST, Sid Kalra
no flags Details | Diff | Splinter Review
Patch for hg_templates to load more entries onScroll (63.09 KB, patch)
2008-12-07 09:44 PST, Sid Kalra
no flags Details | Diff | Splinter Review
Patch for hg_templates to load more entries onScroll (82.94 KB, patch)
2009-01-23 15:58 PST, Sid Kalra
no flags Details | Diff | Splinter Review
Patch for hgpoller to load more entries onScroll (3.73 KB, patch)
2009-02-21 22:08 PST, Sid Kalra
no flags Details | Diff | Splinter Review
Patch for hg_templates to load more entries onScroll (80.96 KB, patch)
2009-02-21 22:10 PST, Sid Kalra
no flags Details | Diff | Splinter Review
Patch for hgpoller to load more entries onScroll (3.90 KB, patch)
2009-03-15 13:18 PDT, Sid Kalra
no flags Details | Diff | Splinter Review
Patch for hg_templates to load more entries onScroll (81.11 KB, patch)
2009-03-15 13:19 PDT, Sid Kalra
bugspam.Callek: review-
Details | Diff | Splinter Review
Patch for hgpoller to load more entries onScroll (4.03 KB, patch)
2009-03-29 12:26 PDT, Sid Kalra
no flags Details | Diff | Splinter Review
Patch for hg_templates to load more entries onScroll (80.68 KB, patch)
2009-03-29 12:28 PDT, Sid Kalra
no flags Details | Diff | Splinter Review
updated hgpoller scroll patch (4.56 KB, patch)
2009-04-23 07:03 PDT, Ted Mielczarek [:ted.mielczarek]
ted: review-
Details | Diff | Splinter Review
updated hg_templates scroll patch (25.20 KB, patch)
2009-04-23 07:04 PDT, Ted Mielczarek [:ted.mielczarek]
ted: review-
Details | Diff | Splinter Review
Patch for hgpoller to load more entries onScroll (6.27 KB, patch)
2009-04-24 08:14 PDT, Sid Kalra
no flags Details | Diff | Splinter Review
Patch for hg_templates to load more entries onScroll (24.88 KB, patch)
2009-04-24 08:16 PDT, Sid Kalra
no flags Details | Diff | Splinter Review

Description Sid Kalra 2008-10-13 14:23:07 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.0.3) Gecko/2008092417 Firefox/3.0.3 (.NET CLR 3.5.30729)
Build Identifier: 

Currently, http://hg.mozilla.org/mozilla-central/pushloghtml/ only shows 10 entries at a time. To see the next set of entries one has to click on the the next link. 

Pushloghtml should scroll forever so that all the push log entries are on the same page. This way the user won't be forced to click on the on the next link. This will also give the user the option of using the browser's ctrl-f search feature.

Reproducible: Always

Steps to Reproduce:
1. Navigate to http://hg.mozilla.org/mozilla-central/pushloghtml/
Actual Results:  
Pushloghtml displayed 10 entries from 1 week ago to now with links available to view the next 10 entries

Expected Results:  
Pushloghtml should display all the entries from 1 week ago to now on the same page. It should scroll forever
Comment 1 Ted Mielczarek [:ted.mielczarek] 2008-10-13 15:36:25 PDT
Just for clarification, it shouldn't just dump every single checkin on one page, but as you scroll down it should progressively load more.
Comment 2 Sid Kalra 2008-10-14 09:22:10 PDT
Agreed, Ted. Thanks for clearing that up. Just dumping the data onto the one page increases the loading time significantly. Loading more data on scrolling is more user friendly for sure.
Comment 3 Justin Wood (:Callek) 2008-10-15 08:39:39 PDT
My only comment is that if you do that, we "should" [must imo] move the footer, and its related RSS link to "above the fold" on the pushlog... because otherwise you'd have to load _full_ history of pushlog to get to it reasonably.
Comment 4 Jason Orendorff [:jorendorff] 2008-10-15 08:47:06 PDT
Right.  There could be a large div in the middle that scrolls, with the footer always shown below that.
Comment 5 Sid Kalra 2008-11-16 12:35:05 PST
Created attachment 348458 [details] [diff] [review]
Patch for pushlog to load more entries OnScroll
Comment 6 Sid Kalra 2008-11-16 12:36:24 PST
Created attachment 348459 [details] [diff] [review]
Patch for templates to load more entries OnScroll
Comment 7 Sid Kalra 2008-11-16 12:39:36 PST
Created attachment 348460 [details]
Loader GIF for templates

Add this to the hg_templates\static directory after applying the pushlog and template patches. This is the GIF that will be shown every time the user scrolls down to indicate that more entries are being loaded
Comment 8 Jason Orendorff [:jorendorff] 2008-12-01 15:36:32 PST
Reassigning this review to Ted.  Offhand it looks like you're returning "max" a lot of times instead of just once; also, to calculate the max you should use

  SELECT COUNT(*) FROM ...

instead of asking for all the rows and then counting them.
Comment 9 Jason Orendorff [:jorendorff] 2008-12-01 16:31:43 PST
Comment on attachment 348459 [details] [diff] [review]
Patch for templates to load more entries OnScroll

Redirecting this one to ted like a big coward.

Offhand:

1.  This loads more results when the user scrolls.  What if the initial results don't fill up the window?

2.  You call getMaxEntries, then use 'start' once the user scrolls.  But the user might scroll before the first result comes back, while start is still zero -- a race condition.  Do you see a way around this?

3.  `new Function("return " + entries.responseText) ()`
is another way of saying
`eval(entries.responseText)`.

Perhaps what you want here is more like
`JSON.parse(entries.responseText)`.

4.  It would be better to move CSS and formatting to stylesheets where possible.

5.  The JS code isn't consistently indented.

6.  This patch seems to contain some non-ASCII characters.  Bugzilla doesn't render it properly, and if Bugzilla screws it up, so will other software.  Consider using an HTML entity reference, like – or —, instead of an actual Unicode character.

7.  In a few places you use <b> where the server uses <strong> and stuff like that; try to be consistent.
Comment 10 Sid Kalra 2008-12-02 11:17:11 PST
(In reply to comment #9)

Concerning calculating the Max: I can change it to use gettotalpushlogentries() in pushlog-feed.py which does a SELECT COUNT(*) FROM ...

1) I was concerned about that so I changed it so that by default 20 entries are loaded which makes sure that the initial entries are enough to fill up the page.

2) I set the value of start in getMaxEntries() which is called onPageLoad. So the value of start will be set by the time the page has been loaded. The user can't scroll before the page loads so I don't see how start could still be equal to 0 when the user is allowed to scroll down.  

3) I'm wondering what is wrong with using 'new Function("return " + entries.responseText) ()'. I can change it to use what you have recommended but I was just wondering for curiosities sake.

4) OK, I'll try to do that

5) It will be fixed

6) I see one character that bugzilla isn't rendering properly on line 109, I can fix that

7) Sounds good, I'll change all places that use <b> to <strong>
Comment 11 Justin Wood (:Callek) 2008-12-02 12:34:54 PST
(In reply to comment #10)
> (In reply to comment #9)
> 
> Concerning calculating the Max: I can change it to use gettotalpushlogentries()
> in pushlog-feed.py which does a SELECT COUNT(*) FROM ...
> 
> 1) I was concerned about that so I changed it so that by default 20 entries are
> loaded which makes sure that the initial entries are enough to fill up the
> page.

And in high res screens in future, or preople who prefer very small fonts, very wide displays, etc... The "xyz # of entries will always cause a scroll to be needed" is not a good assumption to make imo.

> 2) I set the value of start in getMaxEntries() which is called onPageLoad. So
> the value of start will be set by the time the page has been loaded. The user
> can't scroll before the page loads so I don't see how start could still be
> equal to 0 when the user is allowed to scroll down.  

You can definately scroll before a page has finished loading... you scroll WHILE it is loading. (onload waits for load to finish)

> 3) I'm wondering what is wrong with using 'new Function("return " +
> entries.responseText) ()'. I can change it to use what you have recommended but
> I was just wondering for curiosities sake.

Makes a potential for XSS attacks, I think (I'm no expert here)
Comment 12 Sid Kalra 2008-12-02 12:54:44 PST
(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > 
> > Concerning calculating the Max: I can change it to use gettotalpushlogentries()
> > in pushlog-feed.py which does a SELECT COUNT(*) FROM ...
> > 
> > 1) I was concerned about that so I changed it so that by default 20 entries are
> > loaded which makes sure that the initial entries are enough to fill up the
> > page.
> 
> And in high res screens in future, or preople who prefer very small fonts, very
> wide displays, etc... The "xyz # of entries will always cause a scroll to be
> needed" is not a good assumption to make imo.
> 

Good point, Justin. I totally forgot about larger screens or people using smaller fonts. I need some way for the scroll bar to always be present because new entries will only load if there is a scroll bar which can be used to trigger the OnScroll event. Any suggestions as to how I can do that or is there another way I could use?

> > 2) I set the value of start in getMaxEntries() which is called onPageLoad. So
> > the value of start will be set by the time the page has been loaded. The user
> > can't scroll before the page loads so I don't see how start could still be
> > equal to 0 when the user is allowed to scroll down.  
> 
> You can definately scroll before a page has finished loading... you scroll
> WHILE it is loading. (onload waits for load to finish)
> 

I see your point now. I'll see if I can call getMaxEntries() within renderMorePushLogResults() which should fix the race condition

> > 3) I'm wondering what is wrong with using 'new Function("return " +
> > entries.responseText) ()'. I can change it to use what you have recommended but
> > I was just wondering for curiosities sake.
> 
> Makes a potential for XSS attacks, I think (I'm no expert here)

Anybody else have any info as to how this method could create vulnerabilities for an XSS attack?
Comment 13 Sid Kalra 2008-12-03 16:58:01 PST
Created attachment 351285 [details] [diff] [review]
Patch for hgpoller to load more entries onScroll

View my blog post for the details...

http://blog.sidkalra.com/2008/12/v03-release-fixing-the-problems-with-my-patch-for-bug-459727/
Comment 14 Sid Kalra 2008-12-03 17:00:19 PST
Created attachment 351287 [details] [diff] [review]
Patch for hg_templates to load more entries onScroll

View my blog post for the details...

http://blog.sidkalra.com/2008/12/v03-release-fixing-the-problems-with-my-patch-for-bug-459727/
Comment 15 Justin Wood (:Callek) 2008-12-03 17:19:15 PST
Comment on attachment 351287 [details] [diff] [review]
Patch for hg_templates to load more entries onScroll

>diff -r 2279eae99af9 gitweb_mozilla/pushlog.tmpl
>--- a/gitweb_mozilla/pushlog.tmpl	Fri Oct 03 11:54:50 2008 -0700
>+++ b/gitweb_mozilla/pushlog.tmpl	Wed Dec 03 19:44:38 2008 -0500
>+html { 
>+  overflow: scroll; overflow-x: auto; 
>+}

I'm not so sure this has the effect you think it does...

Try it on a manually generated page of only 2 entries for example.

The scrollbar will appear but you still will be unable to scroll (since page content is smaller than content area on the page).

I'm not sure on the exact JS involved, but try getting the computed height of the page (using 10-entries [or 20]) and if the page does not need to scroll then auto-load the next set and append them, until the computed height of the whole page is larger than the viewport height.

That should ensure you'll be able to scroll.
Comment 16 Sid Kalra 2008-12-03 18:48:34 PST
(In reply to comment #15)
> (From update of attachment 351287 [details] [diff] [review])
> >diff -r 2279eae99af9 gitweb_mozilla/pushlog.tmpl
> >--- a/gitweb_mozilla/pushlog.tmpl	Fri Oct 03 11:54:50 2008 -0700
> >+++ b/gitweb_mozilla/pushlog.tmpl	Wed Dec 03 19:44:38 2008 -0500
> >+html { 
> >+  overflow: scroll; overflow-x: auto; 
> >+}
> 
> I'm not so sure this has the effect you think it does...
> 
> Try it on a manually generated page of only 2 entries for example.
> 
> The scrollbar will appear but you still will be unable to scroll (since page
> content is smaller than content area on the page).
> 
> I'm not sure on the exact JS involved, but try getting the computed height of
> the page (using 10-entries [or 20]) and if the page does not need to scroll
> then auto-load the next set and append them, until the computed height of the
> whole page is larger than the viewport height.
> 
> That should ensure you'll be able to scroll.

Yeah, I guess you are right. I thought it might have worked but the scroll event doesn't get called. So now I'm trying something along the lines that you have recommended. It works; more entries are loaded according to the users window height but I'm getting an unresponsive script error that I can't seem to get rid of. 

Have a look at my blog post: http://blog.sidkalra.com/2008/12/v03-release-further-problems-with-loading-more-items-bug-459727/
Comment 17 Justin Wood (:Callek) 2008-12-04 07:28:19 PST
Comment on attachment 351287 [details] [diff] [review]
Patch for hg_templates to load more entries onScroll

>+    start = 0;
>+    getMaxEntries();
>+ 
>+    var docHeight = document.body.clientHeight;
>+    while(docHeight <= window.innerHeight) {
>+      if(start > 0) {

Note that you set start = 0 here, then never break out to the main thread unless we have a height that is correct (skipping any action if start <= 0)

>+function getMaxEntries() {
>+  var entries = new XMLHttpRequest();
>+  var max = 0;
>+  entries.open('GET', '/json-pushes?startID=0&endID=5', true);
>+  entries.onreadystatechange = function() {
>+  if(entries.readyState == 4)  {
>+    if(entries.status != 404) {
>+      var entryData = new Function("return " + entries.responseText) ();
>+      max = entryData[1].max;
>+      
>+      start = max - 20;
>+    }
>+  }
>+  else 
>+    return 0;
>+ } 
>+ entries.send(null);
>+}

Note that we never get to set start here because onradystatechange is called from main UI thread, whenever the state changes on the Request Object... 

My suggestion use a "setTimeOut" to a function that can iterate and add entries if needed (initial setTimeout from the onreadystatechange function) and the additional ones only if needed.
Comment 18 Sid Kalra 2008-12-07 09:42:55 PST
Created attachment 351800 [details] [diff] [review]
Patch for hgpoller to load more entries onScroll

Improved the bug link functionality for entries loaded OnScroll
Comment 19 Sid Kalra 2008-12-07 09:44:31 PST
Created attachment 351801 [details] [diff] [review]
Patch for hg_templates to load more entries onScroll

Improved the bug link functionality for entries loaded OnScroll
Comment 20 Sid Kalra 2009-01-23 15:58:51 PST
Created attachment 358506 [details] [diff] [review]
Patch for hg_templates to load more entries onScroll

For detailed information about the changes made in this patch please have a look at http://blog.sidkalra.com/2009/01/v04-release-complete/
Comment 21 Ted Mielczarek [:ted.mielczarek] 2009-02-06 10:40:36 PST
I'm sorry, I've let these go for so long that they've bitrotted. :-( Could you pull the latest updates from both repositories and update your code? The pushlog code has been refactored quite a bit, so it may take some work. I promise a speedy review if you give me updated patches, ok?
Comment 22 Sid Kalra 2009-02-21 22:08:47 PST
Created attachment 363537 [details] [diff] [review]
Patch for hgpoller to load more entries onScroll

Refactored the patch to fix the bitrotting issue
Comment 23 Sid Kalra 2009-02-21 22:10:18 PST
Created attachment 363538 [details] [diff] [review]
Patch for hg_templates to load more entries onScroll 

Refactored the patch to fix the bitrotting issue
Comment 24 Sid Kalra 2009-03-15 13:18:14 PDT
Created attachment 367497 [details] [diff] [review]
Patch for hgpoller to load more entries onScroll   

Made a variety of improvements to the patch. Please see the following for details: http://blog.sidkalra.com/2009/03/v07-release-complete/
Comment 25 Sid Kalra 2009-03-15 13:19:42 PDT
Created attachment 367498 [details] [diff] [review]
Patch for hg_templates to load more entries onScroll

Made a variety of improvements to the patch. Please see the following for
details: http://blog.sidkalra.com/2009/03/v07-release-complete/
Comment 26 Justin Wood (:Callek) 2009-03-15 17:16:54 PDT
Comment on attachment 367498 [details] [diff] [review]
Patch for hg_templates to load more entries onScroll

I'm not technically a peer, so take my r- with that grain of salt, but:

>diff -r 244027b2b6dd gitweb_mozilla/pushlog.tmpl
>--- a/gitweb_mozilla/pushlog.tmpl	Mon Feb 09 01:27:51 2009 -0600
>+++ b/gitweb_mozilla/pushlog.tmpl	Sun Mar 15 15:16:01 2009 -0400
>+function fillPage() {
>+  start = ($("div").attr("id")) - 10;

JQuery $("div") will return a LIST of ALL div elements on the page... (see: http://docs.jquery.com/Selectors/element#element)

>-<div class="page_header">
>+<div id="#max#" class="page_header">

Better, and much faster would be using the id attribute to query the dib you want, and an alternate attribute for the max, something like:

<div id="page_header" class="page_header" maxentries="#max#">

and then:

start = ($("#page_header").attr("maxentries")) - 10;

see-also: http://docs.jquery.com/Selectors/id#id
Comment 27 Sid Kalra 2009-03-29 12:26:08 PDT
Created attachment 369914 [details] [diff] [review]
Patch for hgpoller to load more entries onScroll
Comment 28 Sid Kalra 2009-03-29 12:28:24 PDT
Created attachment 369915 [details] [diff] [review]
Patch for hg_templates to load more entries onScroll

Implemented the change that Justin outlined in comment #26

For more details on the other changes: http://blog.sidkalra.com/2009/03/v08-release-improving-the-coding-style-for-the-onscroll-patch/
Comment 29 Ted Mielczarek [:ted.mielczarek] 2009-04-23 06:51:37 PDT
Ok, this is pretty cool in practice. I have some comments here, and I'd like Pike to look at the hgpoller changes because you're changing the JSON output.
Comment 30 Ted Mielczarek [:ted.mielczarek] 2009-04-23 07:03:42 PDT
Created attachment 374264 [details] [diff] [review]
updated hgpoller scroll patch

Just updated to apply to tip.
Comment 31 Ted Mielczarek [:ted.mielczarek] 2009-04-23 07:04:50 PDT
Created attachment 374265 [details] [diff] [review]
updated hg_templates scroll patch

Updated to apply to tip.
Comment 32 Ted Mielczarek [:ted.mielczarek] 2009-04-23 07:16:42 PDT
Comment on attachment 374264 [details] [diff] [review]
updated hgpoller scroll patch

+def getMergeData(ctx, mergeData):
+  for cs in ctx.parents():
+    mergeData.append(hex(cs.node()) + '|-|' + clean(person(cs.user())) + '|-|' + clean(cs.description()))

I'm not sure what this is really doing, but concatenating these with a random separator seems like a bad idea. You're returning this as JSON, why not just use a data structure here? Something like:
mergeData.append({'node':hex(cs.node()), 'user':clean(person(cs.user())), 'desc':clean(cs.description)})
?

             pushes[id] = {'user': user,
                           'date': date,
-                          'changesets': [node]
+                          'changesets': [node],
+                          'formattedDate': util.datestr(localdate(date)),
+                          'individualChangeset': hex(ctx.node()),
+                          'author': clean(person(ctx.user())),
+                          'desc': clean(ctx.description()),
+                          'mergeData': mergeData,
+                          'id': id
                           }

This seems wrong. Things like 'author' and 'desc', and 'individualChangeset' are associated with the changeset, not with the push. (One user can push changesets from multiple authors...) Also, can you not do decent date formatting in JS?

Finally, hgpoller has unittests (in runtests.py), and these changes make a bunch of them fail, since you changed the JSON output without changing the tests. You'll need to fix that before this can land. You'll notice I rolled a change to runtests.py into this patch so that you can do:
"python runtests.py /path/to/hg_templates". Previously runtests.py would clone a fresh hg_templates repo from hg.mozilla.org, which isn't great if your patch has template changes it needs.
Comment 33 Ted Mielczarek [:ted.mielczarek] 2009-04-23 07:29:22 PDT
Comment on attachment 374265 [details] [diff] [review]
updated hg_templates scroll patch

First of all, all this JS needs to move to an external script file. This is just way too much JS to have inline in a <script> tag.

+  start = ($("#page_header").attr("maxentries")) - 10;

Why stick this in an attribute on page_header? Why not (in the template) just do:
<script>
var maxentries = #max#;
</script>

Also, this doesn't seem to handle start going below 0, I was able to hit that in my brief testing.

+    for(var count = 0, offSetHeight = document.body.offsetHeight; 
+        offSetHeight < vHeight; 
+        count++, offSetHeight += 180);

I don't like for loops with empty bodies like this, they're confusing to read. Just put the offsetHeight += 180 in the loop body? (Also, where does 180 come from?)

+    var req = new XMLHttpRequest();

jQuery has some AJAX helpers, they might simplify your code a little bit:
http://docs.jquery.com/Ajax
(also your code as written won't work on IE, for example)

+          //var counter = 0;
+            if(counter == 0) {

You commented that variable out, but you're still using it. Also, if you want a parity counter here, you could just increment an index, and do:
row.className = "parity" + (counter % 2);

+            //Check whether it is a merge changeset or not
+            if(pushData[i].MergeData != []) {

This whole block of code seems really complicated, and very deeply nested. Maybe you could simplify it somehow?

+  td.innerHTML +=
+    '<a href=\"/rev/' +

You don't need to escape double quotes in a single-quoted string, FYI.

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

Do these match the regexes in buglink.py?

I'm a bit worried about duplicating all the logic from the templates into JS here, seems like it's bound to get out of sync and reveal subtle bugs. Also, how will this code handle queries? I don't think people will want the pushlog page loading extra changesets on them if they're trying to track down a regression range with a start and end changeset (or date).

Finally, the additional info you're adding to the JSON output would be useful for other purposes, so maybe we can factor that out into a separate patch to make these patches a bit smaller. mstange filed bug 489793 on the JSON output.
Comment 34 Axel Hecht [:Pike] 2009-04-23 07:42:15 PDT
In addition to what ted said, I'm hitting the json api rather hard, so I'd prefer that to stay simple, at least by default. I'd rather see something like "show=detail_wanted" or so. Seems like bug 489793 is the better spot to make that happen, though.
Comment 35 Dirkjan Ochtman (:djc) 2009-04-23 08:02:57 PDT
Also be aware that the ## syntax will be deprecated in 1.3 and probably cease to function in 1.4. You should try to use {} everywhere.
Comment 36 Dirkjan Ochtman (:djc) 2009-04-23 08:03:19 PDT
(Sorry, to be clear, that's about the hg template syntax.)
Comment 37 Dirkjan Ochtman (:djc) 2009-04-23 08:05:27 PDT
Oh, and hex(ctx.node()) might be better written as ctx.hex().
Comment 38 Sid Kalra 2009-04-23 14:39:34 PDT
(In reply to comment #32)
> (From update of attachment 374264 [details] [diff] [review])
> +def getMergeData(ctx, mergeData):
> +  for cs in ctx.parents():
> +    mergeData.append(hex(cs.node()) + '|-|' + clean(person(cs.user())) + '|-|'
> + clean(cs.description()))
> 
> I'm not sure what this is really doing, but concatenating these with a random
> separator seems like a bad idea. You're returning this as JSON, why not just
> use a data structure here? Something like:
> mergeData.append({'node':hex(cs.node()), 'user':clean(person(cs.user())),
> 'desc':clean(cs.description)})
> ?

Thanks for the review first of all. As for using a data structure I'll use what you have recommended. You are right, what I'm using is not very pretty. 

> 
>              pushes[id] = {'user': user,
>                            'date': date,
> -                          'changesets': [node]
> +                          'changesets': [node],
> +                          'formattedDate': util.datestr(localdate(date)),
> +                          'individualChangeset': hex(ctx.node()),
> +                          'author': clean(person(ctx.user())),
> +                          'desc': clean(ctx.description()),
> +                          'mergeData': mergeData,
> +                          'id': id
>                            }
> 
> This seems wrong. Things like 'author' and 'desc', and 'individualChangeset'
> are associated with the changeset, not with the push. (One user can push
> changesets from multiple authors...) Also, can you not do decent date
> formatting in JS?

hmm, I foundthat  'author' and 'desc' are returning the right data. So what would you recommend I use to get the data for a push instead of what I'm using currently?

Also, yeah I think I should be able to format the data in JS if that is what you prefer. I haven't tried it out though so I'll see what I can do there.

> 
> Finally, hgpoller has unittests (in runtests.py), and these changes make a
> bunch of them fail, since you changed the JSON output without changing the
> tests. You'll need to fix that before this can land. You'll notice I rolled a
> change to runtests.py into this patch so that you can do:
> "python runtests.py /path/to/hg_templates". Previously runtests.py would clone
> a fresh hg_templates repo from hg.mozilla.org, which isn't great if your patch
> has template changes it needs.

Yeah, i'm aware and I had tried running them but all of them completely failed so that gave me the impression that something was wrong with my setup. Plus I read your comment saying that they only worked on your mac
Comment 39 Sid Kalra 2009-04-24 08:14:58 PDT
Created attachment 374467 [details] [diff] [review]
Patch for hgpoller to load more entries onScroll
Comment 40 Sid Kalra 2009-04-24 08:16:15 PDT
Created attachment 374468 [details] [diff] [review]
Patch for hg_templates to load more entries onScroll
Comment 41 Sid Kalra 2009-04-24 08:19:34 PDT
I think I've handled most of the things you guys have outlined with my new patch:

- Now using a dictionary data structure
- Now accessing max by using {}
- No longer using start = ($("#page_header").attr("maxentries")) - 10; to get the value of max
- No longer using an empty body for loop
- Changed a lot of the code to use jquery instead of normal JavaScript
- Changed how I deal with the parity counter
- Move all the client-side JavaScript to an external file
- Improved bug functionality and the double back slashes have been removed

Right now I've managed to reduce the size of the loadEntries() function. It was just too big and messy before. It should be better now.

Also regarding duplicating template functionality in JS i.e. the bug links. How else do you propose I handle this? I can't use buglinks.py on the client-side

Another question regarding the JSON API. So you guys don't want me to be messing with the JSON output in this patch. So, basically then I would have to wait and see what gets changed in bug 489793 before I can continue to work on this patch?

I've outlined all my changes on my blog so please go to the following link for more details: 

http://blog.sidkalra.com/2009/04/v10-release-completev10-release-complete/
Comment 42 Sid Kalra 2009-04-24 08:21:51 PDT
Sorry, provided the wrong link. This is the correct one: http://blog.sidkalra.com/2009/04/v10-release-complete/
Comment 43 Damon Sicore (:damons) 2010-03-01 17:41:21 PST
Ted, looking at your review queue, these patches go back a while.  Do you have review comments that haven't been addressed in order for you to r+?  If not, are you planning to review?  And, if you don't think we should take these, can you r-?  

If there are comments that have yet to be addressed, does it make sense to clear these requests and ask for another request after the comments have been addressed?
Comment 44 Ted Mielczarek [:ted.mielczarek] 2010-10-12 11:11:59 PDT
Comment on attachment 374467 [details] [diff] [review]
Patch for hgpoller to load more entries onScroll

Clearing old review requests. We can revisit these at a later date.
Comment 45 Ted Mielczarek [:ted.mielczarek] 2010-10-12 11:12:07 PDT
Comment on attachment 374468 [details] [diff] [review]
Patch for hg_templates to load more entries onScroll

Clearing old review requests. We can revisit these at a later date.
Comment 46 Gregory Szorc [:gps] 2014-12-31 15:25:43 PST
pushloghtml can render more than 10 entries if you ask it to. This was likely fixed years ago. This should probably be a dupe. But I'm too lazy to find it.
Comment 47 Ted Mielczarek [:ted.mielczarek] 2015-01-05 08:31:11 PST
This bug as filed was asking for infinite scrolling, but I don't know that anyone really wants that nowadays.
Comment 48 Gregory Szorc [:gps] 2015-01-05 16:46:27 PST
As it turns out, upstream Mercurial implemented infinite scrolling in their styles. I'm working backporting all the upstream goodness into hg.mozilla.org as part of bug 1117021. Adding infinite scroll should just be a matter of adding a quick JavaScript blob, assuming the HTTP API follows the same query standards (which it likely doesn't, but at least the HTML/JS bits are there).

Note You need to log in before you can comment on or make changes to this bug.