Code review on date picker branch to date

[Ray Ryan]

General comments

It seems like there is a lot of overlap between DatePicker and CalendarView. Should the methods in DatePicker that are redundant with CalendarView methods be stricken, given DatePicker#getCalendarView? Or, should getCalendarView perhaps go away, or become protected or package--is it really useful/safe for clients to talk to it directly?

ElementMapper.java

We need ElementMapperTest

The param type MappedType should be a single letter, perhaps U

What's going on Iterator#remove? This class claims to work on UIObjects, but here you are casting to Widget and asserting that parent is an HTMLTable. 

CalendarModel.java

We need CalendarModelTest

So this class has the notion of the current date shared by the various moving parts, and it does formatting, yes? Could punch up its javadoc to that effect. 

What is that no-op refresh() method there for?

DatePicker.java

1. It's confusing that there is a CalendarView named calendar, and a CalendarModel named model. I'd expect the view to be named view, and the model to be named calendar.

2. It seems like many fields here could be final. 

3. The Css interface is protected, but I though we were going to make it private, or at least package protected. It is also featured in a ton of public methods and constructors. That's no good. 

If it does wind up staying visible, having a public static setDefaultCss method is a bad idea. Allowing it to be overridden per instance is plenty.

4. getDateShown() is implemented as getModel().getCurrentMonth(). Isn't that a bug? If not, shouldn't the method be called getMonthShown()?

5. It seems like these methods:

  public final void addStyleToVisibleDate(Date visibleDate, String styleName) {

    calendar.addStyleToDate(visibleDate, styleName);

  }

  public final void addStyleToVisibleDates(Iterable<Date> visibleDates,

      String styleName) {

    getCalendarView().addStyleToDates(visibleDates, styleName);

  }

could instead be

  public final void addStyleToVisibleDates(String styleName, Date... visibleDates) {

    addStyleToVisibleDates(styleName, Arrays.asList(visibleDates));

  }

  public final void addStyleToVisibleDates(String styleName, Iterable<Date> visibleDates) {

    getCalendarView().addStyleToDates(visibleDates, styleName);

  }

Even if not, why is one using calendar and the other using getCalendarView()?

And note that the remove method is not parallel to these--there is only one.

6. getHighlightedDate() javadoc should explain diff between selected and highlighted. class doc should probably do the same.

7.  getGlobalStyleOfDate() What is a global style? Is this a GWT wide concept? If not, needs explicating.

8. showDate() Need to explain how this differs from setValue(), and link between the two methods.

9. What is the package private setModel(CalendarModel) needed for?

10. Some methods are final, and some aren't. It seems very arbitrary. And we should be documenting final methods to explain why.

11. This seems harsh (the assert, I mean):

  public final void setEnabledOnVisibleDate(Date date, boolean enabled) {

    assert isDateVisible(date) : date

        + " cannot be enabled or disabled as it is not visible";

    getCalendarView().setDateEnabled(date, enabled);

  }

What happens if they disable a date, the user scrolls it away, and then scrolls it back? Do they have to listen for these scroll events and then reapply the disables? Is this a method people actually asked for, can we get rid of it?

12. setAllowableDates says it is "not yet implemented for the default case". Why is it here, then? 

DateBox.java

1. Constitutent fields (box, picker, popup) should be final

DefaultMonthSelector.java

JavaDoc says that it is "not part of the public API". If it's a public class and can be used as an arg to a protected constructor, it's public, let's be honest here. Perhaps you meant "not extensible"? As for "please feel to copy it...", that's a pretty severe mixed message. 

I don't really see what we're gaining by encouraging people to copy it and then claiming that it's not public and subject to change. If they copy it, we will break them. We're providing a protected constructor that accepts this. That's public api. We need to make up our minds--providing your own CalendarView and MonthSelector is supported, or it isn't. 

MonthSelector.java

More of the same. This class contributes basically nothing. We're trying to allow custom MonthSelector and at the same time trying to avoid it. We need to make up our minds.

CalendarUtil.java

Some of the methods in this public class are not public, seemingly arbitrarily. Is there a real reason we don't want people to call isWeekend or hasTime? Why is resetTime private?

[dflorey]

