Closed Bug 379174 Opened 17 years ago Closed 17 years ago

readonly events should not be dragable in the views

Categories

(Calendar :: Calendar Frontend, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: Fallen, Assigned: Fallen)

References

Details

(Whiteboard: [roadmap 0.8][gdata-0.4])

Attachments

(4 files, 4 obsolete files)

Currently, it is possible to drag events of readOnly calendars around in the view. The event is not changed. In the multiday view, the event just snaps back to where it came from, in the month view it disappears until the next refresh.

I have a patch in hand that stops the event from being draggable and removes the grippies on hover for multiday views. The following questions are open:

* How should readOnly events be visualized? Some icon that the event is readonly?
* Should readOnly events be selectable?
  - If yes, should there be any notification if many events are deleted and 
    the readOnly event stays?
  - Are there any other implications regarding operations on readOnly that are
    selected?
* Since they are not draggable, they can not be dragged into another
  application. Is this ok?
Whiteboard: [patch in hand]
I don't think we need a read-only visualization for every event. But we could add some kind of icon to the calendar in the calendar list. Calendar color and the icon let you identify a read-only calendar.

Read-only events should be selectable (at least for exporting selected events) and there needs to be a warning or message if someone tries to delete read-only events.
(In reply to comment #1)
> I don't think we need a read-only visualization for every event. But we could
> add some kind of icon to the calendar in the calendar list. Calendar color and
> the icon let you identify a read-only calendar.

Agreed. This wold make sense. We can add a lock , aligned right to the Calendar list.

> 
> Read-only events should be selectable (at least for exporting selected events)

But the should not be scalable. Thus the grippies should not occour.
They might be draggable. One scenario could be, that one wants to make a copy of the event. For example, by dragging the event onto the "personal" calendar -- or turning it into an e-mail, or task.

> and there needs to be a warning or message if someone tries to delete
> read-only events.

Maybe we want to inform users before they run into this trap. An event could also display a small lock. This would indicate that users can't modify the event.

Some other ideas:
Extend the event summary dialog [1] by an "Add this event to my calendar" button.

[1] http://wiki.mozilla.org/Calendar:SMB_Event_Summary
(In reply to comment #3)
> One scenario could be, that one wants to make a copy
> of the event. For example, by dragging the event onto the "personal" calendar
> -- or turning it into an e-mail, or task.
I think that can be taken care of in a separate bug. Especially since the drag semantics and targets are a bit unclear.

> Some other ideas:
> Extend the event summary dialog [1] by an "Add this event to my calendar"
> button.
Followup bug. Feel free to file one when you have some ideas on where to place that button.

> Maybe we want to inform users before they run into this trap. An event could
> also display a small lock. This would indicate that users can't modify the
> event.
So that means we *do* want an indicator on each event? This means I will need one or two icons. Also CC'ing Jordan Wan, since he also made the alarm indicator icon. 

We will need a general decision on the icon size, since there will be some more in the future. For the alarm indicator, 14x10 px was the chosen size, which for the chosen icon I think is the smallest size where it still looks good. The downside of course is that many icons may clutter the event, and eventually force us to put the event title under the icons. In comparison to other products, Google Calendar uses very small (partially cryptic) icons to show: alarm, privacy, recurrence.

The icons that are pending for the event boxen are: alarm indicator (bug 288496), readonly state, item is task (bug ).
I'd like to do bug 388405 first, since I only need to change code once then.
Depends on: 388405
About the icons, remember also that it must still look good when the item is selected. My placeholder icon needs to be inverted to look good on selection for example :)
> How should readOnly events be visualized?
> Some icon that the event is readonly?

I vote against having an icon on readonly events.  Consider holidays, IMO a readonly icon would make them less attractive and unnecessarily clutter them.  Besides, don't people generally know which of their calendars and events are owned by other people and are thus read-only?

Also, even if there's a readonly icon on the event, some users might still try to incorrectly drag the event, so I think that this situation has to be handled elegantly regardless of whether or not there's an icon.

> Extend the event summary dialog [1] by an
> "Add this event to my calendar" button.

I like this idea a lot.
> Extend the event summary dialog [1] by an
> "Add this event to my calendar" button.

Another thought for the spinoff bug:  this would be a nice command to have in the event's context menu.  I'm thinking about the possibility to use the Ctrl key to select, for example, several birthdays in month view from someone else's calendar (readonly) and then right click on one of them and choose "add to my calendar".
Blocks: 395187
Keywords: uiwanted
(In reply to comment #8)
> I vote against having an icon on readonly events.
What about showing an icon as soon as the user clicks on the event, or if it is selected. This way the icon is not always shown, but the user still sees why its not possible to drag the event.

Alternativly, we could show the event boxes in a different way, that make them look more static, although I think this would be far more troublesome.

> Also, even if there's a readonly icon on the event, some users might still try
> to incorrectly drag the event, so I think that this situation has to be handled
> elegantly regardless of whether or not there's an icon.
What do you mean by elegantly? Only as much as that the event box should not be moveable, or more?

Christian, when you are back, could you take care of creating the icons? Also a decision regarding the readonly icons on the event box would be great.
(In reply to comment #10)

Whatever you all decide about the readonly icon is okay but hopefully I'll be able to disable it via userchrome.css if the icon is always visible.

Another thought: instead of the icon there could be a "balloon popup" that appears at the mouse cursor for a few seconds when the user tries to drag a readonly event.  This might be clearer to the user than an icon.
(In reply to comment #11)
> Another thought: instead of the icon there could be a "balloon popup" that
> appears at the mouse cursor for a few seconds when the user tries to drag a
> readonly event.  This might be clearer to the user than an icon.

p.s. When I wrote that, I was thinking about what Windows Explorer does when you're renaming a file and you type an illegal character such as a slash (/).  You could try this in Windows Explorer to see what I mean (I use WinXP).
Whiteboard: [patch in hand] → [roadmap 0.8]
(In reply to comment #10)
> (In reply to comment #8)
> > I vote against having an icon on readonly events.
> What about showing an icon as soon as the user clicks on the event, or if it is
> selected. This way the icon is not always shown, but the user still sees why
> its not possible to drag the event.
> 
> Alternativly, we could show the event boxes in a different way, that make them
> look more static, although I think this would be far more troublesome.
> 
> > Also, even if there's a readonly icon on the event, some users might still try
> > to incorrectly drag the event, so I think that this situation has to be handled
> > elegantly regardless of whether or not there's an icon.
> What do you mean by elegantly? Only as much as that the event box should not be
> moveable, or more?
> 
> Christian, when you are back, could you take care of creating the icons? Also a
> decision regarding the readonly icons on the event box would be great.
> 

I propose to do the following:

Calendar List:
- If a calendar is RO, display a lock symbol on the calendar color box

Event and Task in Calendar Views:
- Selection should deactivation
- Display no grippes while on mouse over

See attachment.

> Extend the event summary dialog [1] by an
> "Add this event to my calendar" button.

I like this idea a lot.

I've attached a mock-up for inspiration ;-)


Attached image Lock-symbol 14x14px (obsolete) —
(In reply to comment #15)
> Created an attachment (id=285709) [details]
> Calendar List showing Lock
> 

What about calendars with white colour?
(In reply to comment #17)
> What about calendars with white colour?
I will be using getContrastingTextColor to either use a black or white lock icon. I'm sure I can get away with just inverting the lock color :)

(In reply to comment #13)
> > Extend the event summary dialog [1] by an
> > "Add this event to my calendar" button.
> 
> I like this idea a lot.
I think this should be part of a different bug.
Attached patch Add readonly symbol - v2 (obsolete) — Splinter Review
(In reply to comment #13)
> Event and Task in Calendar Views:
> - Selection should deactivation
Selection should what? ;) If you mean selection should be deactivated, I don't agree. The user should still be able to select events, i.e to export. 


Additionally I put the lock symbol (in black) in the header column, to make clear that that column toggles the readonly status.

So you don't think there should be a readonly icon on the event?
Attachment #285769 - Flags: ui-review?(christian.jansen)
Attachment #285769 - Flags: review?(michael.buettner)
Attached image Lock symbol 14x14 in black (obsolete) —
(In reply to comment #19)
> So you don't think there should be a readonly icon on the event?
I know that you're asking Christian, but I just want to clarify that my request is only about not showing the padlock on multiday events such as holidays.  I realize that that would make the UI inconsistent, but I think that it would look nicer for holidays.  Well, whatever you think is best.
Not showing them for holiday calendars is kind of hard to realize. Codewise its easier for me the way it is, otherwise I personally would prefer only showing a lock (or maybe a diabled grippy or such?) on hover.
(In reply to comment #19)
> Created an attachment (id=285769) [details]
> Add readonly symbol - v2
> 
> (In reply to comment #13)
> > Event and Task in Calendar Views:
> > - Selection should deactivation
> Selection should what? ;) If you mean selection should be deactivated, I don't
> agree. The user should still be able to select events, i.e to export. 
> 
> 
> Additionally I put the lock symbol (in black) in the header column, to make
> clear that that column toggles the readonly status.
> 
> So you don't think there should be a readonly icon on the event?
> 

Right. I don't think that we should display a readonly icon on the event.
First of all, the patch works like a charm. But there are two minor issues that I would like to discuss before going ahead.

First of all, this patch adds a notification method onCalendarReadOnlyState() to the calIObserver interface. I have the impression that this should be better handled in terms of a generic notification instead of introducing separate methods for individual attributes. Fortunately, a patch that generically introduces this mechanism is already on the way, see bug 356569. The relevant patch adds the generic onPrefChanged() to the calIObserver interface. I suggest that we change this patch to support this new notification mechanism.

Second, the icons are currently two separate image files. I suggest that we create an image list that contains all icons that will be displayed in the event boxes in order to reduce the total number of files. This will be even more important when we want to introduce those other icons that will be supported in the near future (recurrence, alarm, etc.).
Attachment #285769 - Flags: review?(michael.buettner) → review-
Attached patch Add readonly symbol - v3 (obsolete) — Splinter Review
Of course I should have used one image, silly me.

This patch now uses the properties. I'm not sure its the right way to go, since I am notifying onPropertyChanged for "readOnly", which is not a property thats being set using setProperty, but much more the direct attribute. I need notification of this attribute for this patch though.

We might want to get rid of the readOnly attribute and only use setProperty, but on the other hand, readOnly is a nice and clean attribute which is easily accessible.

The patch also disables some commands when no calendars are writable. The ones I disabled seemed ok, but there are still some problems with disabling the cut/paste commands:
  - the cut command should be disabled both when no events are selected and no 
    writable calendars are in the list. Since we currently just disable/enable
    when the state of one changes, the disabled state will be incorrect.
  - In lightning, we need to change the CalendarController to customly change 
    isCommandEnabled. This sounds easy, but it seems wrong to me to put a lot of
    custom code from different parts of the code in there. I guess I just need a
    good idea how to structure the code there.

It would be great if we could unify this command controller, I don't know if thats feasible though.
Attachment #285769 - Attachment is obsolete: true
Attachment #286804 - Flags: review?(michael.buettner)
Attachment #285769 - Flags: ui-review?(christian.jansen)
Black and white lock symbols. The Images regions are only 10x10 each, since the original 14x14 image was just a 10x10 image with some transparent padding.
Attachment #285708 - Attachment is obsolete: true
Attachment #285771 - Attachment is obsolete: true
Comment on attachment 286804 [details] [diff] [review]
Add readonly symbol - v3

> This patch now uses the properties. I'm not sure its the right way to go, since
> I am notifying onPropertyChanged for "readOnly", which is not a property thats
> being set using setProperty, but much more the direct attribute. I need
> notification of this attribute for this patch though.
I find this solution much better, as we don't need separate methods in the observer interface, even if direct attributes are changing. As we already discussed that with Daniel, I think it's just fine to go ahead with this approach.

r=mickey.
Attachment #286804 - Flags: review?(michael.buettner) → review+
Yes, we could change the readOnly attribute to plain call getProperty/setProperty later when we store readOnly persistently (as e.g. attribute name already does).
BTW: Philipp, why have you changed providers' setProperty to return the set value?
(In reply to comment #28)
> Yes, we could change the readOnly attribute to plain call
> getProperty/setProperty later when we store readOnly persistently (as e.g.
> attribute name already does).
> BTW: Philipp, why have you changed providers' setProperty to return the set
> value?
I did this to follow the logic that setters should return the value, so that calls can be chained, i.e

set readOnly(val) {
  return this.setProperty("readOnly", val);
}

Or are there any reasons not to do so? I will prepare a version that makes readOnly a property and letting the getters/setters handle it, similar to the code snippet above.


What should we do about the cut/paste commands?


(In reply to comment #29)
> What should we do about the cut/paste commands?
I'm fine with addressing this in a spin-off bug as I would like to land this one as soon as possible. As you already said, the command handler (at least for Lightning), see [1], needs to take care. But wiring in that logic directly in the command handler results in spaghetti code. We indeed need a better solution to the problem, but that's outside of the scope of this bug.

[1] http://lxr.mozilla.org/mozilla1.8/source/calendar/lightning/content/messenger-overlay-sidebar.js#73
Blocks: 402325
Flags: wanted-calendar0.8+
I had to change some things in the calendar management (take apart the observers) to fix the disabling of the toolbar buttons, but now it works.

Asking for review again to make sure everything is ok.
Attachment #286804 - Attachment is obsolete: true
Attachment #287440 - Flags: review?(michael.buettner)
Just a thought: is the lock a good metaphor here? My problem with it is that a lock is used for a lot of other things too, like security.
Maybe a 'no access' sign would work better? (a red circle with a diagonal line). Adding a pen to it would even be better, but i don't think you can make that work in 16x16 pixels.
anyway, that was just my thought. Any opinions?
Comment on attachment 287440 [details] [diff] [review]
Add readonly symbol - v4

r=mickey.
Attachment #287440 - Flags: review?(michael.buettner) → review+
(In reply to comment #32)
> Just a thought: is the lock a good metaphor here? My problem with it is that a
> lock is used for a lot of other things too, like security.
I think that using a 'lock' symbol is good enough for now. Probably you're right and some other symbol would fit better, but I wouldn't want to hold this bug off because of that. Christian already filed bug 401883 in order to address the issue that the (Lightning) icons don't agree with Thunderbird's style. At the same time we could go through all icons and decide whether or not the symbol really fits the meaning of it. I would like to go ahead with this patch as it currently stands in order to get the feature done and think about the symbols in a separate issue that addresses all icons at once.
Checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Keywords: uiwanted
Resolution: --- → FIXED
Whiteboard: [roadmap 0.8] → [roadmap 0.8][gdata-cvs]
Target Milestone: --- → 0.8
Depends on: 403517
Depends on: 403523
Verified in lightning build 2007111803 and sunbird build 20071118 -> task is fixed. 
Status: RESOLVED → VERIFIED
Whiteboard: [roadmap 0.8][gdata-cvs] → [roadmap 0.8][gdata-0.4]
You need to log in before you can comment on or make changes to this bug.