Enable synchronized saving for changes

[calidion, 2010-03-25 10:26:08]

Enable synchronized saving for changes

[calidion, 2010-03-29 09:11:56]

review reminder

2010/3/25 <calidion@gmail.com>

> Reviewers: lychen, xan_google.com, weiliu, leio.chen, baiming, qiaojian,
>
> Message:
> Enable synchronized saving for changes
>
>
>
> Please review this at http://codereview.appspot.com/750041/show
>
> Affected files:
>  M     gtfsscheduleviewer/files/transit_editor.js
>  M     schedule_editor.py
>  M     transitfeed_editor.py
>
>
>

[weiliu, 2010-03-29 09:14:39]

Since python changes are really small, I've only one question:
- would you please explain a bit on why you need to introduce
arrival_time/departure_time in UpdatePattern? Or add it to comments?

2010/3/29 李白，字一日 <calidion@gmail.com>

> review reminder
>
> 2010/3/25 <calidion@gmail.com>
>
> Reviewers: lychen, xan_google.com, weiliu, leio.chen, baiming, qiaojian,
>>
>> Message:
>> Enable synchronized saving for changes
>>
>>
>>
>> Please review this at http://codereview.appspot.com/750041/show
>>
>> Affected files:
>>  M     gtfsscheduleviewer/files/transit_editor.js
>>  M     schedule_editor.py
>>  M     transitfeed_editor.py
>>
>>
>>
>


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

[baiming, 2010-03-29 09:20:28]

Sorry for the delay. But the change looks really huge to me...
I will start the review from tomorrow morning.

2010/3/29 李白，字一日 <calidion@gmail.com>:
> review reminder
>
> 2010/3/25 <calidion@gmail.com>
>>
>> Reviewers: lychen, xan_google.com, weiliu, leio.chen, baiming, qiaojian,
>>
>> Message:
>> Enable synchronized saving for changes
>>
>>
>>
>> Please review this at http://codereview.appspot.com/750041/show
>>
>> 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

[calidion, 2010-03-29 09:35:03]

the last node must have arrival_time/departure_time for properly
initializing the pattern in server side to generate the shape desc in
function _BindTripShapeStopTimesInfo.

在 2010年3月29日 下午5:14，Wei Liu <weiliu@google.com>写道：

> Since python changes are really small, I've only one question:
> - would you please explain a bit on why you need to introduce
> arrival_time/departure_time in UpdatePattern? Or add it to comments?
>
> 2010/3/29 李白，字一日 <calidion@gmail.com>
>
> review reminder
>>
>> 2010/3/25 <calidion@gmail.com>
>>
>> Reviewers: lychen, xan_google.com, weiliu, leio.chen, baiming, qiaojian,
>>>
>>> Message:
>>> Enable synchronized saving for changes
>>>
>>>
>>>
>>> Please review this at http://codereview.appspot.com/750041/show
>>>
>>> Affected files:
>>>  M     gtfsscheduleviewer/files/transit_editor.js
>>>  M     schedule_editor.py
>>>  M     transitfeed_editor.py
>>>
>>>
>>>
>>
>
>
> --
> Best Regards,
> Wei Liu
> 86-10-62503256(o)
>

[calidion, 2010-03-29 09:35:32]

thanks.

在 2010年3月29日 下午5:20，Ming Bai 白明 <baiming@google.com>写道：

> Sorry for the delay. But the change looks really huge to me...
> I will start the review from tomorrow morning.
>
> 2010/3/29 李白，字一日 <calidion@gmail.com>:
> > review reminder
> >
> > 2010/3/25 <calidion@gmail.com>
> >>
> >> Reviewers: lychen, xan_google.com, weiliu, leio.chen, baiming,
> qiaojian,
> >>
> >> Message:
> >> Enable synchronized saving for changes
> >>
> >>
> >>
> >> Please review this at http://codereview.appspot.com/750041/show
> >>
> >> 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-03-30 03:31:09]

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

