Issue 627041: enabled deleting stops from patterns

Can't Edit
Can't Publish+Mail
Start Review

Created:
14 years, 1 month ago by calidion

Modified:
7 years, 4 months ago

Reviewers:
xan, weiliu, baiming, leio.chen, lychen, qiaojian

CC:
xinxing_google.com, lishuangfeng_gmail.com

Base URL:
http://scheduleeditor.googlecode.com/svn/trunk/python/

Visibility:
Public.

Patch Set 2 : updated python code #

Total comments: 23

Patch Set 3 : fixed a bug on removing mid stops #

Patch Set 4 : added comments #

Patch Set 6 : renamed functions #

Patch Set 7 : updated #

Created: 14 years, 1 month ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+235 lines, -17 lines)			Patch
M	gtfsscheduleviewer/files/transit_editor.js	View	1 2 3 4	16 chunks	+190 lines, -16 lines	0 comments	Download
M	schedule_editor.py	View	1 2 3 4 5 6	2 chunks	+13 lines, -1 line	0 comments	Download
M	transitfeed_editor.py	View	1 2 3 4 5	2 chunks	+32 lines, -0 lines	0 comments	Download

Messages

Total messages: 19

Expand All Messages | Collapse All Messages

calidion

enabled deleting stops from patterns server url: http://211.100.227.25:8765/

14 years, 1 month ago (2010-03-18 10:06:49 UTC) #1

weiliu

http://codereview.appspot.com/627041/diff/1/3 File transitfeed_editor.py (right): http://codereview.appspot.com/627041/diff/1/3#newcode116 transitfeed_editor.py:116: """Delete stoptime from certain pattern""" Given the stop_id, stop_sequence ...

14 years, 1 month ago (2010-03-18 14:18:50 UTC) #2

baiming

Your demo seems to be down. On Thu, Mar 18, 2010 at 6:06 PM, <calidion@gmail.com> ...

14 years, 1 month ago (2010-03-19 02:16:44 UTC) #3

calidion

it is ok now. 2010/3/19 Ming Bai 白明 <baiming@google.com> > Your demo seems to be ...

14 years, 1 month ago (2010-03-19 02:24:40 UTC) #4

calidion

updated python code http://codereview.appspot.com/627041/diff/1/3 File transitfeed_editor.py (right): http://codereview.appspot.com/627041/diff/1/3#newcode116 transitfeed_editor.py:116: """Delete stoptime from certain pattern""" On ...

14 years, 1 month ago (2010-03-19 02:25:25 UTC) #5

weiliu

http://codereview.appspot.com/627041/diff/6001/7002 File transitfeed_editor.py (right): http://codereview.appspot.com/627041/diff/6001/7002#newcode528 transitfeed_editor.py:528: def DeletePattern(self, patternId, sequence, stopId, schedule): Rename it to ...

14 years, 1 month ago (2010-03-19 02:28:11 UTC) #6

baiming

Wenxin, As we are supporting the stop deletion, did you carefully consider the garbage collection ...

14 years, 1 month ago (2010-03-19 04:00:23 UTC) #7

baiming

On Fri, Mar 19, 2010 at 12:00 PM, <baiming@google.com> wrote: > Wenxin, > > As ...

14 years, 1 month ago (2010-03-19 04:01:18 UTC) #8

calidion

fixed a bug on remove mid stops. http://codereview.appspot.com/627041/diff/6001/7001 File gtfsscheduleviewer/files/transit_editor.js (right): http://codereview.appspot.com/627041/diff/6001/7001#newcode1673 gtfsscheduleviewer/files/transit_editor.js:1673: * @param ...

14 years, 1 month ago (2010-03-19 07:39:40 UTC) #9

fixed a bug on remove mid stops.

http://codereview.appspot.com/627041/diff/6001/7001
File gtfsscheduleviewer/files/transit_editor.js (right):

