Issue 323041: enabled adding stops on the client side
Patch Set 1 #
Total comments: 31
Patch Set 2 : merged full function of adding stop and updated format #
Total comments: 49
Patch Set 3 : updated #
Total comments: 6
Patch Set 4 : updated #Patch Set 5 : always save #
Total comments: 4
Patch Set 6 : updated #Patch Set 7 : update #
MessagesTotal messages: 17
1.reformat html, css, javascript with netbeans. and fixed an error on css. 2.enabled adding stops in client side. you can see a new checkbox when you click 'Stop editable', check it and you will be able to add new stop to the current shape. Sever url: http://211.100.227.25:8765/
Sign in to reply to this message.
Sorry for the delay. I'm starting the review now. Hopefully the first batch of comments will come out by the end of day. On Tue, Mar 9, 2010 at 4:18 PM, <calidion@gmail.com> wrote: > Reviewers: lychen, xan_google.com, weiliu, leio.chen, baiming, qiaojian, > > Message: > 1.reformat html, css, javascript with netbeans. and fixed an error on > css. > 2.enabled adding stops in client side. > you can see a new checkbox when you click 'Stop editable', check it > and you will be able to add new stop to the current shape. > > Sever url: http://211.100.227.25:8765/ > > > > Please review this at http://codereview.appspot.com/323041/show > > Affected files: > M gtfsscheduleviewer/files/index.html > M gtfsscheduleviewer/files/style.css > M gtfsscheduleviewer/files/transit_editor.js > > > -- Ming Bai 白明 Software Engineer Google Inc. Tel(Office): 86 (10) 6250-3361 Tel(Cell): +86-159-0153-4908 Email: baiming@google.com
Sign in to reply to this message.
thanks. 2010/3/11 Ming Bai 白明 <baiming@google.com> > Sorry for the delay. I'm starting the review now. Hopefully the first > batch of comments will come out by the end of day. > > On Tue, Mar 9, 2010 at 4:18 PM, <calidion@gmail.com> wrote: > > Reviewers: lychen, xan_google.com, weiliu, leio.chen, baiming, qiaojian, > > > > Message: > > 1.reformat html, css, javascript with netbeans. and fixed an error on > > css. > > 2.enabled adding stops in client side. > > you can see a new checkbox when you click 'Stop editable', check it > > and you will be able to add new stop to the current shape. > > > > Sever url: http://211.100.227.25:8765/ > > > > > > > > Please review this at http://codereview.appspot.com/323041/show > > > > Affected files: > > M gtfsscheduleviewer/files/index.html > > M gtfsscheduleviewer/files/style.css > > M gtfsscheduleviewer/files/transit_editor.js > > > > > > > > > > -- > Ming Bai 白明 > Software Engineer > Google Inc. > Tel(Office): 86 (10) 6250-3361 > Tel(Cell): +86-159-0153-4908 > Email: baiming@google.com >
Sign in to reply to this message.
Really sorry, I can't send out the comments today since I haven't really spent much time on it so far. :( 2010/3/11 李白，字一日 <calidion@gmail.com>: > thanks. > > 2010/3/11 Ming Bai 白明 <baiming@google.com> >> >> Sorry for the delay. I'm starting the review now. Hopefully the first >> batch of comments will come out by the end of day. >> >> On Tue, Mar 9, 2010 at 4:18 PM, <calidion@gmail.com> wrote: >> > Reviewers: lychen, xan_google.com, weiliu, leio.chen, baiming, qiaojian, >> > >> > Message: >> > 1.reformat html, css, javascript with netbeans. and fixed an error on >> > css. >> > 2.enabled adding stops in client side. >> > you can see a new checkbox when you click 'Stop editable', check it >> > and you will be able to add new stop to the current shape. >> > >> > Sever url: http://211.100.227.25:8765/ >> > >> > >> > >> > Please review this at http://codereview.appspot.com/323041/show >> > >> > Affected files: >> > M gtfsscheduleviewer/files/index.html >> > M gtfsscheduleviewer/files/style.css >> > M gtfsscheduleviewer/files/transit_editor.js >> > >> > >> > >> >> >> >> -- >> 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
Sign in to reply to this message.
Jian, Please help me to give this CL an overall look first. I will be out of office tomorrow, but don't want it pending too long. Just focusing on general coding style is enough. I will look at the rest part later. Thanks very much! 2010/3/11 Ming Bai 白明 <baiming@google.com>: > Really sorry, I can't send out the comments today since I haven't > really spent much time on it so far. :( > > 2010/3/11 李白，字一日 <calidion@gmail.com>: >> thanks. >> >> 2010/3/11 Ming Bai 白明 <baiming@google.com> >>> >>> Sorry for the delay. I'm starting the review now. Hopefully the first >>> batch of comments will come out by the end of day. >>> >>> On Tue, Mar 9, 2010 at 4:18 PM, <calidion@gmail.com> wrote: >>> > Reviewers: lychen, xan_google.com, weiliu, leio.chen, baiming, qiaojian, >>> > >>> > Message: >>> > 1.reformat html, css, javascript with netbeans. and fixed an error on >>> > css. >>> > 2.enabled adding stops in client side. >>> > you can see a new checkbox when you click 'Stop editable', check it >>> > and you will be able to add new stop to the current shape. >>> > >>> > Sever url: http://211.100.227.25:8765/ >>> > >>> > >>> > >>> > Please review this at http://codereview.appspot.com/323041/show >>> > >>> > Affected files: >>> > M gtfsscheduleviewer/files/index.html >>> > M gtfsscheduleviewer/files/style.css >>> > M gtfsscheduleviewer/files/transit_editor.js >>> > >>> > >>> > >>> >>> >>> >>> -- >>> 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
Sign in to reply to this message.
http://codereview.appspot.com/323041/diff/1/2 File gtfsscheduleviewer/files/transit_editor.js (right): http://codereview.appspot.com/323041/diff/1/2#newcode152 gtfsscheduleviewer/files/transit_editor.js:152: names[i], Indent 4 here, why change it? http://codereview.appspot.com/323041/diff/1/2#newcode169 gtfsscheduleviewer/files/transit_editor.js:169: name, Same as above. http://codereview.appspot.com/323041/diff/1/2#newcode309 gtfsscheduleviewer/files/transit_editor.js:309: + "&limit=50"; Could you use [].join() to connect string? I think it will be more efficient. http://codereview.appspot.com/323041/diff/1/2#newcode324 gtfsscheduleviewer/files/transit_editor.js:324: + '&time=0'; Indent 4, and put '+' in the first line. http://codereview.appspot.com/323041/diff/1/2#newcode511 gtfsscheduleviewer/files/transit_editor.js:511: 'Show all stops'); Indent 4 spaces. http://codereview.appspot.com/323041/diff/1/2#newcode1422 gtfsscheduleviewer/files/transit_editor.js:1422: + '] <b style="color:red">' + (idx + 1) + '</b> of ' + stopNumber Use [].join('') to connect string. http://codereview.appspot.com/323041/diff/1/2#newcode1487 gtfsscheduleviewer/files/transit_editor.js:1487: + src + '"' + strAttr + '></embed>'; Indent 4 and put '+' in the first line. http://codereview.appspot.com/323041/diff/1/2#newcode1491 gtfsscheduleviewer/files/transit_editor.js:1491: + 'to get SVG support in IE</p>'; same as above. http://codereview.appspot.com/323041/diff/1/2#newcode1498 gtfsscheduleviewer/files/transit_editor.js:1498: + '</a></p></object>'; same as above. http://codereview.appspot.com/323041/diff/1/2#newcode1804 gtfsscheduleviewer/files/transit_editor.js:1804: stoptime.onStopNameChange); Indent 4. http://codereview.appspot.com/323041/diff/1/2#newcode2001 gtfsscheduleviewer/files/transit_editor.js:2001: pos, leg.toStop.getLatLng(), idx, addMidStop); Indent 4. http://codereview.appspot.com/323041/diff/1/2#newcode2192 gtfsscheduleviewer/files/transit_editor.js:2192: if (fromStop.toLeg[this.id]) { I think you can use "else if" here. http://codereview.appspot.com/323041/diff/1/2#newcode2709 gtfsscheduleviewer/files/transit_editor.js:2709: (from.lng() + to.lng()) /2 ); Indent 4 or align with the first param. http://codereview.appspot.com/323041/diff/1/2#newcode3146 gtfsscheduleviewer/files/transit_editor.js:3146: + this.latlng.lat() + ', ' + this.latlng.lng() + ')<br>'; Use [].join() to connect string. http://codereview.appspot.com/323041/diff/1/2#newcode3595 gtfsscheduleviewer/files/transit_editor.js:3595: - bottombarHeight; Indent 4 and put '-' behind the first line. http://codereview.appspot.com/323041/diff/1/2#newcode3605 gtfsscheduleviewer/files/transit_editor.js:3605: 'Map API is not loaded!'; Indent wrong.
Sign in to reply to this message.
updated and merged full function of adding stop. now only support one stop per saving. http://codereview.appspot.com/323041/diff/1/2 File gtfsscheduleviewer/files/transit_editor.js (right): http://codereview.appspot.com/323041/diff/1/2#newcode152 gtfsscheduleviewer/files/transit_editor.js:152: names[i], On 2010/03/11 15:36:57, qiaojian wrote: > Indent 4 here, why change it? Done. http://codereview.appspot.com/323041/diff/1/2#newcode309 gtfsscheduleviewer/files/transit_editor.js:309: + "&limit=50"; On 2010/03/11 15:36:57, qiaojian wrote: > Could you use [].join() to connect string? I think it will be more efficient. Done. http://codereview.appspot.com/323041/diff/1/2#newcode324 gtfsscheduleviewer/files/transit_editor.js:324: + '&time=0'; On 2010/03/11 15:36:57, qiaojian wrote: > Indent 4, and put '+' in the first line. Done. http://codereview.appspot.com/323041/diff/1/2#newcode511 gtfsscheduleviewer/files/transit_editor.js:511: 'Show all stops'); On 2010/03/11 15:36:57, qiaojian wrote: > Indent 4 spaces. Done. http://codereview.appspot.com/323041/diff/1/2#newcode1422 gtfsscheduleviewer/files/transit_editor.js:1422: + '] <b style="color:red">' + (idx + 1) + '</b> of ' + stopNumber On 2010/03/11 15:36:57, qiaojian wrote: > Use [].join('') to connect string. prefer not to use join for the fields are isomeric from each other. http://codereview.appspot.com/323041/diff/1/2#newcode1487 gtfsscheduleviewer/files/transit_editor.js:1487: + src + '"' + strAttr + '></embed>'; On 2010/03/11 15:36:57, qiaojian wrote: > Indent 4 and put '+' in the first line. Done. http://codereview.appspot.com/323041/diff/1/2#newcode1491 gtfsscheduleviewer/files/transit_editor.js:1491: + 'to get SVG support in IE</p>'; On 2010/03/11 15:36:57, qiaojian wrote: > same as above. Done. http://codereview.appspot.com/323041/diff/1/2#newcode1498 gtfsscheduleviewer/files/transit_editor.js:1498: + '</a></p></object>'; On 2010/03/11 15:36:57, qiaojian wrote: > same as above. Done. http://codereview.appspot.com/323041/diff/1/2#newcode1804 gtfsscheduleviewer/files/transit_editor.js:1804: stoptime.onStopNameChange); On 2010/03/11 15:36:57, qiaojian wrote: > Indent 4. Done. http://codereview.appspot.com/323041/diff/1/2#newcode2001 gtfsscheduleviewer/files/transit_editor.js:2001: pos, leg.toStop.getLatLng(), idx, addMidStop); On 2010/03/11 15:36:57, qiaojian wrote: > Indent 4. Done. http://codereview.appspot.com/323041/diff/1/2#newcode2192 gtfsscheduleviewer/files/transit_editor.js:2192: if (fromStop.toLeg[this.id]) { On 2010/03/11 15:36:57, qiaojian wrote: > I think you can use "else if" here. it would be better to remain because the condition (fromStop.toLeg[this.id]) is a subset of fromStop.toLeg http://codereview.appspot.com/323041/diff/1/2#newcode2709 gtfsscheduleviewer/files/transit_editor.js:2709: (from.lng() + to.lng()) /2 ); On 2010/03/11 15:36:57, qiaojian wrote: > Indent 4 or align with the first param. Done. http://codereview.appspot.com/323041/diff/1/2#newcode3146 gtfsscheduleviewer/files/transit_editor.js:3146: + this.latlng.lat() + ', ' + this.latlng.lng() + ')<br>'; On 2010/03/11 15:36:57, qiaojian wrote: > Use [].join() to connect string. seems not necessary to use join here. http://codereview.appspot.com/323041/diff/1/2#newcode3595 gtfsscheduleviewer/files/transit_editor.js:3595: - bottombarHeight; On 2010/03/11 15:36:57, qiaojian wrote: > Indent 4 and put '-' behind the first line. Done. http://codereview.appspot.com/323041/diff/1/2#newcode3605 gtfsscheduleviewer/files/transit_editor.js:3605: 'Map API is not loaded!'; On 2010/03/11 15:36:57, qiaojian wrote: > Indent wrong. Done.
Sign in to reply to this message.
http://codereview.appspot.com/323041/diff/3002/9001 File gtfsscheduleviewer/files/transit_editor.js (right): http://codereview.appspot.com/323041/diff/3002/9001#newcode2 gtfsscheduleviewer/files/transit_editor.js:2: var INFINITELY_LARGE = 99999999999; Better to use simple word as variable name. e.g, INFINITY, or MAX_INT http://codereview.appspot.com/323041/diff/3002/9001#newcode481 gtfsscheduleviewer/files/transit_editor.js:481: InnerToolbar.prototype.onAddStopChange = function() { A bug: 1) check "stop editable" 2) check "add stop" 3) the cursor appears 4) uncheck "stop editable", which means the stop can _NOT_ be edited, including renaming, moving and adding. 5) But, "add stop" is still checked, thus the cursor is there and I can still add a new stop. http://codereview.appspot.com/323041/diff/3002/9001#newcode482 gtfsscheduleviewer/files/transit_editor.js:482: Remove this useless blank line. http://codereview.appspot.com/323041/diff/3002/9001#newcode495 gtfsscheduleviewer/files/transit_editor.js:495: var timer = setTimeout(1000, function() { Inversed argument order? http://codereview.appspot.com/323041/diff/3002/9001#newcode495 gtfsscheduleviewer/files/transit_editor.js:495: var timer = setTimeout(1000, function() { I can't understand what this timer is used for. http://codereview.appspot.com/323041/diff/3002/9001#newcode986 gtfsscheduleviewer/files/transit_editor.js:986: Route.prototype.insertPatternStop = function(patternId, stop, idx, sequence) { Add comments. http://codereview.appspot.com/323041/diff/3002/9001#newcode987 gtfsscheduleviewer/files/transit_editor.js:987: if (!this.addStops) { Better to rename it to "addedStops", or more precisely "addedPatternStops"? http://codereview.appspot.com/323041/diff/3002/9001#newcode993 gtfsscheduleviewer/files/transit_editor.js:993: this.addStops[patternId].push([stop, idx, sequence]); What's the relation and difference between idx and sequence? http://codereview.appspot.com/323041/diff/3002/9001#newcode994 gtfsscheduleviewer/files/transit_editor.js:994: } ; after } http://codereview.appspot.com/323041/diff/3002/9001#newcode1780 gtfsscheduleviewer/files/transit_editor.js:1780: for (var trip; trip = trips[i]; i++) { I don't remember clear why you use this loop style. I will ask you over phone. http://codereview.appspot.com/323041/diff/3002/9001#newcode1849 gtfsscheduleviewer/files/transit_editor.js:1849: * Trip.mainPolyline is the first polyline we can find(Must can be found) Please write comments separately for each variable. http://codereview.appspot.com/323041/diff/3002/9001#newcode1884 gtfsscheduleviewer/files/transit_editor.js:1884: Trip.prototype.onMainPolylineEnd = function(polyline) { onMainPolylineFound would be better? Up to you. http://codereview.appspot.com/323041/diff/3002/9001#newcode1898 gtfsscheduleviewer/files/transit_editor.js:1898: Trip.prototype.onSecondPolyEnd = function(polyline) { Ditto. http://codereview.appspot.com/323041/diff/3002/9001#newcode1909 gtfsscheduleviewer/files/transit_editor.js:1909: * Create a new object of StopTime by given stop and sequence ... with given stop and ... http://codereview.appspot.com/323041/diff/3002/9001#newcode1926 gtfsscheduleviewer/files/transit_editor.js:1926: stoptime.stop.addListener('edit', stoptime, stoptime.onStopEdit); It looks rather similar to code in parseStoptimes_(). Since createStoptime() is already dedicated to create the entry, can you simply call it from parseStoptimes_, to avoid the duplicated code? http://codereview.appspot.com/323041/diff/3002/9001#newcode1938 gtfsscheduleviewer/files/transit_editor.js:1938: * Create a new object of Stop by given pos with given position. one suggestion: pos or position is usually used to present the meaning of "index" (position of an entry in the list). Can you use "location" or "latlng" instead? http://codereview.appspot.com/323041/diff/3002/9001#newcode1988 gtfsscheduleviewer/files/transit_editor.js:1988: * Create new Leg with given polyline and Display it s/Display/display http://codereview.appspot.com/323041/diff/3002/9001#newcode2073 gtfsscheduleviewer/files/transit_editor.js:2073: Trip.prototype.recalculateDistance = function(pos) { When will it be used? http://codereview.appspot.com/323041/diff/3002/9001#newcode2082 gtfsscheduleviewer/files/transit_editor.js:2082: Trip.prototype.addStopPosUpdate = function(pos, add) { onAddStopPosUpdate? http://codereview.appspot.com/323041/diff/3002/9001#newcode2087 gtfsscheduleviewer/files/transit_editor.js:2087: var maxDistance = 300; in what unit? meters? feet? pixels? http://codereview.appspot.com/323041/diff/3002/9001#newcode2102 gtfsscheduleviewer/files/transit_editor.js:2102: if (!leg) { Please add comments in which case leg is null http://codereview.appspot.com/323041/diff/3002/9001#newcode2226 gtfsscheduleviewer/files/transit_editor.js:2226: leg.setColor(Leg.Color.DEFAULT); Is it necessary to set color for the leg which is only for calculating the distance? http://codereview.appspot.com/323041/diff/3002/9001#newcode2227 gtfsscheduleviewer/files/transit_editor.js:2227: dist = leg.nearestDistance(pos); Why not var dist = leg.nearestDistance(pos); that can get rid of the meaningless initialization "var dist = INFINITY_LARGE"?
Sign in to reply to this message.
updated. http://codereview.appspot.com/323041/diff/3002/9001 File gtfsscheduleviewer/files/transit_editor.js (right): http://codereview.appspot.com/323041/diff/3002/9001#newcode2 gtfsscheduleviewer/files/transit_editor.js:2: var INFINITELY_LARGE = 99999999999; On 2010/03/15 03:14:23, baiming wrote: > Better to use simple word as variable name. > e.g, INFINITY, or MAX_INT Done. http://codereview.appspot.com/323041/diff/3002/9001#newcode481 gtfsscheduleviewer/files/transit_editor.js:481: InnerToolbar.prototype.onAddStopChange = function() { On 2010/03/15 03:14:23, baiming wrote: > A bug: > 1) check "stop editable" > 2) check "add stop" > 3) the cursor appears > 4) uncheck "stop editable", which means the stop can _NOT_ be edited, including > renaming, moving and adding. > 5) But, "add stop" is still checked, thus the cursor is there and I can still > add a new stop. Done. http://codereview.appspot.com/323041/diff/3002/9001#newcode482 gtfsscheduleviewer/files/transit_editor.js:482: On 2010/03/15 03:14:23, baiming wrote: > Remove this useless blank line. Done. http://codereview.appspot.com/323041/diff/3002/9001#newcode495 gtfsscheduleviewer/files/transit_editor.js:495: var timer = setTimeout(1000, function() { On 2010/03/15 03:14:23, baiming wrote: > I can't understand what this timer is used for. it is useless now. http://codereview.appspot.com/323041/diff/3002/9001#newcode495 gtfsscheduleviewer/files/transit_editor.js:495: var timer = setTimeout(1000, function() { On 2010/03/15 03:14:23, baiming wrote: > Inversed argument order? Done. http://codereview.appspot.com/323041/diff/3002/9001#newcode993 gtfsscheduleviewer/files/transit_editor.js:993: this.addStops[patternId].push([stop, idx, sequence]); On 2010/03/15 03:14:23, baiming wrote: > What's the relation and difference between idx and sequence? there are no much relationships between idx and sequence. idx is the index of the stop to be inserted to the stoptimes array of that period. sequence is the sequence value of the stoptimes in the trip. they are of the same order, but of no direct relationship. since the idx is a positive integer starting from zero with fixed step, while the sequnce is positive integer may starting from any integer and having no fixed step . http://codereview.appspot.com/323041/diff/3002/9001#newcode1780 gtfsscheduleviewer/files/transit_editor.js:1780: for (var trip; trip = trips[i]; i++) { On 2010/03/15 03:14:23, baiming wrote: > I don't remember clear why you use this loop style. I will ask you over phone. of no useless now, removed http://codereview.appspot.com/323041/diff/3002/9001#newcode1849 gtfsscheduleviewer/files/transit_editor.js:1849: * Trip.mainPolyline is the first polyline we can find(Must can be found) On 2010/03/15 03:14:23, baiming wrote: > Please write comments separately for each variable. Done. http://codereview.appspot.com/323041/diff/3002/9001#newcode1884 gtfsscheduleviewer/files/transit_editor.js:1884: Trip.prototype.onMainPolylineEnd = function(polyline) { On 2010/03/15 03:14:23, baiming wrote: > onMainPolylineFound would be better? Up to you. Done. http://codereview.appspot.com/323041/diff/3002/9001#newcode1898 gtfsscheduleviewer/files/transit_editor.js:1898: Trip.prototype.onSecondPolyEnd = function(polyline) { On 2010/03/15 03:14:23, baiming wrote: > Ditto. Done. http://codereview.appspot.com/323041/diff/3002/9001#newcode1909 gtfsscheduleviewer/files/transit_editor.js:1909: * Create a new object of StopTime by given stop and sequence On 2010/03/15 03:14:23, baiming wrote: > ... with given stop and ... Done. http://codereview.appspot.com/323041/diff/3002/9001#newcode1938 gtfsscheduleviewer/files/transit_editor.js:1938: * Create a new object of Stop by given pos On 2010/03/15 03:14:23, baiming wrote: > with given position. > one suggestion: pos or position is usually used to present the meaning of > "index" (position of an entry in the list). Can you use "location" or "latlng" > instead? Done.
Sign in to reply to this message.
http://codereview.appspot.com/323041/diff/3002/9001 File gtfsscheduleviewer/files/transit_editor.js (right): http://codereview.appspot.com/323041/diff/3002/9001#newcode987 gtfsscheduleviewer/files/transit_editor.js:987: if (!this.addStops) { On 2010/03/15 03:14:23, baiming wrote: > Better to rename it to "addedStops", or more precisely "addedPatternStops"? Missed this one? http://codereview.appspot.com/323041/diff/3002/9001#newcode1938 gtfsscheduleviewer/files/transit_editor.js:1938: * Create a new object of Stop by given pos On 2010/03/16 06:52:13, calidion wrote: > On 2010/03/15 03:14:23, baiming wrote: > > with given position. > > one suggestion: pos or position is usually used to present the meaning of > > "index" (position of an entry in the list). Can you use "location" or "latlng" > > instead? > > Done. "Done" for changing "by" to "with". Not "done" for the "pos" problem. http://codereview.appspot.com/323041/diff/3002/9001#newcode1988 gtfsscheduleviewer/files/transit_editor.js:1988: * Create new Leg with given polyline and Display it On 2010/03/15 03:14:23, baiming wrote: > s/Display/display It seems that you haven't replied to all the comments yet. Please take a look at this and others below. http://codereview.appspot.com/323041/diff/14001/15001#newcode1481 gtfsscheduleviewer/files/transit_editor.js:1481: var svgInfo = this.getSVGTag('/ttablegraph?height=100&trip=' + this.id, var stoptime = this.stoptimes[i]; http://codereview.appspot.com/323041/diff/14001/15001#newcode1832 gtfsscheduleviewer/files/transit_editor.js:1832: * @param {Boolean} editable Editable or not This can be deleted. http://codereview.appspot.com/323041/diff/14001/15001#newcode3602 gtfsscheduleviewer/files/transit_editor.js:3602: newStop.lat = stop.lat; Wrong indentation.
Sign in to reply to this message.
updated http://codereview.appspot.com/323041/diff/3002/9001 File gtfsscheduleviewer/files/transit_editor.js (right): http://codereview.appspot.com/323041/diff/3002/9001#newcode986 gtfsscheduleviewer/files/transit_editor.js:986: Route.prototype.insertPatternStop = function(patternId, stop, idx, sequence) { On 2010/03/15 03:14:23, baiming wrote: > Add comments. Done. http://codereview.appspot.com/323041/diff/3002/9001#newcode987 gtfsscheduleviewer/files/transit_editor.js:987: if (!this.addStops) { On 2010/03/15 03:14:23, baiming wrote: > Better to rename it to "addedStops", or more precisely "addedPatternStops"? Done. http://codereview.appspot.com/323041/diff/3002/9001#newcode994 gtfsscheduleviewer/files/transit_editor.js:994: } On 2010/03/15 03:14:23, baiming wrote: > ; after } Done. http://codereview.appspot.com/323041/diff/3002/9001#newcode1938 gtfsscheduleviewer/files/transit_editor.js:1938: * Create a new object of Stop by given pos On 2010/03/16 08:38:37, baiming wrote: > On 2010/03/16 06:52:13, calidion wrote: > > On 2010/03/15 03:14:23, baiming wrote: > > > with given position. > > > one suggestion: pos or position is usually used to present the meaning of > > > "index" (position of an entry in the list). Can you use "location" or > "latlng" > > > instead? > > > > Done. > > "Done" for changing "by" to "with". > Not "done" for the "pos" problem. Done. http://codereview.appspot.com/323041/diff/3002/9001#newcode1988 gtfsscheduleviewer/files/transit_editor.js:1988: * Create new Leg with given polyline and Display it On 2010/03/15 03:14:23, baiming wrote: > s/Display/display Done. http://codereview.appspot.com/323041/diff/3002/9001#newcode2073 gtfsscheduleviewer/files/transit_editor.js:2073: Trip.prototype.recalculateDistance = function(pos) { On 2010/03/15 03:14:23, baiming wrote: > When will it be used? it is now useless, it is used to recalculate the distance for each stoptime when new stop inserted. http://codereview.appspot.com/323041/diff/3002/9001#newcode2082 gtfsscheduleviewer/files/transit_editor.js:2082: Trip.prototype.addStopPosUpdate = function(pos, add) { On 2010/03/15 03:14:23, baiming wrote: > onAddStopPosUpdate? it is not a event handler, but a function for event handler. with tag add to check the adding of the stop or not http://codereview.appspot.com/323041/diff/3002/9001#newcode2087 gtfsscheduleviewer/files/transit_editor.js:2087: var maxDistance = 300; On 2010/03/15 03:14:23, baiming wrote: > in what unit? meters? feet? pixels? in accordance with the GLatLng.distanceFrom . http://codereview.appspot.com/323041/diff/3002/9001#newcode2102 gtfsscheduleviewer/files/transit_editor.js:2102: if (!leg) { On 2010/03/15 03:14:23, baiming wrote: > Please add comments in which case leg is null Done. http://codereview.appspot.com/323041/diff/3002/9001#newcode2226 gtfsscheduleviewer/files/transit_editor.js:2226: leg.setColor(Leg.Color.DEFAULT); On 2010/03/15 03:14:23, baiming wrote: > Is it necessary to set color for the leg which is only for calculating the > distance? Yes, or we will have multiple legs having the same nearest color. http://codereview.appspot.com/323041/diff/3002/9001#newcode2227 gtfsscheduleviewer/files/transit_editor.js:2227: dist = leg.nearestDistance(pos); On 2010/03/15 03:14:23, baiming wrote: > Why not > var dist = leg.nearestDistance(pos); > that can get rid of the meaningless initialization "var dist = INFINITY_LARGE"? Done. http://codereview.appspot.com/323041/diff/14001/15001#newcode1481 gtfsscheduleviewer/files/transit_editor.js:1481: var svgInfo = this.getSVGTag('/ttablegraph?height=100&trip=' + this.id, On 2010/03/16 08:38:37, baiming wrote: > var stoptime = this.stoptimes[i]; stoptime is defined in for (var i = 0, stoptime; http://codereview.appspot.com/323041/diff/14001/15001#newcode1832 gtfsscheduleviewer/files/transit_editor.js:1832: * @param {Boolean} editable Editable or not On 2010/03/16 08:38:37, baiming wrote: > This can be deleted. Done. http://codereview.appspot.com/323041/diff/14001/15001#newcode3602 gtfsscheduleviewer/files/transit_editor.js:3602: newStop.lat = stop.lat; On 2010/03/16 08:38:37, baiming wrote: > Wrong indentation. Done.
Sign in to reply to this message.
added notification for shape on leg dragend fixed a bug on saving.
Sign in to reply to this message.
JS code LGTM.
Sign in to reply to this message.
http://codereview.appspot.com/323041/diff/19002/25005 File schedule_editor.py (right): http://codereview.appspot.com/323041/diff/19002/25005#newcode145 schedule_editor.py:145: """Add a stop to the sharp""" sharp? Should it be "shape"? http://codereview.appspot.com/323041/diff/19002/25005#newcode159 schedule_editor.py:159: stop.parent_station = '' Can you creat an EditableStop instance with the default value as '', 0 etc. instead of initializing here?
Sign in to reply to this message.
updated http://codereview.appspot.com/323041/diff/19002/25005 File schedule_editor.py (right): http://codereview.appspot.com/323041/diff/19002/25005#newcode145 schedule_editor.py:145: """Add a stop to the sharp""" On 2010/03/17 03:14:02, weiliu wrote: > sharp? Should it be "shape"? Done. http://codereview.appspot.com/323041/diff/19002/25005#newcode159 schedule_editor.py:159: stop.parent_station = '' On 2010/03/17 03:14:02, weiliu wrote: > Can you creat an EditableStop instance with the default value as '', 0 etc. > instead of initializing here? Done.
Sign in to reply to this message.
LGTM for python code. On Wed, Mar 17, 2010 at 2:15 PM, <calidion@gmail.com> wrote: > updated > > > > http://codereview.appspot.com/323041/diff/19002/25005 > File schedule_editor.py (right): > > http://codereview.appspot.com/323041/diff/19002/25005#newcode145 > schedule_editor.py:145: """Add a stop to the sharp""" > On 2010/03/17 03:14:02, weiliu wrote: > >> sharp? Should it be "shape"? >> > > Done. > > > http://codereview.appspot.com/323041/diff/19002/25005#newcode159 > schedule_editor.py:159: stop.parent_station = '' > On 2010/03/17 03:14:02, weiliu wrote: > >> Can you creat an EditableStop instance with the default value as '', 0 >> > etc. > >> instead of initializing here? >> > > Done. > > > http://codereview.appspot.com/323041/show > -- Best Regards, Wei Liu 86-10-62503256(o)
Sign in to reply to this message.
update to rev 58. 2010/3/17 Wei Liu <weiliu@google.com> > LGTM for python code. > > > On Wed, Mar 17, 2010 at 2:15 PM, <calidion@gmail.com> wrote: > >> updated >> >> >> >> http://codereview.appspot.com/323041/diff/19002/25005 >> File schedule_editor.py (right): >> >> http://codereview.appspot.com/323041/diff/19002/25005#newcode145 >> schedule_editor.py:145: """Add a stop to the sharp""" >> On 2010/03/17 03:14:02, weiliu wrote: >> >>> sharp? Should it be "shape"? >>> >> >> Done. >> >> >> http://codereview.appspot.com/323041/diff/19002/25005#newcode159 >> schedule_editor.py:159: stop.parent_station = '' >> On 2010/03/17 03:14:02, weiliu wrote: >> >>> Can you creat an EditableStop instance with the default value as '', 0 >>> >> etc. >> >>> instead of initializing here? >>> >> >> Done. >> >> >> http://codereview.appspot.com/323041/show >> > > > > -- > Best Regards, > Wei Liu > 86-10-62503256(o) >
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