http://codereview.appspot.com/750041/diff/1/2#newcode128
gtfsscheduleviewer/files/transit_editor.js:128: * List of actions recorded
Please describe what this list stores and how will it be used. It'd be better if
you could give it an example.

For instance:
/**
  * List of all the actions which need to be saved.
  * In the list, each entry is an action (change) made by user to a GTFS object
  * with specific action type, like:
  * {
  *    type: 'stop', 
  *    act: 'modify', 
  *    obj: aStop,
  *    attr: { stoptime: aStoptime }
  * }
  *
  * The actions are mainly chronologically ordered, except for the
  * sequence-independent changes (e.g, shape modification).
  */

http://codereview.appspot.com/750041/diff/1/2#newcode131
gtfsscheduleviewer/files/transit_editor.js:131: var actionList = [];
As I mentioned in the example above, I suggest you to make the object, which the
change is made to, to a separate field in the action entry, and rename the
current "obj" to "attr" (attributes). That's because object is usually (almost?)
required in an action.

http://codereview.appspot.com/750041/diff/1/2#newcode137
gtfsscheduleviewer/files/transit_editor.js:137: var Pattern = function() {
Since you've introduced this Pattern class, I do feel we should move many
functions from other classes to be Pattern's members, such as
StopTime.prototype.onPatternStoptimeRemoved.

We don't have to do it this time, so leave a TODO to here.
i.e, TODO (wenxin): Move pattern related functions from other classes here.

http://codereview.appspot.com/750041/diff/1/2#newcode201
gtfsscheduleviewer/files/transit_editor.js:201: Stop.handleAct(act, saveStep);
Do you really think handleAct is a good name? How about handleSave, or
saveAction?
It's up to you. I don't insist.

http://codereview.appspot.com/750041/diff/1/2#newcode1576
gtfsscheduleviewer/files/transit_editor.js:1576: saveActions();
Add a TODO here to explain why trip.onSave will save all the changes, and you
remind you to change it to a global function or manager.save

http://codereview.appspot.com/750041/diff/1/2#newcode2062
gtfsscheduleviewer/files/transit_editor.js:2062: if (idx == this.stops.length) {
You should move the arrival/departure time setting to createStoptime()

http://codereview.appspot.com/750041/diff/1/2#newcode3206
gtfsscheduleviewer/files/transit_editor.js:3206: Stop.handleAdd = function(act,
cb) {
Are those functions (handleAdd, handleCopy, handleModify) really needed? Why not
calling saveForAdd/saveForCopy/saveForModify directly from Stop.handleAct?

http://codereview.appspot.com/750041/diff/1/2#newcode3971
gtfsscheduleviewer/files/transit_editor.js:3971: act: 'deletestoptime',
Why 'deletestoptime', an action belonging to Pattern, is called through
StopTime?
And why don't we have things like 'addstoptime'?

Don't need to answer them. I'm just a bit confused. Please think to a small
refactoring in future.

[weiliu, 2010-03-30 05:56:45]

As offline talking to wenxin, the logic for arrival_time/departure_time for
all trips of the same pattern is not correct and I'll review the python code
after he fixes it.

Thanks!

2010/3/29 李白，字一日 <calidion@gmail.com>

> the last node must have arrival_time/departure_time for properly
> initializing the pattern in server side to generate the shape desc in
> function _BindTripShapeStopTimesInfo.
>
> 在 2010年3月29日 下午5:14，Wei Liu <weiliu@google.com>写道：
>
> Since python changes are really small, I've only one question:
>> - would you please explain a bit on why you need to introduce
>> arrival_time/departure_time in UpdatePattern? Or add it to comments?
>>
>> 2010/3/29 李白，字一日 <calidion@gmail.com>
>>
>> review reminder
>>>
>>> 2010/3/25 <calidion@gmail.com>
>>>
>>> Reviewers: lychen, xan_google.com, weiliu, leio.chen, baiming, qiaojian,
>>>>
>>>> Message:
>>>> Enable synchronized saving for changes
>>>>
>>>>
>>>>
>>>> Please review this at http://codereview.appspot.com/750041/show
>>>>
>>>> Affected files:
>>>>  M     gtfsscheduleviewer/files/transit_editor.js
>>>>  M     schedule_editor.py
>>>>  M     transitfeed_editor.py
>>>>
>>>>
>>>>
>>>
>>
>>
>> --
>> Best Regards,
>> Wei Liu
>> 86-10-62503256(o)
>>
>
>


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

[calidion, 2010-03-30 07:56:07]

updated

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

http://codereview.appspot.com/750041/diff/1/2#newcode128
gtfsscheduleviewer/files/transit_editor.js:128: * List of actions recorded
On 2010/03/30 03:31:10, baiming wrote:
> Please describe what this list stores and how will it be used. It'd be better
if
> you could give it an example.
> 
> For instance:
> /**
>   * List of all the actions which need to be saved.
>   * In the list, each entry is an action (change) made by user to a GTFS
object
>   * with specific action type, like:
>   * {
>   *    type: 'stop', 
>   *    act: 'modify', 
>   *    obj: aStop,
>   *    attr: { stoptime: aStoptime }
>   * }
>   *
>   * The actions are mainly chronologically ordered, except for the
>   * sequence-independent changes (e.g, shape modification).
>   */

Done.

http://codereview.appspot.com/750041/diff/1/2#newcode131
gtfsscheduleviewer/files/transit_editor.js:131: var actionList = [];
On 2010/03/30 03:31:10, baiming wrote:
> As I mentioned in the example above, I suggest you to make the object, which
the
> change is made to, to a separate field in the action entry, and rename the
> current "obj" to "attr" (attributes). That's because object is usually
(almost?)
> required in an action.

we can change it when its benefit becomes more evident.

http://codereview.appspot.com/750041/diff/1/2#newcode137
gtfsscheduleviewer/files/transit_editor.js:137: var Pattern = function() {
On 2010/03/30 03:31:10, baiming wrote:
> Since you've introduced this Pattern class, I do feel we should move many
> functions from other classes to be Pattern's members, such as
> StopTime.prototype.onPatternStoptimeRemoved.
> 
> We don't have to do it this time, so leave a TODO to here.
> i.e, TODO (wenxin): Move pattern related functions from other classes here.

I would prefer to keep it in classes before we can see the clear necessity to
move them into the Pattern class. for this case, although the change will be 
pattern wide, but the action is usually originated from a stop, then the
stoptime is bound to the action,  finally the pattern is changed. so the change
to the pattern is the combination of many changes from stop / stoptime.

http://codereview.appspot.com/750041/diff/1/2#newcode201
gtfsscheduleviewer/files/transit_editor.js:201: Stop.handleAct(act, saveStep);
On 2010/03/30 03:31:10, baiming wrote:
> Do you really think handleAct is a good name? How about handleSave, or
> saveAction?
> It's up to you. I don't insist.

handle act would be more adjustable when the program grown.
if we need to support the undo/redo function, we may use it as the stack for
popping and pushing.

http://codereview.appspot.com/750041/diff/1/2#newcode1576
gtfsscheduleviewer/files/transit_editor.js:1576: saveActions();
On 2010/03/30 03:31:10, baiming wrote:
> Add a TODO here to explain why trip.onSave will save all the changes, and you
> remind you to change it to a global function or manager.save

Done.

http://codereview.appspot.com/750041/diff/1/2#newcode2062
gtfsscheduleviewer/files/transit_editor.js:2062: if (idx == this.stops.length) {
On 2010/03/30 03:31:10, baiming wrote:
> You should move the arrival/departure time setting to createStoptime()

only the stop inserted at the end of a trip needs setting.

http://codereview.appspot.com/750041/diff/1/2#newcode3206
gtfsscheduleviewer/files/transit_editor.js:3206: Stop.handleAdd = function(act,
cb) {
On 2010/03/30 03:31:10, baiming wrote:
> Are those functions (handleAdd, handleCopy, handleModify) really needed? Why
not
> calling saveForAdd/saveForCopy/saveForModify directly from Stop.handleAct?

Done.

http://codereview.appspot.com/750041/diff/1/2#newcode3971
gtfsscheduleviewer/files/transit_editor.js:3971: act: 'deletestoptime',
On 2010/03/30 03:31:10, baiming wrote:
> Why 'deletestoptime', an action belonging to Pattern, is called through
> StopTime?
> And why don't we have things like 'addstoptime'?
> 
> Don't need to answer them. I'm just a bit confused. Please think to a small
> refactoring in future.
explained offline.

[baiming, 2010-03-30 09:30:20]

I didn't look at the logic very carefully this time, partly because the code
becomes more and more complicated. It is my bad not spending enough time
reviewing the code in each iteration. But anyway, it is your code, you are
responsible for the code quality.

After the functionalities are done, I strongly suggest you to spend some time
thinking of the code structure and trying to figure out a way to make the code
easier to understand.

Thanks!

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

http://codereview.appspot.com/750041/diff/1/2#newcode137
gtfsscheduleviewer/files/transit_editor.js:137: var Pattern = function() {
On 2010/03/30 07:56:07, calidion wrote:
> On 2010/03/30 03:31:10, baiming wrote:
> > Since you've introduced this Pattern class, I do feel we should move many
> > functions from other classes to be Pattern's members, such as
> > StopTime.prototype.onPatternStoptimeRemoved.
> > 
> > We don't have to do it this time, so leave a TODO to here.
> > i.e, TODO (wenxin): Move pattern related functions from other classes here.
> 
> I would prefer to keep it in classes before we can see the clear necessity to
> move them into the Pattern class. for this case, although the change will be 
> pattern wide, but the action is usually originated from a stop, then the
> stoptime is bound to the action,  finally the pattern is changed. so the
change
> to the pattern is the combination of many changes from stop / stoptime.
> 

As we talked over phone, I think there indeed are some functions we should make
them to be Pattern's members, even if the example is not.

I don't know. If you are sure that no function should be moved here, forget it.

http://codereview.appspot.com/750041/diff/1/2#newcode2062
gtfsscheduleviewer/files/transit_editor.js:2062: if (idx == this.stops.length) {
On 2010/03/30 07:56:07, calidion wrote:
> On 2010/03/30 03:31:10, baiming wrote:
> > You should move the arrival/departure time setting to createStoptime()
> 
> only the stop inserted at the end of a trip needs setting.
> 

Even that, I think we still should move it into createStoptime(), including the
if clause.

http://codereview.appspot.com/750041/diff/15001/16001#newcode131
gtfsscheduleviewer/files/transit_editor.js:131: var actionList = [];
Leave a blank space between // and Type ...

http://codereview.appspot.com/750041/diff/15001/16001#newcode134
gtfsscheduleviewer/files/transit_editor.js:134: * A Class of pattern, handles
pattern related operations.
Ditto

http://codereview.appspot.com/750041/diff/15001/16001#newcode135
gtfsscheduleviewer/files/transit_editor.js:135: * @constructor
Do you prefer to add the comments at the rear or in the line above? Please
choose one.

http://codereview.appspot.com/750041/diff/15001/16001#newcode139
gtfsscheduleviewer/files/transit_editor.js:139: }
Don't indent this way. Change it to:
obj: {
  stoptime: ...
}

[calidion, 2010-03-30 10:03:22]

updated.

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

http://codereview.appspot.com/750041/diff/1/2#newcode2062
gtfsscheduleviewer/files/transit_editor.js:2062: if (idx == this.stops.length) {
On 2010/03/30 09:30:20, baiming wrote:
> On 2010/03/30 07:56:07, calidion wrote:
> > On 2010/03/30 03:31:10, baiming wrote:
> > > You should move the arrival/departure time setting to createStoptime()
> > 
> > only the stop inserted at the end of a trip needs setting.
> > 
> 
> Even that, I think we still should move it into createStoptime(), including
the
> if clause.

the reason we reassign the value of the arrival / departure time to the stoptime
is that the end of the trip stoptime will need a correct arrival / departure
time, not the creation of the stoptime.
we have default value for the stoptime creation now.

[weiliu, 2010-03-30 10:11:58]

http://codereview.appspot.com/750041/diff/20002/23002
File transitfeed_editor.py (right):

http://codereview.appspot.com/750041/diff/20002/23002#newcode151
transitfeed_editor.py:151: iDT = departureTime
I don't understand this part, why do you use end time string here? Shouldn't the
arrival and departure be interploated for non-edited trips? Also, iDT is not a
good naming, please make it easier for understanding.

[calidion, 2010-03-30 14:13:18]

updated

http://codereview.appspot.com/750041/diff/20002/23002
File transitfeed_editor.py (right):

http://codereview.appspot.com/750041/diff/20002/23002#newcode151
transitfeed_editor.py:151: iDT = departureTime
On 2010/03/30 10:11:58, weiliu wrote:
> I don't understand this part, why do you use end time string here? Shouldn't
the
> arrival and departure be interploated for non-edited trips? Also, iDT is not a
> good naming, please make it easier for understanding.

1. iDT stands for iDepartureTime, a temporary variable
2. departure time is equals to arrival time mostly
3. what needs to be copied is the arrival / departure time of the node inserted
at the end of the trip. so if the arrival / departure time is assigned, we
update the last one's stoptime accordingly.

fixed a bug

[weiliu, 2010-03-30 14:25:34]

On Tue, Mar 30, 2010 at 10:13 PM, <calidion@gmail.com> wrote:

> updated
>
>
>
> http://codereview.appspot.com/750041/diff/20002/23002
> File transitfeed_editor.py (right):
>
> http://codereview.appspot.com/750041/diff/20002/23002#newcode151
> transitfeed_editor.py:151: iDT = departureTime
> On 2010/03/30 10:11:58, weiliu wrote:
>
>> I don't understand this part, why do you use end time string here?
>>
> Shouldn't the
>
>> arrival and departure be interploated for non-edited trips? Also, iDT
>>
> is not a
>
>> good naming, please make it easier for understanding.
>>
>
> 1. iDT stands for iDepartureTime, a temporary variable
> 2. departure time is equals to arrival time mostly
> 3. what needs to be copied is the arrival / departure time of the node
> inserted at the end of the trip. so if the arrival / departure time is
> assigned, we update the last one's stoptime accordingly.
>

How about inserting a stop in the middle of a trip?

>
> fixed a bug
>
>
> http://codereview.appspot.com/750041/show
>



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

[calidion, 2010-03-30 14:46:32]

will have no arrival / departure value

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

>
>
> On Tue, Mar 30, 2010 at 10:13 PM, <calidion@gmail.com> wrote:
>
>> updated
>>
>>
>>
>> http://codereview.appspot.com/750041/diff/20002/23002
>> File transitfeed_editor.py (right):
>>
>> http://codereview.appspot.com/750041/diff/20002/23002#newcode151
>> transitfeed_editor.py:151: iDT = departureTime
>> On 2010/03/30 10:11:58, weiliu wrote:
>>
>>> I don't understand this part, why do you use end time string here?
>>>
>> Shouldn't the
>>
>>> arrival and departure be interploated for non-edited trips? Also, iDT
>>>
>> is not a
>>
>>> good naming, please make it easier for understanding.
>>>
>>
>> 1. iDT stands for iDepartureTime, a temporary variable
>> 2. departure time is equals to arrival time mostly
>> 3. what needs to be copied is the arrival / departure time of the node
>> inserted at the end of the trip. so if the arrival / departure time is
>> assigned, we update the last one's stoptime accordingly.
>>
>
> How about inserting a stop in the middle of a trip?
>
>>
>> fixed a bug
>>
>>
>> http://codereview.appspot.com/750041/show
>>
>
>
>
> --
> Best Regards,
> Wei Liu
> 86-10-62503256(o)
>

[weiliu, 2010-03-30 14:59:34]

So will you fix it later? If yes, please add TODO.

2010/3/30 李白，字一日 <calidion@gmail.com>

> will have no arrival / departure value
>
> 2010/3/30 Wei Liu <weiliu@google.com>
>
>
>>
>> On Tue, Mar 30, 2010 at 10:13 PM, <calidion@gmail.com> wrote:
>>
>>> updated
>>>
>>>
>>>
>>> http://codereview.appspot.com/750041/diff/20002/23002
>>> File transitfeed_editor.py (right):
>>>
>>> http://codereview.appspot.com/750041/diff/20002/23002#newcode151
>>> transitfeed_editor.py:151: iDT = departureTime
>>> On 2010/03/30 10:11:58, weiliu wrote:
>>>
>>>> I don't understand this part, why do you use end time string here?
>>>>
>>> Shouldn't the
>>>
>>>> arrival and departure be interploated for non-edited trips? Also, iDT
>>>>
>>> is not a
>>>
>>>> good naming, please make it easier for understanding.
>>>>
>>>
>>> 1. iDT stands for iDepartureTime, a temporary variable
>>> 2. departure time is equals to arrival time mostly
>>> 3. what needs to be copied is the arrival / departure time of the node
>>> inserted at the end of the trip. so if the arrival / departure time is
>>> assigned, we update the last one's stoptime accordingly.
>>>
>>
>> How about inserting a stop in the middle of a trip?
>>
>>>
>>> fixed a bug
>>>
>>>
>>> http://codereview.appspot.com/750041/show
>>>
>>
>>
>>
>> --
>> Best Regards,
>> Wei Liu
>> 86-10-62503256(o)
>>
>
>


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

[calidion, 2010-03-31 03:30:17]

OK.
And this problem will disappear  after we rearrange the structure of the
gtfs in the database.

在 2010年3月30日 下午10:59，Wei Liu <weiliu@google.com>写道：

> So will you fix it later? If yes, please add TODO.
>
> 2010/3/30 李白，字一日 <calidion@gmail.com>
>
> will have no arrival / departure value
>>
>> 2010/3/30 Wei Liu <weiliu@google.com>
>>
>>
>>>
>>> On Tue, Mar 30, 2010 at 10:13 PM, <calidion@gmail.com> wrote:
>>>
>>>> updated
>>>>
>>>>
>>>>
>>>> http://codereview.appspot.com/750041/diff/20002/23002
>>>> File transitfeed_editor.py (right):
>>>>
>>>> http://codereview.appspot.com/750041/diff/20002/23002#newcode151
>>>> transitfeed_editor.py:151: iDT = departureTime
>>>> On 2010/03/30 10:11:58, weiliu wrote:
>>>>
>>>>> I don't understand this part, why do you use end time string here?
>>>>>
>>>> Shouldn't the
>>>>
>>>>> arrival and departure be interploated for non-edited trips? Also, iDT
>>>>>
>>>> is not a
>>>>
>>>>> good naming, please make it easier for understanding.
>>>>>
>>>>
>>>> 1. iDT stands for iDepartureTime, a temporary variable
>>>> 2. departure time is equals to arrival time mostly
>>>> 3. what needs to be copied is the arrival / departure time of the node
>>>> inserted at the end of the trip. so if the arrival / departure time is
>>>> assigned, we update the last one's stoptime accordingly.
>>>>
>>>
>>> How about inserting a stop in the middle of a trip?
>>>
>>>>
>>>> fixed a bug
>>>>
>>>>
>>>> http://codereview.appspot.com/750041/show
>>>>
>>>
>>>
>>>
>>> --
>>> Best Regards,
>>> Wei Liu
>>> 86-10-62503256(o)
>>>
>>
>>
>
>
> --
> Best Regards,
> Wei Liu
> 86-10-62503256(o)
>

[baiming, 2010-03-31 03:38:02]

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

http://codereview.appspot.com/750041/diff/1/2#newcode2062
gtfsscheduleviewer/files/transit_editor.js:2062: if (idx == this.stops.length) {
On 2010/03/30 10:03:23, calidion wrote:
> On 2010/03/30 09:30:20, baiming wrote:
> > On 2010/03/30 07:56:07, calidion wrote:
> > > On 2010/03/30 03:31:10, baiming wrote:
> > > > You should move the arrival/departure time setting to createStoptime()
> > > 
> > > only the stop inserted at the end of a trip needs setting.
> > > 
> > 
> > Even that, I think we still should move it into createStoptime(), including
> the
> > if clause.
> 
> the reason we reassign the value of the arrival / departure time to the
stoptime
> is that the end of the trip stoptime will need a correct arrival / departure
> time, not the creation of the stoptime.
> we have default value for the stoptime creation now.

I'm not asking you how the value is calculated/assigned, but just curious why we
shouldn't move the code here into the function. Isn't it natural to let
createStoptime judge whether the stoptime to be created is to appended to the
rear?

http://codereview.appspot.com/750041/diff/12005/31001#newcode1570
gtfsscheduleviewer/files/transit_editor.js:1570: 
Assign the TODO to an owner, like TODO(calidion): make it ...

[weiliu, 2010-03-31 03:45:05]

http://codereview.appspot.com/750041/diff/12005/31002
File transitfeed_editor.py (right):

http://codereview.appspot.com/750041/diff/12005/31002#newcode148
transitfeed_editor.py:148: if arrivalTime or departureTime:
Where are arrivalTime/departureTime coming from? User input?

http://codereview.appspot.com/750041/diff/12005/31002#newcode153
transitfeed_editor.py:153: iDepartureTime = departureTime
I still don't see your comments on the case "if user inserts a stop in the
middle of a trip" and how you will handle that. Please explain why "this problem
will disappear after we rearrange the structure of the gtfs in the database",
what kind of rearrange?

[calidion, 2010-03-31 05:16:08]

2010/3/31 <weiliu@google.com>

>
> http://codereview.appspot.com/750041/diff/12005/31002
> File transitfeed_editor.py (right):
>
> http://codereview.appspot.com/750041/diff/12005/31002#newcode148
> transitfeed_editor.py:148: if arrivalTime or departureTime:
> Where are arrivalTime/departureTime coming from? User input?
>
it is copied from the last stop.


>
> http://codereview.appspot.com/750041/diff/12005/31002#newcode153
> transitfeed_editor.py:153: iDepartureTime = departureTime
> I still don't see your comments on the case "if user inserts a stop in
> the middle of a trip" and how you will handle that. Please explain why
>
> "this problem will disappear after we rearrange the structure of the
> gtfs in the database", what kind of rearrange?


if we separate the time and the stop sequence from stoptime.
we can cut the data dramatically from current data.
and reduce the redundancy of data


>
>
> http://codereview.appspot.com/750041/show
>

[weiliu, 2010-03-31 05:19:40]

2010/3/31 李白，字一日 <calidion@gmail.com>

>
>
> 2010/3/31 <weiliu@google.com>
>
>
>> http://codereview.appspot.com/750041/diff/12005/31002
>> File transitfeed_editor.py (right):
>>
>> http://codereview.appspot.com/750041/diff/12005/31002#newcode148
>> transitfeed_editor.py:148: if arrivalTime or departureTime:
>> Where are arrivalTime/departureTime coming from? User input?
>>
> it is copied from the last stop.
>
>
>>
>> http://codereview.appspot.com/750041/diff/12005/31002#newcode153
>> transitfeed_editor.py:153: iDepartureTime = departureTime
>> I still don't see your comments on the case "if user inserts a stop in
>> the middle of a trip" and how you will handle that. Please explain why
>>
>> "this problem will disappear after we rearrange the structure of the
>> gtfs in the database", what kind of rearrange?
>
>
> if we separate the time and the stop sequence from stoptime.
> we can cut the data dramatically from current data.
> and reduce the redundancy of data
>
>

Your plan is to handle the case "user inserts a stop in the middle of a
trip" after that, right?

>
>

>

>>
>> http://codereview.appspot.com/750041/show
>>
>
>


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

[calidion, 2010-03-31 07:22:50]

updated.

fixed a bug

http://codereview.appspot.com/750041/diff/12005/31001
File gtfsscheduleviewer/files/transit_editor.js (right):

http://codereview.appspot.com/750041/diff/12005/31001#newcode1570
gtfsscheduleviewer/files/transit_editor.js:1570: // TODO: make it a global
function
On 2010/03/31 03:38:03, baiming wrote:
> Assign the TODO to an owner, like TODO(calidion): make it ...

Done.

[weiliu, 2010-03-31 07:30:41]

Please also add comments that currently the arrival/departure_time are only
filled for start and end stations, we'll decide the right behavior for other
stations later.

On Wed, Mar 31, 2010 at 3:22 PM, <calidion@gmail.com> wrote:

> updated.
>
> fixed a bug
>
>
>
> http://codereview.appspot.com/750041/diff/12005/31001
> File gtfsscheduleviewer/files/transit_editor.js (right):
>
>
> http://codereview.appspot.com/750041/diff/12005/31001#newcode1570
> gtfsscheduleviewer/files/transit_editor.js:1570: // TODO: make it a
> global function
>
> On 2010/03/31 03:38:03, baiming wrote:
>
>> Assign the TODO to an owner, like TODO(calidion): make it ...
>>
>
> Done.
>
>
> http://codereview.appspot.com/750041/show
>



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

[calidion, 2010-03-31 08:03:49]

I have added comments in javascript code.

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

> Please also add comments that currently the arrival/departure_time are only
> filled for start and end stations, we'll decide the right behavior for other
> stations later.
>
>
> On Wed, Mar 31, 2010 at 3:22 PM, <calidion@gmail.com> wrote:
>
>> updated.
>>
>> fixed a bug
>>
>>
>>
>> http://codereview.appspot.com/750041/diff/12005/31001
>> File gtfsscheduleviewer/files/transit_editor.js (right):
>>
>>
>> http://codereview.appspot.com/750041/diff/12005/31001#newcode1570
>> gtfsscheduleviewer/files/transit_editor.js:1570: // TODO: make it a
>> global function
>>
>> On 2010/03/31 03:38:03, baiming wrote:
>>
>>> Assign the TODO to an owner, like TODO(calidion): make it ...
>>>
>>
>> Done.
>>
>>
>> http://codereview.appspot.com/750041/show
>>
>
>
>
> --
> Best Regards,
> Wei Liu
> 86-10-62503256(o)
>

[baiming, 2010-03-31 08:35:16]

LGTM

[calidion, 2010-03-31 08:53:52]

add comments

[weiliu, 2010-03-31 08:56:46]

LGTM

On Wed, Mar 31, 2010 at 4:53 PM, <calidion@gmail.com> wrote:

> add comments
>
>
> http://codereview.appspot.com/750041/show
>



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

[calidion, 2010-03-31 09:08:17]

updated to rev 67

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

> LGTM
>
>
> On Wed, Mar 31, 2010 at 4:53 PM, <calidion@gmail.com> wrote:
>
>> add comments
>>
>>
>> http://codereview.appspot.com/750041/show
>>
>
>
>
> --
> Best Regards,
> Wei Liu
> 86-10-62503256(o)
>