Closed Bug 257428 Opened 20 years ago Closed 16 years ago

use alarms? -- preference on a per-calendar basis

Categories

(Calendar :: Preferences, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: s.marechal, Assigned: sebo.moz)

References

Details

Attachments

(3 files, 7 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7) Gecko/20040803 Firefox/0.9.3
Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7) Gecko/20040803 Firefox/0.9.3

I would like the option to publish a calendar without publishing their alarms.
This way, if you share your calender and other people view it they will not get
any of the alarm popups. This is very usefull in a business enviroment where
people will occasionally need to chech eachother's calenders. This option would
also be a usefull workaround  bug 212076 (
http://bugzilla.mozilla.org/show_bug.cgi?id=212076 ) that asks for alarm
acknowledgements everytime you refresh a remote calendar.

Reproducible: Always
Steps to Reproduce:
1.
2.
3.
How about a per-calendar option like, "Ignore alarms"? This way the alarm info
won't be lost in a publish and will be left as an option for the subscriber to
ignore them. Would be easier to implement too.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Come to think of it, that would probabely be a much cleaner solution :-) The
alarm options in Calendar are already there on a global basis, having the
sound/popup/show alarm options on a per calendar basis would certainly be
superior to my initial idea. And it will still remain a usefull workaround to
bug 212076

I'll change the summary to reflect this
Summary: add "Don't publish alarms" to preferences → alarm sound / popup box / show preferences on a per-calendar basis
To make this possible, I've added the suppressAlarms attribute to the oeICal
interface and the suppressAlarmsByDefault attribute to the oeICalContainer
interface:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=mozilla%2Fcalendar%2Flibxpical&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2004-09-01+13%3A11&maxdate=2004-09-01+13%3A11&cvsroot=%2Fcvsroot

The reason for the suppressAlarmsByDefault attribute is that addCalendar will
add the calendar and alarms will fire within this procedure so there is no use
in suppressing them after a call to addCalendar. So to make sure alarms are
suppressed in time, suppressAlarmsByDefault should be set to "true" and then
addCalendar should be called for the calendar which wants its alarms suppressed.