Comment on DateBox:
Would be cool if there would be a way to get the value of the DateBox.
Right now I struggle to find a way to listen to value changes and get
the value afterwards.
I tried to listen to DatePicker and TextBox changes, but it's very
complicated to find the proper value there.
Proposal: Add ChangeListener support and add a "Date getValue()"
method.

[Emily Crutcher]

On Wed, Nov 19, 2008 at 10:26 PM, Ray Ryan <rj...@google.com> wrote:

General comments

It seems like there is a lot of overlap between DatePicker and CalendarView. Should the methods in DatePicker that are redundant with CalendarView methods be stricken, given DatePicker#getCalendarView? Or, should getCalendarView perhaps go away, or become protected or package--is it really useful/safe for clients to talk to it directly?

getCalendarView is already protected, so I'm not sure what you are suggesting here?
 

ElementMapper.java

We need ElementMapperTest

Yep, that's part of the "basic unit test task" on tracker.
 

The param type MappedType should be a single letter, perhaps U

 

Yep. All the classes still are using the old style XType syntax rather then the single letter syntax.

What's going on Iterator#remove? This class claims to work on UIObjects, but here you are casting to Widget and asserting that parent is an HTMLTable. 

My bad. I converted over to UIObject as we are not using any specific "widget" like information when I moved it, should have waited until after the initial commit like the other changes.

CalendarModel.java

We need CalendarModelTest

 
Yep.

So this class has the notion of the current date shared by the various moving parts, and it does formatting, yes? Could punch up its javadoc to that effect. 

Yep.

What is that no-op refresh() method there for?

So subclasses of calendar model have the ability to refresh.
 

DatePicker.java

1. It's confusing that there is a CalendarView named calendar, and a CalendarModel named model. I'd expect the view to be named view, and the model to be named calendar.

how about just view and model? 

 

2. It seems like many fields here could be final. 

Yep.
 

3. The Css interface is protected, but I though we were going to make it private, or at least package protected. It is also featured in a ton of public methods and constructors. That's no good. 

Yep, that's in the current tracker schedule as well.  Now, I'm not entirely sure how we are going to get rid of it, but I figure we can knock our heads together and figure something out.
 

If it does wind up staying visible, having a public static setDefaultCss method is a bad idea. Allowing it to be overridden per instance is plenty.

I'm 99% certain that whatever solution we come up with in the general case will NOT have this method.
 

4. getDateShown() is implemented as getModel().getCurrentMonth(). Isn't that a bug? If not, shouldn't the method be called getMonthShown()?

In the default implementation, the current month field in the calendar model is the date shown, so it is correct for the default case. 

We've tried to implement the API so it would be tolerant of a calendar view that showed multiple months at once, which is why the date picker methods are getDateShown() and showDate() rather then getMonthShown() and showMonth()

5. It seems like these methods:

  public final void addStyleToVisibleDate(Date visibleDate, String styleName) {

    calendar.addStyleToDate(visibleDate, styleName);

  }

  public final void addStyleToVisibleDates(Iterable<Date> visibleDates,

      String styleName) {

    getCalendarView().addStyleToDates(visibleDates, styleName);

  }

could instead be

  public final void addStyleToVisibleDates(String styleName, Date... visibleDates) {

    addStyleToVisibleDates(styleName, Arrays.asList(visibleDates));

  }

  public final void addStyleToVisibleDates(String styleName, Iterable<Date> visibleDates) {

    getCalendarView().addStyleToDates(visibleDates, styleName);

  }

Even if not, why is one using calendar and the other using getCalendarView()?

For efficiency and code clearity I would still be inclined to support the singleton version as well, but adding the Date... version as an option to the plural versions seems like a terrific idea. I don't think it matters if we use calendar/getCalendar, but we should probably pick only one for consistancy!

And note that the remove method is not parallel to these--there is only one.

While no one has requested the plural version of remove, I think you are right and we should add it anyway.
 

6. getHighlightedDate() javadoc should explain diff between selected and highlighted. class doc should probably do the same.

Yep.  

7.  getGlobalStyleOfDate() What is a global style? Is this a GWT wide concept? If not, needs explicating.

Will add more explination then.
 

8. showDate() Need to explain how this differs from setValue(), and link between the two methods.

Yep, can add more javadoc.
 

