Closed
Bug 349520
Opened 18 years ago
Closed 18 years ago
working hours for calendar-multiday-view
Categories
(Calendar :: Calendar Frontend, defect, P1)
Tracking
(Not tracked)
VERIFIED
FIXED
Sunbird 0.5
People
(Reporter: thomas.benisch, Assigned: thomas.benisch)
References
Details
Attachments
(12 files, 7 obsolete files)
47.97 KB,
image/png
|
Details | |
47.84 KB,
image/png
|
Details | |
41.08 KB,
image/png
|
Details | |
40.57 KB,
image/png
|
Details | |
51.18 KB,
image/png
|
Details | |
50.28 KB,
image/png
|
Details | |
50.25 KB,
image/png
|
Details | |
47.33 KB,
image/png
|
Details | |
24.17 KB,
image/png
|
Details | |
25.84 KB,
image/png
|
Details | |
66.83 KB,
patch
|
mattwillis
:
first-review+
|
Details | Diff | Splinter Review |
67.75 KB,
patch
|
Details | Diff | Splinter Review |
The calendar-multiday-view should support working hours.
This includes:
- different background colour for working and non-working hours
- preferences for start and end time of working hours
The UI in the Tools/Options dialog may be similar to the recently
removed UI for the start and end time of the view.
*** Bug 349866 has been marked as a duplicate of this bug. ***
some ideas for this bug to improve usability :
- allow custom working time
- allow only show working time
- allow automatic extend if there's a special event running out of working hours (I mean usually there's nothing during the night so I don't wan't to see it but if exceptionnally, I have a event during off hours, the view should extend automagically to include it)
- add a visual effect if some event is out of the window, so the user knows it is interesting to scroll down/up
- if the window is too small to have the whole day and it's possible to move the scrollbar to show more events, do it automatically (exemple : the default view shows 8h00-18h00, there's one event 14h00-15h00 and another event 18h30-23h00 which is seen by default. By automatically scrolling of 5 hours, the screen will show 13h00-23h00, which will include both events )
Comment 3•18 years ago
|
||
*** Bug 356449 has been marked as a duplicate of this bug. ***
Comment 4•18 years ago
|
||
tbe's salient point from bug 356449:
> I think the main point is, that a user is not interested in configuring the
> start/end time for whole view, but rather in configuring the start/end time for
> the visible region. Therefore an option would be to display all working hours in
> the visible region and adjust the height of the grid boxes accordingly. The
> non-working hours would be accessible by scrolling up or down.
Comment 5•18 years ago
|
||
Please add the ability to customize the hours on the single day view.
Comment 6•18 years ago
|
||
Per newsgroup thread and user feedback, marking this blocking-calendar0.5+
Flags: blocking-calendar0.5+
OS: Windows XP → All
Hardware: PC → All
Whiteboard: swag: 2d
Target Milestone: --- → Sunbird 0.5
Version: unspecified → Sunbird 0.3
Comment 7•18 years ago
|
||
Personally, being able to restrict the time range of displayed hours in order to better utilize available screen space is my #1 'fault' with an otherwise very promising product.
Perhaps limiting the time ranges for the 'work week' view, while leaving the standard view alone, might make for a good solution.
Comment 8•18 years ago
|
||
Bumping the priority on this.
This is on my "must fix" hit list for 0.5. We'll need to make some decisions soon about how to fix this.
Priority: -- → P1
Comment 10•18 years ago
|
||
Bug 366989 (Grid controls) has a prototype implementation that provides controls to zoom in (decrease hours in view) or zoom out (increase hours in view), and the setting is persistent. This should satisfy requests for ways to change the number of hours in the views, and associated preferences, which seem to be the high priority parts of this request.
Assignee | ||
Comment 11•18 years ago
|
||
This patch implements working hours for the calendar-multiday-view.
I added the following preferences for the start and end time of
working hours:
calendar.view.startworkhour
calendar.view.endworkhour
Those preferences can be set in the Tools/Options dialog.
Sunbird: I added a 'Start time' and a 'End time' field
to the workweek groupbox.
Lightning: In order to align the Sunbird and the Lightning
views options, I added a workweek groupbox with
'Start time' and 'End time' field. In addition
I moved the 'Start the week on' field into a
'General' groupbox.
I used different background colors for working hours and non-working
hours in the week and in the day (!) view. As working hours refer
to working days only, I aligned the background colors for non-working
days and non-working hours.
The size of the grid boxes is adjusted at application startup and
when resizing the application window in that way, that the number
of hours shown in the visible area is equal to the number of working
hours. In addition, at application startup the view scrolls to the
first working hour.
Attachment #252323 -
Flags: ui-review?(dmose)
Attachment #252323 -
Flags: first-review?(lilmatt)
Assignee | ||
Comment 12•18 years ago
|
||
Assignee | ||
Comment 13•18 years ago
|
||
Assignee | ||
Comment 14•18 years ago
|
||
Assignee | ||
Comment 15•18 years ago
|
||
Assignee | ||
Comment 16•18 years ago
|
||
Assignee | ||
Comment 17•18 years ago
|
||
Comment 18•18 years ago
|
||
We should really align the available settings in the options window between Sunbird and Lightning (in another bug).
That being said, why can the start and end hour in Lightning be entered in a textbox and in Sunbird it can just be selected from a combobox? IMO it shouldn't be possible to enter a value here?
Assignee | ||
Comment 19•18 years ago
|
||
(In reply to comment #18)
You're absolutely right.
I used textboxes in Lightning because the former start and end time
fields I removed in bug #133107 used the same approach.
But I agree, that the Lightning and Sunbird options should be aligned,
that means to use comboboxes also for Lightning.
Before I start to modify this patch, I'd like to have a comment from
the UI reviewers.
Assignee | ||
Comment 20•18 years ago
|
||
revised version of working hours patch
Attachment #252323 -
Attachment is obsolete: true
Attachment #252587 -
Flags: ui-review?(dmose)
Attachment #252587 -
Flags: first-review?(lilmatt)
Attachment #252323 -
Flags: ui-review?(dmose)
Attachment #252323 -
Flags: first-review?(lilmatt)
Comment 21•18 years ago
|
||
I'm holding off on review until ui-r happens.
I'll try to poke mvl/dmose about that today.
Comment 22•18 years ago
|
||
The resizing behavior looks good.
Without this patch, we already have four possible background colors for unused boxes, which already kinda feels like too many. The weekview screenshots here now show six, which to me seems like confusing visual clutter. I'm not entirely sure what to do about this however. Suggestions?
Re the prefs, dropdown menus (a la Sunbird) does sound like the better choice. One thing that seems unclear to me is that, as a naive user, looking at the pref windows, I wouldn't know what the semantics of something being part of the "work week" would actually be.
One possible way to address both issues would be to simply ditch the background color changes for non-work hours/days entirely, and change the labels to something like "Default Hours in View".
Thoughts?
Assignee | ||
Comment 23•18 years ago
|
||
(In reply to comment #22)
> The resizing behavior looks good.
>
> Without this patch, we already have four possible background colors for unused
> boxes, which already kinda feels like too many. The weekview screenshots here
> now show six, which to me seems like confusing visual clutter. I'm not
> entirely sure what to do about this however. Suggestions?
You're right, that there are really too much colors in the week view.
That's why I choose the same color for non-working hours and weekend days.
Nevertheless there are six colors left.
I think this problem can be attacked by the following strategy.
In the mid-term I would remove the selected day feature and allow to select
half hour boxes only. Then one can use the same selection color for working
hours and non-working hours. Another option would be to make the today
colors less prominent.
In addition Christian Jansen is working on a new view design. I think his
approach is to have the view less colored, but probably Christian can comment
this.
> Re the prefs, dropdown menus (a la Sunbird) does sound like the better choice.
I think everybody agrees here.
> One thing that seems unclear to me is that, as a naive user, looking at the
> pref windows, I wouldn't know what the semantics of something being part of the
> "work week" would actually be.
I didn't get this point, can you please elaborate.
> One possible way to address both issues would be to simply ditch the background
> color changes for non-work hours/days entirely, and change the labels to
> something like "Default Hours in View".
I personally would favour an additional preference, e.g. number of displayed
hours, in the long-term. But this conflicts with gekacheka's approach in
Bug 366989.
Your proposal of renaming working hours to default hours in view and removing
the different background color for those hours definitely kills the working
hours feature. Therefore I'd like to have a clear statement, if you want to
have working hours or not.
Comment 24•18 years ago
|
||
Comment on attachment 252587 [details] [diff] [review]
working hours v2
Removing review request until this makes it through ui-r
Attachment #252587 -
Flags: first-review?(lilmatt)
Comment 25•18 years ago
|
||
(In reply to comment #23)
>
> I think this problem can be attacked by the following strategy.
> In the mid-term I would remove the selected day feature and allow to select
> half hour boxes only. Then one can use the same selection color for working
> hours and non-working hours. Another option would be to make the today
> colors less prominent.
I just looked at iCal, and the way they handle it is a combination of very faint colors and not having the timescale be in separately colored boxes. mvl, any thoughts here?
> > One thing that seems unclear to me is that, as a naive user, looking at the
> > pref windows, I wouldn't know what the semantics of something being part of the
> > "work week" would actually be.
>
> I didn't get this point, can you please elaborate.
As an end user, when I first happen to be in the prefs window of a new program, for whatever reason, I typically explore a bit, to see what exactly is configurable in a program. If I ran across the pref window pictured, I would be confused, in the sense that I'd wonder "what does changing these settings actually do?". Of course, the existing dialog text already has this issue. :-/
> > One possible way to address both issues would be to simply ditch the background
> > color changes for non-work hours/days entirely, and change the labels to
> > something like "Default Hours in View".
>
> I personally would favour an additional preference, e.g. number of displayed
> hours, in the long-term. But this conflicts with gekacheka's approach in
> Bug 366989.
>
> Your proposal of renaming working hours to default hours in view and removing
> the different background color for those hours definitely kills the working
> hours feature. Therefore I'd like to have a clear statement, if you want to
> have working hours or not.
So what you call "the working hours feature" seems to me to be a collection of sub-features. In particular:
a) sets the number of hours shown
b) chooses what time to scroll to at startup
c) tweaks the background colors
d) relates a) and b) to the days of the workweek
It seems clear to me that a) and b) are important, c) seems useful but perhaps not critical, and d) I'm somewhat skeptical about. In particular, thinking about d) in terms of our two target users: if the student works, it's likely to be a part-time addition to school, so it's not likely that they'll want to only their work hours; the second user is defined as a small-business person in both work and personal life, which makes it seem reasonably likely that they'll want to view more than just their work hours at once also. As such, I think it's both clearer to first viewers of the pref dialog, and more accurate for our target users not to label these hours as "work hours". iCal does the additional preference thing that you suggest, and it uses the following UI strings, which seem pretty reasonable:
Day Starts At:
Day Ends At:
Show __ hours at a time
Comment 26•18 years ago
|
||
I think the most important reason for wanting to have a way to indicate work hours is that the non-workhours are scrolled out of view by default, and the available space can be used to make more room for work hours.
I'm not sure if non-work hours need to be colored. How about just a 2px line between work and non-work, instead of the default 1px?
If that doesn't work, I suggest with playing with brightness and keeping the color the same.
And I agree with dmose's suggested wording of the pref panel. Don't link the work hours to work days.
Comment 27•18 years ago
|
||
As a keen user of Sunbird 0.2 & 0.3, I think the issue is about the default range of hours shown in the view.
These are not necessarily "work hours", they may include events before breakfast or after dinner, there may not even be any "work" as such. Most people whether students, office workers, or business people think in terms of a "normal" day e.g. 6am - midnight, or 8am to 10pm, etc, depending on their lifestyle. Users will mostly prefer to see that chosen range by default, but with automatic resizing for unusual events say at 3am, as already suggested in this thread by others.
The key problem is that Sunbird 0.2 did this nicely, but 0.3 always displays the least likely scenario, 24hour day everyday, with no option to customise it.
I agree with those who find the term "workday hours" either ambiguous or misleading, and suggest "default viewing hours" or similar is much clearer.
Assignee | ||
Comment 28•18 years ago
|
||
I'll try to summarize the feedback from UI review and refer to the
sub-features of comment #25:
a) sets the number of hours shown
------------------------------
I think there's an agreement to have preferences for start and end time.
In the Tools/Options dialog those preferences are called
Day Starts At:
Day Ends At:
Those preferences can be configured by a combobox.
Also the current resizing behaviour, which adjusts the size of the grid
boxes to the number of hours between start and end time, seems to be
accepted.
Open questions:
- Where to place those preferences?
The Workweek groupbox seems to be disfavoured.
Proposal: Add them to the General groupbox below 'Start the week on'.
- What about a preference for the number of displayed hours, called
'Show __ hours at a time' in iCal?
Is this something you want to have?
If yes, when (this patch or later)?
Please not, that the resizing behaviour will change in this case,
that means the size of the grid boxes is determined by this preference.
b) chooses what time to scroll to at startup
-----------------------------------------
It seems to be accepted to scroll to the hour defined by the start time
preference.
Note, that this behaviour might change, if a number of displayed hours
preference is added.
Proposal: If the number of displayed hours preference is added, scroll
in that way, that the display hour range is centered in the
visible region.
c) tweaks the background colors
----------------------------
I think those colors are important. I personally don't like the 2px proposal
from mvl and prefer color boxes. Playing with brightness of the same color is
actually what I did in the patch (at least I tried to). Probably playing with
opacity gives some better results.
d) relates a) and b) to the days of the workweek
---------------------------------------------
I interpret your comments in that way, that you don't want to have this
feature. I will accept this, but what I really don't understand is the fact,
that you agreed to have workdays but don't want to have working hours.
For me that's rather inconsistent and by reusing your statements about
our target users I can argue for removing working days.
Comment 29•18 years ago
|
||
> I think there's an agreement to have preferences for start and end time.
> In the Tools/Options dialog those preferences are called
> Day Starts At:
> Day Ends At:
> Those preferences can be configured by a combobox.
Yes.
> Also the current resizing behaviour, which adjusts the size of the grid
> boxes to the number of hours between start and end time, seems to be
> accepted.
Depends on whether you want to make the "show ___ hours at a time"
preference part of this patch or not.
> Open questions:
>
> - Where to place those preferences?
> The Workweek groupbox seems to be disfavoured.
> Proposal: Add them to the General groupbox below 'Start the week on'.
Sounds good.
> - What about a preference for the number of displayed hours, called
> 'Show __ hours at a time' in iCal?
> Is this something you want to have?
Yes.
> If yes, when (this patch or later)?
Your choice.
> Please not, that the resizing behaviour will change in this case,
> that means the size of the grid boxes is determined by this preference.
That's fine.
> b) chooses what time to scroll to at startup
> -----------------------------------------
>
> It seems to be accepted to scroll to the hour defined by the start time
> preference.
>
> Note, that this behaviour might change, if a number of displayed hours
> preference is added.
>
> Proposal: If the number of displayed hours preference is added, scroll
> in that way, that the display hour range is centered in the
> visible region.
If I'm understanding you correctly, by "display hour range", you're
referring to the set of hours bounded by dayStartsAt and dayEndsAt.
Correct? If so, that works if displayedHours >= dayEndsAt - dayStartsAt,
but seems strange in the other case. It seems to me that "scroll to
dayStartsAt" works in both situations. Is there some disadvantage to that
I'm missing?
> c) tweaks the background colors
> ----------------------------
>
> I think those colors are important. I personally don't like the 2px proposal
> from mvl and prefer color boxes. Playing with brightness of the same color
> is actually what I did in the patch (at least I tried to). Probably playing
> with opacity gives some better results.
The 2px proposal has the problem that it's less visually clear that
something is "out of the normal space". I'd be interested in seeing
screenshots of much fainter colors here. How would changing the
opacity make any difference at all?
> d) relates a) and b) to the days of the workweek
> ---------------------------------------------
>
> I interpret your comments in that way, that you don't want to have this
> feature.
Correct.
> I will accept this, but what I really don't understand is the fact,
> that you agreed to have workdays but don't want to have working hours.
> For me that's rather inconsistent and by reusing your statements about
> our target users I can argue for removing working days.
I don't believe these are "working hours". I believe that they are
the hours that the day starts and ends at, but for most of our target
users, this will include more than just the hours that they happen to
be at work.
Comment 30•18 years ago
|
||
W.r.t. why have workdays, this is a visual indicator for when you can easily schedule events (e.g. trips) that take a day or more.
Comment 31•18 years ago
|
||
re WorkDays vs Working Hours, I agree with Dan M ...
quoting Dan...
"I don't believe these are "working hours". I believe that they are
the hours that the day starts and ends at, but for most of our target
users, this will include more than just the hours that they happen to
be at work."
Most ppl incl myself work Mon-Fri, so work days means I'll mostly be at work, but may want to schedule events before/after work. So working hours of 9-5 have little relevance to my schedule, I may want to see by default my waking hours say 6am - midnight for example. In my case I only use Sunbird at home, and the "working hours" are simply blocked out 9-5 with a recurring event. The only events of interest to me are before/after working hours on the work days, i.e what I aim to get done that day other than work, but on non-workdays, the interesting events cover the full days hours.
So while I may occassionally note an event for 3AM e.g. a software rollout or similar for work, done from home, most of the time, I only want to see from 6am to midnight, which I would set as my default viewing/scrolling window.
At the same time the workdays concept is good because not everyone works Mon-Fri, my son for example is currently working Tues,Wed,Fri only.
The earlier Sunbird 0.2 actually did this quite nicely, I'm puzzled why this feature disappeared from 0.3
Updated•18 years ago
|
Whiteboard: swag: 2d → [needs ui-review]
Assignee | ||
Comment 32•18 years ago
|
||
week view after scrolling with fainter colors
Assignee | ||
Comment 33•18 years ago
|
||
(In reply to comment #29)
> The 2px proposal has the problem that it's less visually clear that
> something is "out of the normal space". I'd be interested in seeing
> screenshots of much fainter colors here. How would changing the
> opacity make any difference at all?
You're right, that using opacity won't solve the original problem.
For a screenshot with fainter colors please have a look at the
attachment of comment #32.
Assignee | ||
Comment 34•18 years ago
|
||
(In reply to comment #29)
> If I'm understanding you correctly, by "display hour range", you're
> referring to the set of hours bounded by dayStartsAt and dayEndsAt.
> Correct? If so, that works if displayedHours >= dayEndsAt - dayStartsAt,
> but seems strange in the other case. It seems to me that "scroll to
> dayStartsAt" works in both situations. Is there some disadvantage to that
> I'm missing?
You're right. In the case displayedHours >= dayEndsAt - dayStartsAt
centering the display hour range looks better, but in the other case
it's really strange and probably not what the user wants.
Therefore I will use your proposal and scroll to dayStartsAt.
Comment 35•18 years ago
|
||
Comment on attachment 256457 [details]
week view after scrolling with fainter colors
What is the reason for
1. changing color of current day?
2. changing color of selected day?
3. changing color of non-workweek days?
Assignee | ||
Comment 36•18 years ago
|
||
(In reply to comment #35)
> (From update of attachment 256457 [details])
This screenshot is a proposal which I made due to comment #29
from Dan: "I'd be interested in seeing screenshots of much
fainter colors here."
My original proposal used the same colors for selection and today,
but wasn't favored by Dan and Michiel. Of course there's also
the possibility to improve the original proposal by choosing better
colors for "non-working" hours (e.g. lighter gray) and for the
overlap regions (blue-gray and yellow-gray).
It seems that choosing the right colors is the only problem
left for getting UI review. Therefore any help or suggestions
are appreciated.
Assignee | ||
Comment 37•18 years ago
|
||
This is a modified version of my original proposal.
The colors for the selected and current day are
not changed for "working hours", but only for
"non-working hours". In addition, the gray for
"non-working hours" is a little bit brighter.
Assignee | ||
Comment 38•18 years ago
|
||
(In reply to comment #37)
> The colors for the selected and current day are
> not changed for "working hours", but only for
Uups, this statement is wrong. I did change
the colors for the selected day. Unfortunately I
mixed something up. A new screenshot follows.
Assignee | ||
Comment 39•18 years ago
|
||
This is a modified version of my original proposal.
The colors for the selected and current day are
not changed for "working hours", but only for
"non-working hours". In addition, the gray for
"non-working hours" is a little bit brighter.
Assignee | ||
Comment 40•18 years ago
|
||
Just for comparison, this is the status quo of the
week view. Due to the yellow used for weekend days,
I think that my proposal from comment #39
(week view after scrolling v4) appears to be even
less colorful.
Comment 41•18 years ago
|
||
(In reply to comment #40)
> Created an attachment (id=256619) [details]
> week view after scrolling (status quo)
>
> Just for comparison, this is the status quo of the
> week view. Due to the yellow used for weekend days,
> I think that my proposal from comment #39
> (week view after scrolling v4) appears to be even
> less colorful.
I agree; this looks nice! ui-r=dmose. Thanks for you patience sorting through these issues.
Comment 42•18 years ago
|
||
Comment on attachment 252587 [details] [diff] [review]
working hours v2
I think the version of this patch for which I've just given ui-review+ hasn't yet been posted, so removing the ui-r? from this one. Feel free to mark the patch corresponding to v4 as ui-r+ once it's up.
Attachment #252587 -
Flags: ui-review?(dmose)
Assignee | ||
Comment 43•18 years ago
|
||
Attachment #252324 -
Attachment is obsolete: true
Assignee | ||
Comment 44•18 years ago
|
||
Attachment #252325 -
Attachment is obsolete: true
Assignee | ||
Comment 45•18 years ago
|
||
This patch addresses all comments from ui review.
I added the following preferences for the day start hour
and day end hour:
calendar.view.daystarthour
calendar.view.dayendhour
In addition I added a preference for the number of
visible hours:
calendar.view.visiblehours
In previous discussions there was consent of adding
those preferences to the 'General' groupbox
of the Lightning/Sunbird options dialog.
As those preferences refer to the day and week view
only, I changed my mind and added a 'Day and Week View'
groubox. Those changes can be seen in the screenshots
from comments #43 and #44.
Due to those changes I didn't carry over the ui review
flag and ask for ui review again.
Attachment #252587 -
Attachment is obsolete: true
Attachment #256793 -
Flags: ui-review?(dmose)
Attachment #256793 -
Flags: first-review?(lilmatt)
Comment 46•18 years ago
|
||
Comment on attachment 256793 [details] [diff] [review]
working hours v4
ui-review+ = dmose, if you change the verbiage from "Day and Week View" to "Day and Week Views".
Attachment #256793 -
Flags: ui-review?(dmose) → ui-review+
Updated•18 years ago
|
Status: NEW → ASSIGNED
Whiteboard: [needs ui-review] → [ui-review+][needs review lilmatt]
Assignee | ||
Comment 47•18 years ago
|
||
This patch addresses comment #46. In addition I fixed a scrolling
problem, which appeared only in Sunbird.
I carried over the ui-review flag from the last patch.
Attachment #256793 -
Attachment is obsolete: true
Attachment #256916 -
Flags: ui-review+
Attachment #256916 -
Flags: first-review?(lilmatt)
Attachment #256793 -
Flags: first-review?(lilmatt)
Updated•18 years ago
|
Whiteboard: [ui-review+][needs review lilmatt] → [ui-review+][needs review lilmatt][l10n impact]
Comment 48•18 years ago
|
||
Comment on the preference dialog:
Sunbird and Lightning now share the same code. You'll only have to add the new controls to m/calendar/base/content/preferences/views.xul, ship views.js in m/calendar/base/jar.mn and call gViewsPane.init() from preferences.xul and messenger-overlay-preferences.xul.
I'm not sure if all the content will fit into Lightning pane due to the extra space required for tabs. One solution might be to merge the General and Workweek section into one section or to hide the Multiweek section in Lightning (e.g. via css rule).
Assignee | ||
Comment 49•18 years ago
|
||
(In reply to comment #48)
> Comment on the preference dialog:
>
> Sunbird and Lightning now share the same code. You'll only have to add the new
> controls to m/calendar/base/content/preferences/views.xul, ship views.js in
> m/calendar/base/jar.mn and call gViewsPane.init() from preferences.xul and
> messenger-overlay-preferences.xul.
Thanks for those hints.
> I'm not sure if all the content will fit into Lightning pane due to the extra
> space required for tabs. One solution might be to merge the General and
> Workweek section into one section or to hide the Multiweek section in Lightning
> (e.g. via css rule).
You're right, there's not enough space in the views tab. I prefer to hide
the Multiweek section simply due to the fact that Lightning doesn't have
a multiweek view at all. I think instead of adding a css rule the multiweek
groupbox can be hided in messenger-overlay-preferences.xul, because that's
what overlays are for.
Assignee | ||
Comment 50•18 years ago
|
||
This is a new revision of the patch which broke due to the checkin for
Bug 372014. As the Lightning and Sunbird preferences now share the
same code, I adjusted the patch accordingly.
In addition I removed the multiweek groupbox from the Lightning
views option tab, see also comment #49. I'm not sure if this
requires a new UI review, but I consider this as a bug which came in
with 372014.
Attachment #256916 -
Attachment is obsolete: true
Attachment #257964 -
Flags: first-review?(lilmatt)
Attachment #256916 -
Flags: first-review?(lilmatt)
Comment 51•18 years ago
|
||
Comment on attachment 257964 [details] [diff] [review]
working hours v6
I already asked tbe in IRC to make the changes to day and week view in a common location, rather than duplicating code.
>+++ mozilla/calendar/base/content/preferences/views.xul 2007-03-09 12:31:04.921875000 +0100
>+ <hbox align="center" pack="center">
>+ <label value="&pref.calendar.view.visiblehours.label;"
>+ accesskey="&pref.calendar.view.visiblehours.accesskey;"
>+ control="visiblehours" />
Remove the space before the /
>+++ mozilla/calendar/locales/en-US/chrome/calendar/preferences/views.dtd 2007-03-09 12:27:49.890625000 +0100
>+<!ENTITY pref.calendar.view.daystart.label "Day starts at:">
>+<!ENTITY pref.calendar.view.daystart.accesskey "d">
This should be a capital D ^^^
>+<!ENTITY pref.calendar.view.dayend.label "Day ends at:">
>+<!ENTITY pref.calendar.view.dayend.accesskey "y">
>+<!ENTITY pref.calendar.view.visiblehours.label "Show">
Add a : to this ^^^^ "Show:"
>+++ mozilla/calendar/base/jar.mn 2007-03-09 12:27:49.843750000 +0100
>+ content/calendar/preferences/views.js (content/preferences/views.js)
views.js appears to be missing from the patch.
Attachment #257964 -
Flags: first-review?(lilmatt) → first-review-
Assignee | ||
Comment 52•18 years ago
|
||
(In reply to comment #51)
> >+++ mozilla/calendar/base/jar.mn 2007-03-09 12:27:49.843750000 +0100
> >+ content/calendar/preferences/views.js (content/preferences/views.js)
> views.js appears to be missing from the patch.
views.js is an old file which was removed in Bug 133107. It was known, that this file
will be added later again. Therefore the file was only removed from the jar.mn, but
not from CVS. Therefore it's still in the CVS.
Comment 53•18 years ago
|
||
Thanks for clearing that up.
As discussed in IRC, you have r=lilmatt to land the .dtd changes (including the above review comments) today to beat the string freeze.
Assignee | ||
Comment 54•18 years ago
|
||
calendar/locales/en-US/chrome/calendar/preferences/views.dtd
checked in on HEAD and MOZILLA_1_8_BRANCH
due to string freeze
Assignee | ||
Comment 55•18 years ago
|
||
This patch addresses comment #51.
In order to prevent code duplication I added a
calendar-decorated-multiday-base-view binding,
where the calendar-decorated-day-view and
calendar-decorated-week-view derive binding from.
I also managed to share the code for the pref observer.
As in a derived binding the base method of an overriden
method cannot be called, I made the approach with the
handlePreference() and the handleCommonPreference()
method.
Attachment #257964 -
Attachment is obsolete: true
Attachment #258306 -
Flags: first-review?(lilmatt)
Comment 56•18 years ago
|
||
Comment on attachment 258306 [details] [diff] [review]
working hours v7
>+++ mozilla/calendar/base/content/calendar-decorated-day-view.xml 2007-03-12 16:20:41.906250000 +0100
>- <binding id="calendar-decorated-day-view" extends="chrome://calendar/content/calendar-decorated-base.xml#calendar-decorated-base-view">
>+ <binding id="calendar-decorated-day-view" extends="chrome://calendar/content/calendar-decorated-multiday-base-view.xml#calendar-decorated-multiday-base-view">
Since you're touching this line, please put "extends" beneath "id"
>+++ mozilla/calendar/base/content/calendar-decorated-multiday-base-view.xml 2007-03-12 17:18:48.750000000 +0100
>+<?xml version="1.0"?>
>+<!--
>+ - ***** BEGIN LICENSE BLOCK *****
Please format this like so:
<?xml version="1.0" encoding="UTF-8"?>
<!-- ***** BEGIN LICENSE BLOCK *****
>+ - ***** END LICENSE BLOCK *****
>+-->
Please format this like so:
- ***** END LICENSE BLOCK ***** -->
>+
>+<bindings id="calendar-specific-view-bindings"
>+ xmlns="http://www.mozilla.org/xbl"
>+ xmlns:html="http://www.w3.org/1999/xhtml"
>+ xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>+ xmlns:xbl="http://www.mozilla.org/xbl">
>+
>+ <binding id="calendar-decorated-multiday-base-view" extends="chrome://calendar/content/calendar-decorated-base.xml#calendar-decorated-base-view">
Please put "extends" beneath "id".
>+ <field name="mPrefObserver"><![CDATA[
>+ ({ calView: this,
>+ observe: function calDecWeekViewPrefChange(subj, topic, pref) {
This inner function name should probably be changed.
>+ this.calView.handlePreference(subj, topic, pref);
>+ return;
>+ }
>+ })
>+ ]]></field>
>+
>+ <method name="handlePreference">
>+ <parameter name="aSubject"/>
>+ <parameter name="aTopic"/>
>+ <parameter name="aPreference"/>
>+ <body><![CDATA[
>+ this.handleCommonPreference(aSubject, aTopic, aPreference);
>+ ]]></body>
>+ </method>
Do we really need this method since you're defining "handlePreference" in the child views?
>+ <method name="handleCommonPreference">
>+ <parameter name="aSubject"/>
>+ <parameter name="aTopic"/>
>+ <parameter name="aPreference"/>
>+ <body><![CDATA[
>+ aSubject.QueryInterface(Components.interfaces.nsIPrefBranch2);
>+
>+ switch (aPreference) {
Everything inside this switch statement should be indented 4 more spaces.
>+
>+ case "calendar.week.d0sundaysoff":
>+ case "calendar.week.d1mondaysoff":
>+ case "calendar.week.d2tuesdaysoff":
>+ case "calendar.week.d3wednesdaysoff":
>+ case "calendar.week.d4thursdaysoff":
>+ case "calendar.week.d5fridaysoff":
>+ case "calendar.week.d6saturdaysoff":
>+ debugger;
I'm guessing you didn't mean to leave this line ^^^ in
>+ if (!this.startDay || !this.endDay) {
>+ // Don't refresh if we're not initialized
>+ return;
>+ }
>+ this.updateDaysOffPrefs();
>+ this.goToDay(this.selectedDay);
>+ break;
>+
>+ case "calendar.view.daystarthour":
>+ this.mDayStartMin = aSubject.getIntPref(aPreference) * 60;
>+ var viewElem = document.getAnonymousElementByAttribute(
>+ this, "anonid", "view-element");
>+ viewElem.setDayStartEndMinutes(this.mDayStartMin,
>+ this.mDayEndMin);
>+ if (!this.startDay || !this.endDay) {
>+ // Don't refresh if we're not initialized
>+ return;
>+ }
>+ this.goToDay(this.selectedDay);
>+ break;
>+
>+ case "calendar.view.dayendhour":
>+ this.mDayEndMin = aSubject.getIntPref(aPreference) * 60;
>+ var viewElem = document.getAnonymousElementByAttribute(
>+ this, "anonid", "view-element");
>+ viewElem.setDayStartEndMinutes(this.mDayStartMin,
>+ this.mDayEndMin);
>+ if (!this.startDay || !this.endDay) {
>+ // Don't refresh if we're not initialized
>+ return;
>+ }
>+ this.goToDay(this.selectedDay);
>+ break;
>+
>+ case "calendar.view.visiblehours":
>+ var visibleMinutes = aSubject.getIntPref(aPreference) * 60;
>+ var viewElem = document.getAnonymousElementByAttribute(
>+ this, "anonid", "view-element");
>+ viewElem.setVisibleMinutes(visibleMinutes);
>+ if (!this.startDay || !this.endDay) {
>+ // Don't refresh if we're not initialized
>+ return;
>+ }
>+ this.goToDay(this.selectedDay);
>+ break;
>+
>+ case "calendar.timezone.local":
>+ var viewElem = document.getAnonymousElementByAttribute(
>+ this, "anonid", "view-element");
>+ viewElem.timezone = aSubject.getCharPref(aPreference);
>+ if (!this.startDay || !this.endDay) {
>+ // Don't refresh if we're not initialized
>+ return;
>+ }
>+ this.goToDay(this.selectedDay);
>+ break;
First off, I think we can either move the "var viewElem = doc..." line above the switch statement, since we use it in each case, or we could move it to its own method, since we use it in updateDaysOffPrefs below as well.
Secondly, I'd also like to move the "if (!this.startDay..." bit into a helper function, so as to reduce code duplication. Maybe it can simply go _after_ the case statement?
>+
>+ default:
>+ break;
>+ }
>+ return;
>+ ]]></body>
>+ </method>
>+
Trailing spaces on this line ^^^
>diff -x CVS -a -U 8 -pN -r mozilla_ref/calendar/base/content/calendar-decorated-week-view.xml mozilla/calendar/base/content/calendar-decorated-week-view.xml
>--- mozilla_ref/calendar/base/content/calendar-decorated-week-view.xml 2007-03-12 12:33:24.953125000 +0100
>+++ mozilla/calendar/base/content/calendar-decorated-week-view.xml 2007-03-12 16:54:46.453125000 +0100
>@@ -41,113 +41,70 @@
> <!-- Note that we depend on some helper functions in calUtils.js here-->
>
> <bindings id="calendar-specific-view-bindings"
> xmlns="http://www.mozilla.org/xbl"
> xmlns:html="http://www.w3.org/1999/xhtml"
> xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> xmlns:xbl="http://www.mozilla.org/xbl">
>
>- <binding id="calendar-decorated-week-view" extends="chrome://calendar/content/calendar-decorated-base.xml#calendar-decorated-base-view">
>+ <binding id="calendar-decorated-week-view" extends="chrome://calendar/content/calendar-decorated-multiday-base-view.xml#calendar-decorated-multiday-base-view">
Since you're touching this line, please put "extends" beneath "id"
>+ <method name="handlePreference">
>+ <parameter name="aSubject"/>
>+ <parameter name="aTopic"/>
>+ <parameter name="aPreference"/>
>+ <body><![CDATA[
>+ aSubject.QueryInterface(Components.interfaces.nsIPrefBranch2);
>+
>+ switch (aPreference) {
>+
>+ case "calendar.week.start":
>+ this.mWeekStartOffset = aSubject.getIntPref(aPreference);
>+ viewElem = document.getAnonymousElementByAttribute(
>+ this, "anonid", "view-element");
>+ viewElem.weekStartOffset = aSubject.getIntPref(aPreference);
>+ if (!this.startDay || !this.endDay) {
>+ // Don't refresh if we're not initialized
>+ return;
>+ }
>+ // Refresh the view so the settings take effect
>+ this.goToDay(this.selectedDay);
>+ break;
>+
>+ default:
>+ this.handleCommonPreference(aSubject, aTopic, aPreference);
>+ break;
>+ }
The contents of this switch statement should be indented 4 more spaces.
>diff -x CVS -a -U 8 -pN -r mozilla_ref/calendar/base/content/calendar-multiday-view.xml mozilla/calendar/base/content/calendar-multiday-view.xml
>--- mozilla_ref/calendar/base/content/calendar-multiday-view.xml 2007-03-12 12:33:25.031250000 +0100
>+++ mozilla/calendar/base/content/calendar-multiday-view.xml 2007-03-12 12:55:48.984375000 +0100
>@@ -1266,16 +1308,31 @@
> Ci.nsIScriptableDateFormat.timeFormatNoSeconds,
> endhr, endmin, 0);
>
> this.fgboxes.startlabel.setAttribute("value", startstr);
> this.fgboxes.endlabel.setAttribute("value", endstr);
>
> ]]></body>
> </method>
>+ <method name="setDayStartEndMinutes">
>+ <parameter name="aDayStartMin"/>
>+ <parameter name="aDayEndMin"/>
>+ <body><![CDATA[
>+ if (aDayStartMin < this.mStartMin || aDayStartMin > aDayEndMin ||
>+ aDayEndMin > this.mEndMin) {
>+ throw Components.results.NS_ERROR_INVALID_ARG;
>+ }
>+ if (this.mDayStartMin != aDayStartMin ||
>+ this.mDayEndMin != aDayEndMin) {
>+ this.mDayStartMin = aDayStartMin;
>+ this.mDayEndMin = aDayEndMin;
>+ }
>+ ]]></body>
>+ </method>
These should use 2 space indenting inside the if statments to match the rest of the file.
>@@ -2732,34 +2798,70 @@
> } else {
> minute = Math.round(x.value/this.mPixPerMin);
> }
> }
> return minute;
> ]]></body>
> </method>
>
>+ <method name="setFirstVisibleMinute">
>+ <parameter name="aMinute"/>
>+ <body><![CDATA[
>+ this.mFirstVisibleMinute = aMinute;
>+ ]]></body>
>+ </method>
also return this.mFirstVisibleMinute for convenience
>+ <method name="setVisibleMinutes">
>+ <parameter name="aVisibleMinutes"/>
>+ <body><![CDATA[
>+ if (aVisibleMinutes <= 0 ||
>+ aVisibleMinutes > (this.mEndMin - this.mStartMin)) {
>+ throw Components.results.NS_ERROR_INVALID_ARG;
>+ }
>+ if (this.mVisibleMinutes != aVisibleMinutes) {
>+ this.mVisibleMinutes = aVisibleMinutes;
>+ }
>+ ]]></body>
also return this.mVisibleMinutes for convenience
Please also use 2-space indenting for the inner methods. I know the file goes back and forth, but it looks like 2 is the most common in this file.
r=lilmatt with those nits addressed
Attachment #258306 -
Flags: first-review?(lilmatt) → first-review+
Assignee | ||
Comment 57•18 years ago
|
||
(In reply to comment #56)
> (From update of attachment 258306 [details] [diff] [review])
> >+++ mozilla/calendar/base/content/calendar-decorated-day-view.xml 2007-03-12 16:20:41.906250000 +0100
> >- <binding id="calendar-decorated-day-view" extends="chrome://calendar/content/calendar-decorated-base.xml#calendar-decorated-base-view">
> >+ <binding id="calendar-decorated-day-view" extends="chrome://calendar/content/calendar-decorated-multiday-base-view.xml#calendar-decorated-multiday-base-view">
> Since you're touching this line, please put "extends" beneath "id"
done
> >+++ mozilla/calendar/base/content/calendar-decorated-multiday-base-view.xml 2007-03-12 17:18:48.750000000 +0100
> >+<?xml version="1.0"?>
> >+<!--
> >+ - ***** BEGIN LICENSE BLOCK *****
> Please format this like so:
> <?xml version="1.0" encoding="UTF-8"?>
> <!-- ***** BEGIN LICENSE BLOCK *****
done
> >+ - ***** END LICENSE BLOCK *****
> >+-->
> Please format this like so:
> - ***** END LICENSE BLOCK ***** -->
done
> >+<bindings id="calendar-specific-view-bindings"
> >+ xmlns="http://www.mozilla.org/xbl"
> >+ xmlns:html="http://www.w3.org/1999/xhtml"
> >+ xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> >+ xmlns:xbl="http://www.mozilla.org/xbl">
> >+
> >+ <binding id="calendar-decorated-multiday-base-view" extends="chrome://calendar/content/calendar-decorated-base.xml#calendar-decorated-base-view">
> Please put "extends" beneath "id".
done
> >+ <field name="mPrefObserver"><![CDATA[
> >+ ({ calView: this,
> >+ observe: function calDecWeekViewPrefChange(subj, topic, pref) {
> This inner function name should probably be changed.
I changed the name to calDecMultidayViewPrefChange.
> >+ <method name="handlePreference">
> >+ <parameter name="aSubject"/>
> >+ <parameter name="aTopic"/>
> >+ <parameter name="aPreference"/>
> >+ <body><![CDATA[
> >+ this.handleCommonPreference(aSubject, aTopic, aPreference);
> >+ ]]></body>
> >+ </method>
> Do we really need this method since you're defining "handlePreference" in the
> child views?
I defined handlePreference only for the week view, for the day view the base
implementation is used. An alternative would be to define handlePreference also
for the day view and remove it from the multiday base binding. Nevertheless I
prefer the current approach, because the code path is more obvious
(mPrefObserver calls handlePreference, which calls handleCommonPreference).
But if you want that I change this, please let me know.
> >+ <method name="handleCommonPreference">
> >+ <parameter name="aSubject"/>
> >+ <parameter name="aTopic"/>
> >+ <parameter name="aPreference"/>
> >+ <body><![CDATA[
> >+ aSubject.QueryInterface(Components.interfaces.nsIPrefBranch2);
> >+
> >+ switch (aPreference) {
> Everything inside this switch statement should be indented 4 more spaces.
done
> >+ case "calendar.week.d0sundaysoff":
> >+ case "calendar.week.d1mondaysoff":
> >+ case "calendar.week.d2tuesdaysoff":
> >+ case "calendar.week.d3wednesdaysoff":
> >+ case "calendar.week.d4thursdaysoff":
> >+ case "calendar.week.d5fridaysoff":
> >+ case "calendar.week.d6saturdaysoff":
> >+ debugger;
> I'm guessing you didn't mean to leave this line ^^^ in
Uups, removed.
> >+ case "calendar.timezone.local":
> >+ var viewElem = document.getAnonymousElementByAttribute(
> >+ this, "anonid", "view-element");
> >+ viewElem.timezone = aSubject.getCharPref(aPreference);
> >+ if (!this.startDay || !this.endDay) {
> >+ // Don't refresh if we're not initialized
> >+ return;
> >+ }
> >+ this.goToDay(this.selectedDay);
> >+ break;
> First off, I think we can either move the "var viewElem = doc..." line above
> the switch statement, since we use it in each case, or we could move it to its
> own method, since we use it in updateDaysOffPrefs below as well.
I added a 'viewElem' property to the calendar-decorated-base-view binding.
In the calendar-decorated-multiday-base-view binding I used this property
instead of calling getAnonymousElementByAttribute().
There are about 30 locations in calendar-decorated-base.xml,
calendar-decorated-day-view.xml, calendar-decorated-week-view.xml,
calendar-decorated-multiweek-view.xml and calendar-decorated-month-view.xml
left, which can all use the viewElem property. If you want, I can adjust all
those locations, but probably that bloats this patch too much.
> Secondly, I'd also like to move the "if (!this.startDay..." bit into a helper
> function, so as to reduce code duplication. Maybe it can simply go _after_ the
> case statement?
This check cannot be moved after the case statement, because it's necessary
that the new pref settings are propagated to the view element. Therefore this
check only prevents a view refresh, which doesn't work, if the view is not
initialized (this.selectedDay gives an error).
While looking at the code I found another error. For the
'calendar.week.d...off' preferences the check is performed after the case
statement and should be moved before the goToDay statement.
Concerning code duplication:
a) Instead of checking this.startDay and this.endDay also the 'initialized'
property from calendar-decorated-base.xml can be used. This property
checks in addition this.displayCalendar, but this also works.
b) I personally prefer to have the return statement in the case statement,
because then the return is more obvious. But if you want I can also move
the check with return to a helper method in calendar-decorated-base.xml,
e.g. checkInitialized.
What do you think?
> >+
> >+ default:
> >+ break;
> >+ }
> >+ return;
> >+ ]]></body>
> >+ </method>
> >+
> Trailing spaces on this line ^^^
done
> >diff -x CVS -a -U 8 -pN -r mozilla_ref/calendar/base/content/calendar-decorated-week-view.xml mozilla/calendar/base/content/calendar-decorated-week-view.xml
> >--- mozilla_ref/calendar/base/content/calendar-decorated-week-view.xml 2007-03-12 12:33:24.953125000 +0100
> >+++ mozilla/calendar/base/content/calendar-decorated-week-view.xml 2007-03-12 16:54:46.453125000 +0100
> >@@ -41,113 +41,70 @@
> > <!-- Note that we depend on some helper functions in calUtils.js here-->
> >
> > <bindings id="calendar-specific-view-bindings"
> > xmlns="http://www.mozilla.org/xbl"
> > xmlns:html="http://www.w3.org/1999/xhtml"
> > xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> > xmlns:xbl="http://www.mozilla.org/xbl">
> >
> >- <binding id="calendar-decorated-week-view" extends="chrome://calendar/content/calendar-decorated-base.xml#calendar-decorated-base-view">
> >+ <binding id="calendar-decorated-week-view" extends="chrome://calendar/content/calendar-decorated-multiday-base-view.xml#calendar-decorated-multiday-base-view">
> Since you're touching this line, please put "extends" beneath "id"
done
> >+ <method name="handlePreference">
> >+ <parameter name="aSubject"/>
> >+ <parameter name="aTopic"/>
> >+ <parameter name="aPreference"/>
> >+ <body><![CDATA[
> >+ aSubject.QueryInterface(Components.interfaces.nsIPrefBranch2);
> >+
> >+ switch (aPreference) {
> >+
> >+ case "calendar.week.start":
> >+ this.mWeekStartOffset = aSubject.getIntPref(aPreference);
> >+ viewElem = document.getAnonymousElementByAttribute(
> >+ this, "anonid", "view-element");
> >+ viewElem.weekStartOffset = aSubject.getIntPref(aPreference);
> >+ if (!this.startDay || !this.endDay) {
> >+ // Don't refresh if we're not initialized
> >+ return;
> >+ }
> >+ // Refresh the view so the settings take effect
> >+ this.goToDay(this.selectedDay);
> >+ break;
> >+
> >+ default:
> >+ this.handleCommonPreference(aSubject, aTopic, aPreference);
> >+ break;
> >+ }
> The contents of this switch statement should be indented 4 more spaces.
done
> >diff -x CVS -a -U 8 -pN -r mozilla_ref/calendar/base/content/calendar-multiday-view.xml mozilla/calendar/base/content/calendar-multiday-view.xml
> >--- mozilla_ref/calendar/base/content/calendar-multiday-view.xml 2007-03-12 12:33:25.031250000 +0100
> >+++ mozilla/calendar/base/content/calendar-multiday-view.xml 2007-03-12 12:55:48.984375000 +0100
> >@@ -1266,16 +1308,31 @@
> > Ci.nsIScriptableDateFormat.timeFormatNoSeconds,
> > endhr, endmin, 0);
> >
> > this.fgboxes.startlabel.setAttribute("value", startstr);
> > this.fgboxes.endlabel.setAttribute("value", endstr);
> >
> > ]]></body>
> > </method>
> >+ <method name="setDayStartEndMinutes">
> >+ <parameter name="aDayStartMin"/>
> >+ <parameter name="aDayEndMin"/>
> >+ <body><![CDATA[
> >+ if (aDayStartMin < this.mStartMin || aDayStartMin > aDayEndMin ||
> >+ aDayEndMin > this.mEndMin) {
> >+ throw Components.results.NS_ERROR_INVALID_ARG;
> >+ }
> >+ if (this.mDayStartMin != aDayStartMin ||
> >+ this.mDayEndMin != aDayEndMin) {
> >+ this.mDayStartMin = aDayStartMin;
> >+ this.mDayEndMin = aDayEndMin;
> >+ }
> >+ ]]></body>
> >+ </method>
> These should use 2 space indenting inside the if statments to match the rest of
> the file.
done
> >@@ -2732,34 +2798,70 @@
> > } else {
> > minute = Math.round(x.value/this.mPixPerMin);
> > }
> > }
> > return minute;
> > ]]></body>
> > </method>
> >
> >+ <method name="setFirstVisibleMinute">
> >+ <parameter name="aMinute"/>
> >+ <body><![CDATA[
> >+ this.mFirstVisibleMinute = aMinute;
> >+ ]]></body>
> >+ </method>
> also return this.mFirstVisibleMinute for convenience
done
> >+ <method name="setVisibleMinutes">
> >+ <parameter name="aVisibleMinutes"/>
> >+ <body><![CDATA[
> >+ if (aVisibleMinutes <= 0 ||
> >+ aVisibleMinutes > (this.mEndMin - this.mStartMin)) {
> >+ throw Components.results.NS_ERROR_INVALID_ARG;
> >+ }
> >+ if (this.mVisibleMinutes != aVisibleMinutes) {
> >+ this.mVisibleMinutes = aVisibleMinutes;
> >+ }
> >+ ]]></body>
> also return this.mVisibleMinutes for convenience
done
> Please also use 2-space indenting for the inner methods. I know the file goes
> back and forth, but it looks like 2 is the most common in this file.
done
Comment 58•18 years ago
|
||
(In reply to comment #57)
> > Do we really need this method since you're defining "handlePreference" in the
> > child views?
>
> I defined handlePreference only for the week view, for the day view the base
> implementation is used. An alternative would be to define handlePreference also
> for the day view and remove it from the multiday base binding. Nevertheless I
> prefer the current approach, because the code path is more obvious
> (mPrefObserver calls handlePreference, which calls handleCommonPreference).
> But if you want that I change this, please let me know.
No, I think it's fine as is now that you've explained it. Maybe you want to add a comment in the base version explaining this?
> > First off, I think we can either move the "var viewElem = doc..." line above
> > the switch statement, since we use it in each case, or we could move it to its
> > own method, since we use it in updateDaysOffPrefs below as well.
>
> I added a 'viewElem' property to the calendar-decorated-base-view binding.
> In the calendar-decorated-multiday-base-view binding I used this property
> instead of calling getAnonymousElementByAttribute().
> There are about 30 locations in calendar-decorated-base.xml,
> calendar-decorated-day-view.xml, calendar-decorated-week-view.xml,
> calendar-decorated-multiweek-view.xml and calendar-decorated-month-view.xml
> left, which can all use the viewElem property. If you want, I can adjust all
> those locations, but probably that bloats this patch too much.
You're right about bloat. We should fix the places in the files that we're touching, and clearly use the viewElem in the new multiday base, but file a follow up to fix the rest.
> Concerning code duplication:
> a) Instead of checking this.startDay and this.endDay also the 'initialized'
> property from calendar-decorated-base.xml can be used. This property
> checks in addition this.displayCalendar, but this also works.
> b) I personally prefer to have the return statement in the case statement,
> because then the return is more obvious. But if you want I can also move
> the check with return to a helper method in calendar-decorated-base.xml,
> e.g. checkInitialized.
I still think I'd prefer the helper method if we can make it work simply to reduce code duplication. I haven't tested the .initialized bit so I'm not sure if we want to change to that at this point, but using a helper method would make it easy to do so. I think we should spin off another bug to use initialized after 0.5 so it can be tested thoroughly.
Assignee | ||
Comment 59•18 years ago
|
||
(In reply to comment #57)
> (In reply to comment #56)
> > (From update of attachment 258306 [details] [diff] [review] [details])
> While looking at the code I found another error. For the
> 'calendar.week.d...off' preferences the check is performed after the case
> statement and should be moved before the goToDay statement.
I also fixed that.
Assignee | ||
Comment 60•18 years ago
|
||
(In reply to comment #58)
> (In reply to comment #57)
> > > Do we really need this method since you're defining "handlePreference" in the
> > > child views?
> >
> > I defined handlePreference only for the week view, for the day view the base
> > implementation is used. An alternative would be to define handlePreference also
> > for the day view and remove it from the multiday base binding. Nevertheless I
> > prefer the current approach, because the code path is more obvious
> > (mPrefObserver calls handlePreference, which calls handleCommonPreference).
> > But if you want that I change this, please let me know.
>
> No, I think it's fine as is now that you've explained it. Maybe you want to
> add a comment in the base version explaining this?
done
> > > First off, I think we can either move the "var viewElem = doc..." line above
> > > the switch statement, since we use it in each case, or we could move it to its
> > > own method, since we use it in updateDaysOffPrefs below as well.
> >
> > I added a 'viewElem' property to the calendar-decorated-base-view binding.
> > In the calendar-decorated-multiday-base-view binding I used this property
> > instead of calling getAnonymousElementByAttribute().
> > There are about 30 locations in calendar-decorated-base.xml,
> > calendar-decorated-day-view.xml, calendar-decorated-week-view.xml,
> > calendar-decorated-multiweek-view.xml and calendar-decorated-month-view.xml
> > left, which can all use the viewElem property. If you want, I can adjust all
> > those locations, but probably that bloats this patch too much.
> You're right about bloat. We should fix the places in the files that we're
> touching, and clearly use the viewElem in the new multiday base, but file a
> follow up to fix the rest.
I filed bug 373888 as follow-up issue.
> > Concerning code duplication:
> > a) Instead of checking this.startDay and this.endDay also the 'initialized'
> > property from calendar-decorated-base.xml can be used. This property
> > checks in addition this.displayCalendar, but this also works.
> > b) I personally prefer to have the return statement in the case statement,
> > because then the return is more obvious. But if you want I can also move
> > the check with return to a helper method in calendar-decorated-base.xml,
> > e.g. checkInitialized.
> I still think I'd prefer the helper method if we can make it work simply to
> reduce code duplication. I haven't tested the .initialized bit so I'm not sure
> if we want to change to that at this point, but using a helper method would
> make it easy to do so. I think we should spin off another bug to use
> initialized after 0.5 so it can be tested thoroughly.
As discussed with lilmatt in IRC I didn't introduce a helper method.
[2007-03-13 21:04:06] <tbe> I found another problem with the helper function for checking if the view is initialized.
[2007-03-13 21:04:15] <lilmatt> ok?
[2007-03-13 21:04:53] <tbe> If I move the whole if statement into a helper function, the return statement is somehow useless, because return means return from that function.
[2007-03-13 21:05:18] <tbe> That means I must have a return in the case statement anyway.
[2007-03-13 21:05:41] <lilmatt> So ignore that and leave that as is
[2007-03-13 21:05:41] <tbe> In addition I want to have the no refresh comment in the case statement.
[2007-03-13 21:06:37] <tbe> O.K., I think in a follow-up one can use this.initialized instead of this.startDay and this.endDay
For using the initialized property I filed Bug 373892 as follow-up issue.
Assignee | ||
Comment 61•18 years ago
|
||
Just for reference, this is the patch I finally checked in.
Assignee | ||
Comment 62•18 years ago
|
||
patch checked in on HEAD and MOZILLA_1_8_BRANCH
Assignee | ||
Updated•18 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 63•18 years ago
|
||
Here's a list of follow-up issues:
Bug 349518 visual markers for out-of-sight events in calendar-multiday-view
Bug 373888 code cleanup: use viewElem property in decorated views
Bug 373892 use initialized property in decorated views
Bug 373898 rounding issues for grid boxes in calendar multiday view
Updated•18 years ago
|
Whiteboard: [ui-review+][needs review lilmatt][l10n impact] → [ui-review+][l10n impact]
Comment 64•18 years ago
|
||
VERIFIED with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.4pre) Gecko/20070522 Calendar/0.5pre
Status: RESOLVED → VERIFIED
Updated•17 years ago
|
Flags: blocking-calendar0.5+
Whiteboard: [ui-review+][l10n impact]
You need to log in
before you can comment on or make changes to this bug.
Description
•