Issue 194123: Enable edition of Legs
Patch Set 1 #
Total comments: 58
Patch Set 2 : updated #
Total comments: 2
Patch Set 3 : updated #Patch Set 4 : updated #
MessagesTotal messages: 10
1. import getDirPoly to easy to way of GDirections. 2. add switcher to enable edition on Legs 3. show the drag marker for legs editable demo: http://211.100.227.25:8765/
Sign in to reply to this message.
you should be patient to view the demo, because the server is really in low bandwidth. and a new resource mm_20_red.png is added, which is the image for draggable marker. 2010/1/28 <calidion@gmail.com> > Reviewers: lychen, xan_google.com, weiliu, leio.chen, baiming, qiaojian, > > Message: > 1. import getDirPoly to easy to way of GDirections. > 2. add switcher to enable edition on Legs > 3. show the drag marker for legs editable > > demo: > http://211.100.227.25:8765/ > > > > Please review this at http://codereview.appspot.com/194123/show > > Affected files: > A gtfsscheduleviewer/files/mm_20_red.png > M gtfsscheduleviewer/files/transit_editor.js > > >
Sign in to reply to this message.
Thanks for bring a demo, it will be better to write a description about how to play with the new feature in this demo, something like "you can edit legs by xxx, and you can see drag marker yyy ..." 2010/1/28 李白，字一日 <calidion@gmail.com> > you should be patient to view the demo, because the server is really in low > bandwidth. > > and a new resource mm_20_red.png is added, which is the image for draggable > marker. > > 2010/1/28 <calidion@gmail.com> > > Reviewers: lychen, xan_google.com, weiliu, leio.chen, baiming, qiaojian, >> >> Message: >> 1. import getDirPoly to easy to way of GDirections. >> 2. add switcher to enable edition on Legs >> 3. show the drag marker for legs editable >> >> demo: >> http://211.100.227.25:8765/ >> >> >> >> Please review this at http://codereview.appspot.com/194123/show >> >> Affected files: >> A gtfsscheduleviewer/files/mm_20_red.png >> M gtfsscheduleviewer/files/transit_editor.js >> >> >> >
Sign in to reply to this message.
Sorry for not attaching the link. Most previous uploads were bound to demo too after the server had started to work. so for each upload issued you can turn to the url for a preview. in this upload, we've make the leg possible to edit. 1. check the checkbox to make the leg editable 2. when the mouse is on the leg you would like to edit, the drag marker will show up if you paused for a while 3. now you can click the polyline to execute a ReGDirections or drag the marker to place a middle point for GDirections. 4. both actions will result in the color change of the leg. 2010/1/28 Liyong Chen <lychen@google.com> > Thanks for bring a demo, it will be better to write a description about how > to play with the new feature in this demo, something like "you can edit legs > by xxx, and you can see drag marker yyy ..." > > 2010/1/28 李白，字一日 <calidion@gmail.com> > > you should be patient to view the demo, because the server is really in low >> bandwidth. >> >> and a new resource mm_20_red.png is added, which is the image for >> draggable marker. >> >> 2010/1/28 <calidion@gmail.com> >> >> Reviewers: lychen, xan_google.com, weiliu, leio.chen, baiming, qiaojian, >>> >>> Message: >>> 1. import getDirPoly to easy to way of GDirections. >>> 2. add switcher to enable edition on Legs >>> 3. show the drag marker for legs editable >>> >>> demo: >>> http://211.100.227.25:8765/ >>> >>> >>> >>> Please review this at http://codereview.appspot.com/194123/show >>> >>> Affected files: >>> A gtfsscheduleviewer/files/mm_20_red.png >>> M gtfsscheduleviewer/files/transit_editor.js >>> >>> >>> >> >
Sign in to reply to this message.
http://codereview.appspot.com/194123/diff/1/2 File gtfsscheduleviewer/files/transit_editor.js (right): http://codereview.appspot.com/194123/diff/1/2#newcode18 gtfsscheduleviewer/files/transit_editor.js:18: * Get a poly by feed a query Rephrase it to: Load (or Get) the driving direction's polyline with the given direction query, and invokes the callback when direction is loaded. http://codereview.appspot.com/194123/diff/1/2#newcode20 gtfsscheduleviewer/files/transit_editor.js:20: * @param {Object} self Pointer of this to callback In other functions like this, the description of "self" is "The pointer to this of the callback". http://codereview.appspot.com/194123/diff/1/2#newcode21 gtfsscheduleviewer/files/transit_editor.js:21: * @param {Function} callback Function invoked at the end of the GDirections Function invoked after the direction is loaded. http://codereview.appspot.com/194123/diff/1/2#newcode23 gtfsscheduleviewer/files/transit_editor.js:23: var getDirPoly = function(query, self, callback) { Can you rename it to getDirPolyline? "Poly" can refer to too many things, not necessarily means polyline. REMEMBER: we should make the function/class/parameter names meaningful to let readers can understand them easily. http://codereview.appspot.com/194123/diff/1/2#newcode26 gtfsscheduleviewer/files/transit_editor.js:26: var pol = this.getPolyline(); What does "this" refer to? Is is "dir"? I can't understand why "this" can access .getPolyline. http://codereview.appspot.com/194123/diff/1/2#newcode27 gtfsscheduleviewer/files/transit_editor.js:27: if (callback) callback.call(self, pol); We shouldn't check the existence of "callback", because it is not an optional parameter. It must be there all the time. http://codereview.appspot.com/194123/diff/1/2#newcode105 gtfsscheduleviewer/files/transit_editor.js:105: var legEditable = false; Should it be a global variable, or just a field of Trip because the innerToolbar is bound to trip? Will it change when switching from trips? http://codereview.appspot.com/194123/diff/1/2#newcode1419 gtfsscheduleviewer/files/transit_editor.js:1419: var lat = Math.abs(points[i].lat - latlng.lat()); I remembered I asked you to use GLatLng.distanceFrom to calculate the distance? http://codereview.appspot.com/194123/diff/1/2#newcode1470 gtfsscheduleviewer/files/transit_editor.js:1470: end = this.getNearestPoint(this.metaPoints, start, this.stops[i].getLatLng()); Just happened to notice this line length problem. http://codereview.appspot.com/194123/diff/1/2#newcode1557 gtfsscheduleviewer/files/transit_editor.js:1557: Leg.dragMarker = null; Shouldn't it be private? http://codereview.appspot.com/194123/diff/1/2#newcode1605 gtfsscheduleviewer/files/transit_editor.js:1605: Leg.prototype.bindEvents = function() { Shouldn't it be private? I see lots of functions/variables should've been private, but they are not now. Can you give them a check, in this CL or in a separate cleanup CL? http://codereview.appspot.com/194123/diff/1/2#newcode1606 gtfsscheduleviewer/files/transit_editor.js:1606: if(this.polyline){ Space between if and ( http://codereview.appspot.com/194123/diff/1/2#newcode1606 gtfsscheduleviewer/files/transit_editor.js:1606: if(this.polyline){ In which cases this.polyline is null or undefined? http://codereview.appspot.com/194123/diff/1/2#newcode1617 gtfsscheduleviewer/files/transit_editor.js:1617: if (!legEditable) return; Please break this line to if (!legEditable) return; for better readability. http://codereview.appspot.com/194123/diff/1/2#newcode1618 gtfsscheduleviewer/files/transit_editor.js:1618: if (Leg.dragging) return; Ditto. http://codereview.appspot.com/194123/diff/1/2#newcode1622 gtfsscheduleviewer/files/transit_editor.js:1622: a.innerHTML = "ReGDirections"; What does "ReGDirections" mean? It looks not friendly to (human) users. How about changing it to "Get driving direction"? http://codereview.appspot.com/194123/diff/1/2#newcode1631 gtfsscheduleviewer/files/transit_editor.js:1631: Leg.prototype.onReGDirections = function() { s/onReGDirections/onGetGDirections http://codereview.appspot.com/194123/diff/1/2#newcode1633 gtfsscheduleviewer/files/transit_editor.js:1633: this.reGDirections(); s/reGDirections/getGDirections http://codereview.appspot.com/194123/diff/1/2#newcode1686 gtfsscheduleviewer/files/transit_editor.js:1686: self.reGDirections(point); So we can't add more than one mid waypoints (拐点) for getting GDirections? Did the old demo version support that? I am not very sure if we should provide this functionality, can you check it with Xinxing please? http://codereview.appspot.com/194123/diff/1/2#newcode1696 gtfsscheduleviewer/files/transit_editor.js:1696: Leg.prototype.reGDirections = function(point) { s/reGDirections/getGDirections s/point/midPoint http://codereview.appspot.com/194123/diff/1/2#newcode1714 gtfsscheduleviewer/files/transit_editor.js:1714: Leg.prototype.onDirPoly = function(poly) { Now it doesn't handle the error condition, why? http://codereview.appspot.com/194123/diff/1/2#newcode1714 gtfsscheduleviewer/files/transit_editor.js:1714: Leg.prototype.onDirPoly = function(poly) { s/onDirPoly/onDirPolylineLoad s/poly/polyline http://codereview.appspot.com/194123/diff/1/2#newcode1737 gtfsscheduleviewer/files/transit_editor.js:1737: Drop the blank line. http://codereview.appspot.com/194123/diff/1/2#newcode1749 gtfsscheduleviewer/files/transit_editor.js:1749: return; It should be something wrong happened, right? Please log a error message. http://codereview.appspot.com/194123/diff/1/2#newcode2355 gtfsscheduleviewer/files/transit_editor.js:2355: }); Keep the indentation consistent (with above).
Sign in to reply to this message.
updated. http://codereview.appspot.com/194123/diff/1/2 File gtfsscheduleviewer/files/transit_editor.js (right): http://codereview.appspot.com/194123/diff/1/2#newcode18 gtfsscheduleviewer/files/transit_editor.js:18: * Get a poly by feed a query On 2010/02/01 04:14:21, baiming wrote: > Rephrase it to: > Load (or Get) the driving direction's polyline with the given direction query, > and invokes the callback when direction is loaded. Done. http://codereview.appspot.com/194123/diff/1/2#newcode20 gtfsscheduleviewer/files/transit_editor.js:20: * @param {Object} self Pointer of this to callback On 2010/02/01 04:14:21, baiming wrote: > In other functions like this, the description of "self" is "The pointer to this > of the callback". Done. http://codereview.appspot.com/194123/diff/1/2#newcode21 gtfsscheduleviewer/files/transit_editor.js:21: * @param {Function} callback Function invoked at the end of the GDirections On 2010/02/01 04:14:21, baiming wrote: > Function invoked after the direction is loaded. Done. http://codereview.appspot.com/194123/diff/1/2#newcode23 gtfsscheduleviewer/files/transit_editor.js:23: var getDirPoly = function(query, self, callback) { On 2010/02/01 04:14:21, baiming wrote: > Can you rename it to getDirPolyline? "Poly" can refer to too many things, not > necessarily means polyline. > > REMEMBER: we should make the function/class/parameter names meaningful to let > readers can understand them easily. Done. http://codereview.appspot.com/194123/diff/1/2#newcode26 gtfsscheduleviewer/files/transit_editor.js:26: var pol = this.getPolyline(); On 2010/02/01 04:14:21, baiming wrote: > What does "this" refer to? Is is "dir"? I can't understand why "this" can access > .getPolyline. yes, in default, the functions passed to GDirections.load will have the this pointer to the object of GDirections when the GDirections object is loaded. http://codereview.appspot.com/194123/diff/1/2#newcode27 gtfsscheduleviewer/files/transit_editor.js:27: if (callback) callback.call(self, pol); On 2010/02/01 04:14:21, baiming wrote: > We shouldn't check the existence of "callback", because it is not an optional > parameter. It must be there all the time. Done. http://codereview.appspot.com/194123/diff/1/2#newcode105 gtfsscheduleviewer/files/transit_editor.js:105: var legEditable = false; On 2010/02/01 04:14:21, baiming wrote: > Should it be a global variable, or just a field of Trip because the innerToolbar > is bound to trip? Will it change when switching from trips? it would be ok to be a global variable just like show stops in bounds, it is global. it is an editing option. http://codereview.appspot.com/194123/diff/1/2#newcode1419 gtfsscheduleviewer/files/transit_editor.js:1419: var lat = Math.abs(points[i].lat - latlng.lat()); On 2010/02/01 04:14:21, baiming wrote: > I remembered I asked you to use GLatLng.distanceFrom to calculate the distance? It is because of that the points are not GLatLng objects, it is not necessory to create new GLatLng objects just for distance calculation. http://codereview.appspot.com/194123/diff/1/2#newcode1470 gtfsscheduleviewer/files/transit_editor.js:1470: end = this.getNearestPoint(this.metaPoints, start, this.stops[i].getLatLng()); On 2010/02/01 04:14:21, baiming wrote: > Just happened to notice this line length problem. Done. http://codereview.appspot.com/194123/diff/1/2#newcode1557 gtfsscheduleviewer/files/transit_editor.js:1557: Leg.dragMarker = null; On 2010/02/01 04:14:21, baiming wrote: > Shouldn't it be private? it is readable to any Legs and any other ui related objects. http://codereview.appspot.com/194123/diff/1/2#newcode1605 gtfsscheduleviewer/files/transit_editor.js:1605: Leg.prototype.bindEvents = function() { On 2010/02/01 04:14:21, baiming wrote: > Shouldn't it be private? > > I see lots of functions/variables should've been private, but they are not now. > Can you give them a check, in this CL or in a separate cleanup CL? Done. http://codereview.appspot.com/194123/diff/1/2#newcode1606 gtfsscheduleviewer/files/transit_editor.js:1606: if(this.polyline){ On 2010/02/01 04:14:21, baiming wrote: > Space between if and ( Done. http://codereview.appspot.com/194123/diff/1/2#newcode1606 gtfsscheduleviewer/files/transit_editor.js:1606: if(this.polyline){ On 2010/02/01 04:14:21, baiming wrote: > In which cases this.polyline is null or undefined? Done. http://codereview.appspot.com/194123/diff/1/2#newcode1617 gtfsscheduleviewer/files/transit_editor.js:1617: if (!legEditable) return; On 2010/02/01 04:14:21, baiming wrote: > Please break this line to > if (!legEditable) > return; > for better readability. Done. http://codereview.appspot.com/194123/diff/1/2#newcode1618 gtfsscheduleviewer/files/transit_editor.js:1618: if (Leg.dragging) return; On 2010/02/01 04:14:21, baiming wrote: > Ditto. Done. http://codereview.appspot.com/194123/diff/1/2#newcode1622 gtfsscheduleviewer/files/transit_editor.js:1622: a.innerHTML = "ReGDirections"; On 2010/02/01 04:14:21, baiming wrote: > What does "ReGDirections" mean? It looks not friendly to (human) users. How > about changing it to "Get driving direction"? Done. http://codereview.appspot.com/194123/diff/1/2#newcode1631 gtfsscheduleviewer/files/transit_editor.js:1631: Leg.prototype.onReGDirections = function() { On 2010/02/01 04:14:21, baiming wrote: > s/onReGDirections/onGetGDirections Done. http://codereview.appspot.com/194123/diff/1/2#newcode1633 gtfsscheduleviewer/files/transit_editor.js:1633: this.reGDirections(); On 2010/02/01 04:14:21, baiming wrote: > s/reGDirections/getGDirections Done. http://codereview.appspot.com/194123/diff/1/2#newcode1686 gtfsscheduleviewer/files/transit_editor.js:1686: self.reGDirections(point); On 2010/02/01 04:14:21, baiming wrote: > So we can't add more than one mid waypoints (拐点) for getting GDirections? Did > the old demo version support that? > > I am not very sure if we should provide this functionality, can you check it > with Xinxing please? it will fail in most cases, we have tried it previously. http://codereview.appspot.com/194123/diff/1/2#newcode1696 gtfsscheduleviewer/files/transit_editor.js:1696: Leg.prototype.reGDirections = function(point) { On 2010/02/01 04:14:21, baiming wrote: > s/reGDirections/getGDirections > s/point/midPoint Done. http://codereview.appspot.com/194123/diff/1/2#newcode1714 gtfsscheduleviewer/files/transit_editor.js:1714: Leg.prototype.onDirPoly = function(poly) { On 2010/02/01 04:14:21, baiming wrote: > Now it doesn't handle the error condition, why? Done. http://codereview.appspot.com/194123/diff/1/2#newcode1714 gtfsscheduleviewer/files/transit_editor.js:1714: Leg.prototype.onDirPoly = function(poly) { On 2010/02/01 04:14:21, baiming wrote: > s/onDirPoly/onDirPolylineLoad > s/poly/polyline Done. http://codereview.appspot.com/194123/diff/1/2#newcode1737 gtfsscheduleviewer/files/transit_editor.js:1737: On 2010/02/01 04:14:21, baiming wrote: > Drop the blank line. Done. http://codereview.appspot.com/194123/diff/1/2#newcode1749 gtfsscheduleviewer/files/transit_editor.js:1749: return; On 2010/02/01 04:14:21, baiming wrote: > It should be something wrong happened, right? Please log a error message. Done. http://codereview.appspot.com/194123/diff/1/2#newcode2355 gtfsscheduleviewer/files/transit_editor.js:2355: }); On 2010/02/01 04:14:21, baiming wrote: > Keep the indentation consistent (with above). Done.
Sign in to reply to this message.
http://codereview.appspot.com/194123/diff/1/2 File gtfsscheduleviewer/files/transit_editor.js (right): http://codereview.appspot.com/194123/diff/1/2#newcode26 gtfsscheduleviewer/files/transit_editor.js:26: var pol = this.getPolyline(); On 2010/02/01 04:14:21, baiming wrote: > What does "this" refer to? Is is "dir"? I can't understand why "this" can access > .getPolyline. I see. But as it (this pointing to dir) is like an implicit behavior, maybe depending on the internal implementation. It may not be that robust, and hard to understand. I suggest you to either 1) replace this with dir or 2) use GEvent.bind(dir, 'load', dir, onDirSuccess); http://codereview.appspot.com/194123/diff/1/2#newcode1419 gtfsscheduleviewer/files/transit_editor.js:1419: var lat = Math.abs(points[i].lat - latlng.lat()); On 2010/02/01 06:09:50, calidion wrote: > On 2010/02/01 04:14:21, baiming wrote: > > I remembered I asked you to use GLatLng.distanceFrom to calculate the > distance? > It is because of that the points are not GLatLng objects, it is not necessory to > create new GLatLng objects just for distance calculation. > But this kind of calculation is not correct. At least, you must handle the boundary case of lng (two points crossing over +180 (-180) ). http://codereview.appspot.com/194123/diff/1/2#newcode1714 gtfsscheduleviewer/files/transit_editor.js:1714: Leg.prototype.onDirPoly = function(poly) { On 2010/02/01 06:09:50, calidion wrote: > On 2010/02/01 04:14:21, baiming wrote: > > Now it doesn't handle the error condition, why? > > Done. I think a warning alert is better than silent return. http://codereview.appspot.com/194123/diff/1/2#newcode2355 gtfsscheduleviewer/files/transit_editor.js:2355: }); On 2010/02/01 06:09:50, calidion wrote: > On 2010/02/01 04:14:21, baiming wrote: > > Keep the indentation consistent (with above). > > Done. I mean, keep it consistent with what you did for manager.getAgentcy(). http://codereview.appspot.com/194123/diff/1005/2001#newcode18 gtfsscheduleviewer/files/transit_editor.js:18: * Get a poly by feed a query Sorry for not making it clear. I meant you can phrase it "Load the ..." or "Get the ...".
Sign in to reply to this message.
updated http://codereview.appspot.com/194123/diff/1/2 File gtfsscheduleviewer/files/transit_editor.js (right): http://codereview.appspot.com/194123/diff/1/2#newcode26 gtfsscheduleviewer/files/transit_editor.js:26: var pol = this.getPolyline(); On 2010/02/01 06:59:21, baiming wrote: > On 2010/02/01 04:14:21, baiming wrote: > > What does "this" refer to? Is is "dir"? I can't understand why "this" can > access > > .getPolyline. > > I see. But as it (this pointing to dir) is like an implicit behavior, maybe > depending on the internal implementation. It may not be that robust, and hard to > understand. I suggest you to either > 1) replace this with dir > or > 2) use GEvent.bind(dir, 'load', dir, onDirSuccess); Done. http://codereview.appspot.com/194123/diff/1/2#newcode1419 gtfsscheduleviewer/files/transit_editor.js:1419: var lat = Math.abs(points[i].lat - latlng.lat()); On 2010/02/01 06:59:21, baiming wrote: > On 2010/02/01 06:09:50, calidion wrote: > > On 2010/02/01 04:14:21, baiming wrote: > > > I remembered I asked you to use GLatLng.distanceFrom to calculate the > > distance? > > It is because of that the points are not GLatLng objects, it is not necessory > to > > create new GLatLng objects just for distance calculation. > > > > But this kind of calculation is not correct. > At least, you must handle the boundary case of lng (two points crossing over > +180 (-180) ). Done. http://codereview.appspot.com/194123/diff/1/2#newcode1714 gtfsscheduleviewer/files/transit_editor.js:1714: Leg.prototype.onDirPoly = function(poly) { On 2010/02/01 06:59:21, baiming wrote: > On 2010/02/01 06:09:50, calidion wrote: > > On 2010/02/01 04:14:21, baiming wrote: > > > Now it doesn't handle the error condition, why? > > > > Done. > > I think a warning alert is better than silent return. Done. http://codereview.appspot.com/194123/diff/1/2#newcode2355 gtfsscheduleviewer/files/transit_editor.js:2355: }); On 2010/02/01 06:59:21, baiming wrote: > On 2010/02/01 06:09:50, calidion wrote: > > On 2010/02/01 04:14:21, baiming wrote: > > > Keep the indentation consistent (with above). > > > > Done. > > I mean, keep it consistent with what you did for manager.getAgentcy(). Done. http://codereview.appspot.com/194123/diff/1005/2001#newcode18 gtfsscheduleviewer/files/transit_editor.js:18: * Get a poly by feed a query On 2010/02/01 06:59:21, baiming wrote: > Sorry for not making it clear. I meant you can phrase it "Load the ..." or "Get > the ...". Done.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
updated to rev 51. 2010/2/1 <baiming@google.com> > LGTM > > > http://codereview.appspot.com/194123/show >
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