Another way of doing this was to add a third argument to the addCalendar
function but I thought that changing the API is not good and this way we can
easily add a "Ignore alarms by default" option in the preferences.
fix summary to be consolidated
Summary: alarm sound / popup box / show preferences on a per-calendar basis → use alarms? -- preference on a per-calendar basis
*** Bug 263487 has been marked as a duplicate of this bug. ***
*** Bug 243921 has been marked as a duplicate of this bug. ***
*** Bug 286466 has been marked as a duplicate of this bug. ***
*** Bug 297328 has been marked as a duplicate of this bug. ***
QA Contact: gurganbl → general
Attached patch patch v1 (obsolete) — Splinter Review
Patch actually implements suppressAlarms for all calendar providers.  The alarm service is already wired to check for it.  We may want to go back and remove some global prefs after that, but that can be evaluated after the whole pref situation settles down.
Assignee: mostafah → jminta
Status: NEW → ASSIGNED
Attachment #218826 - Flags: first-review?(mvl)
IMO suppressAlarms is still missing for calCompositeCalendar and calMemoryCalendar, isn't it?
Blocks: wcap
(In reply to comment #10)
> IMO suppressAlarms is still missing for calCompositeCalendar and
> calMemoryCalendar, isn't it?
> 
Those were intentionally left unimplemented, as it didn't make much semantical sense to me.  No one interacts directly with the memory calendar, so it wouldn't make sense there, and the composite calendar isn't really intended to hold much data of its own, as I understand it.  That's why things like uri and readOnly are also NS_ERROR_NOT_IMPLEMENTED.
(In reply to comment #11)
> (In reply to comment #10)
> > IMO suppressAlarms is still missing for calCompositeCalendar and
> > calMemoryCalendar, isn't it?
> > 
> Those were intentionally left unimplemented, as it didn't make much semantical
> sense to me.  No one interacts directly with the memory calendar, so it
> wouldn't make sense there, and the composite calendar isn't really intended to
> hold much data of its own, as I understand it.  That's why things like uri and
> readOnly are also NS_ERROR_NOT_IMPLEMENTED.
> 

Agree, though I would prefer NS_ERROR_NOT_IMPLEMENTED with comment over not implementing those at all, because one might think they've simply been forgotten... as you experience right now :)
No longer blocks: wcap
Can you provide screenshots of the changed UI?
Whiteboard: [cal-ui-review needed]
Attached image New Calendar screenshot (obsolete) —
Screenshot of the New Calendar wizard with the patch.
Attached image Edit Calendar screenshot (obsolete) —
Screenshot of the edit calendar dialog
Any reason to not continue the vertical layout of the dialog? The dialogs are not that cluttered, you can easily put the checkbox at the bottom.
In the edit calendar window, it might be reasonable to put the checkbox to the right of the read-only checkbox.  I don't have a strong opinion about the new calendar window.
*** Bug 357645 has been marked as a duplicate of this bug. ***
I am new to you discussion and read it with much interest. 
I think this a important bug to fix. The solution with the checkbox "display alarms" is probably sufficient although I think a deactivated calendar should simply be deactivated and nothing else.

Maybe it would be a good idea to change the text of this checkbox as new users will not understand this option without more information. 

I would suggest to use "display alarms from all calendars" or something like this. 

What happens then to the alarm of the active calendars? Are they also suppressed if the checkbox is unchecked?

Greeting from Cologne, Germany
Arni
Comment on attachment 218826 [details] [diff] [review]
patch v1

Moving first-review to Clint, since mvl's queue is pretty big and he can only cope with his ui-reviews and 2nd reviews.
Attachment #218826 - Flags: ui-review?(mvl)
Attachment #218826 - Flags: first-review?(mvl)
Attachment #218826 - Flags: first-review?(ctalbert.moz)
Comment on attachment 218826 [details] [diff] [review]
patch v1

I agree with the spirit of this patch, and it looks good. However, it is rather bitrotted.
Also, could spin-off bugs be filed to add the "suppressAlarms" additions to the WCAP and Google providers?

I will r- for now, but will look forward to an updated patch.

Patch output:

E:\BUILD\sunbird\mozilla\calendar>patch -p0 <alarmpref.diff
(Stripping trailing CRs from patch.)
patching file providers/caldav/calDavCalendar.js
Hunk #1 FAILED at 157.
1 out of 1 hunk FAILED -- saving rejects to file providers/caldav/calDavCalendar.js.rej
(Stripping trailing CRs from patch.)
patching file providers/ics/calICSCalendar.js
Hunk #1 FAILED at 115.
1 out of 1 hunk FAILED -- saving rejects to file providers/ics/calICSCalendar.js.rej
(Stripping trailing CRs from patch.)
patching file providers/storage/calStorageCalendar.js
Hunk #1 succeeded at 337 (offset 19 lines).
(Stripping trailing CRs from patch.)
patching file resources/content/calendarCreation.js
Hunk #1 succeeded at 112 (offset 9 lines).
(Stripping trailing CRs from patch.)
patching file resources/content/calendarCreation.xul
Hunk #1 succeeded at 130 (offset 1 line).
(Stripping trailing CRs from patch.)
patching file resources/content/calendarProperties.js
Hunk #1 FAILED at 51.
Hunk #2 succeeded at 74 (offset 2 lines).
1 out of 2 hunks FAILED -- saving rejects to file resources/content/calendarProperties.js.rej
(Stripping trailing CRs from patch.)
patching file resources/content/calendarProperties.xul
(Stripping trailing CRs from patch.)
can't find file to patch at input line 249
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|Index: resources/locale/en-US/calendar.dtd
|===================================================================
|RCS file: /cvsroot/mozilla/calendar/resources/locale/en-US/calendar.dtd,v
|retrieving revision 1.127
|diff -p -U8 -r1.127 calendar.dtd
|--- resources/locale/en-US/calendar.dtd        27 Mar 2006 21:00:58 -0000      1.127
|+++ resources/locale/en-US/calendar.dtd        18 Apr 2006 15:15:15 -0000
--------------------------
File to patch:locales/en-US/chrome/calendar/calendar.dtd
locales/en-US/chrome/calendar/calendar.dtd: No such file or directory
Skip this patch? [y] n
File to patch: calendar/locales/en-US/chrome/calendar/calendar.dtd
calendar/locales/en-US/chrome/calendar/calendar.dtd: No such file or directory
Skip this patch? [y] n
File to patch: locales\en-US\chrome\calendar\calendar.dtd
patching file 'locales\en-US\chrome\calendar\calendar.dtd'
Hunk #1 FAILED at 499.
1 out of 1 hunk FAILED -- saving rejects to file 'locales\en-US\chrome\calendar\calendar.dtd.rej'

E:\BUILD\sunbird\mozilla\calendar>
Attachment #218826 - Flags: first-review?(ctalbert.moz) → first-review-
If the only thing required for this to work with other providers is to connect calICalendar's supressAlarms with the calendar managers setCalendarPref(), then this is already implemented for the Google provider.
Comment on attachment 218826 [details] [diff] [review]
patch v1

Move UI review to Christian, to ease mvl's load.
Attachment #218826 - Flags: ui-review?(mvl) → ui-review?(christian.jansen)
Overall I have the feeling that this solution does not fit to the RFE.
The feature request says that users want have an option which allows them to publish a calendar without alarms.  Thus, I would not expect this option in the Wizard or the Properties dialog, but therefor in the "Publish Calendar" dialog.

+-----------------------------------------+
| Publish Calendar                      X |
+-----------------------------------------+
|                                         |
| Publishing URL: [                      ]|
|                  dfkdlsfkldskdsfkl      |
|                                         |
| [ ] Publish Alarms                      |
|                                         |
+-----------------------------------------+
|                         [[OK]] [Cancel] |
+-----------------------------------------+

Some general comments about the proposed design:

Properties Dialog:
*  The option should be located below, or above "Read Only"
*  The option should be visually aligned to "Read Only"

Calendar Creation Wizard:
I would recommend to remove the option from the wizard.

 * The option adds extra complexity
 * User's might not understand why they have to decide
   on this that early.
 * If turned off, users might don't know where to turn
   on, again.
(In reply to comment #24)
Christian, the reporter changed the RFE in Comment #2 to have the option to enable/disable displaying of alarms. That's what the patches are for.

See also Bug 333554.
Oops, I've overlooked that. Sorry. So, ignore my comments regarding publishing calendars.
(In reply to comment #24)
> Overall I have the feeling that this solution does not fit to the RFE.
> The feature request says that users want have an option which allows them to
> publish a calendar without alarms.  Thus, I would not expect this option in the
> Wizard or the Properties dialog, but therefor in the "Publish Calendar" dialog.


If you are going to implement this, don't forget that you need to do the same thing in the "New Calendar" dialog, because this can be used to create a public calendar without using the "Publish Calendar" menu item. Not sure if those end up using the same dialog/code.
Component: General → Preferences
QA Contact: general → preferences
Flags: blocking-calendar0.7?
Whiteboard: [cal-ui-review needed]
Re-assigning my bugs to nobody@mozilla.org due to recent developments.
Assignee: jminta → nobody
Status: ASSIGNED → NEW
why has this bug been reassigned? i was waiting for this one, or at least a feature that ignores alarms on read-only remote calendars.
Comment on attachment 218826 [details] [diff] [review]
patch v1

The patch has bitrotted. Removing UI-review until a new patch appears.
Attachment #218826 - Flags: ui-review?(christian.jansen)
Given our tight schedule and the fact that the Calendar devs already have too much on their plate for 0.7, this is unlikely to make it for 0.7 unless someone steps up and finishes this.
Flags: blocking-calendar0.7? → blocking-calendar0.7-
Shouldn't an option to only show alarms if you are an attendee be what we want?

Lightning would default yourself as an attendee since you created the event so you will still get an alarm.
Flags: blocking-calendar0.7- → wanted-calendar0.8+
Depends on: 400951
Its a little bit complicated this as it depends on how you use the shared calendars, I have a small office of 5 people who all update each others calendars (Therefore they are not read only)  but obviously only want to see alarms for their own calendars,  Also it is not just alarms, they also see every one else's tasks which they do not want, so maybe an approach that handles both these scenarios by marking a calendar as a certain type would be better. 
(In reply to comment #34)
> Its a little bit complicated this as it depends on how you use the shared
> calendars, I have a small office of 5 people who all update each others
> calendars (Therefore they are not read only)  but obviously only want to see
> alarms for their own calendars,  Also it is not just alarms, they also see
> every one else's tasks which they do not want, so maybe an approach that
> handles both these scenarios by marking a calendar as a certain type would be
> better. 
> 

This would nearly suit my purposes. My use case is several calendars I share (via WebDAV) with my wife. There is my own calendar (which is mostly work-related) for which I want to see tasks and receive alarms, and there is her calendar for which I do not, plus a couple of shared calendars for which there are neither tasks nor alarms. I do, however, want to be able to add tasks to her calendar and have her add tasks to mine.
Attached patch patch v2 (obsolete) — Splinter Review
Updated version of Joeys patch. "suppressAlarms" is now available via calendar.getProperty() and calendar.setProperty(). Screenshots coming up.
Assignee: nobody → sebo.moz
Attachment #218826 - Attachment is obsolete: true
Attachment #235304 - Attachment is obsolete: true
Attachment #235305 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #294904 - Flags: review?(philipp)
I kept the requested vertical layout and aligned label and checkbox with the upper elements.
Attachment #294905 - Flags: ui-review?(mvl)
Attached image Edit Calendar Screenshot v2 (obsolete) —
Attachment #294906 - Flags: ui-review?(mvl)
Attachment #294905 - Flags: ui-review?(mvl) → ui-review?(christian.jansen)
Attachment #294906 - Flags: ui-review?(mvl) → ui-review?(christian.jansen)
Comment on attachment 294904 [details] [diff] [review]
patch v2

Passing in the (boolean) property "suppressAlarms" as string is wrong, as mvl pointed out on IRC. However getProperty returns a boolean property as "0" (string) which causes problems. It looks like we need to fix the getProperty/setProperty implementation first.
Attachment #294904 - Attachment is obsolete: true
Attachment #294904 - Flags: review?(philipp)
Comment on attachment 294904 [details] [diff] [review]
patch v2

>Index: mozilla/calendar/resources/content/calendarCreation.xul
>===================================================================
>                 <row align="center">
>                     <label value="&calendarproperties.color.label;" control="calendar-color"/>
>                     <hbox align="center">
>                         <colorpicker id="calendar-color"
>                                      class="small-margin"
>                                      type="button"
>                                      palettename="standard"/>
>+
>+                    </hbox>
>+                </row>
>+                <row>
>+                    <hbox>
>+                        <label value="&calendarproperties.firealarms.label;:" control="fire-alarms"/>
>+                    </hbox>
>+                    <hbox>
>+                        <checkbox id="fire-alarms" checked="true"/>
>                     </hbox>
>                 </row>

You don't need the first <hbox> for the <label> here.

>Index: mozilla/calendar/resources/content/calendarProperties.xul
>===================================================================
>             <row align="center">
>-                <checkbox id="read-only" label="&calendarproperties.readonly.label;"/>
>-                <spacer />
>+                <hbox>
>+                    <label value="&calendarproperties.readonly.label;:" control="read-only"/>
>+                </hbox>
>+                <hbox>
>+                    <checkbox id="read-only"/>
>+                </hbox>
>+            </row>
>+            <row align="center">
>+                <hbox>
>+                    <label value="&calendarproperties.firealarms.label;:" control="fire-alarms"/>
>+                </hbox>
>+                <hbox>
>+                    <checkbox id="fire-alarms"/>
>+                </hbox>
>             </row>

Same here.
Depends on: 410287
Attached patch patch v3 (obsolete) — Splinter Review
Incorporated mvl's and Simons comments.
This patch depends on bug 410287.
Comment on attachment 294924 [details] [diff] [review]
patch v3

>Index: mozilla/calendar/resources/content/calendarCreation.xul
>===================================================================
>                         <colorpicker id="calendar-color"
>                                      class="small-margin"
>                                      type="button"
>                                      palettename="standard"/>
>+
>+                    </hbox>
>+                </row>
>+                <row>
>+                    <label value="&calendarproperties.firealarms.label;:" control="fire-alarms"/>
>+                    <hbox>
>+                        <checkbox id="fire-alarms" checked="true"/>
>                     </hbox>

Please lose the blank line at the end of the first <hbox>.
Comment on attachment 294906 [details]
Edit Calendar Screenshot v2

r=christian
Attachment #294906 - Flags: ui-review?(christian.jansen) → ui-review+
Comment on attachment 294905 [details]
New Calendar Screenshot v2

r=christian
Attachment #294905 - Flags: ui-review?(christian.jansen) → ui-review+
Sebo, are you continuing on this feature? I'd greatly appreciate it for 0.8. Please mind the string freeze on 2008-01-21; you may separate out your strings upfront.
With bug 393395 being fixed, this /should/ be read for review. Unfortunately I don't have time at the moment to test again.
Daniel or Phillipp or anyone...: If you are willing to test and review, that would be great.
With the checkin of bug 393395 there was introduced a rather long checkbox label which doesn't fit in the proposed layout. Find attached is another proposal.
Attachment #294906 - Attachment is obsolete: true
Attachment #297444 - Flags: ui-review?(christian.jansen)
Attached patch patch v4 (obsolete) — Splinter Review
Attachment #294924 - Attachment is obsolete: true
Attachment #297446 - Flags: review?(philipp)
Comment on attachment 297444 [details]
Edit Calendar Screenshot v3

r=christian
Attachment #297444 - Flags: ui-review?(christian.jansen) → ui-review+
Blocks: 412765
No longer depends on: 410287
Comment on attachment 297446 [details] [diff] [review]
patch v4

>-                if (!aOldValue && aValue) {
>+                if (aOldValue && !aValue) {
>                     this.alarmService.initAlarms([aCalendar]);
>-                } else if (aOldValue && !aValue) {
>+                } else if (!aOldValue && aValue) {
>                     this.alarmService.notifyObservers("onRemoveAlarmsByCalendar", [aCalendar]);
>                 }
Why did you change the order of this? if supressAlarms was false and is now true, then alarms should be initialized, right?

>+                <row>
>+                    <label value="&calendarproperties.firealarms.label;:" control="fire-alarms"/>
>+                    <hbox>
>+                        <checkbox id="fire-alarms" checked="true"/>
>                     </hbox>
Theoretically, you don't need a hbox for one element. Does it work without?

We might want to disable the alarm field in the event dialog for calendars that have suppressAlarms set, and introduce capabilities.alarms.supported that does the same. But thats for a different bug.
r=philipp
Attachment #297446 - Flags: review?(philipp) → review+
Comment on attachment 297446 [details] [diff] [review]
patch v4

>Index: mozilla/calendar/resources/content/calendarProperties.xul
>===================================================================
>             <row align="center">
>-                <checkbox id="read-only" label="&calendarproperties.readonly.label;"/>
>-                <spacer />
>+                <spacer/>
>+                <hbox>
>+                    <checkbox id="read-only" label="&calendarproperties.readonly.label;"/>
>+                </hbox>
>             </row>
>             <row align="center">
>-                <checkbox id="cache" label="&calendarproperties.cache.label;"/>
>                 <spacer/>
>+                <hbox>
>+                    <checkbox id="fire-alarms" label="&calendarproperties.firealarms.label;"/>
>+                </hbox>
>+            </row>
>+            <row align="center">
>+                <spacer/>
>+                <hbox>
>+                    <checkbox id="cache" label="&calendarproperties.cache.label;"/>
>+                </hbox>
>             </row>
>+            <spacer/>
>         </rows>
>     </grid> 
> </dialog>

I find this overly complex. 

Why don't you just remove the <row> statements totally and go with this code after the <grid> closes?

<vbox>
  <checkbox id="read-only" label="&calendarproperties.readonly.label;"/>
  <checkbox id="fire-alarms" label="&calendarproperties.firealarms.label;"/>
  <checkbox id="cache" label="&calendarproperties.cache.label;"/>
</vbox>

This works beautifully here on my box.
Keywords: checkin-needed
Simon: checkin is not yet needed since I need to address Philipps and your comments.
Keywords: checkin-needed
Dear all,

Thanks for working on this new feature, the "display alarms" checkbox in
the "Edit calendar" form would be highly appreciated since we are using each other calendars in remote readonly mode (file://) and we just want to see
the calendars of others but not their alarm popups.

Will it be implemented in 0.8?

Today, the alarms popup from other calendars are so boring that it limits
the use and spread of lightning in our company.

Best regards.
Stéphane.
(In reply to comment #53)
Sebo, the strings are ready to be checked in, right? Think of the coming string freeze on monday...
Attached patch patch v5Splinter Review
Attachment #297446 - Attachment is obsolete: true
Attachment #297891 - Flags: review+
(In reply to comment #51)
> >-                if (!aOldValue && aValue) {
> >+                if (aOldValue && !aValue) {
> >                     this.alarmService.initAlarms([aCalendar]);
> >-                } else if (aOldValue && !aValue) {
> >+                } else if (!aOldValue && aValue) {
> >                     this.alarmService.notifyObservers("onRemoveAlarmsByCalendar", [aCalendar]);
> >                 }
> Why did you change the order of this? if supressAlarms was false and is now
> true, then alarms should be initialized, right?
No, if suppressAlarms becomes true, alarms should be removed and vice versa.

> 
> >+                <row>
> >+                    <label value="&calendarproperties.firealarms.label;:" control="fire-alarms"/>
> >+                    <hbox>
> >+                        <checkbox id="fire-alarms" checked="true"/>
> >                     </hbox>
> Theoretically, you don't need a hbox for one element. Does it work without?
Yes, works without and is changed accordingly in patch v5.

> 
> We might want to disable the alarm field in the event dialog for calendars that
> have suppressAlarms set, and introduce capabilities.alarms.supported that does
> the same. But thats for a different bug.
There might be a use case for creating an event with alarm, but still suppressing it - if you create it for a colleague for example.
We should, however, remove the alarm bell from those events.

(In reply to comment #52)
> I find this overly complex. 
> 
> Why don't you just remove the <row> statements totally and go with this code
> after the <grid> closes?
> 
> <vbox>
>   <checkbox id="read-only" label="&calendarproperties.readonly.label;"/>
>   <checkbox id="fire-alarms" label="&calendarproperties.firealarms.label;"/>
>   <checkbox id="cache" label="&calendarproperties.cache.label;"/>
> </vbox>
> 
> This works beautifully here on my box.
> 
The layout is a bit different with this solution.

I carried over r+. This should be checked in before string freeze.
Keywords: checkin-needed
Patch checked in on HEAD and MOZILLA_1_8_BRANCH.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 0.8
Sebo, can you take care of removing the alarm bell also? its quite easy probably, you just need to update one check in calendar-view-core.xml
You need to log in before you can comment on or make changes to this bug.