using GTFS Trip object

[calidion, 2010-01-07 08:38:12]

using GTFS Trip object

[baiming, 2010-01-08 05:37:39]

Overall the JS code looks good. Only a few minor comments.

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

http://codereview.appspot.com/181167/diff/1/2#newcode734
gtfsscheduleviewer/files/transit_editor.js:734: * @type {Trip}
For private members, please add @private annotation in jsDoc.

http://codereview.appspot.com/181167/diff/1/2#newcode805
gtfsscheduleviewer/files/transit_editor.js:805: Route.prototype.reset =
function() {
It looks your are widely using "reset" as function names in the code, where I
think it is not that suitable. Reset usually means clearing the status of the
object, to make it "empty" or as "initialized". But here as far as I know, it
just stands for "setting it as unselected". So please rename all "reset"s in the
file to "unselect".

http://codereview.appspot.com/181167/diff/1/2#newcode835
gtfsscheduleviewer/files/transit_editor.js:835: this.patterns = {};
Shouldn't be private?

http://codereview.appspot.com/181167/diff/1/2#newcode865
gtfsscheduleviewer/files/transit_editor.js:865:
Route.prototype.initPatternSection = function() {
Shouldn't be private?

http://codereview.appspot.com/181167/diff/1/2#newcode885
gtfsscheduleviewer/files/transit_editor.js:885: var hideButton = null;
rename it to toggleButton, because it is not only hiding items, but switching
the show/hide status.

http://codereview.appspot.com/181167/diff/1/2#newcode890
gtfsscheduleviewer/files/transit_editor.js:890: if (!hideButton)
if (!hideButton) {

http://codereview.appspot.com/181167/diff/1/2#newcode911
gtfsscheduleviewer/files/transit_editor.js:911: Route.prototype.onButtonSelected
= function() {
Rename it to onToggleButtonClick

http://codereview.appspot.com/181167/diff/1/2#newcode911
gtfsscheduleviewer/files/transit_editor.js:911: Route.prototype.onButtonSelected
= function() {
This function actually is not a member of Route, because it will be bound to
hideButton that "this" in the function refers to hideButton.
Separately looking to this function is rather confusing.

How about we don't assign any status to hideButton, instead let Route itself
handle the hide/show logic?

Route.prototype.onToggleButtonClick = function() {
  for (int i = 3; i < this.trips.length; ++i) {
    this.trips[i].node.display = this.trips[i].node.display == 'none' ? '' :
'none';
  }
};

Then in line 898, bind this function to "this" instead of the button.

http://codereview.appspot.com/181167/diff/1/2#newcode932
gtfsscheduleviewer/files/transit_editor.js:932: * Handler for trip to be
selected
Add @param for data.

http://codereview.appspot.com/181167/diff/1/2#newcode993
gtfsscheduleviewer/files/transit_editor.js:993: this.route = null;
How do you plan to use .route for a trip? Is it necessary for the trip to know
who is its parent?

http://codereview.appspot.com/181167/diff/1/2#newcode1011
gtfsscheduleviewer/files/transit_editor.js:1011: Trip.prototype.reset =
function(route) {
See comment in line 805.

http://codereview.appspot.com/181167/diff/1/2#newcode1020
gtfsscheduleviewer/files/transit_editor.js:1020: Trip.prototype.setDomNode =
function(node) {
Is parameter "node" the parent of the trip.node? If so, please 
1) rename parameter "node" to  "parentNode"
1) rename this function to createNode, because we don't set, but create its own
node.

[calidion, 2010-01-08 05:37:47]

would anyone please review this upload.

2010/1/7 <calidion@gmail.com>

> Reviewers: lychen, xan_google.com, weiliu, leio.chen, baiming, qiaojian,
>
> Message:
> using GTFS Trip object
>
>
>
> Please review this at http://codereview.appspot.com/181167
>
> Affected files:
>  M     gtfsscheduleviewer/files/transit_editor.js
>  M     schedule_editor.py
>  M     transitfeed_editor.py
>
>
>

[baiming, 2010-01-08 05:39:42]

Do you mean the python code?

2010/1/8 李白，字一日 <calidion@gmail.com>:
> would anyone please review this upload.
>
> 2010/1/7 <calidion@gmail.com>
>>
>> Reviewers: lychen, xan_google.com, weiliu, leio.chen, baiming, qiaojian,
>>
>> Message:
>> using GTFS Trip object
>>
>>
>>
>> Please review this at http://codereview.appspot.com/181167
>>
>> Affected files:
>>  M     gtfsscheduleviewer/files/transit_editor.js
>>  M     schedule_editor.py
>>  M     transitfeed_editor.py
>>
>>
>
>



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

[weiliu, 2010-01-08 05:43:12]

For python code, mostly looks good.

http://codereview.appspot.com/181167/diff/1/3
File transitfeed_editor.py (right):

http://codereview.appspot.com/181167/diff/1/3#newcode830
transitfeed_editor.py:830: trip.start_time =
stoptimes_in_trip[relation[0]][0][3]
Would you please elaborate why first_stop_in_trip is not needed anymore? Is it
because sth changed?

[calidion, 2010-01-08 05:43:20]

yes, python code needs reviewing too.

2010/1/8 Ming Bai 白明 <baiming@google.com>

> Do you mean the python code?
>
> 2010/1/8 李白，字一日 <calidion@gmail.com>:
> > would anyone please review this upload.
> >
> > 2010/1/7 <calidion@gmail.com>
> >>
> >> Reviewers: lychen, xan_google.com, weiliu, leio.chen, baiming,
> qiaojian,
> >>
> >> Message:
> >> using GTFS Trip object
> >>
> >>
> >>
> >> Please review this at http://codereview.appspot.com/181167
> >>
> >> Affected files:
> >>  M     gtfsscheduleviewer/files/transit_editor.js
> >>  M     schedule_editor.py
> >>  M     transitfeed_editor.py
> >>
> >>
> >
> >
>
>
>
> --
> Ming Bai 白明
> Software Engineer
> Google Inc.
> Tel(Office): 86 (10) 6250-3361
> Tel(Cell): +86-159-0153-4908
> Email: baiming@google.com
>

[baiming, 2010-01-08 06:01:43]

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

http://codereview.appspot.com/181167/diff/1/2#newcode911
gtfsscheduleviewer/files/transit_editor.js:911: Route.prototype.onButtonSelected
= function() {
Suppose you want to change the innerHTML of the button, either you can set it as
a member of Route, or you can benefit from the event parameter of the handler
function:

onToggleButtonClick = function(e) {
  var btn = e.currentTarget;
  btn.innerHTML = ...
}; 

On 2010/01/08 05:37:41, baiming wrote:
> This function actually is not a member of Route, because it will be bound to
> hideButton that "this" in the function refers to hideButton.
> Separately looking to this function is rather confusing.
> 
> How about we don't assign any status to hideButton, instead let Route itself
> handle the hide/show logic?
> 
> Route.prototype.onToggleButtonClick = function() {
>   for (int i = 3; i < this.trips.length; ++i) {
>     this.trips[i].node.display = this.trips[i].node.display == 'none' ? '' :
> 'none';
>   }
> };
> 
> Then in line 898, bind this function to "this" instead of the button.

[calidion, 2010-01-08 08:45:12]

updated on comments

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

http://codereview.appspot.com/181167/diff/1/2#newcode734
gtfsscheduleviewer/files/transit_editor.js:734: * @type {Trip}
On 2010/01/08 05:37:41, baiming wrote:
> For private members, please add @private annotation in jsDoc.

Done.

http://codereview.appspot.com/181167/diff/1/2#newcode805
gtfsscheduleviewer/files/transit_editor.js:805: Route.prototype.reset =
function() {
On 2010/01/08 05:37:41, baiming wrote:
> It looks your are widely using "reset" as function names in the code, where I
> think it is not that suitable. Reset usually means clearing the status of the
> object, to make it "empty" or as "initialized". But here as far as I know, it
> just stands for "setting it as unselected". So please rename all "reset"s in
the
> file to "unselect".

reset means that in normal state, the route should be like this way. and it
doesn't mean that it is not selected or something.
in a way to be selected is an unusual state for the route.

http://codereview.appspot.com/181167/diff/1/2#newcode835
gtfsscheduleviewer/files/transit_editor.js:835: this.patterns = {};
On 2010/01/08 05:37:41, baiming wrote:
> Shouldn't be private?
I don't know it would be changed or not since in the demo we group trips by
shapes, in consistence reason, i am here using patterns instead. and it would
possibly be used outside the Route.

http://codereview.appspot.com/181167/diff/1/2#newcode865
gtfsscheduleviewer/files/transit_editor.js:865:
Route.prototype.initPatternSection = function() {
On 2010/01/08 05:37:41, baiming wrote:
> Shouldn't be private?

Done.

http://codereview.appspot.com/181167/diff/1/2#newcode885
gtfsscheduleviewer/files/transit_editor.js:885: var hideButton = null;
On 2010/01/08 05:37:41, baiming wrote:
> rename it to toggleButton, because it is not only hiding items, but switching
> the show/hide status.

Done.

http://codereview.appspot.com/181167/diff/1/2#newcode890
gtfsscheduleviewer/files/transit_editor.js:890: if (!hideButton)
On 2010/01/08 05:37:41, baiming wrote:
> if (!hideButton) {

Done.

http://codereview.appspot.com/181167/diff/1/2#newcode911
gtfsscheduleviewer/files/transit_editor.js:911: Route.prototype.onButtonSelected
= function() {
On 2010/01/08 05:37:41, baiming wrote:
> Rename it to onToggleButtonClick

Done.

http://codereview.appspot.com/181167/diff/1/2#newcode911
gtfsscheduleviewer/files/transit_editor.js:911: Route.prototype.onButtonSelected
= function() {
On 2010/01/08 05:37:41, baiming wrote:
> This function actually is not a member of Route, because it will be bound to
> hideButton that "this" in the function refers to hideButton.
> Separately looking to this function is rather confusing.
> 
> How about we don't assign any status to hideButton, instead let Route itself
> handle the hide/show logic?
> 
> Route.prototype.onToggleButtonClick = function() {
>   for (int i = 3; i < this.trips.length; ++i) {
>     this.trips[i].node.display = this.trips[i].node.display == 'none' ? '' :
> 'none';
>   }
> };
> 
> Then in line 898, bind this function to "this" instead of the button.

using inner functions instead.

http://codereview.appspot.com/181167/diff/1/2#newcode932
gtfsscheduleviewer/files/transit_editor.js:932: * Handler for trip to be
selected
On 2010/01/08 05:37:41, baiming wrote:
> Add @param for data.

Done.

http://codereview.appspot.com/181167/diff/1/2#newcode993
gtfsscheduleviewer/files/transit_editor.js:993: this.route = null;
On 2010/01/08 05:37:41, baiming wrote:
> How do you plan to use .route for a trip? Is it necessary for the trip to know
> who is its parent?
yes

http://codereview.appspot.com/181167/diff/1/2#newcode1020
gtfsscheduleviewer/files/transit_editor.js:1020: Trip.prototype.setDomNode =
function(node) {
On 2010/01/08 05:37:41, baiming wrote:
> Is parameter "node" the parent of the trip.node? If so, please 
> 1) rename parameter "node" to  "parentNode"
> 1) rename this function to createNode, because we don't set, but create its
own
> node.

changed to make it much clearer.

http://codereview.appspot.com/181167/diff/1/3
File transitfeed_editor.py (right):

http://codereview.appspot.com/181167/diff/1/3#newcode830
transitfeed_editor.py:830: trip.start_time =
stoptimes_in_trip[relation[0]][0][3]
On 2010/01/08 05:43:12, weiliu wrote:
> Would you please elaborate why first_stop_in_trip is not needed anymore? Is it
> because sth changed?
yes. currently the way to determine the start_time of a trip is changed to sort
the stop_sequence so the first stop in trip is no longer useful.

[weiliu, 2010-01-08 08:52:49]

LGTM for python changes.

[baiming, 2010-01-08 09:21:01]

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

http://codereview.appspot.com/181167/diff/1/2#newcode805
gtfsscheduleviewer/files/transit_editor.js:805: Route.prototype.reset =
function() {
No. As I said, "reset" implies that all status and member values of the object
will be cleared.

I think "unSelect" precisely describes what the function does, switching from
the status "selected" to "unselected", right?

If you got some other names which make more sense, let's discuss it. But, do NOT
use "reset".

On 2010/01/08 08:45:12, calidion wrote:
> On 2010/01/08 05:37:41, baiming wrote:
> > It looks your are widely using "reset" as function names in the code, where
I
> > think it is not that suitable. Reset usually means clearing the status of
the
> > object, to make it "empty" or as "initialized". But here as far as I know,
it
> > just stands for "setting it as unselected". So please rename all "reset"s in
> the
> > file to "unselect".
> 
> reset means that in normal state, the route should be like this way. and it
> doesn't mean that it is not selected or something.
> in a way to be selected is an unusual state for the route.
>

http://codereview.appspot.com/181167/diff/1/2#newcode835
gtfsscheduleviewer/files/transit_editor.js:835: this.patterns = {};
OK, fine with me to leave it as public for now.

On 2010/01/08 08:45:12, calidion wrote:
> On 2010/01/08 05:37:41, baiming wrote:
> > Shouldn't be private?
> I don't know it would be changed or not since in the demo we group trips by
> shapes, in consistence reason, i am here using patterns instead. and it would
> possibly be used outside the Route.
>

http://codereview.appspot.com/181167/diff/1/2#newcode993
gtfsscheduleviewer/files/transit_editor.js:993: this.route = null;
On 2010/01/08 08:45:12, calidion wrote:
> On 2010/01/08 05:37:41, baiming wrote:
> > How do you plan to use .route for a trip? Is it necessary for the trip to
know
> > who is its parent?
> yes
> 
Can you please elaborate a bit more how do you plan to use it?

http://codereview.appspot.com/181167/diff/1/2#newcode1020
gtfsscheduleviewer/files/transit_editor.js:1020: Trip.prototype.setDomNode =
function(node) {
Thanks for the change. but I still would feel a lit strange for this function
name because from the name we cannot know:
1) it creates its own node
2) it returns the node

If you think "createNode" is not containing the information about that the node
will be inserted in the parentNode, how about "createNodeUnderParent" ?

On 2010/01/08 08:45:12, calidion wrote:
> On 2010/01/08 05:37:41, baiming wrote:
> > Is parameter "node" the parent of the trip.node? If so, please 
> > 1) rename parameter "node" to  "parentNode"
> > 1) rename this function to createNode, because we don't set, but create its
> own
> > node.
> 
> changed to make it much clearer.

http://codereview.appspot.com/181167/diff/14/1004#newcode213
gtfsscheduleviewer/files/transit_editor.js:213: // Just in case.
btw, there's no Integer type in JS. use "number".

[calidion, 2010-01-08 10:19:16]

updated on comments.

http://codereview.appspot.com/181167/diff/14/1004
File gtfsscheduleviewer/files/transit_editor.js (right):

http://codereview.appspot.com/181167/diff/14/1004#newcode213
gtfsscheduleviewer/files/transit_editor.js:213: * @param {Integer} idx Index to
be removed
On 2010/01/08 09:21:02, baiming wrote:
> btw, there's no Integer type in JS. use "number".

Done.

[baiming, 2010-01-11 03:45:17]

We are almost there, except two comments below.

http://codereview.appspot.com/181167/diff/1010/21
File gtfsscheduleviewer/files/transit_editor.js (right):

http://codereview.appspot.com/181167/diff/1010/21#newcode852
gtfsscheduleviewer/files/transit_editor.js:852: GEvent.bindDom(toggleButton,
'click', toggleButton, onToggleButtonClick);
Line length.

http://codereview.appspot.com/181167/diff/1010/21#newcode949
gtfsscheduleviewer/files/transit_editor.js:949: Trip.prototype.createNode =
function(notShow) {
1) Add @param for notShow. And as it is an optional parameter, please name it as
opt_notShow.
2) It is no longer a private function, remove @private.

[calidion, 2010-01-11 04:44:16]

updated

[baiming, 2010-01-11 04:57:38]

http://codereview.appspot.com/181167/diff/2002/3001
File gtfsscheduleviewer/files/transit_editor.js (right):

http://codereview.appspot.com/181167/diff/2002/3001#newcode949
gtfsscheduleviewer/files/transit_editor.js:949: * @private
Remove @private. Please pay enough attention to the comment. Thanks!

[calidion, 2010-01-11 04:59:25]

updated

2010/1/11 <baiming@google.com>

>
> http://codereview.appspot.com/181167/diff/2002/3001
>
> File gtfsscheduleviewer/files/transit_editor.js (right):
>
> http://codereview.appspot.com/181167/diff/2002/3001#newcode949
> gtfsscheduleviewer/files/transit_editor.js:949: * @private
> Remove @private. Please pay enough attention to the comment. Thanks!
>
>
> http://codereview.appspot.com/181167
>

[baiming, 2010-01-11 05:03:16]

LGTM.

[calidion, 2010-01-11 05:04:49]

thanks,
updated to rev 44.

2010/1/11 <baiming@google.com>

> LGTM.
>
>
> http://codereview.appspot.com/181167
>