Enable stop copy

[calidion, 2010-02-25 09:11:56]

1.enabled stop copy

操作步骤(operation)：
1.选择stop editable, (enable "stop editable" from toolbar)
2.点击stop (select one stop)，
3.在弹出的对话框里选择"Clone and replace current stop" 
(click "Clone and replace current stop" from the pop dialog)
4.然后确定 (confirm)
5.再点击工具栏上的Save (click 'save' from toolbar)
6.等保存成功后，再点击该站点，发现id变化了 (after successfully saved, then clicking the stop
marker, you will see the change of the stop id
7.选择同一个pattern下的其它trip.发现stop的id也变成最新的stop id
(and so to the trips of the same pattern)

server url:http://211.100.227.25:8765/

[weiliu, 2010-02-26 06:51:50]

Python code mostly looks good to me.

http://codereview.appspot.com/223049/diff/1/4
File schedule_editor.py (right):

http://codereview.appspot.com/223049/diff/1/4#newcode118
schedule_editor.py:118: pid = int(form['pid'].value)
Please use route_id, sequence, etc. to make good names for variables.

http://codereview.appspot.com/223049/diff/1/4#newcode171
schedule_editor.py:171: return 1
When do you use "return 1" and when do you use"return True"?

[baiming, 2010-02-26 08:07:23]

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

http://codereview.appspot.com/223049/diff/1/2#newcode121
gtfsscheduleviewer/files/transit_editor.js:121: * List of Stops to be created
Please describe 
1) when/why do we need to create new stops?
2) are the created stops temporary or permanent?

http://codereview.appspot.com/223049/diff/1/2#newcode1554
gtfsscheduleviewer/files/transit_editor.js:1554: stoptime.onStopNameChange);
Need two more spaces indentation.

http://codereview.appspot.com/223049/diff/1/2#newcode1568
gtfsscheduleviewer/files/transit_editor.js:1568: * @param {Object} msg Message
for trip containing the influenced stoptime
It's more common to use "event" as the event handler's parameter.