9. What is the package private setModel(CalendarModel) needed for?

It looks like it can be made private. Will do so.
 

10. Some methods are final, and some aren't. It seems very arbitrary. And we should be documenting final methods to explain why.

Will add javadoc to explain why final methods are final.  Also, we've globally moved our policy about final methods, so some of those finals may end up coming off.
 

11. This seems harsh (the assert, I mean):

  public final void setEnabledOnVisibleDate(Date date, boolean enabled) {

    assert isDateVisible(date) : date

        + " cannot be enabled or disabled as it is not visible";

    getCalendarView().setDateEnabled(date, enabled);

  }

What happens if they disable a date, the user scrolls it away, and then scrolls it back? Do they have to listen for these scroll events and then reapply the disables? Is this a method people actually asked for, can we get rid of it?

 Not sure I understand your use case.  What does "scrolling" mean in the context of a date picker?

12. setAllowableDates says it is "not yet implemented for the default case". Why is it here, then? 

I thought I got all of those! As we decided we were not supporting that method in the intial port. 

DateBox.java

1. Constitutent fields (box, picker, popup) should be final

Yep.

 

DefaultMonthSelector.java

JavaDoc says that it is "not part of the public API". If it's a public class and can be used as an arg to a protected constructor, it's public, let's be honest here. Perhaps you meant "not extensible"? As for "please feel to copy it...", that's a pretty severe mixed message. 

I don't really see what we're gaining by encouraging people to copy it and then claiming that it's not public and subject to change. If they copy it, we will break them. We're providing a protected constructor that accepts this. That's public api. We need to make up our minds--providing your own CalendarView and MonthSelector is supported, or it isn't. 

Yes, should have read "not exensible".  I'm not sure I understand your last comments.  Why do you interpret this as saying that we do not support custom calendar views and month selectors?

 

MonthSelector.java

More of the same. This class contributes basically nothing. We're trying to allow custom MonthSelector and at the same time trying to avoid it. We need to make up our minds.

How are we avoiding people creating custom month selectors?

 

CalendarUtil.java

Some of the methods in this public class are not public, seemingly arbitrarily. Is there a real reason we don't want people to call isWeekend or hasTime? Why is resetTime private?

We were trying to expose only the methods we were absolutely certain other people would want, mostely because we used them ourselves, rather then exposing all of them.

 

--
"There are only 10 types of people in the world: Those who understand binary, and those who don't"

[Emily Crutcher]

DateBox should implement the HasValue interface long term, which using the new terminology, does basically what you expect here.

[John Tamplin]

On Thu, Nov 20, 2008 at 10:04 AM, Emily Crutcher <e...@google.com> wrote:

For efficiency and code clearity I would still be inclined to support the singleton version as well, but adding the Date... version as an option to the plural versions seems like a terrific idea. I don't think it matters if we use calendar/getCalendar, but we should probably pick only one for consistancy!

You can still call the varargs version with a single value, as in addStyleToVisibleDates(styleName, date), and the extra cost should be minimal.  I think the extra weight of additional API surface area is more costly.

--
John A. Tamplin
Software Engineer (GWT), Google

[Ray Ryan]

On Thu, Nov 20, 2008 at 10:04 AM, Emily Crutcher <e...@google.com> wrote:

On Wed, Nov 19, 2008 at 10:26 PM, Ray Ryan <rj...@google.com> wrote:

General comments

It seems like there is a lot of overlap between DatePicker and CalendarView. Should the methods in DatePicker that are redundant with CalendarView methods be stricken, given DatePicker#getCalendarView? Or, should getCalendarView perhaps go away, or become protected or package--is it really useful/safe for clients to talk to it directly?

getCalendarView is already protected, so I'm not sure what you are suggesting here?

Sorry, could have sworn that was public. Btw., DatePicker is littered with direct uses of the calendar field. If getCalendarView() is protected, we should always call it.

Still, the amount of duplication btw. the DatePicker API and CalendarView is odd. Can CalendarView be reduced to a smaller set of "primitive" calls at all? E.g., it doesn't really need the setDatesEnabled convenience wrapper, DatePicker can be in charge of that kind thing. 

 

ElementMapper.java

We need ElementMapperTest

Yep, that's part of the "basic unit test task" on tracker.
 

