enabled verifying of trip

[calidion, 2010-02-08 06:22:24]

added verify button.
enabled verification for shape on each trip.

[calidion, 2010-02-08 06:48:12]

1.removed async tag
2.added hint info when verifying.

[calidion, 2010-02-08 06:51:47]

with server update:
http://211.100.227.25:8765/

[baiming, 2010-02-22 04:03:56]

btw, there seems to be a bug that after I open one trip by clicking on the time,
another copy of that trip will be displayed below the original one (need some
time to fetch data from server for reproducing that).

http://codereview.appspot.com/203064/diff/5/6
File gtfsscheduleviewer/files/transit_editor.js (right):

http://codereview.appspot.com/203064/diff/5/6#newcode25
gtfsscheduleviewer/files/transit_editor.js:25: self = self || this;
Why adding this?

http://codereview.appspot.com/203064/diff/5/6#newcode1289
gtfsscheduleviewer/files/transit_editor.js:1289: var count =
self.stoptimes.length;
count is only used for one time, and the name is too ambiguous (count of what).
I suggest you to avoid this shorthand and use self.stoptimes.length in anonymous
function.

http://codereview.appspot.com/203064/diff/5/6#newcode1291
gtfsscheduleviewer/files/transit_editor.js:1291: var startDir = function() {
This function is really complicated. I hope we can give it a small refactoring
for readability.

A simple approach might be making startDir as a member function of Trip, that it
can directly access this.shape, this.route and other members of Trip, and also
can make this function shorter.

And the anonymous  callback function of leg.verify can also be a member of Trip.

Trip.prototype.onLegVerify_ = function(legIndex, polyline, error) {
  ...
  this.verifyLeg_(legIndex + 1);
};

Trip.prototype.verifyLeg_ = function(legIndex, callback) {
  if (legIndex == this.stoptimes.length - 1) {
    if (callback) {
      ...
    }
    return;
  }

  ...
  var leg...
  leg.verify(this, this.onLegVerify_);
};

so that Trip.prototype.verify can be removed as well.

Trip.prototype.onVerify = function(callback) {
  map.closeInfoWindow();
  this.verifyLeg_(0, callback);
};

http://codereview.appspot.com/203064/diff/5/6#newcode1296
gtfsscheduleviewer/files/transit_editor.js:1296: }, 200);
Can we replace the hard-coded 200 (milliseconds) with a constant variable, such
as VERIFY_CALLBACK_TIMEOUT?

http://codereview.appspot.com/203064/diff/5/6#newcode1304
gtfsscheduleviewer/files/transit_editor.js:1304: self.toolbar.hint.innerHTML =
'Generating Shape [' + shape.id + '] <b style="color:red">' +
Line length exceeds 80 char.

