enabled direct link between anchors

[calidion, 2010-04-21 09:56:50]

replace " with '

[baiming, 2010-04-22 09:01:39]

Hi Wenxin,

I just started the review and noticed this change is kind of big. Could you
please give us a summary of this CL that describes what particular change is
included?

For example:  Why do you introduce a new class SubLeg? What is it used for? What
is the relationship between it and Leg? What changes are made to the original
Leg class? 

Thank you!

[calidion, 2010-04-23 02:09:44]

according to Weiliu's advise,
we should also enable the anchors to be linked with each other directly,
so the Leg alone can not make it now.
I added SubLeg in Leg to act as the Leg in shape.
and Anchors as Stops in Trip/Shape.
Now the Leg will be added Anchors freely and set the SubLeg as will(to be
direct line or polyline by loading GDirections).

2010/4/22 <baiming@google.com>

> Hi Wenxin,
>
> I just started the review and noticed this change is kind of big. Could
> you please give us a summary of this CL that describes what particular
> change is included?
>
> For example:  Why do you introduce a new class SubLeg? What is it used
> for? What is the relationship between it and Leg? What changes are made
> to the original Leg class?
>
> Thank you!
>
>
> http://codereview.appspot.com/959041/show
>

[baiming, 2010-04-23 06:29:30]

First batch of comments. More will come later.

btw, the demo looks really impressive, really great job!

http://codereview.appspot.com/959041/diff/1/2
File gtfsscheduleviewer/files/transit_editor.js (right):