The param type MappedType should be a single letter, perhaps U

 

Yep. All the classes still are using the old style XType syntax rather then the single letter syntax.

What's going on Iterator#remove? This class claims to work on UIObjects, but here you are casting to Widget and asserting that parent is an HTMLTable. 

My bad. I converted over to UIObject as we are not using any specific "widget" like information when I moved it, should have waited until after the initial commit like the other changes.

CalendarModel.java

We need CalendarModelTest

 
Yep.

So this class has the notion of the current date shared by the various moving parts, and it does formatting, yes? Could punch up its javadoc to that effect. 

Yep.

What is that no-op refresh() method there for?

So subclasses of calendar model have the ability to refresh.
 

DatePicker.java

1. It's confusing that there is a CalendarView named calendar, and a CalendarModel named model. I'd expect the view to be named view, and the model to be named calendar.

how about just view and model? 

k 

 

2. It seems like many fields here could be final. 

Yep.
 

3. The Css interface is protected, but I though we were going to make it private, or at least package protected. It is also featured in a ton of public methods and constructors. That's no good. 

Yep, that's in the current tracker schedule as well.  Now, I'm not entirely sure how we are going to get rid of it, but I figure we can knock our heads together and figure something out.
 

If it does wind up staying visible, having a public static setDefaultCss method is a bad idea. Allowing it to be overridden per instance is plenty.

I'm 99% certain that whatever solution we come up with in the general case will NOT have this method.
 

4. getDateShown() is implemented as getModel().getCurrentMonth(). Isn't that a bug? If not, shouldn't the method be called getMonthShown()?

In the default implementation, the current month field in the calendar model is the date shown, so it is correct for the default case. 

We've tried to implement the API so it would be tolerant of a calendar view that showed multiple months at once, which is why the date picker methods are getDateShown() and showDate() rather then getMonthShown() and showMonth()

So what is the actual date returned in such a case? The first day of the month(s) displayed? Or can a Date be filled with null day info? Sounds like another excellent javadoc opportunity.

Anyway, can such a CalendarView really be implemented? CalendarModel seems very locked down to a single month.

5. It seems like these methods:

  public final void addStyleToVisibleDate(Date visibleDate, String styleName) {

    calendar.addStyleToDate(visibleDate, styleName);

  }

  public final void addStyleToVisibleDates(Iterable<Date> visibleDates,

      String styleName) {

    getCalendarView().addStyleToDates(visibleDates, styleName);

  }

could instead be

  public final void addStyleToVisibleDates(String styleName, Date... visibleDates) {

    addStyleToVisibleDates(styleName, Arrays.asList(visibleDates));

  }

  public final void addStyleToVisibleDates(String styleName, Iterable<Date> visibleDates) {

    getCalendarView().addStyleToDates(visibleDates, styleName);

  }

Even if not, why is one using calendar and the other using getCalendarView()?

For efficiency and code clearity I would still be inclined to support the singleton version as well, but adding the Date... version as an option to the plural versions seems like a terrific idea. I don't think it matters if we use calendar/getCalendar, but we should probably pick only one for consistancy!

That might not be possible--the compiler may find itself unable to tell whether to use the Date or the Date... version. 

And note that the remove method is not parallel to these--there is only one.

While no one has requested the plural version of remove, I think you are right and we should add it anyway.
 

6. getHighlightedDate() javadoc should explain diff between selected and highlighted. class doc should probably do the same.

Yep.  

7.  getGlobalStyleOfDate() What is a global style? Is this a GWT wide concept? If not, needs explicating.

Will add more explination then.
 

8. showDate() Need to explain how this differs from setValue(), and link between the two methods.

Yep, can add more javadoc.
 

9. What is the package private setModel(CalendarModel) needed for?

It looks like it can be made private. Will do so.
 

10. Some methods are final, and some aren't. It seems very arbitrary. And we should be documenting final methods to explain why.

Will add javadoc to explain why final methods are final.  Also, we've globally moved our policy about final methods, so some of those finals may end up coming off.
 

11. This seems harsh (the assert, I mean):

  public final void setEnabledOnVisibleDate(Date date, boolean enabled) {

    assert isDateVisible(date) : date

        + " cannot be enabled or disabled as it is not visible";

    getCalendarView().setDateEnabled(date, enabled);

  }

