enabled shape saving for the trip

[calidion, 2010-02-02 06:51:01]

Enabled Shape saving after Legs changed.
1.after normal click or move on legs,
2.now you can click "Save" button to save the data be saved into server side
3. refresh the page, you will see the shape changed.

[baiming, 2010-02-03 02:59:08]

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

http://codereview.appspot.com/198056/diff/1/2#newcode1530
gtfsscheduleviewer/files/transit_editor.js:1530: Shape.prototype.serialize =
function(callback) {
I think it is a misuse of "serialize". It just stands for converting an object
to a string from which the original object can be de parsed from, but not saving
it to server.

Actually the "toSaveString" exactly does the "serialization" work. Please
consider changing their names.

http://codereview.appspot.com/198056/diff/1/2#newcode1556
gtfsscheduleviewer/files/transit_editor.js:1556: Shape.prototype.unitPostInfo =
function(point, distance) {
Rename it to pointToString. It'd be better.

http://codereview.appspot.com/198056/diff/1/2#newcode1579
gtfsscheduleviewer/files/transit_editor.js:1579:
shapePostArray.push(this.unitPostInfo(from, distance));
Will the very last point of the shape be included in the array?

http://codereview.appspot.com/198056/diff/1/2#newcode1583
gtfsscheduleviewer/files/transit_editor.js:1583: return 'shapeId=' + this.id +
'&points[]=' + shapePostArray.join('&points[]=');
Line length.

What will the eventual formatted string be?
shapeId=ID&points[]=xx&points[]=xx&points[]=xx?

Is it a code bug or just intended? Why not using things like
"&points=[xx,xx,xx]"?

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

http://codereview.appspot.com/198056/diff/1/4#newcode95
schedule_editor.py:95: points = form.getlist('points[]')
Oh I see...
You are taking the advantage of .getList() function, that's why you format the
"strange" url parameters. It's good, but can you at least get rid of the "[]"
which is of no use but increase the load?

[weiliu, 2010-02-03 04:19:39]

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

http://codereview.appspot.com/198056/diff/1/4#newcode54
schedule_editor.py:54: parsed_params = {}
Comments, please

http://codereview.appspot.com/198056/diff/1/4#newcode88
schedule_editor.py:88: fp=self.rfile,
(fp=self.rfile,

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

http://codereview.appspot.com/198056/diff/1/3#newcode125
transitfeed_editor.py:125: self._cursor.execute(sql)
Do we need to add error handling here?

[calidion, 2010-02-03 08:03:04]

updated

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

http://codereview.appspot.com/198056/diff/1/2#newcode1530
gtfsscheduleviewer/files/transit_editor.js:1530: Shape.prototype.serialize =
function(callback) {
On 2010/02/03 02:59:08, baiming wrote:
> I think it is a misuse of "serialize". It just stands for converting an object
> to a string from which the original object can be de parsed from, but not
saving
> it to server.
> 
> Actually the "toSaveString" exactly does the "serialization" work. Please
> consider changing their names.

Done.

http://codereview.appspot.com/198056/diff/1/2#newcode1556
gtfsscheduleviewer/files/transit_editor.js:1556: Shape.prototype.unitPostInfo =
function(point, distance) {
On 2010/02/03 02:59:08, baiming wrote:
> Rename it to pointToString. It'd be better.

Done.

http://codereview.appspot.com/198056/diff/1/2#newcode1579
gtfsscheduleviewer/files/transit_editor.js:1579:
shapePostArray.push(this.unitPostInfo(from, distance));
On 2010/02/03 02:59:08, baiming wrote:
> Will the very last point of the shape be included in the array?
Yes, because legs are create with at lease two points.
and we have selected the first point of the fisrt leg.
and we ignore all the first point of the leg, 
because the first point of a leg is the last point of the former leg.

http://codereview.appspot.com/198056/diff/1/2#newcode1583
gtfsscheduleviewer/files/transit_editor.js:1583: return 'shapeId=' + this.id +
'&points[]=' + shapePostArray.join('&points[]=');
On 2010/02/03 02:59:08, baiming wrote:
> Line length.
> 
> What will the eventual formatted string be?
> shapeId=ID&points[]=xx&points[]=xx&points[]=xx?
> 
> Is it a code bug or just intended? Why not using things like
> "&points=[xx,xx,xx]"?

Done.

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

http://codereview.appspot.com/198056/diff/1/4#newcode54
schedule_editor.py:54: parsed_params = {}
On 2010/02/03 04:19:39, weiliu wrote:
> Comments, please

Done.

http://codereview.appspot.com/198056/diff/1/4#newcode88
schedule_editor.py:88: fp=self.rfile,
On 2010/02/03 04:19:39, weiliu wrote:
> (fp=self.rfile,

Done.

http://codereview.appspot.com/198056/diff/1/4#newcode95
schedule_editor.py:95: points = form.getlist('points[]')
On 2010/02/03 02:59:08, baiming wrote:
> Oh I see...
> You are taking the advantage of .getList() function, that's why you format the
> "strange" url parameters. It's good, but can you at least get rid of the "[]"
> which is of no use but increase the load?

Done.

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

http://codereview.appspot.com/198056/diff/1/3#newcode125
transitfeed_editor.py:125: self._cursor.execute(sql)
On 2010/02/03 04:19:39, weiliu wrote:
> Do we need to add error handling here?

Done.

[baiming, 2010-02-03 08:16:00]

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

http://codereview.appspot.com/198056/diff/1/2#newcode1579
gtfsscheduleviewer/files/transit_editor.js:1579:
shapePostArray.push(this.unitPostInfo(from, distance));
On 2010/02/03 08:03:06, calidion wrote:
> On 2010/02/03 02:59:08, baiming wrote:
> > Will the very last point of the shape be included in the array?
> Yes, because legs are create with at lease two points.
> and we have selected the first point of the fisrt leg.
> and we ignore all the first point of the leg, 
> because the first point of a leg is the last point of the former leg.
> 

But it seems you no only drop the first point of each leg, but also drop the
last one. See what's in the code: for (var j = 1; j < count - 1; j++), shouldn't
it be j < count instead of count -1 ?

http://codereview.appspot.com/198056/diff/1004/11#newcode1554
gtfsscheduleviewer/files/transit_editor.js:1554: * @param {Number} distance The
distance from the point to the start point
Add @return

http://codereview.appspot.com/198056/diff/1004/11#newcode1564
gtfsscheduleviewer/files/transit_editor.js:1564: * @param {Function} callback A
function invoked at the
Add @return

[calidion, 2010-02-03 08:34:13]

updated

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

http://codereview.appspot.com/198056/diff/1004/11#newcode1554
gtfsscheduleviewer/files/transit_editor.js:1554: * @param {Number} distance The
distance from the point to the start point
On 2010/02/03 08:16:00, baiming wrote:
> Add @return

Done.

http://codereview.appspot.com/198056/diff/1004/11#newcode1564
gtfsscheduleviewer/files/transit_editor.js:1564: * @param {Function} callback A
function invoked at the
On 2010/02/03 08:16:00, baiming wrote:
> Add @return

Done.

[baiming, 2010-02-03 08:42:34]

LGTM

[weiliu, 2010-02-03 08:43:58]

LGTM

On Wed, Feb 3, 2010 at 4:42 PM, <baiming@google.com> wrote:

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



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

[calidion, 2010-02-03 08:47:21]

updated to rev 52.

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

> LGTM
>
>
> On Wed, Feb 3, 2010 at 4:42 PM, <baiming@google.com> wrote:
>
>> LGTM
>>
>>
>> http://codereview.appspot.com/198056/show
>>
>
>
>
> --
> Best Regards,
> Wei Liu
> 86-10-62503256(o)
>