[gwt-contrib] Re: Code review on date picker branch to date
On Thu, Nov 20, 2008 at 11:21 AM, Emily Crutcher <[EMAIL PROTECTED]> 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. > >>> >> > > > --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: Code review on date picker branch to date
> > > 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. > > -- > John A. Tamplin > Software Engineer (GWT), Google > > > > -- "There are only 10 types of people in the world: Those who understand binary, and those who don't" --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: Code review on date picker branch to date
> > 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. > >> > --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: Code review on date picker branch to date
On Thu, Nov 20, 2008 at 10:55 AM, Emily Crutcher <[EMAIL PROTECTED]> 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. -- John A. Tamplin Software Engineer (GWT), Google --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: Code review on date picker branch to date
On Thu, Nov 20, 2008 at 10:23 AM, John Tamplin <[EMAIL PROTECTED]> wrote: > On Thu, Nov 20, 2008 at 10:04 AM, Emily Crutcher <[EMAIL PROTECTED]> 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. > 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 A. Tamplin > Software Engineer (GWT), Google > > > > > -- "There are only 10 types of people in the world: Those who understand binary, and those who don't" --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: Code review on date picker branch to date
On Thu, Nov 20, 2008 at 10:32 AM, Ray Ryan <[EMAIL PROTECTED]> wrote: > "Long term"? 1.6, yes? > > On Thu, Nov 20, 2008 at 10:07 AM, Emily Crutcher <[EMAIL PROTECTED]> 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! --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: Code review on date picker branch to date
"Long term"? 1.6, yes?
On Thu, Nov 20, 2008 at 10:07 AM, Emily Crutcher <[EMAIL PROTECTED]> wrote:
> DateBox should implement the HasValue interface long term, which using the
> new terminology, does basically what you expect here.
>
>
>
> On Thu, Nov 20, 2008 at 4:41 AM, dflorey <[EMAIL PROTECTED]> wrote:
>
>>
>> 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.
>>
>> On 20 Nov., 04:26, Ray Ryan <[EMAIL PROTECTED]> 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?
>> >
>> > 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 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
>> > 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
>> >
>>
[gwt-contrib] Re: Code review on date picker branch to date
On Thu, Nov 20, 2008 at 10:04 AM, Emily Crutcher <[EMAIL PROTECTED]> wrote:
>
>
> On Wed, Nov 19, 2008 at 10:26 PM, Ray Ryan <[EMAIL PROTECTED]> 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 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 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
[gwt-contrib] Re: Code review on date picker branch to date
On Thu, Nov 20, 2008 at 10:04 AM, Emily Crutcher <[EMAIL PROTECTED]> 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 --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: Code review on date picker branch to date
DateBox should implement the HasValue interface long term, which using the
new terminology, does basically what you expect here.
On Thu, Nov 20, 2008 at 4:41 AM, dflorey <[EMAIL PROTECTED]> wrote:
>
> 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.
>
> On 20 Nov., 04:26, Ray Ryan <[EMAIL PROTECTED]> 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?
> >
> > 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 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
> > 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
[gwt-contrib] Re: Code review on date picker branch to date
On Wed, Nov 19, 2008 at 10:26 PM, Ray Ryan <[EMAIL PROTECTED]> 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 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
> 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
[gwt-contrib] Re: Code review on date picker branch to date
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.
On 20 Nov., 04:26, Ray Ryan <[EMAIL PROTECTED]> 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?
>
> 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 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
> 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 b