What happens if they disable a date, the user scrolls it away, and then scrolls it back? Do they have to listen for these scroll events and then reapply the disables? Is this a method people actually asked for, can we get rid of it?

 Not sure I understand your use case.  What does "scrolling" mean in the context of a date picker?

February is visible. The 10th is disabled. The user moves the picker to March, and back to February. What is the state of Feb. 10? 

12. setAllowableDates says it is "not yet implemented for the default case". Why is it here, then? 

I thought I got all of those! As we decided we were not supporting that method in the intial port. 

DateBox.java

1. Constitutent fields (box, picker, popup) should be final

Yep.

 

DefaultMonthSelector.java

JavaDoc says that it is "not part of the public API". If it's a public class and can be used as an arg to a protected constructor, it's public, let's be honest here. Perhaps you meant "not extensible"? As for "please feel to copy it...", that's a pretty severe mixed message. 

I don't really see what we're gaining by encouraging people to copy it and then claiming that it's not public and subject to change. If they copy it, we will break them. We're providing a protected constructor that accepts this. That's public api. We need to make up our minds--providing your own CalendarView and MonthSelector is supported, or it isn't. 

Yes, should have read "not exensible".  I'm not sure I understand your last comments.  Why do you interpret this as saying that we do not support custom calendar views and month selectors?

Custom CalendarView seems okay, but MonthSelector is an empty class, and its default implementation is final. So I have protected API that says "your month selector here," and no way to actually provide one without copying and pasting our code. That seems inconsistent.

 

MonthSelector.java

More of the same. This class contributes basically nothing. We're trying to allow custom MonthSelector and at the same time trying to avoid it. We need to make up our minds.

How are we avoiding people creating custom month selectors?

 

CalendarUtil.java

Some of the methods in this public class are not public, seemingly arbitrarily. Is there a real reason we don't want people to call isWeekend or hasTime? Why is resetTime private?

We were trying to expose only the methods we were absolutely certain other people would want, mostely because we used them ourselves, rather then exposing all of them.

If we have methods we aren't using, why are they there? 

[Ray Ryan]

"Long term"? 1.6, yes?

[Emily Crutcher]

On Thu, Nov 20, 2008 at 10:32 AM, Ray Ryan <rj...@google.com> wrote:

"Long term"? 1.6, yes?

On Thu, Nov 20, 2008 at 10:07 AM, Emily Crutcher <e...@google.com> wrote:

DateBox should implement the HasValue interface long term, which using the new terminology, does basically what you expect here.

Yep, so long term may have been a bit of an exaggeration!

 

[Emily Crutcher]

Well, we can always add the single-arg one back in, so if the compiler actually generates reasonably efficent code for this, I think you're probably right.

[John Tamplin]

On Thu, Nov 20, 2008 at 10:55 AM, Emily Crutcher <e...@google.com> wrote:

Well, we can always add the single-arg one back in, so if the compiler actually generates reasonably efficent code for this, I think you're probably right.

Actually as Ray points out, foo(Bar) and foo(Bar...) are not distinguishable overloads, so you would have to reverse the order of the arguments to keep the single-arg version.

[Emily Crutcher]

 

Sorry, could have sworn that was public. Btw., DatePicker is littered with direct uses of the calendar field. If getCalendarView() is protected, we should always call it.

Still, the amount of duplication btw. the DatePicker API and CalendarView is odd. Can CalendarView be reduced to a smaller set of "primitive" calls at all? E.g., it doesn't really need the setDatesEnabled convenience wrapper, DatePicker can be in charge of that kind thing. 

 

Sure, we can get rid of the convinience methods.

So what is the actual date returned in such a case? The first day of the month(s) displayed? Or can a Date be filled with null day info? Sounds like another excellent javadoc opportunity.

 

Yep. Also, I think it does make sense to extend this API down to the calendar model level, as that would probably help mitigate the confusion.

 

 

Anyway, can such a CalendarView really be implemented? CalendarModel seems very locked down to a single month.

The calendar view specifies a month at a time, the calendar view can increment the month to help it fill in its date views. 

 

 

That might not be possible--the compiler may find itself unable to tell whether to use the Date or the Date... version. 

 