http://codereview.appspot.com/959041/diff/1/2#newcode1685
gtfsscheduleviewer/files/transit_editor.js:1685: this.verify(function() {
Add comments here explaining what we did in the callback and why.

http://codereview.appspot.com/959041/diff/1/2#newcode1745
gtfsscheduleviewer/files/transit_editor.js:1745: hintNode.innerHTML = 'Error on
index <b style=\'color:red\'>'
One of the biggest benefits of using single-quote in js code is that we can
avoid the escaping " in HTML string as much as possible.

i.e, hintNode.innerHTML = 'Error on index <b style="color:red">' + ...
Yes, just the same as what you did in this line.

So the principle is using single quote (') in js code and using double quote (")
in HTML string.

http://codereview.appspot.com/959041/diff/1/2#newcode3061
gtfsscheduleviewer/files/transit_editor.js:3061: if (!this.sub) {
Once leg.sub is created, it is impossible to be deleted, right?

Why do we need to check the existence of this.sub in both functions? Will
leg.setStops be invoked before leg.setPoints? or after? or no certain order?
If we have some specific reasons, feel free to add comments in code.

http://codereview.appspot.com/959041/diff/1/2#newcode3081
gtfsscheduleviewer/files/transit_editor.js:3081: Leg.prototype.rebuild =
function() {
Are lines from here to 3280 simply moved from other place? If so, I wont' pay
much attention to it.

[baiming, 2010-04-23 08:05:31]

A small bug was founded in the demo:

1) Edit a leg by adding several anchor points
2) Switch another trip (of different pattern)
3) Come back to the original trip which was edited

Result:
Those anchor points were still taking effect, but not visible. So we can't
delete or move those invisible anchors.

Expected:
The anchors are still there so that we could change them.

On Fri, Apr 23, 2010 at 2:29 PM, <baiming@google.com> wrote:

> First batch of comments. More will come later.
>
> btw, the demo looks really impressive, really great job!
>
>
> http://codereview.appspot.com/959041/diff/1/2
> File gtfsscheduleviewer/files/transit_editor.js (right):
>
> http://codereview.appspot.com/959041/diff/1/2#newcode1685
> gtfsscheduleviewer/files/transit_editor.js:1685: this.verify(function()
> {
> Add comments here explaining what we did in the callback and why.
>
> http://codereview.appspot.com/959041/diff/1/2#newcode1745
> gtfsscheduleviewer/files/transit_editor.js:1745: hintNode.innerHTML =
> 'Error on index <b style=\'color:red\'>'
> One of the biggest benefits of using single-quote in js code is that we
> can avoid the escaping " in HTML string as much as possible.
>
> i.e, hintNode.innerHTML = 'Error on index <b style="color:red">' + ...
> Yes, just the same as what you did in this line.
>
> So the principle is using single quote (') in js code and using double
> quote (") in HTML string.
>
> http://codereview.appspot.com/959041/diff/1/2#newcode3061
> gtfsscheduleviewer/files/transit_editor.js:3061: if (!this.sub) {
> Once leg.sub is created, it is impossible to be deleted, right?
>
> Why do we need to check the existence of this.sub in both functions?
> Will leg.setStops be invoked before leg.setPoints? or after? or no
> certain order?
> If we have some specific reasons, feel free to add comments in code.
>
> http://codereview.appspot.com/959041/diff/1/2#newcode3081
> gtfsscheduleviewer/files/transit_editor.js:3081: Leg.prototype.rebuild =
> function() {
> Are lines from here to 3280 simply moved from other place? If so, I
> wont' pay much attention to it.
>
>
> http://codereview.appspot.com/959041/show
>



-- 
Ming Bai 白明
Software Engineer
Google Inc.
Tel(Office): 86 (10) 6250-3361
Tel(Cell): +86-159-0153-4908
Email: baiming@google.com

[calidion, 2010-04-23 08:23:50]

1.bug fixed
2.updated

http://codereview.appspot.com/959041/diff/1/2
File gtfsscheduleviewer/files/transit_editor.js (right):

http://codereview.appspot.com/959041/diff/1/2#newcode3061
gtfsscheduleviewer/files/transit_editor.js:3061: if (!this.sub) {
Removed for the seeming precedence of the setStops to setPoints.
On 2010/04/23 06:29:30, baiming wrote:
> Once leg.sub is created, it is impossible to be deleted, right?
> 
> Why do we need to check the existence of this.sub in both functions? Will
> leg.setStops be invoked before leg.setPoints? or after? or no certain order?
> If we have some specific reasons, feel free to add comments in code.
> 
> 

Done.

http://codereview.appspot.com/959041/diff/1/2#newcode3081
gtfsscheduleviewer/files/transit_editor.js:3081: Leg.prototype.rebuild =
function() {
On 2010/04/23 06:29:30, baiming wrote:
> Are lines from here to 3280 simply moved from other place? If so, I wont' pay
> much attention to it.

mostly have slight changes.

[baiming, 2010-04-23 10:43:49]

Second batch.

Sorry that I can't continue to finish the review now, due to a bad head ache.

http://codereview.appspot.com/959041/diff/1/2
File gtfsscheduleviewer/files/transit_editor.js (right):

http://codereview.appspot.com/959041/diff/1/2#newcode1745
gtfsscheduleviewer/files/transit_editor.js:1745: hintNode.innerHTML = 'Error on
index <b style=\'color:red\'>'
Thanks for the change.

On 2010/04/23 06:29:30, baiming wrote:
> One of the biggest benefits of using single-quote in js code is that we can
> avoid the escaping " in HTML string as much as possible.
> 
> i.e, hintNode.innerHTML = 'Error on index <b style="color:red">' + ...
> Yes, just the same as what you did in this line.
> 
> So the principle is using single quote (') in js code and using double quote
(")
> in HTML string.

http://codereview.appspot.com/959041/diff/1/2#newcode3328
gtfsscheduleviewer/files/transit_editor.js:3328: SubLeg.prototype.setAnchor =
function(fromAnchor, toAnchor) {
Sorry, where is it used?

http://codereview.appspot.com/959041/diff/1/2#newcode3382
gtfsscheduleviewer/files/transit_editor.js:3382: directLine.type = 'checkbox';
I'm curious why "get directions" is a link but "direct link" is a checkbox? I
think they should be both links as they are mutual exclusive.

http://codereview.appspot.com/959041/diff/1/2#newcode3386
gtfsscheduleviewer/files/transit_editor.js:3386: p.appendChild($T('Direct
link'));
Is it better to phrase it as "direct connection"?

http://codereview.appspot.com/959041/diff/1/2#newcode3400
gtfsscheduleviewer/files/transit_editor.js:3400: SubLeg.prototype.transform =
function() {
If we make "direct connection" a link, we don't need this function.

http://codereview.appspot.com/959041/diff/10001/11001#newcode3026
gtfsscheduleviewer/files/transit_editor.js:3026: }
Tow questions:
1) Is this.sub always the first sub-leg of the leg?
2) Why do you choose this linked-list-like data structure to store the sub legs?
I doubt it is because you want to benefit from the convenience of inserting?

If so, please rename it to this.firstSub or so, and add comment for it to
explain why you use this linked list instead of an array.

http://codereview.appspot.com/959041/diff/10001/11001#newcode3478
gtfsscheduleviewer/files/transit_editor.js:3478: GLog.write('Anchor not
finised');
I would encourage you to add a "next" field in SubLeg which directly points the
following SubLeg, that can make things easier to loop the sub-legs for a leg,
and improve readability for cases like

 3263     if (this.startAnchor.toLeg.toAnchor != this.endAnchor) {
 3264       this.unify();
 3265     }

it will be

 3263     if (this.startAnchor.next != this.endAnchor) {
 3264       this.unify();
 3265     }

[calidion, 2010-04-26 03:05:55]

http://codereview.appspot.com/959041/diff/1/2
File gtfsscheduleviewer/files/transit_editor.js (right):

http://codereview.appspot.com/959041/diff/1/2#newcode3328
gtfsscheduleviewer/files/transit_editor.js:3328: SubLeg.prototype.setAnchor =
function(fromAnchor, toAnchor) {
On 2010/04/23 10:43:49, baiming wrote:
> Sorry, where is it used?

Done.

http://codereview.appspot.com/959041/diff/1/2#newcode3382
gtfsscheduleviewer/files/transit_editor.js:3382: directLine.type = 'checkbox';
It is a state to indicate if the subleg is directly linked or directioned, they
are different from each other, each click will change the state of the subleg.

On 2010/04/23 10:43:49, baiming wrote:
> I'm curious why "get directions" is a link but "direct link" is a checkbox? I
> think they should be both links as they are mutual exclusive.

http://codereview.appspot.com/959041/diff/1/2#newcode3386
gtfsscheduleviewer/files/transit_editor.js:3386: p.appendChild($T('Direct
link'));
On 2010/04/23 10:43:49, baiming wrote:
> Is it better to phrase it as "direct connection"?

Done.

http://codereview.appspot.com/959041/diff/1/2#newcode3400
gtfsscheduleviewer/files/transit_editor.js:3400: SubLeg.prototype.transform =
function() {
isDirect is a state and we must record it.
if we don't use a check box, we will need two button respectively to represent
the two states.

On 2010/04/23 10:43:49, baiming wrote:
> If we make "direct connection" a link, we don't need this function.

http://codereview.appspot.com/959041/diff/10001/11001#newcode3026
gtfsscheduleviewer/files/transit_editor.js:3026: }
I don't know if there is any reason for us to use array instead.

On 2010/04/23 10:43:50, baiming wrote:
> Tow questions:
> 1) Is this.sub always the first sub-leg of the leg?
> 2) Why do you choose this linked-list-like data structure to store the sub
legs?
> I doubt it is because you want to benefit from the convenience of inserting?
> 
> If so, please rename it to this.firstSub or so, and add comment for it to
> explain why you use this linked list instead of an array.

http://codereview.appspot.com/959041/diff/10001/11001#newcode3478
gtfsscheduleviewer/files/transit_editor.js:3478: GLog.write('Anchor not
finised');
it will add more codes on adding or removing a SubLeg.
and this is more clear that sublegs are of no direct relationship.

On 2010/04/23 10:43:50, baiming wrote:
> I would encourage you to add a "next" field in SubLeg which directly points
the
> following SubLeg, that can make things easier to loop the sub-legs for a leg,
> and improve readability for cases like
> 
>  3263     if (this.startAnchor.toLeg.toAnchor != this.endAnchor) {
>  3264       this.unify();
>  3265     }
> 
> it will be
> 
>  3263     if (this.startAnchor.next != this.endAnchor) {
>  3264       this.unify();
>  3265     }
>

[baiming, 2010-04-26 03:48:46]

http://codereview.appspot.com/959041/diff/10001/11001
File gtfsscheduleviewer/files/transit_editor.js (right):

http://codereview.appspot.com/959041/diff/10001/11001#newcode3026
gtfsscheduleviewer/files/transit_editor.js:3026: if (!this.sub) {
On 2010/04/26 03:05:55, calidion wrote:
> I don't know if there is any reason for us to use array instead.
> 
> On 2010/04/23 10:43:50, baiming wrote:
> > Tow questions:
> > 1) Is this.sub always the first sub-leg of the leg?

Yes or no?

> > 2) Why do you choose this linked-list-like data structure to store the sub
> legs?
> > I doubt it is because you want to benefit from the convenience of inserting?
> > 
> > If so, please rename it to this.firstSub or so, and add comment for it to
> > explain why you use this linked list instead of an array.
> 
> 

Yes, I see.

But please add comments. And if sub is always the first sub leg, please rename
it to "firstSub" or "firstSubLeg".

[calidion, 2010-04-26 05:05:36]

updated.

[baiming, 2010-04-26 05:11:04]

LGTM

[calidion, 2010-04-26 05:23:56]

updated to rev 75

2010/4/26 <baiming@google.com>

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