http://codereview.appspot.com/627041/diff/6001/7001#newcode1673
gtfsscheduleviewer/files/transit_editor.js:1673: * @param {Stop} instop Stop to
be determined
On 2010/03/19 04:00:23, baiming wrote:
> instop is not a good parameter name. It is a little misleading.
> I think "stop" is enough.

Done.

http://codereview.appspot.com/627041/diff/6001/7001#newcode2083
gtfsscheduleviewer/files/transit_editor.js:2083: * @param {Boolean} isSet Set
stoptime tag or not
On 2010/03/19 04:00:23, baiming wrote:
> isSet doesn't tell what is to set.
> And what does stoptime tag mean?
> 
> Please rephrase the description to make it clearer.

Done.

http://codereview.appspot.com/627041/diff/6001/7001#newcode2085
gtfsscheduleviewer/files/transit_editor.js:2085: Trip.prototype.onDeleteStop =
function(stop, isSet) {
On 2010/03/19 04:00:23, baiming wrote:
> It isn't a handler, is it?
> Use verbal function name, such as "deleteStop".

Yes, it is.

http://codereview.appspot.com/627041/diff/6001/7001#newcode2101
gtfsscheduleviewer/files/transit_editor.js:2101: fromLeg.getGDirections();
On 2010/03/19 04:00:23, baiming wrote:
> You also need to set this.stops[i + 1].fromLeg[shape.id] = null.

Done.

http://codereview.appspot.com/627041/diff/6001/7001#newcode2117
gtfsscheduleviewer/files/transit_editor.js:2117: Trip.prototype.deleteStoptime =
function(stoptime) {
On 2010/03/19 04:00:23, baiming wrote:
> Add comments.

Done.

http://codereview.appspot.com/627041/diff/6001/7001#newcode2120
gtfsscheduleviewer/files/transit_editor.js:2120: for (var j = 0; j <
patternDeleteList.length; j++) {
On 2010/03/19 04:00:23, baiming wrote:
> Can you write a helper function, like deleteFromArray, for deleting an element
> from an array?

Done.

http://codereview.appspot.com/627041/diff/6001/7001#newcode2492
gtfsscheduleviewer/files/transit_editor.js:2492: Shape.prototype.DeleteStop =
function(stop) {
On 2010/03/19 04:00:23, baiming wrote:
> Member function should start with an lower-case letter.
> s/DeleteStop/deleteStop

seems of no use right now. removed.

http://codereview.appspot.com/627041/diff/6001/7001#newcode3328
gtfsscheduleviewer/files/transit_editor.js:3328: if(confirm('Sure to Delete
stop? It is not revertable.')) {
On 2010/03/19 04:00:23, baiming wrote:
> Space between if and (

Done.

http://codereview.appspot.com/627041/diff/6001/7001#newcode3801
gtfsscheduleviewer/files/transit_editor.js:3801: 
On 2010/03/19 04:00:23, baiming wrote:
> Delete the blank line.

Done.

baiming

http://codereview.appspot.com/627041/diff/6001/7001 File gtfsscheduleviewer/files/transit_editor.js (right): http://codereview.appspot.com/627041/diff/6001/7001#newcode2085 gtfsscheduleviewer/files/transit_editor.js:2085: Trip.prototype.onDeleteStop = function(stop, isSet) { On 2010/03/19 07:39:40, calidion ...

14 years, 1 month ago (2010-03-19 09:47:51 UTC) #10

calidion

Function onDeleteNodeClick is a dom event handler, while onDeleteStop is a real gtfs stop deleting ...

14 years, 1 month ago (2010-03-19 09:59:43 UTC) #11

baiming

2010/3/19 李白，字一日 <calidion@gmail.com> > Function onDeleteNodeClick is a dom event handler, > while onDeleteStop is ...

14 years, 1 month ago (2010-03-19 10:10:46 UTC) #12

calidion

Firstly, before deleting the stop, we need a confirm from the user. Secondly, we can ...

14 years, 1 month ago (2010-03-19 11:46:00 UTC) #13

Firstly, before deleting the stop, we need a confirm from the user.
Secondly, we can not bind the trip to stop previously to handle the click
event of the deleting node.
for the stop is shared with all the trips, and that is why the Trip.current
is stand here for handling the action.
so onDeleteStop is the real event handler of trip. while the
onDeleteNodeClick is a bridge to bind the stop event to current trip, and
give a confirmation to the end user to make sure he/she really wants to do
it.


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

>
>
> 2010/3/19 李白，字一日 <calidion@gmail.com>
>
> Function onDeleteNodeClick is a dom event handler,
>> while onDeleteStop is a real gtfs stop deleting handler.
>> they are handlers of different types.
>>
>
> Please think about:
> Who fires the GTFS stop deleting event?
> Who listens that event?
>
> In onDeleteNodeClick, the stop directly calls Trip.current to do deletion.
> No event here, right?
>
> If you really want to make it pure event-driven,  trigger a "delete" event
> in onDeleteNodeClick and let Trip.current listen to stop's "delete" event.
>
>
>> though onDeleteStop is not a direct handler, it is the handler for
>> deleting stop when the deleteNode is clicked and permission is confirmed by
>> the user.
>>
>>
>> 2010/3/19 <baiming@google.com>
>>
>>
>>> http://codereview.appspot.com/627041/diff/6001/7001
>>> File gtfsscheduleviewer/files/transit_editor.js (right):
>>>
>>> http://codereview.appspot.com/627041/diff/6001/7001#newcode2085
>>> gtfsscheduleviewer/files/transit_editor.js:2085:
>>> Trip.prototype.onDeleteStop = function(stop, isSet) {
>>> On 2010/03/19 07:39:40, calidion wrote:
>>>
>>>> On 2010/03/19 04:00:23, baiming wrote:
>>>> > It isn't a handler, is it?
>>>> > Use verbal function name, such as "deleteStop".
>>>>
>>>
>>>  Yes, it is.
>>>>
>>>
>>>
>>>
>>> I don't think so. Is "deleteStop" an event and "onDeleteStop" the
>>> handler of that event?
>>>
>>> Actually, the real event is "click on the delete node", and handler is
>>> "onDeleteNodeClick". And in the handler, the trip does the deletion by
>>> calling this function.
>>>
>>>
>>> http://codereview.appspot.com/627041/show
>>>
>>
>>
>
>
> --
> Ming Bai 白明
> Software Engineer
> Google Inc.
> Tel(Office): 86 (10) 6250-3361
> Tel(Cell): +86-159-0153-4908
> Email: baiming@google.com
>

baiming

well, check it in if you insist on the point. 2010/3/19 李白，字一日 <calidion@gmail.com> > Firstly, ...

14 years, 1 month ago (2010-03-21 13:51:15 UTC) #14

well, check it in if you insist on the point.

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

> Firstly, before deleting the stop, we need a confirm from the user.
> Secondly, we can not bind the trip to stop previously to handle the click
> event of the deleting node.
> for the stop is shared with all the trips, and that is why the Trip.current
> is stand here for handling the action.
> so onDeleteStop is the real event handler of trip. while the
> onDeleteNodeClick is a bridge to bind the stop event to current trip, and
> give a confirmation to the end user to make sure he/she really wants to do
> it.
>
>
> 在 2010年3月19日 下午6:10，Ming Bai 白明 <baiming@google.com>写道：
>
>
>>
>> 2010/3/19 李白，字一日 <calidion@gmail.com>
>>
>> Function onDeleteNodeClick is a dom event handler,
>>> while onDeleteStop is a real gtfs stop deleting handler.
>>> they are handlers of different types.
>>>
>>
>> Please think about:
>> Who fires the GTFS stop deleting event?
>> Who listens that event?
>>
>> In onDeleteNodeClick, the stop directly calls Trip.current to do deletion.
>> No event here, right?
>>
>> If you really want to make it pure event-driven,  trigger a "delete" event
>> in onDeleteNodeClick and let Trip.current listen to stop's "delete" event.
>>
>>
>>> though onDeleteStop is not a direct handler, it is the handler for
>>> deleting stop when the deleteNode is clicked and permission is confirmed by
>>> the user.
>>>
>>>
>>> 2010/3/19 <baiming@google.com>
>>>
>>>
>>>> http://codereview.appspot.com/627041/diff/6001/7001
>>>> File gtfsscheduleviewer/files/transit_editor.js (right):
>>>>
>>>> http://codereview.appspot.com/627041/diff/6001/7001#newcode2085
>>>> gtfsscheduleviewer/files/transit_editor.js:2085:
>>>> Trip.prototype.onDeleteStop = function(stop, isSet) {
>>>> On 2010/03/19 07:39:40, calidion wrote:
>>>>
>>>>> On 2010/03/19 04:00:23, baiming wrote:
>>>>> > It isn't a handler, is it?
>>>>> > Use verbal function name, such as "deleteStop".
>>>>>
>>>>
>>>>  Yes, it is.
>>>>>
>>>>
>>>>
>>>>
>>>> I don't think so. Is "deleteStop" an event and "onDeleteStop" the
>>>> handler of that event?
>>>>
>>>> Actually, the real event is "click on the delete node", and handler is
>>>> "onDeleteNodeClick". And in the handler, the trip does the deletion by
>>>> calling this function.
>>>>
>>>>
>>>> http://codereview.appspot.com/627041/show
>>>>
>>>
>>>
>>
>>
>> --
>> Ming Bai 白明
>> Software Engineer
>> Google Inc.
>> Tel(Office): 86 (10) 6250-3361
>> Tel(Cell): +86-159-0153-4908
>> Email: baiming@google.com
>>
>
>


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

baiming

LGTM 2010/3/21 Ming Bai 白明 <baiming@google.com>: > well, check it in if you insist on ...

14 years, 1 month ago (2010-03-22 02:17:08 UTC) #15

LGTM

2010/3/21 Ming Bai 白明 <baiming@google.com>:
> well, check it in if you insist on the point.
> 2010/3/19 李白，字一日 <calidion@gmail.com>
>>
>> Firstly, before deleting the stop, we need a confirm from the user.
>> Secondly, we can not bind the trip to stop previously to handle the click
>> event of the deleting node.
>> for the stop is shared with all the trips, and that is why the
>> Trip.current is stand here for handling the action.
>> so onDeleteStop is the real event handler of trip. while the
>> onDeleteNodeClick is a bridge to bind the stop event to current trip, and
>> give a confirmation to the end user to make sure he/she really wants to do
>> it.
>>
>> 在 2010年3月19日 下午6:10，Ming Bai 白明 <baiming@google.com>写道：
>>>
>>>
>>> 2010/3/19 李白，字一日 <calidion@gmail.com>
>>>>
>>>> Function onDeleteNodeClick is a dom event handler,
>>>> while onDeleteStop is a real gtfs stop deleting handler.
>>>> they are handlers of different types.
>>>
>>> Please think about:
>>> Who fires the GTFS stop deleting event?
>>> Who listens that event?
>>> In onDeleteNodeClick, the stop directly calls Trip.current to do
>>> deletion. No event here, right?
>>> If you really want to make it pure event-driven,  trigger a "delete"
>>> event in onDeleteNodeClick and let Trip.current listen to stop's "delete"
>>> event.
>>>
>>>>
>>>> though onDeleteStop is not a direct handler, it is the handler for
>>>> deleting stop when the deleteNode is clicked and permission is confirmed by
>>>> the user.
>>>>
>>>> 2010/3/19 <baiming@google.com>
>>>>>
>>>>> http://codereview.appspot.com/627041/diff/6001/7001
>>>>> File gtfsscheduleviewer/files/transit_editor.js (right):
>>>>>
>>>>> http://codereview.appspot.com/627041/diff/6001/7001#newcode2085
>>>>> gtfsscheduleviewer/files/transit_editor.js:2085:
>>>>> Trip.prototype.onDeleteStop = function(stop, isSet) {
>>>>> On 2010/03/19 07:39:40, calidion wrote:
>>>>>>
>>>>>> On 2010/03/19 04:00:23, baiming wrote:
>>>>>> > It isn't a handler, is it?
>>>>>> > Use verbal function name, such as "deleteStop".
>>>>>
>>>>>> Yes, it is.
>>>>>
>>>>>
>>>>>
>>>>> I don't think so. Is "deleteStop" an event and "onDeleteStop" the
>>>>> handler of that event?
>>>>>
>>>>> Actually, the real event is "click on the delete node", and handler is
>>>>> "onDeleteNodeClick". And in the handler, the trip does the deletion by
>>>>> calling this function.
>>>>>
>>>>> http://codereview.appspot.com/627041/show
>>>>
>>>
>>>
>>>
>>> --
>>> Ming Bai 白明
>>> Software Engineer
>>> Google Inc.
>>> Tel(Office): 86 (10) 6250-3361
>>> Tel(Cell): +86-159-0153-4908
>>> Email: baiming@google.com
>>
>
>
>
> --
> Ming Bai 白明
> Software Engineer
> Google Inc.
> Tel(Office): 86 (10) 6250-3361
> Tel(Cell): +86-159-0153-4908
> Email: baiming@google.com
>



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

weiliu

Ping? Seems you forgot updating the following... Please submit after you fix them. On Fri, ...

14 years, 1 month ago (2010-03-22 02:20:21 UTC) #16

calidion

updated. http://codereview.appspot.com/627041/diff/6001/7002 File transitfeed_editor.py (right): http://codereview.appspot.com/627041/diff/6001/7002#newcode528 transitfeed_editor.py:528: def DeletePattern(self, patternId, sequence, stopId, schedule): On 2010/03/19 ...

14 years, 1 month ago (2010-03-22 02:45:51 UTC) #17

weiliu

LGTM On Mon, Mar 22, 2010 at 10:45 AM, <calidion@gmail.com> wrote: > updated. > > ...

14 years, 1 month ago (2010-03-22 02:47:16 UTC) #18

calidion

14 years, 1 month ago (2010-03-22 02:56:33 UTC) #19

update to rev 59

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

> LGTM
>
>
> On Mon, Mar 22, 2010 at 10:45 AM, <calidion@gmail.com> wrote:
>
>> updated.
>>
>>
>>
>> http://codereview.appspot.com/627041/diff/6001/7002
>> File transitfeed_editor.py (right):
>>
>> http://codereview.appspot.com/627041/diff/6001/7002#newcode528
>> transitfeed_editor.py:528: def DeletePattern(self, patternId, sequence,
>> stopId, schedule):
>> On 2010/03/19 02:28:11, weiliu wrote:
>>
>>> Rename it to "DeleteStopTimeFromPattern"
>>>
>>
>> Done.
>>
>>
>> http://codereview.appspot.com/627041/diff/6001/7002#newcode535
>> transitfeed_editor.py:535: db.DeleteTripStoptime(sequence, stopId,
>> tripIds)
>> On 2010/03/19 02:28:11, weiliu wrote:
>>
>>> Rename it to "DeleteStopTimeFromTrip"
>>>
>>
>> Done.
>>
>>
>> http://codereview.appspot.com/627041/show
>>
>
>
>
> --
> Best Regards,
> Wei Liu
> 86-10-62503256(o)
>

Expand All Messages | Collapse All Messages