Yep, and if we actually implement ".." efficiently in our compiler, no reason to do this either!

 

February is visible. The 10th is disabled. The user moves the picker to March, and back to February. What is the state of Feb. 10? 

 

It is no longer disabled. That's why the API has the assertion actually, as all the api calls that include "visible dates" specifically are reset when the date picker refreshes.

 

Custom CalendarView seems okay, but MonthSelector is an empty class, and its default implementation is final. So I have protected API that says "your month selector here," and no way to actually provide one without copying and pasting our code. That seems inconsistent.

 

 

So you think we should include an extendable month selector as well? We actually even have a googler working on one, so we can see how far he has gotten or create one ourselves if he hasn't gotten far enough.

 

  

CalendarUtil.java

Some of the methods in this public class are not public, seemingly arbitrarily. Is there a real reason we don't want people to call isWeekend or hasTime? Why is resetTime private?

We were trying to expose only the methods we were absolutely certain other people would want, mostely because we used them ourselves, rather then exposing all of them.

If we have methods we aren't using, why are they there? 

 

We are using them for calendar model, we just don't know if anyone else would need to use them. 

 

[Emily Crutcher]

 

Actually as Ray points out, foo(Bar) and foo(Bar...) are not distinguishable overloads, so you would have to reverse the order of the arguments to keep the single-arg version.

 

One is plural, one is not, so they would hopefully not overlap at all. The bigger point though, is if the the compiler implements the ".." sytax efficiently, no reason for the clutter.

[Ray Ryan]

On Thu, Nov 20, 2008 at 11:21 AM, Emily Crutcher <e...@google.com> wrote:

 

Sorry, could have sworn that was public. Btw., DatePicker is littered with direct uses of the calendar field. If getCalendarView() is protected, we should always call it.

Still, the amount of duplication btw. the DatePicker API and CalendarView is odd. Can CalendarView be reduced to a smaller set of "primitive" calls at all? E.g., it doesn't really need the setDatesEnabled convenience wrapper, DatePicker can be in charge of that kind thing. 

 

Sure, we can get rid of the convinience methods.

So what is the actual date returned in such a case? The first day of the month(s) displayed? Or can a Date be filled with null day info? Sounds like another excellent javadoc opportunity.

 

Yep. Also, I think it does make sense to extend this API down to the calendar model level, as that would probably help mitigate the confusion.

 

 

Anyway, can such a CalendarView really be implemented? CalendarModel seems very locked down to a single month.

The calendar view specifies a month at a time, the calendar view can increment the month to help it fill in its date views. 

 

 

That might not be possible--the compiler may find itself unable to tell whether to use the Date or the Date... version. 

 

Yep, and if we actually implement ".." efficiently in our compiler, no reason to do this either!

 

February is visible. The 10th is disabled. The user moves the picker to March, and back to February. What is the state of Feb. 10? 

 

It is no longer disabled. That's why the API has the assertion actually, as all the api calls that include "visible dates" specifically are reset when the date picker refreshes.

So, again, if I use this api, I am responsible for monitoring the picker and re-applying my setDisabled calls. This strikes me as kind of half hearted support, and surprising behavior. If someone can make this kind of thing happen with a slightly customized CalendarView, I think we should delete this method.

 

Custom CalendarView seems okay, but MonthSelector is an empty class, and its default implementation is final. So I have protected API that says "your month selector here," and no way to actually provide one without copying and pasting our code. That seems inconsistent.

 

 

So you think we should include an extendable month selector as well? We actually even have a googler working on one, so we can see how far he has gotten or create one ourselves if he hasn't gotten far enough.

More that we should get MonthSelector out of the protected constructor.

 

  

CalendarUtil.java

Some of the methods in this public class are not public, seemingly arbitrarily. Is there a real reason we don't want people to call isWeekend or hasTime? Why is resetTime private?

We were trying to expose only the methods we were absolutely certain other people would want, mostely because we used them ourselves, rather then exposing all of them.

If we have methods we aren't using, why are they there? 

 

We are using them for calendar model, we just don't know if anyone else would need to use them. 

So we do use them ourselves, then... 

Basically, I think we should either make the whole thing package protected, or else expose every method we use and make sure it is unit tested. And if time does not permit the test coverage, it should not be public.