http://codereview.appspot.com/203064/diff/5/6#newcode1310
gtfsscheduleviewer/files/transit_editor.js:1310: if (callback) {
Is "callback" to be invoked once when the whole trip is verified or after each
leg is verified?

http://codereview.appspot.com/203064/diff/5/6#newcode1609
gtfsscheduleviewer/files/transit_editor.js:1609: Shape.prototype.createVerifyLeg
= function(fromStop, toStop) {
I didn't see any reason why createVerifyLeg is a member of Shape. You use
"this.id" of the Shape only for accessing the fromVerifyLeg/toVerifyLeg. I'd
prefer to make it a global util function.

http://codereview.appspot.com/203064/diff/5/6#newcode1814
gtfsscheduleviewer/files/transit_editor.js:1814: VERIFIED: '#0F0'
Please make the color expression consistent in character case (大小写). i.e, change
'#7fb80e' to '#7FB80E'.

http://codereview.appspot.com/203064/diff/5/6#newcode1966
gtfsscheduleviewer/files/transit_editor.js:1966: * ReGDirections of this Leg,
pass polyline to callback
I remembered that we have already got rid of this term "ReGDirections" from the
code. Please change it accordingly.

http://codereview.appspot.com/203064/diff/5/6#newcode1971
gtfsscheduleviewer/files/transit_editor.js:1971: var self = this;
Here you override "self" with "this", isn't it a bug?

http://codereview.appspot.com/203064/diff/5/6#newcode1974
gtfsscheduleviewer/files/transit_editor.js:1974: var query = null;
Please assign the value to query right after it is defined.

var query = 'from: ' + getPoint(start) + ' to: ' + getPoint(end);

http://codereview.appspot.com/203064/diff/5/6#newcode1980
gtfsscheduleviewer/files/transit_editor.js:1980: * ReGDirections of this Leg,
with UI changes
s/ReGDirections/Get the directions.

[calidion, 2010-02-22 07:00:22]

updated.

http://codereview.appspot.com/203064/diff/5/6
File gtfsscheduleviewer/files/transit_editor.js (right):

http://codereview.appspot.com/203064/diff/5/6#newcode25
gtfsscheduleviewer/files/transit_editor.js:25: self = self || this;
On 2010/02/22 04:03:56, baiming wrote:
> Why adding this?

assign this to self if self is null.

http://codereview.appspot.com/203064/diff/5/6#newcode1289
gtfsscheduleviewer/files/transit_editor.js:1289: var count =
self.stoptimes.length;
On 2010/02/22 04:03:56, baiming wrote:
> count is only used for one time, and the name is too ambiguous (count of
what).
> I suggest you to avoid this shorthand and use self.stoptimes.length in
anonymous
> function.

Done.

http://codereview.appspot.com/203064/diff/5/6#newcode1291
gtfsscheduleviewer/files/transit_editor.js:1291: var startDir = function() {
On 2010/02/22 04:03:56, baiming wrote:
> This function is really complicated. I hope we can give it a small refactoring
> for readability.
> 
> A simple approach might be making startDir as a member function of Trip, that
it
> can directly access this.shape, this.route and other members of Trip, and also
> can make this function shorter.
> 
> And the anonymous  callback function of leg.verify can also be a member of
Trip.
> 
> Trip.prototype.onLegVerify_ = function(legIndex, polyline, error) {
>   ...
>   this.verifyLeg_(legIndex + 1);
> };
> 
> Trip.prototype.verifyLeg_ = function(legIndex, callback) {
>   if (legIndex == this.stoptimes.length - 1) {
>     if (callback) {
>       ...
>     }
>     return;
>   }
> 
>   ...
>   var leg...
>   leg.verify(this, this.onLegVerify_);
> };
> 
> so that Trip.prototype.verify can be removed as well.
> 
> Trip.prototype.onVerify = function(callback) {
>   map.closeInfoWindow();
>   this.verifyLeg_(0, callback);
> };

it seems it would have problems in send legIndex, and get the leg inside
function onLegVerify_, and it may also introduce parameter changes in
leg.verify,
I have made some slight changes in the function, please check and advise.

http://codereview.appspot.com/203064/diff/5/6#newcode1296
gtfsscheduleviewer/files/transit_editor.js:1296: }, 200);
On 2010/02/22 04:03:56, baiming wrote:
> Can we replace the hard-coded 200 (milliseconds) with a constant variable,
such
> as VERIFY_CALLBACK_TIMEOUT?

Done.

http://codereview.appspot.com/203064/diff/5/6#newcode1304
gtfsscheduleviewer/files/transit_editor.js:1304: self.toolbar.hint.innerHTML =
'Generating Shape [' + shape.id + '] <b style="color:red">' +
On 2010/02/22 04:03:56, baiming wrote:
> Line length exceeds 80 char.

Done.

http://codereview.appspot.com/203064/diff/5/6#newcode1310
gtfsscheduleviewer/files/transit_editor.js:1310: if (callback) {
On 2010/02/22 04:03:56, baiming wrote:
> Is "callback" to be invoked once when the whole trip is verified or after each
> leg is verified?

startDir() will be invoked if leg.verify is succeeded.

http://codereview.appspot.com/203064/diff/5/6#newcode1609
gtfsscheduleviewer/files/transit_editor.js:1609: Shape.prototype.createVerifyLeg
= function(fromStop, toStop) {
On 2010/02/22 04:03:56, baiming wrote:
> I didn't see any reason why createVerifyLeg is a member of Shape. You use
> "this.id" of the Shape only for accessing the fromVerifyLeg/toVerifyLeg. I'd
> prefer to make it a global util function.

The verifying Legs are bound to Shapes and no other else.

http://codereview.appspot.com/203064/diff/5/6#newcode1814
gtfsscheduleviewer/files/transit_editor.js:1814: VERIFIED: '#0F0'
On 2010/02/22 04:03:56, baiming wrote:
> Please make the color expression consistent in character case (大小写). i.e,
change
> '#7fb80e' to '#7FB80E'.

Done.

http://codereview.appspot.com/203064/diff/5/6#newcode1966
gtfsscheduleviewer/files/transit_editor.js:1966: * ReGDirections of this Leg,
pass polyline to callback
On 2010/02/22 04:03:56, baiming wrote:
> I remembered that we have already got rid of this term "ReGDirections" from
the
> code. Please change it accordingly.

Done.

http://codereview.appspot.com/203064/diff/5/6#newcode1971
gtfsscheduleviewer/files/transit_editor.js:1971: var self = this;
On 2010/02/22 04:03:56, baiming wrote:
> Here you override "self" with "this", isn't it a bug?

Done.

http://codereview.appspot.com/203064/diff/5/6#newcode1974
gtfsscheduleviewer/files/transit_editor.js:1974: var query = null;
On 2010/02/22 04:03:56, baiming wrote:
> Please assign the value to query right after it is defined.
> 
> var query = 'from: ' + getPoint(start) + ' to: ' + getPoint(end);

Done.

http://codereview.appspot.com/203064/diff/5/6#newcode1980
gtfsscheduleviewer/files/transit_editor.js:1980: * ReGDirections of this Leg,
with UI changes
On 2010/02/22 04:03:56, baiming wrote:
> s/ReGDirections/Get the directions.

Done.

[calidion, 2010-02-22 07:00:50]

server updated:
http://211.100.227.25:8765/
On 2010/02/22 07:00:22, calidion wrote:
> updated.
> 
> http://codereview.appspot.com/203064/diff/5/6
> File gtfsscheduleviewer/files/transit_editor.js (right):
> 
> http://codereview.appspot.com/203064/diff/5/6#newcode25
> gtfsscheduleviewer/files/transit_editor.js:25: self = self || this;
> On 2010/02/22 04:03:56, baiming wrote:
> > Why adding this?
> 
> assign this to self if self is null.
> 
> http://codereview.appspot.com/203064/diff/5/6#newcode1289
> gtfsscheduleviewer/files/transit_editor.js:1289: var count =
> self.stoptimes.length;
> On 2010/02/22 04:03:56, baiming wrote:
> > count is only used for one time, and the name is too ambiguous (count of
> what).
> > I suggest you to avoid this shorthand and use self.stoptimes.length in
> anonymous
> > function.
> 
> Done.
> 
> http://codereview.appspot.com/203064/diff/5/6#newcode1291
> gtfsscheduleviewer/files/transit_editor.js:1291: var startDir = function() {
> On 2010/02/22 04:03:56, baiming wrote:
> > This function is really complicated. I hope we can give it a small
refactoring
> > for readability.
> > 
> > A simple approach might be making startDir as a member function of Trip,
that
> it
> > can directly access this.shape, this.route and other members of Trip, and
also
> > can make this function shorter.
> > 
> > And the anonymous  callback function of leg.verify can also be a member of
> Trip.
> > 
> > Trip.prototype.onLegVerify_ = function(legIndex, polyline, error) {
> >   ...
> >   this.verifyLeg_(legIndex + 1);
> > };
> > 
> > Trip.prototype.verifyLeg_ = function(legIndex, callback) {
> >   if (legIndex == this.stoptimes.length - 1) {
> >     if (callback) {
> >       ...
> >     }
> >     return;
> >   }
> > 
> >   ...
> >   var leg...
> >   leg.verify(this, this.onLegVerify_);
> > };
> > 
> > so that Trip.prototype.verify can be removed as well.
> > 
> > Trip.prototype.onVerify = function(callback) {
> >   map.closeInfoWindow();
> >   this.verifyLeg_(0, callback);
> > };
> 
> it seems it would have problems in send legIndex, and get the leg inside
> function onLegVerify_, and it may also introduce parameter changes in
> leg.verify,
> I have made some slight changes in the function, please check and advise.
> 
> http://codereview.appspot.com/203064/diff/5/6#newcode1296
> gtfsscheduleviewer/files/transit_editor.js:1296: }, 200);
> On 2010/02/22 04:03:56, baiming wrote:
> > Can we replace the hard-coded 200 (milliseconds) with a constant variable,
> such
> > as VERIFY_CALLBACK_TIMEOUT?
> 
> Done.
> 
> http://codereview.appspot.com/203064/diff/5/6#newcode1304
> gtfsscheduleviewer/files/transit_editor.js:1304: self.toolbar.hint.innerHTML =
> 'Generating Shape [' + shape.id + '] <b style="color:red">' +
> On 2010/02/22 04:03:56, baiming wrote:
> > Line length exceeds 80 char.
> 
> Done.
> 
> http://codereview.appspot.com/203064/diff/5/6#newcode1310
> gtfsscheduleviewer/files/transit_editor.js:1310: if (callback) {
> On 2010/02/22 04:03:56, baiming wrote:
> > Is "callback" to be invoked once when the whole trip is verified or after
each
> > leg is verified?
> 
> startDir() will be invoked if leg.verify is succeeded.
> 
> http://codereview.appspot.com/203064/diff/5/6#newcode1609
> gtfsscheduleviewer/files/transit_editor.js:1609:
Shape.prototype.createVerifyLeg
> = function(fromStop, toStop) {
> On 2010/02/22 04:03:56, baiming wrote:
> > I didn't see any reason why createVerifyLeg is a member of Shape. You use
> > "this.id" of the Shape only for accessing the fromVerifyLeg/toVerifyLeg. I'd
> > prefer to make it a global util function.
> 
> The verifying Legs are bound to Shapes and no other else.
> 
> http://codereview.appspot.com/203064/diff/5/6#newcode1814
> gtfsscheduleviewer/files/transit_editor.js:1814: VERIFIED: '#0F0'
> On 2010/02/22 04:03:56, baiming wrote:
> > Please make the color expression consistent in character case (大小写). i.e,
> change
> > '#7fb80e' to '#7FB80E'.
> 
> Done.
> 
> http://codereview.appspot.com/203064/diff/5/6#newcode1966
> gtfsscheduleviewer/files/transit_editor.js:1966: * ReGDirections of this Leg,
> pass polyline to callback
> On 2010/02/22 04:03:56, baiming wrote:
> > I remembered that we have already got rid of this term "ReGDirections" from
> the
> > code. Please change it accordingly.
> 
> Done.
> 
> http://codereview.appspot.com/203064/diff/5/6#newcode1971
> gtfsscheduleviewer/files/transit_editor.js:1971: var self = this;
> On 2010/02/22 04:03:56, baiming wrote:
> > Here you override "self" with "this", isn't it a bug?
> 
> Done.
> 
> http://codereview.appspot.com/203064/diff/5/6#newcode1974
> gtfsscheduleviewer/files/transit_editor.js:1974: var query = null;
> On 2010/02/22 04:03:56, baiming wrote:
> > Please assign the value to query right after it is defined.
> > 
> > var query = 'from: ' + getPoint(start) + ' to: ' + getPoint(end);
> 
> Done.
> 
> http://codereview.appspot.com/203064/diff/5/6#newcode1980
> gtfsscheduleviewer/files/transit_editor.js:1980: * ReGDirections of this Leg,
> with UI changes
> On 2010/02/22 04:03:56, baiming wrote:
> > s/ReGDirections/Get the directions.
> 
> Done.

[calidion, 2010-02-23 06:04:42]

fixed bug on fetching trips more than once for the route

[calidion, 2010-02-23 06:31:59]

added comments for verify

[baiming, 2010-02-23 07:06:15]

LGTM.

[calidion, 2010-02-23 07:09:23]

updated to rev 54

2010/2/23 <baiming@google.com>

> LGTM.
>
>
> http://codereview.appspot.com/203064/show
>