added switcher to hide trips in bounds but not in trips

[calidion, 2010-01-26 05:58:01]

1.Initialized History object
2.Added asynchronous tag to each object.
3.Extended Base class to be ready for editable
4.Added a simple toolbar
5.Enabled hiding stops not in the trip selected.

[baiming, 2010-01-27 02:43:09]

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

http://codereview.appspot.com/193103/diff/1/3#newcode253
gtfsscheduleviewer/files/transit_editor.js:253: var History = function() {
Since this change is mainly focusing on the switch of trips, and history is not
yet used, can you please remove the code related to history out of this version?
You'd better to include all the history related code, from definition to usage,
in a separate CL in the future.

http://codereview.appspot.com/193103/diff/1/3#newcode312
gtfsscheduleviewer/files/transit_editor.js:312: InnerToolbar = function() {
Why do we call it "Inner"Toolbar? Can you add some description about it in the
comment?

http://codereview.appspot.com/193103/diff/1/3#newcode318
gtfsscheduleviewer/files/transit_editor.js:318: * Get new checkbox
Rephrase it:
Create a new checkbox under the given parent node, and return it.

http://codereview.appspot.com/193103/diff/1/3#newcode324
gtfsscheduleviewer/files/transit_editor.js:324:
InnerToolbar.prototype.getCheckBox = function(pNode, checked, text){
What this function does is to create a new checkbox under the given pNode,
rather than to get an existing checkbox from somewhere. So rename it to
"newCheckbox" or "createCheckbox".

http://codereview.appspot.com/193103/diff/1/3#newcode325
gtfsscheduleviewer/files/transit_editor.js:325: var node =
document.createElement('input');
Why not using the shorthand $C?

http://codereview.appspot.com/193103/diff/1/3#newcode339
gtfsscheduleviewer/files/transit_editor.js:339: InnerToolbar.prototype.getButton
= function(pNode, text){
Ditto. See the three comment above.

http://codereview.appspot.com/193103/diff/1/3#newcode343
gtfsscheduleviewer/files/transit_editor.js:343: archor.color = '#ccc';
Can we use CSS to control styles?

http://codereview.appspot.com/193103/diff/1/3#newcode354
gtfsscheduleviewer/files/transit_editor.js:354: for(stopId in stops) {
Space between for and (

http://codereview.appspot.com/193103/diff/1/3#newcode357
gtfsscheduleviewer/files/transit_editor.js:357: showTripsInBounds =
this.switchTrips.checked;
Move this line out of the for loop.

http://codereview.appspot.com/193103/diff/1/3#newcode367
gtfsscheduleviewer/files/transit_editor.js:367:
InnerToolbar.prototype.initialize = function(map) {
When will this function be invoked?

http://codereview.appspot.com/193103/diff/1/3#newcode368
gtfsscheduleviewer/files/transit_editor.js:368: var container =
document.createElement("div");
$C?

http://codereview.appspot.com/193103/diff/1/3#newcode370
gtfsscheduleviewer/files/transit_editor.js:370: var checkOptions =
document.createElement('span');
Ditto.

http://codereview.appspot.com/193103/diff/1/3#newcode378
gtfsscheduleviewer/files/transit_editor.js:378: var actionOptions =
document.createElement('span');
$C?

http://codereview.appspot.com/193103/diff/1/3#newcode388
gtfsscheduleviewer/files/transit_editor.js:388: var hint =
document.createElement('span');
$?

http://codereview.appspot.com/193103/diff/1/3#newcode397
gtfsscheduleviewer/files/transit_editor.js:397: // By default, the control will
appear in the top left corner of the
Please use the jsDoc style comment.
And @override as it overrides the default method of the base class.

http://codereview.appspot.com/193103/diff/1/3#newcode438
gtfsscheduleviewer/files/transit_editor.js:438: this.history = new History();
Because there's no code demonstrating how you use this history object, I so far
cannot understand why does every instance of Base's subclass need a "history".

Can we get rid of all the history related code from this version, as I mentioned
above?

http://codereview.appspot.com/193103/diff/1/3#newcode516
gtfsscheduleviewer/files/transit_editor.js:516: Base.prototype.serialize =
function() {};
Why is it blank?

http://codereview.appspot.com/193103/diff/1/3#newcode1921
gtfsscheduleviewer/files/transit_editor.js:1921: Stop.prototype.clear=
function() {
Why do you change the name of this function, but not change the logic?

http://codereview.appspot.com/193103/diff/1/3#newcode1921
gtfsscheduleviewer/files/transit_editor.js:1921: Stop.prototype.clear=
function() {
Space between clear and =

[calidion, 2010-01-27 07:04:04]

1.removed History
2.fixed a bug on checkbox selection
3.changed problems on comments.

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

http://codereview.appspot.com/193103/diff/1/3#newcode253
gtfsscheduleviewer/files/transit_editor.js:253: var History = function() {
On 2010/01/27 02:43:09, baiming wrote:
> Since this change is mainly focusing on the switch of trips, and history is
not
> yet used, can you please remove the code related to history out of this
version?
> You'd better to include all the history related code, from definition to
usage,
> in a separate CL in the future.

Done.

http://codereview.appspot.com/193103/diff/1/3#newcode312
gtfsscheduleviewer/files/transit_editor.js:312: InnerToolbar = function() {
On 2010/01/27 02:43:09, baiming wrote:
> Why do we call it "Inner"Toolbar? Can you add some description about it in the
> comment?

Done.

http://codereview.appspot.com/193103/diff/1/3#newcode318
gtfsscheduleviewer/files/transit_editor.js:318: * Get new checkbox
On 2010/01/27 02:43:09, baiming wrote:
> Rephrase it:
> Create a new checkbox under the given parent node, and return it.

Done.

http://codereview.appspot.com/193103/diff/1/3#newcode324
gtfsscheduleviewer/files/transit_editor.js:324:
InnerToolbar.prototype.getCheckBox = function(pNode, checked, text){
On 2010/01/27 02:43:09, baiming wrote:
> What this function does is to create a new checkbox under the given pNode,
> rather than to get an existing checkbox from somewhere. So rename it to
> "newCheckbox" or "createCheckbox".

Done.

http://codereview.appspot.com/193103/diff/1/3#newcode325
gtfsscheduleviewer/files/transit_editor.js:325: var node =
document.createElement('input');
On 2010/01/27 02:43:09, baiming wrote:
> Why not using the shorthand $C?

Done.

http://codereview.appspot.com/193103/diff/1/3#newcode339
gtfsscheduleviewer/files/transit_editor.js:339: InnerToolbar.prototype.getButton
= function(pNode, text){
On 2010/01/27 02:43:09, baiming wrote:
> Ditto. See the three comment above.

Done.

http://codereview.appspot.com/193103/diff/1/3#newcode343
gtfsscheduleviewer/files/transit_editor.js:343: archor.color = '#ccc';
On 2010/01/27 02:43:09, baiming wrote:
> Can we use CSS to control styles?
seems not so necessary now.

http://codereview.appspot.com/193103/diff/1/3#newcode354
gtfsscheduleviewer/files/transit_editor.js:354: for(stopId in stops) {
On 2010/01/27 02:43:09, baiming wrote:
> Space between for and (

Done.

http://codereview.appspot.com/193103/diff/1/3#newcode357
gtfsscheduleviewer/files/transit_editor.js:357: showTripsInBounds =
this.switchTrips.checked;
On 2010/01/27 02:43:09, baiming wrote:
> Move this line out of the for loop.

Done.

http://codereview.appspot.com/193103/diff/1/3#newcode367
gtfsscheduleviewer/files/transit_editor.js:367:
InnerToolbar.prototype.initialize = function(map) {
On 2010/01/27 02:43:09, baiming wrote:
> When will this function be invoked?

it is invoked by map.addControl

http://codereview.appspot.com/193103/diff/1/3#newcode368
gtfsscheduleviewer/files/transit_editor.js:368: var container =
document.createElement("div");
On 2010/01/27 02:43:09, baiming wrote:
> $C?

Done.

http://codereview.appspot.com/193103/diff/1/3#newcode370
gtfsscheduleviewer/files/transit_editor.js:370: var checkOptions =
document.createElement('span');
On 2010/01/27 02:43:09, baiming wrote:
> Ditto.

Done.

http://codereview.appspot.com/193103/diff/1/3#newcode378
gtfsscheduleviewer/files/transit_editor.js:378: var actionOptions =
document.createElement('span');
On 2010/01/27 02:43:09, baiming wrote:
> $C?

Done.

http://codereview.appspot.com/193103/diff/1/3#newcode388
gtfsscheduleviewer/files/transit_editor.js:388: var hint =
document.createElement('span');
On 2010/01/27 02:43:09, baiming wrote:
> $?

Done.

http://codereview.appspot.com/193103/diff/1/3#newcode438
gtfsscheduleviewer/files/transit_editor.js:438: this.history = new History();
On 2010/01/27 02:43:09, baiming wrote:
> Because there's no code demonstrating how you use this history object, I so
far
> cannot understand why does every instance of Base's subclass need a "history".
> 
> Can we get rid of all the history related code from this version, as I
mentioned
> above?

Done.

http://codereview.appspot.com/193103/diff/1/3#newcode516
gtfsscheduleviewer/files/transit_editor.js:516: Base.prototype.serialize =
function() {};
On 2010/01/27 02:43:09, baiming wrote:
> Why is it blank?

it is a abstract function for overriding.

http://codereview.appspot.com/193103/diff/1/3#newcode1921
gtfsscheduleviewer/files/transit_editor.js:1921: Stop.prototype.clear=
function() {
On 2010/01/27 02:43:09, baiming wrote:
> Why do you change the name of this function, but not change the logic?

clear is much shorter and equivalent.

http://codereview.appspot.com/193103/diff/1/3#newcode1921
gtfsscheduleviewer/files/transit_editor.js:1921: Stop.prototype.clear=
function() {
On 2010/01/27 02:43:09, baiming wrote:
> Space between clear and =

Done.

[baiming, 2010-01-27 11:04:36]

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

http://codereview.appspot.com/193103/diff/1/3#newcode516
gtfsscheduleviewer/files/transit_editor.js:516: Base.prototype.serialize =
function() {};
I see. So please add jsDoc to describe it.

On 2010/01/27 07:04:04, calidion wrote:
> On 2010/01/27 02:43:09, baiming wrote:
> > Why is it blank?
> 
> it is a abstract function for overriding.

http://codereview.appspot.com/193103/diff/1/3#newcode1921
gtfsscheduleviewer/files/transit_editor.js:1921: Stop.prototype.clear=
function() {
But ambiguous...

On 2010/01/27 07:04:04, calidion wrote:
> On 2010/01/27 02:43:09, baiming wrote:
> > Why do you change the name of this function, but not change the logic?
> 
> clear is much shorter and equivalent.
>

http://codereview.appspot.com/193103/diff/1/3#newcode1921
gtfsscheduleviewer/files/transit_editor.js:1921: Stop.prototype.clear=
function() {
Really done?

On 2010/01/27 07:04:04, calidion wrote:
> On 2010/01/27 02:43:09, baiming wrote:
> > Space between clear and =
> 
> Done.

http://codereview.appspot.com/193103/diff/2002/1008#newcode315
gtfsscheduleviewer/files/transit_editor.js:315: InnerToolbar.prototype = new
GControl();
Four-char indentation.

http://codereview.appspot.com/193103/diff/2002/1008#newcode341
gtfsscheduleviewer/files/transit_editor.js:341: archor.href =
'javascript:void(0)';
Add @override.

[calidion, 2010-01-28 02:00:46]

updated

http://codereview.appspot.com/193103/diff/2002/1008
File gtfsscheduleviewer/files/transit_editor.js (right):

http://codereview.appspot.com/193103/diff/2002/1008#newcode315
gtfsscheduleviewer/files/transit_editor.js:315: 'Show trips in bounds');
On 2010/01/27 11:04:36, baiming wrote:
> Four-char indentation.

Done.

http://codereview.appspot.com/193103/diff/2002/1008#newcode341
gtfsscheduleviewer/files/transit_editor.js:341: * map with 7 pixels of padding.
On 2010/01/27 11:04:36, baiming wrote:
> Add @override.

Done.

[baiming, 2010-01-28 04:09:46]

LGTM

[calidion, 2010-01-28 05:26:22]

updated to rev 50

2010/1/28 <baiming@google.com>

> LGTM
>
>
> http://codereview.appspot.com/193103/show
>