Issue 201045: enabled stop editing
Patch Set 1 #
Total comments: 2
Patch Set 2 : updated #
Total comments: 6
Patch Set 3 : updated #
Total comments: 8
Patch Set 4 : updated #Patch Set 5 : updated #Patch Set 6 : updated #
Total comments: 2
Patch Set 7 : updated #Patch Set 8 : updated #Patch Set 9 : updated #
MessagesTotal messages: 16
1. added option for editing stops(js) 2. form data is processed in function do_POST now(py) 3. added update for Stop(py) 4. enabled moving and name editing on stop 5. disabled info fetching from server on marker clicking. demo: http://211.100.227.25:8765/
Sign in to reply to this message.
http://codereview.appspot.com/201045/diff/1/3 File transitfeed_editor.py (right): http://codereview.appspot.com/201045/diff/1/3#newcode121 transitfeed_editor.py:121: stop.stop_lon)) Missing stop_id here?
Sign in to reply to this message.
BTW, it seems to me that the stops are not editable on your demo, conflicting with your change description "enabled stop editing"? On Thu, Feb 4, 2010 at 5:13 PM, <weiliu@google.com> wrote: > > http://codereview.appspot.com/201045/diff/1/3 > File transitfeed_editor.py (right): > > http://codereview.appspot.com/201045/diff/1/3#newcode121 > transitfeed_editor.py:121: stop.stop_lon)) > Missing stop_id here? > > > http://codereview.appspot.com/201045/show > -- Best Regards, Wei Liu 86-10-62503256(o)
Sign in to reply to this message.
it is ok for me. please first click "Enable stop editin" on the toolbar 2010/2/4 Wei Liu <weiliu@google.com> > BTW, it seems to me that the stops are not editable on your > demo, conflicting with your change description "enabled stop editing"? > > > On Thu, Feb 4, 2010 at 5:13 PM, <weiliu@google.com> wrote: > >> >> http://codereview.appspot.com/201045/diff/1/3 >> File transitfeed_editor.py (right): >> >> http://codereview.appspot.com/201045/diff/1/3#newcode121 >> transitfeed_editor.py:121: stop.stop_lon)) >> Missing stop_id here? >> >> >> http://codereview.appspot.com/201045/show >> > > > > -- > Best Regards, > Wei Liu > 86-10-62503256(o) >
Sign in to reply to this message.
2010/2/4 李白，字一日 <calidion@gmail.com> > it is ok for me. > please first click "Enable stop editin" on the toolbar > It works well on my firefox now. > > 2010/2/4 Wei Liu <weiliu@google.com> > > BTW, it seems to me that the stops are not editable on your >> demo, conflicting with your change description "enabled stop editing"? >> >> >> On Thu, Feb 4, 2010 at 5:13 PM, <weiliu@google.com> wrote: >> >>> >>> http://codereview.appspot.com/201045/diff/1/3 >>> File transitfeed_editor.py (right): >>> >>> http://codereview.appspot.com/201045/diff/1/3#newcode121 >>> transitfeed_editor.py:121: stop.stop_lon)) >>> Missing stop_id here? >>> >>> >>> http://codereview.appspot.com/201045/show >>> >> >> >> >> -- >> Best Regards, >> Wei Liu >> 86-10-62503256(o) >> > > -- Best Regards, Wei Liu 86-10-62503256(o)
Sign in to reply to this message.
fixed a bug not saving to database when 'Save' button clicked. http://codereview.appspot.com/201045/diff/1/3 File transitfeed_editor.py (right): http://codereview.appspot.com/201045/diff/1/3#newcode121 transitfeed_editor.py:121: stop.stop_lon)) On 2010/02/04 09:13:50, weiliu wrote: > Missing stop_id here? Done.
Sign in to reply to this message.
LGTM for python changes. On Fri, Feb 5, 2010 at 9:37 AM, <calidion@gmail.com> wrote: > fixed a bug not saving to database when 'Save' button clicked. > > > > http://codereview.appspot.com/201045/diff/1/3 > File transitfeed_editor.py (right): > > http://codereview.appspot.com/201045/diff/1/3#newcode121 > transitfeed_editor.py:121: stop.stop_lon)) > On 2010/02/04 09:13:50, weiliu wrote: > >> Missing stop_id here? >> > > Done. > > > http://codereview.appspot.com/201045/show > -- Best Regards, Wei Liu 86-10-62503256(o)
Sign in to reply to this message.
This CL looks almost good to me, but I have got several concerns while I was
playing with the demo.
1) The routes listed in the left panel don't have theirs short names displayed,
can we add that?
2) I found that lots of requests (to fetch stops in bounds) were sent while I
was panning the map, which are actually unnecessary because users only cares
about the stops in the viewport where he stays. I suggest you to set a timeout
to avoid the request flooding. Just like this:
MapTool.prototype.onMoveEnd = function() {
if (this.fetchStopsInBoundsTimeoutId_) {
clearTimeout(this.fetchStopsInBoundsTimeoutId_);
}
var mapTool = this;
this.fetchStopsInBoundsTimeoutId_ =
setTimeout(function() {
var bounds = mapTool.map_.getBounds();
mapToll.fetchStopsInBounds(bounds);
}, 500);
}
3) About the stop moving after shape editing, see comments in code.
Thanks!
http://codereview.appspot.com/201045/diff/1010/5
File gtfsscheduleviewer/files/transit_editor.js (right):
http://codereview.appspot.com/201045/diff/1010/5#newcode109
gtfsscheduleviewer/files/transit_editor.js:109: * Enable Stop Edition
For the use of "edition", if I remembered correct, it means "a version (of book,
film, etc)", but not the noun form of edit. You'd better off to replace it with
"editing".
You can look up it in dictionary to ensure that we use it properly.
http://codereview.appspot.com/201045/diff/1010/5#newcode304
gtfsscheduleviewer/files/transit_editor.js:304: * Fetch Stop infomation from
server side by trip id
By trip id or stop id?
http://codereview.appspot.com/201045/diff/1010/5#newcode1733
gtfsscheduleviewer/files/transit_editor.js:1733: this.getGDirections();
A random thought:
If user already fixed the shape by hand, maybe that cost him a very long time,
then he move the stop a little, all his previous efforts will be lost? I think
it will be annoying.
How about that we provide two modes of the stop editing, one is the current
logic, and the other is not to re-calculate the direction but only to move the
stop? And when a user attempts to move stop after he modified the neighboring
shape, we can give him an alert and let him decide which mode he wants.
And this change will not involve many code changes.
Sign in to reply to this message.
short name will be displayed if long name is missing. http://codereview.appspot.com/201045/diff/1010/5 File gtfsscheduleviewer/files/transit_editor.js (right): http://codereview.appspot.com/201045/diff/1010/5#newcode109 gtfsscheduleviewer/files/transit_editor.js:109: * Enable Stop Edition On 2010/02/05 03:34:32, baiming wrote: > For the use of "edition", if I remembered correct, it means "a version (of book, > film, etc)", but not the noun form of edit. You'd better off to replace it with > "editing". > > You can look up it in dictionary to ensure that we use it properly. Done. http://codereview.appspot.com/201045/diff/1010/5#newcode304 gtfsscheduleviewer/files/transit_editor.js:304: * Fetch Stop infomation from server side by trip id On 2010/02/05 03:34:32, baiming wrote: > By trip id or stop id? Done. http://codereview.appspot.com/201045/diff/1010/5#newcode1733 gtfsscheduleviewer/files/transit_editor.js:1733: this.getGDirections(); On 2010/02/05 03:34:32, baiming wrote: > A random thought: > If user already fixed the shape by hand, maybe that cost him a very long time, > then he move the stop a little, all his previous efforts will be lost? I think > it will be annoying. > > How about that we provide two modes of the stop editing, one is the current > logic, and the other is not to re-calculate the direction but only to move the > stop? And when a user attempts to move stop after he modified the neighboring > shape, we can give him an alert and let him decide which mode he wants. > > And this change will not involve many code changes. Done.
Sign in to reply to this message.
On Fri, Feb 5, 2010 at 2:47 PM, <calidion@gmail.com> wrote: > short name will be displayed if long name is missing. But in most cases, at least in China, people prefer to use the short name, right? such as 731, 355, 运通110. > > > http://codereview.appspot.com/201045/diff/1010/5 > File gtfsscheduleviewer/files/transit_editor.js (right): > > http://codereview.appspot.com/201045/diff/1010/5#newcode109 > gtfsscheduleviewer/files/transit_editor.js:109: * Enable Stop Edition > On 2010/02/05 03:34:32, baiming wrote: >> >> For the use of "edition", if I remembered correct, it means "a version > > (of book, >> >> film, etc)", but not the noun form of edit. You'd better off to > > replace it with >> >> "editing". > >> You can look up it in dictionary to ensure that we use it properly. > > Done. > > http://codereview.appspot.com/201045/diff/1010/5#newcode304 > gtfsscheduleviewer/files/transit_editor.js:304: * Fetch Stop infomation > from server side by trip id > On 2010/02/05 03:34:32, baiming wrote: >> >> By trip id or stop id? > > Done. > > http://codereview.appspot.com/201045/diff/1010/5#newcode1733 > gtfsscheduleviewer/files/transit_editor.js:1733: this.getGDirections(); > On 2010/02/05 03:34:32, baiming wrote: >> >> A random thought: >> If user already fixed the shape by hand, maybe that cost him a very > > long time, >> >> then he move the stop a little, all his previous efforts will be lost? > > I think >> >> it will be annoying. > >> How about that we provide two modes of the stop editing, one is the > > current >> >> logic, and the other is not to re-calculate the direction but only to > > move the >> >> stop? And when a user attempts to move stop after he modified the > > neighboring >> >> shape, we can give him an alert and let him decide which mode he > > wants. > >> And this change will not involve many code changes. > > Done. > > http://codereview.appspot.com/201045/show > -- 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.
You don't have to include this change, that to prepend the route's short name, in this CL. It's fine to do that in a separate CL in future. 2010/2/5 Ming Bai 白明 <baiming@google.com>: > On Fri, Feb 5, 2010 at 2:47 PM, <calidion@gmail.com> wrote: >> short name will be displayed if long name is missing. > > But in most cases, at least in China, people prefer to use the short > name, right? such as 731, 355, 运通110. > >> >> >> http://codereview.appspot.com/201045/diff/1010/5 >> File gtfsscheduleviewer/files/transit_editor.js (right): >> >> http://codereview.appspot.com/201045/diff/1010/5#newcode109 >> gtfsscheduleviewer/files/transit_editor.js:109: * Enable Stop Edition >> On 2010/02/05 03:34:32, baiming wrote: >>> >>> For the use of "edition", if I remembered correct, it means "a version >> >> (of book, >>> >>> film, etc)", but not the noun form of edit. You'd better off to >> >> replace it with >>> >>> "editing". >> >>> You can look up it in dictionary to ensure that we use it properly. >> >> Done. >> >> http://codereview.appspot.com/201045/diff/1010/5#newcode304 >> gtfsscheduleviewer/files/transit_editor.js:304: * Fetch Stop infomation >> from server side by trip id >> On 2010/02/05 03:34:32, baiming wrote: >>> >>> By trip id or stop id? >> >> Done. >> >> http://codereview.appspot.com/201045/diff/1010/5#newcode1733 >> gtfsscheduleviewer/files/transit_editor.js:1733: this.getGDirections(); >> On 2010/02/05 03:34:32, baiming wrote: >>> >>> A random thought: >>> If user already fixed the shape by hand, maybe that cost him a very >> >> long time, >>> >>> then he move the stop a little, all his previous efforts will be lost? >> >> I think >>> >>> it will be annoying. >> >>> How about that we provide two modes of the stop editing, one is the >> >> current >>> >>> logic, and the other is not to re-calculate the direction but only to >> >> move the >>> >>> stop? And when a user attempts to move stop after he modified the >> >> neighboring >>> >>> shape, we can give him an alert and let him decide which mode he >> >> wants. >> >>> And this change will not involve many code changes. >> >> Done. >> >> http://codereview.appspot.com/201045/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
Sign in to reply to this message.
added short name updated server: http://211.100.227.25:8765/
Sign in to reply to this message.
Thanks for the changes. http://codereview.appspot.com/201045/diff/12/13 File gtfsscheduleviewer/files/transit_editor.js (right): http://codereview.appspot.com/201045/diff/12/13#newcode102 gtfsscheduleviewer/files/transit_editor.js:102: * Enable Leg Editing Why do you use capital letter here? http://codereview.appspot.com/201045/diff/12/13#newcode453 gtfsscheduleviewer/files/transit_editor.js:453: 'Leg Reload'); I'm not so sure if adding an option in the toolbar is proper for this use case. In my original thought, it should be an prompt alert with two options, which only appears at the moment the user move a stop after he modify the neighboring legs. Because this option is not useful in most of time, adding it in the toolbar will make the UI more complicated. Um... If you think it is not that easy to add the functionality of alert, just remove this option. We can do it later when I come up with a more detailed design for this. Thanks! http://codereview.appspot.com/201045/diff/12/13#newcode2552 gtfsscheduleviewer/files/transit_editor.js:2552: if (this.moveEndTimer) { It should be private. http://codereview.appspot.com/201045/diff/12/13#newcode2558 gtfsscheduleviewer/files/transit_editor.js:2558: self.fetchStopsInBounds(self.map_.getBounds()); It'd be better to set this.moveEndTimer to null at the right time. http://codereview.appspot.com/201045/diff/31/2003#newcode888 gtfsscheduleviewer/files/transit_editor.js:888: this.nameNode = $C('span'); If the long name is missing, we will see "short name[short name]", and if the short name is missing, it's "long name[]"... var name = ''; if (this.longName) { name += this.longName; } if (this.shortName) { name += '[' + this.shortName + ']'; }
Sign in to reply to this message.
updated http://codereview.appspot.com/201045/diff/12/13 File gtfsscheduleviewer/files/transit_editor.js (right): http://codereview.appspot.com/201045/diff/12/13#newcode102 gtfsscheduleviewer/files/transit_editor.js:102: * Enable Leg Editing On 2010/02/05 07:37:21, baiming wrote: > Why do you use capital letter here? Done. http://codereview.appspot.com/201045/diff/12/13#newcode453 gtfsscheduleviewer/files/transit_editor.js:453: 'Leg Reload'); On 2010/02/05 07:37:21, baiming wrote: > I'm not so sure if adding an option in the toolbar is proper for this use case. > In my original thought, it should be an prompt alert with two options, which > only appears at the moment the user move a stop after he modify the neighboring > legs. Because this option is not useful in most of time, adding it in the > toolbar will make the UI more complicated. > > Um... If you think it is not that easy to add the functionality of alert, just > remove this option. We can do it later when I come up with a more detailed > design for this. Thanks! OK. I would like to have more details. http://codereview.appspot.com/201045/diff/12/13#newcode2552 gtfsscheduleviewer/files/transit_editor.js:2552: if (this.moveEndTimer) { On 2010/02/05 07:37:21, baiming wrote: > It should be private. Done. http://codereview.appspot.com/201045/diff/12/13#newcode2558 gtfsscheduleviewer/files/transit_editor.js:2558: self.fetchStopsInBounds(self.map_.getBounds()); On 2010/02/05 07:37:21, baiming wrote: > It'd be better to set this.moveEndTimer to null at the right time. please specify what you mean. http://codereview.appspot.com/201045/diff/31/2003#newcode888 gtfsscheduleviewer/files/transit_editor.js:888: this.nameNode = $C('span'); On 2010/02/05 07:37:21, baiming wrote: > If the long name is missing, we will see "short name[short name]", and if the > short name is missing, it's "long name[]"... > > var name = ''; > if (this.longName) { > name += this.longName; > } > if (this.shortName) { > name += '[' + this.shortName + ']'; > } Done.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
updated to rev 53. 2010/2/5 <baiming@google.com> > LGTM > > > http://codereview.appspot.com/201045/show >
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
