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<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?
>
>
>
>
>
>
> --
> "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
-~----------~----~----~----~------~----~------~--~---