http://codereview.appspot.com/223049/diff/1/2#newcode1607
gtfsscheduleviewer/files/transit_editor.js:1607: for (var i = 0, trip; trip =
trips[i]; i++) {
Why do you this kind of tricky loop condition? It does harm to the readability,
I think.

Change it to
for (var i = 0, trip; i < trips.length; i++)

http://codereview.appspot.com/223049/diff/1/2#newcode2425
gtfsscheduleviewer/files/transit_editor.js:2425:
Stop.prototype.removeFromNewStopList = function() {
When will this function be invoked?

http://codereview.appspot.com/223049/diff/1/2#newcode2429
gtfsscheduleviewer/files/transit_editor.js:2429: newStopList.splice(i, 1);
Shouldn't it break here?

http://codereview.appspot.com/223049/diff/1/2#newcode2449
gtfsscheduleviewer/files/transit_editor.js:2449: Stop.prototype.onCopy =
function() {
It is really confusing that I at first thought it was the handler to the 'copy'
button's click event, but in fact it is not.
I'd rather put the code in this function into Stop.prototype.save to get rid of
the function which has a lying name.

See comments on Stop.prototype.save for details.

http://codereview.appspot.com/223049/diff/1/2#newcode2462
gtfsscheduleviewer/files/transit_editor.js:2462: Stop.prototype.save =
function() {
Stop.prototype.save = function() {
  if (this.inAsync) {
    return;
  }
  var postVars = {};
  postVars['name'] = encodeURIComponent(this.name);
  postVars['latlng'] = this.latlng.lat() + ',' + this.latlng.lng();
  var callback;
  if (Stop.isCopiedStopId(this.id)) {
    postVars['act'] = 'copy';
    postVars['parentId'] = Stop.getOrigStopId(this.id);
    callback = this.onCopied;
  } else {
    postVars['act'] = 'save';
    callback = this.onSaved;
  }
  var postBody = mapToString(postVars, '=', '&');
  this.inAsync = true;
  this.fetcher.updateStop(postBody, this, callback);
};

mapToString can be a util function like this:
function mapToString(map, keyValueSep, entrySep) {
  var entries = [];
  for (key in map) {
    entries.push(key + keyValueSep + value);
  }
  return pairs.join(entrySep);
};

http://codereview.appspot.com/223049/diff/1/2#newcode2476
gtfsscheduleviewer/files/transit_editor.js:2476: * Get DOM node for copying
Create element which is used for copying the stop.

http://codereview.appspot.com/223049/diff/1/2#newcode2477
gtfsscheduleviewer/files/transit_editor.js:2477: * @return {HTMLElement} Copy
node
@return {HTMLElement} The element for copying the stop.

http://codereview.appspot.com/223049/diff/1/2#newcode2479
gtfsscheduleviewer/files/transit_editor.js:2479: Stop.prototype.getCopyNode =
function() {
This function creates a new element, rather than getting, so please make its
name more precise. 
Stop.prototype.createElemForCopy

http://codereview.appspot.com/223049/diff/1/2#newcode2481
gtfsscheduleviewer/files/transit_editor.js:2481: var copyFromCurrentStop =
$C('input');
s/copyFromCurrentStop/copyBtn

http://codereview.appspot.com/223049/diff/1/2#newcode2493
gtfsscheduleviewer/files/transit_editor.js:2493: Stop.prototype.onCopyNodeClick
= function() {
You can rename this to Stop.prototype.onCopy if you like.

http://codereview.appspot.com/223049/diff/1/2#newcode2513
gtfsscheduleviewer/files/transit_editor.js:2513: var nameNode =
this.getUpdateNameNode(title);
Better to rename it to nameElem.

http://codereview.appspot.com/223049/diff/1/2#newcode2516
gtfsscheduleviewer/files/transit_editor.js:2516: if (this.id.indexOf('copy') ==
-1) {
It looks like that you are using a trick of stop id to identify whether a stop
is original or copied (temporary id, right?). It is acceptable to me though,
we'd better to leave this tricky implementation in a couple of dedicated
functions.

i.e, 
Stop.COPIED_STOP_PREFIX = 'copy';

Stop.isCopiedStopId = function(stopId) {
  return stopId.indexOf(Stop.COPIED_STOP_PREFIX) == 0;
};

Stop.makeCopiedStopId = function(origStopId) {
  if (Stop.isCopiedStopId(origStopId)) {
    return origStopId;
  }
  return Stop.COPIED_STOP_PREFIX + origStopId;
};

Stop.getOrigStopId = function(copiedStopId) {
  if (!Stop.isCopiedStopId(copiedStopId)) {
    alert();
    return copiedStopId;
  }
  return copiedStop.substring(Stop.COPIED_STOP_PREFIX.length);
};

http://codereview.appspot.com/223049/diff/1/2#newcode2525
gtfsscheduleviewer/files/transit_editor.js:2525: * Get stop name updating node
Create element which is used for editing the stop name.

http://codereview.appspot.com/223049/diff/1/2#newcode2526
gtfsscheduleviewer/files/transit_editor.js:2526: * @param {HTMLElement} title
Either "title" or "name" is OK, but at least you should keep the term
consistent.

http://codereview.appspot.com/223049/diff/1/2#newcode2528
gtfsscheduleviewer/files/transit_editor.js:2528:
Stop.prototype.getUpdateNameNode = function(title) {
s/getUpdateNameNode/createNameEditingElem

http://codereview.appspot.com/223049/diff/1/2#newcode2814
gtfsscheduleviewer/files/transit_editor.js:2814: newStop.name = stop.name;
I encourage you to make "clone" function to do this.
i,e. 
function clone(obj) {
  var newObj = {};
  for (key in obj) {
    newObj[key] = obj[key];
  }
  return newObj;
};

http://codereview.appspot.com/223049/diff/1/2#newcode2833
gtfsscheduleviewer/files/transit_editor.js:2833: this.trip.stops.push(newStop);
It shouldn't be appended to the end, otherwise the order of the stops will be
messed.
In case you do it this way, parseShapes won't work properly because shape.stops
is just trip.stops. (and it doesn't work in the demo, for now)

[calidion, 2010-02-26 11:43:01]

updated.

server updated too.

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

http://codereview.appspot.com/223049/diff/1/2#newcode121
gtfsscheduleviewer/files/transit_editor.js:121: * List of Stops to be created
On 2010/02/26 08:07:23, baiming wrote:
> Please describe 
> 1) when/why do we need to create new stops?
> 2) are the created stops temporary or permanent?

this will be used later to handle stops created but not bound to any object

http://codereview.appspot.com/223049/diff/1/2#newcode1554
gtfsscheduleviewer/files/transit_editor.js:1554: stoptime.onStopNameChange);
On 2010/02/26 08:07:23, baiming wrote:
> Need two more spaces indentation.

do we really need them?

http://codereview.appspot.com/223049/diff/1/2#newcode1568
gtfsscheduleviewer/files/transit_editor.js:1568: * @param {Object} msg Message
for trip containing the influenced stoptime
On 2010/02/26 08:07:23, baiming wrote:
> It's more common to use "event" as the event handler's parameter.

msg stands here for data like event messages,
event seems not sufficient for including data.

http://codereview.appspot.com/223049/diff/1/2#newcode1607
gtfsscheduleviewer/files/transit_editor.js:1607: for (var i = 0, trip; trip =
trips[i]; i++) {
On 2010/02/26 08:07:23, baiming wrote:
> Why do you this kind of tricky loop condition? It does harm to the
readability,
> I think.
> 
> Change it to
> for (var i = 0, trip; i < trips.length; i++)

it is according to the google 's javascript coding style, but i find this style
do harmful, especially in chrome, which will issue an error on last execution
where the trip is null and the index is out of bounds.

http://codereview.appspot.com/223049/diff/1/2#newcode2425
gtfsscheduleviewer/files/transit_editor.js:2425:
Stop.prototype.removeFromNewStopList = function() {
On 2010/02/26 08:07:23, baiming wrote:
> When will this function be invoked?

sorry, forgot to use it

http://codereview.appspot.com/223049/diff/1/2#newcode2429
gtfsscheduleviewer/files/transit_editor.js:2429: newStopList.splice(i, 1);
On 2010/02/26 08:07:23, baiming wrote:
> Shouldn't it break here?

Done.

http://codereview.appspot.com/223049/diff/1/2#newcode2449
gtfsscheduleviewer/files/transit_editor.js:2449: Stop.prototype.onCopy =
function() {
On 2010/02/26 08:07:23, baiming wrote:
> It is really confusing that I at first thought it was the handler to the
'copy'
> button's click event, but in fact it is not.
> I'd rather put the code in this function into Stop.prototype.save to get rid
of
> the function which has a lying name.
> 
> See comments on Stop.prototype.save for details.

Done.

http://codereview.appspot.com/223049/diff/1/2#newcode2462
gtfsscheduleviewer/files/transit_editor.js:2462: Stop.prototype.save =
function() {
On 2010/02/26 08:07:23, baiming wrote:
> Stop.prototype.save = function() {
>   if (this.inAsync) {
>     return;
>   }
>   var postVars = {};
>   postVars['name'] = encodeURIComponent(this.name);
>   postVars['latlng'] = this.latlng.lat() + ',' + this.latlng.lng();
>   var callback;
>   if (Stop.isCopiedStopId(this.id)) {
>     postVars['act'] = 'copy';
>     postVars['parentId'] = Stop.getOrigStopId(this.id);
>     callback = this.onCopied;
>   } else {
>     postVars['act'] = 'save';
>     callback = this.onSaved;
>   }
>   var postBody = mapToString(postVars, '=', '&');
>   this.inAsync = true;
>   this.fetcher.updateStop(postBody, this, callback);
> };
> 
> mapToString can be a util function like this:
> function mapToString(map, keyValueSep, entrySep) {
>   var entries = [];
>   for (key in map) {
>     entries.push(key + keyValueSep + value);
>   }
>   return pairs.join(entrySep);
> };

Done.

http://codereview.appspot.com/223049/diff/1/2#newcode2476
gtfsscheduleviewer/files/transit_editor.js:2476: * Get DOM node for copying
On 2010/02/26 08:07:23, baiming wrote:
> Create element which is used for copying the stop.

Done.

http://codereview.appspot.com/223049/diff/1/2#newcode2477
gtfsscheduleviewer/files/transit_editor.js:2477: * @return {HTMLElement} Copy
node
On 2010/02/26 08:07:23, baiming wrote:
> @return {HTMLElement} The element for copying the stop.

Done.

http://codereview.appspot.com/223049/diff/1/2#newcode2479
gtfsscheduleviewer/files/transit_editor.js:2479: Stop.prototype.getCopyNode =
function() {
On 2010/02/26 08:07:23, baiming wrote:
> This function creates a new element, rather than getting, so please make its
> name more precise. 
> Stop.prototype.createElemForCopy

this change will cause many naming style changes for consistent reason.
and it seems that it is of no much differences between node and element

http://codereview.appspot.com/223049/diff/1/2#newcode2481
gtfsscheduleviewer/files/transit_editor.js:2481: var copyFromCurrentStop =
$C('input');
On 2010/02/26 08:07:23, baiming wrote:
> s/copyFromCurrentStop/copyBtn

Done.

http://codereview.appspot.com/223049/diff/1/2#newcode2493
gtfsscheduleviewer/files/transit_editor.js:2493: Stop.prototype.onCopyNodeClick
= function() {
On 2010/02/26 08:07:23, baiming wrote:
> You can rename this to Stop.prototype.onCopy if you like.

the onCopy is occupied.

http://codereview.appspot.com/223049/diff/1/2#newcode2513
gtfsscheduleviewer/files/transit_editor.js:2513: var nameNode =
this.getUpdateNameNode(title);
On 2010/02/26 08:07:23, baiming wrote:
> Better to rename it to nameElem.

postponed.

http://codereview.appspot.com/223049/diff/1/2#newcode2516
gtfsscheduleviewer/files/transit_editor.js:2516: if (this.id.indexOf('copy') ==
-1) {
On 2010/02/26 08:07:23, baiming wrote:
> It looks like that you are using a trick of stop id to identify whether a stop
> is original or copied (temporary id, right?). It is acceptable to me though,
> we'd better to leave this tricky implementation in a couple of dedicated
> functions.
> 
> i.e, 
> Stop.COPIED_STOP_PREFIX = 'copy';
> 
> Stop.isCopiedStopId = function(stopId) {
>   return stopId.indexOf(Stop.COPIED_STOP_PREFIX) == 0;
> };
> 
> Stop.makeCopiedStopId = function(origStopId) {
>   if (Stop.isCopiedStopId(origStopId)) {
>     return origStopId;
>   }
>   return Stop.COPIED_STOP_PREFIX + origStopId;
> };
> 
> Stop.getOrigStopId = function(copiedStopId) {
>   if (!Stop.isCopiedStopId(copiedStopId)) {
>     alert();
>     return copiedStopId;
>   }
>   return copiedStop.substring(Stop.COPIED_STOP_PREFIX.length);
> };

using temp id to make it prepared for stop addition.

http://codereview.appspot.com/223049/diff/1/2#newcode2525
gtfsscheduleviewer/files/transit_editor.js:2525: * Get stop name updating node
On 2010/02/26 08:07:23, baiming wrote:
> Create element which is used for editing the stop name.

Done.

http://codereview.appspot.com/223049/diff/1/2#newcode2526
gtfsscheduleviewer/files/transit_editor.js:2526: * @param {HTMLElement} title
On 2010/02/26 08:07:23, baiming wrote:
> Either "title" or "name" is OK, but at least you should keep the term
> consistent.
what does this mean?

http://codereview.appspot.com/223049/diff/1/2#newcode2528
gtfsscheduleviewer/files/transit_editor.js:2528:
Stop.prototype.getUpdateNameNode = function(title) {
On 2010/02/26 08:07:23, baiming wrote:
> s/getUpdateNameNode/createNameEditingElem

Done.

http://codereview.appspot.com/223049/diff/1/2#newcode2814
gtfsscheduleviewer/files/transit_editor.js:2814: newStop.name = stop.name;
On 2010/02/26 08:07:23, baiming wrote:
> I encourage you to make "clone" function to do this.
> i,e. 
> function clone(obj) {
>   var newObj = {};
>   for (key in obj) {
>     newObj[key] = obj[key];
>   }
>   return newObj;
> };

it's been tried, and it shows that we only need the gtfs defined fields, and
some related fields, 
and the for-in loop will cause potential member share between the two objects
which we are not intended to.

http://codereview.appspot.com/223049/diff/1/2#newcode2833
gtfsscheduleviewer/files/transit_editor.js:2833: this.trip.stops.push(newStop);
On 2010/02/26 08:07:23, baiming wrote:
> It shouldn't be appended to the end, otherwise the order of the stops will be
> messed.
> In case you do it this way, parseShapes won't work properly because
shape.stops
> is just trip.stops. (and it doesn't work in the demo, for now)

Done.

http://codereview.appspot.com/223049/diff/1/4
File schedule_editor.py (right):

http://codereview.appspot.com/223049/diff/1/4#newcode118
schedule_editor.py:118: pid = int(form['pid'].value)
On 2010/02/26 06:51:50, weiliu wrote:
> Please use route_id, sequence, etc. to make good names for variables.

Done.

http://codereview.appspot.com/223049/diff/1/4#newcode171
schedule_editor.py:171: return 1
On 2010/02/26 06:51:50, weiliu wrote:
> When do you use "return 1" and when do you use"return True"? 

1 is default True return value for http response,
True is for normal return

[baiming, 2010-03-01 03:05:08]

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

http://codereview.appspot.com/223049/diff/1/2#newcode1568
gtfsscheduleviewer/files/transit_editor.js:1568: * @param {Object} msg Message
for trip containing the influenced stoptime
On 2010/02/26 11:43:01, calidion wrote:
> On 2010/02/26 08:07:23, baiming wrote:
> > It's more common to use "event" as the event handler's parameter.
> 
> msg stands here for data like event messages,
> event seems not sufficient for including data.
> 

In fact, "event", as a parameter name, means "event object" or "event message".
Please think about "window.event" in IE programming.

http://codereview.appspot.com/223049/diff/1/2#newcode2462
gtfsscheduleviewer/files/transit_editor.js:2462: Stop.prototype.save =
function() {
On 2010/02/26 11:43:01, calidion wrote:
> On 2010/02/26 08:07:23, baiming wrote:
> > Stop.prototype.save = function() {
> >   if (this.inAsync) {
> >     return;
> >   }
> >   var postVars = {};
> >   postVars['name'] = encodeURIComponent(this.name);
> >   postVars['latlng'] = this.latlng.lat() + ',' + this.latlng.lng();
> >   var callback;
> >   if (Stop.isCopiedStopId(this.id)) {
> >     postVars['act'] = 'copy';
> >     postVars['parentId'] = Stop.getOrigStopId(this.id);
> >     callback = this.onCopied;
> >   } else {
> >     postVars['act'] = 'save';
> >     callback = this.onSaved;
> >   }
> >   var postBody = mapToString(postVars, '=', '&');
> >   this.inAsync = true;
> >   this.fetcher.updateStop(postBody, this, callback);
> > };
> > 
> > mapToString can be a util function like this:
> > function mapToString(map, keyValueSep, entrySep) {
> >   var entries = [];
> >   for (key in map) {
> >     entries.push(key + keyValueSep + value);
> >   }
> >   return pairs.join(entrySep);
> > };
> 
> Done.

I don't think it is "Done".

I was not only talking about the use of the "key value object", but also that
you should merge the "onCopy" and "save" up.

http://codereview.appspot.com/223049/diff/1/2#newcode2479
gtfsscheduleviewer/files/transit_editor.js:2479: Stop.prototype.getCopyNode =
function() {
On 2010/02/26 11:43:01, calidion wrote:
> On 2010/02/26 08:07:23, baiming wrote:
> > This function creates a new element, rather than getting, so please make its
> > name more precise. 
> > Stop.prototype.createElemForCopy
> 
> this change will cause many naming style changes for consistent reason.
> and it seems that it is of no much differences between node and element

OK.

http://codereview.appspot.com/223049/diff/1/2#newcode2493
gtfsscheduleviewer/files/transit_editor.js:2493: Stop.prototype.onCopyNodeClick
= function() {
On 2010/02/26 11:43:01, calidion wrote:
> On 2010/02/26 08:07:23, baiming wrote:
> > You can rename this to Stop.prototype.onCopy if you like.
> 
> the onCopy is occupied.

I mean, if you get rid of the original .onCopy, you can use that name then.

http://codereview.appspot.com/223049/diff/1/2#newcode2516
gtfsscheduleviewer/files/transit_editor.js:2516: if (this.id.indexOf('copy') ==
-1) {
On 2010/02/26 11:43:01, calidion wrote:
> On 2010/02/26 08:07:23, baiming wrote:
> > It looks like that you are using a trick of stop id to identify whether a
stop
> > is original or copied (temporary id, right?). It is acceptable to me though,
> > we'd better to leave this tricky implementation in a couple of dedicated
> > functions.
> > 
> > i.e, 
> > Stop.COPIED_STOP_PREFIX = 'copy';
> > 
> > Stop.isCopiedStopId = function(stopId) {
> >   return stopId.indexOf(Stop.COPIED_STOP_PREFIX) == 0;
> > };
> > 
> > Stop.makeCopiedStopId = function(origStopId) {
> >   if (Stop.isCopiedStopId(origStopId)) {
> >     return origStopId;
> >   }
> >   return Stop.COPIED_STOP_PREFIX + origStopId;
> > };
> > 
> > Stop.getOrigStopId = function(copiedStopId) {
> >   if (!Stop.isCopiedStopId(copiedStopId)) {
> >     alert();
> >     return copiedStopId;
> >   }
> >   return copiedStop.substring(Stop.COPIED_STOP_PREFIX.length);
> > };
> 
> using temp id to make it prepared for stop addition.

Good, thanks for the change.

http://codereview.appspot.com/223049/diff/1/2#newcode2526
gtfsscheduleviewer/files/transit_editor.js:2526: * @param {HTMLElement} title
On 2010/02/26 11:43:01, calidion wrote:
> On 2010/02/26 08:07:23, baiming wrote:
> > Either "title" or "name" is OK, but at least you should keep the term
> > consistent.
> what does this mean?
> 

Sorry for misleading. I mean, if the function name is create"Name"EditingName,
please also use "name" in the parameters, instead of "title".

http://codereview.appspot.com/223049/diff/1/2#newcode2814
gtfsscheduleviewer/files/transit_editor.js:2814: newStop.name = stop.name;
On 2010/02/26 11:43:01, calidion wrote:
> On 2010/02/26 08:07:23, baiming wrote:
> > I encourage you to make "clone" function to do this.
> > i,e. 
> > function clone(obj) {
> >   var newObj = {};
> >   for (key in obj) {
> >     newObj[key] = obj[key];
> >   }
> >   return newObj;
> > };
> 
> it's been tried, and it shows that we only need the gtfs defined fields, and
> some related fields, 
> and the for-in loop will cause potential member share between the two objects
> which we are not intended to.

OK.

http://codereview.appspot.com/223049/diff/14/1006#newcode213
gtfsscheduleviewer/files/transit_editor.js:213: }
Do NOT abuse ";"
It is not needed after a block of "if" or "for".

http://codereview.appspot.com/223049/diff/14/1006#newcode1597
gtfsscheduleviewer/files/transit_editor.js:1597: /**
If you you the code is not in use any more, please delete it instead of
commenting it.

http://codereview.appspot.com/223049/diff/14/1006#newcode1601
gtfsscheduleviewer/files/transit_editor.js:1601: * @param {Number} sequence
Stoptime sequence
Ditto.

http://codereview.appspot.com/223049/diff/14/1006#newcode2549
gtfsscheduleviewer/files/transit_editor.js:2549:
self.triggerEvent('stopnamechange');
It is weird.
As you make it as a member of Stop, and "this.id" is used in the function, it
implies that "this.id" will be modified directly, rather than just returning a
value.
Otherwise, you'd better leave it as a util function, that we pass the "id" as
parameter to it.

http://codereview.appspot.com/223049/diff/14/1006#newcode2558
gtfsscheduleviewer/files/transit_editor.js:2558: * Show Stop info fetched
Ditto.

[calidion, 2010-03-01 06:34:38]

updated

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

http://codereview.appspot.com/223049/diff/1/2#newcode1568
gtfsscheduleviewer/files/transit_editor.js:1568: * @param {Object} msg Message
for trip containing the influenced stoptime
On 2010/03/01 03:05:08, baiming wrote:
> On 2010/02/26 11:43:01, calidion wrote:
> > On 2010/02/26 08:07:23, baiming wrote:
> > > It's more common to use "event" as the event handler's parameter.
> > 
> > msg stands here for data like event messages,
> > event seems not sufficient for including data.
> > 
> 
> In fact, "event", as a parameter name, means "event object" or "event
message".
> Please think about "window.event" in IE programming.

Done.

http://codereview.appspot.com/223049/diff/1/2#newcode2462
gtfsscheduleviewer/files/transit_editor.js:2462: Stop.prototype.save =
function() {
On 2010/03/01 03:05:08, baiming wrote:
> On 2010/02/26 11:43:01, calidion wrote:
> > On 2010/02/26 08:07:23, baiming wrote:
> > > Stop.prototype.save = function() {
> > >   if (this.inAsync) {
> > >     return;
> > >   }
> > >   var postVars = {};
> > >   postVars['name'] = encodeURIComponent(this.name);
> > >   postVars['latlng'] = this.latlng.lat() + ',' + this.latlng.lng();
> > >   var callback;
> > >   if (Stop.isCopiedStopId(this.id)) {
> > >     postVars['act'] = 'copy';
> > >     postVars['parentId'] = Stop.getOrigStopId(this.id);
> > >     callback = this.onCopied;
> > >   } else {
> > >     postVars['act'] = 'save';
> > >     callback = this.onSaved;
> > >   }
> > >   var postBody = mapToString(postVars, '=', '&');
> > >   this.inAsync = true;
> > >   this.fetcher.updateStop(postBody, this, callback);
> > > };
> > > 
> > > mapToString can be a util function like this:
> > > function mapToString(map, keyValueSep, entrySep) {
> > >   var entries = [];
> > >   for (key in map) {
> > >     entries.push(key + keyValueSep + value);
> > >   }
> > >   return pairs.join(entrySep);
> > > };
> > 
> > Done.
> 
> I don't think it is "Done".
> 
> I was not only talking about the use of the "key value object", but also that
> you should merge the "onCopy" and "save" up.

I have made modifications to clear the logic and merged the common part.

http://codereview.appspot.com/223049/diff/1/2#newcode2493
gtfsscheduleviewer/files/transit_editor.js:2493: Stop.prototype.onCopyNodeClick
= function() {
On 2010/03/01 03:05:08, baiming wrote:
> On 2010/02/26 11:43:01, calidion wrote:
> > On 2010/02/26 08:07:23, baiming wrote:
> > > You can rename this to Stop.prototype.onCopy if you like.
> > 
> > the onCopy is occupied.
> 
> I mean, if you get rid of the original .onCopy, you can use that name then.

Click doesn't always lead to the copy before the user confirmed.

http://codereview.appspot.com/223049/diff/1/2#newcode2526
gtfsscheduleviewer/files/transit_editor.js:2526: * @param {HTMLElement} title


title is the element where show the title of the info window when the stop
marker is clicked.
the only reason pass the title is that the title uses name as part of it.

http://codereview.appspot.com/223049/diff/14/1006#newcode213
gtfsscheduleviewer/files/transit_editor.js:213: }
On 2010/03/01 03:05:08, baiming wrote:
> Do NOT abuse ";"
> It is not needed after a block of "if" or "for".

Done.

http://codereview.appspot.com/223049/diff/14/1006#newcode1597
gtfsscheduleviewer/files/transit_editor.js:1597: /**
On 2010/03/01 03:05:08, baiming wrote:
> If you you the code is not in use any more, please delete it instead of
> commenting it.

Done.

http://codereview.appspot.com/223049/diff/14/1006#newcode1601
gtfsscheduleviewer/files/transit_editor.js:1601: * @param {Number} sequence
Stoptime sequence
On 2010/03/01 03:05:08, baiming wrote:
> Ditto.

Done.

http://codereview.appspot.com/223049/diff/14/1006#newcode2549
gtfsscheduleviewer/files/transit_editor.js:2549:
self.triggerEvent('stopnamechange');
On 2010/03/01 03:05:08, baiming wrote:
> It is weird.
> As you make it as a member of Stop, and "this.id" is used in the function, it
> implies that "this.id" will be modified directly, rather than just returning a
> value.
> Otherwise, you'd better leave it as a util function, that we pass the "id" as
> parameter to it.

Done.

http://codereview.appspot.com/223049/diff/14/1006#newcode2558
gtfsscheduleviewer/files/transit_editor.js:2558: * Show Stop info fetched
On 2010/03/01 03:05:08, baiming wrote:
> Ditto.

Done.

[baiming, 2010-03-01 06:46:48]

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

http://codereview.appspot.com/223049/diff/1/2#newcode2526
gtfsscheduleviewer/files/transit_editor.js:2526: * @param {HTMLElement} title
On 2010/03/01 06:34:38, calidion wrote:
> 
> 
> title is the element where show the title of the info window when the stop
> marker is clicked.
> the only reason pass the title is that the title uses name as part of it.
> 

I see. Thanks for the explanation.

http://codereview.appspot.com/223049/diff/17/1010#newcode2493
gtfsscheduleviewer/files/transit_editor.js:2493: Stop.prototype.onCopyNodeClick
= function() {
onXXX is not proper for this function because it is not an event handler or a
callback.
How about using saveForCopy? that you can name onModify as saveForModify.

http://codereview.appspot.com/223049/diff/17/1010#newcode2504
gtfsscheduleviewer/files/transit_editor.js:2504: var mod = this.modified ?
'<span style="color:red">*</span>' : '';
Ditto.

[calidion, 2010-03-01 07:10:02]

updated

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

http://codereview.appspot.com/223049/diff/17/1010#newcode2493
gtfsscheduleviewer/files/transit_editor.js:2493: Stop.prototype.onCopy =
function() {
On 2010/03/01 06:46:49, baiming wrote:
> onXXX is not proper for this function because it is not an event handler or a
> callback.
> How about using saveForCopy? that you can name onModify as saveForModify.

Done.

http://codereview.appspot.com/223049/diff/17/1010#newcode2504
gtfsscheduleviewer/files/transit_editor.js:2504: Stop.prototype.onModify =
function() {
On 2010/03/01 06:46:49, baiming wrote:
> Ditto.

Done.

[baiming, 2010-03-01 07:10:47]

LGTM

[weiliu, 2010-03-01 07:11:48]

LGTM

On Mon, Mar 1, 2010 at 3:10 PM, <baiming@google.com> wrote:

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



-- 
Best Regards,
Wei Liu
86-10-62503256(o)

[calidion, 2010-03-01 07:12:45]

updated to rev 55.

2010/3/1 Wei Liu <weiliu@google.com>

> LGTM
>
>
> On Mon, Mar 1, 2010 at 3:10 PM, <baiming@google.com> wrote:
>
>> LGTM
>>
>>
>> http://codereview.appspot.com/223049/show
>>
>
>
>
> --
> Best Regards,
> Wei Liu
> 86-10-62503256(o)
>