363932 - Delete Warning should be redesigned

Status: RESOLVED FIXED

(Calendar :: Calendar Frontend, defect)

Description

[Christian Jansen]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.1) Gecko/20061010 Firefox/2.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.1) Gecko/20061010 Firefox/2.0

The current warning, which appear if user delete one event of a series, is not easy to understand. It should be redesigned like proposed in the wiki.

http://wiki.mozilla.org/Calendar:Alerts#Deleting_an_Event

Reproducible: Always

Steps to Reproduce:
1. Create a repeating event
2. Select one event of the series
3. Hit DEL

Comment 1

[Stefan Sitter [:ssitter]]

The discussion on this topic was originally initiated to find a solution for Bug 320178. Close Bug 320178 and implement the UI proposal with this new bug or mark dependency or ...? As pointed out in the other bug support for 'all future' needs to be added to the calendar backend first.

Comment 2

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix v1

I would like to get the prerequisites in first, therefore I switched the blocks/depends order. While this dialog needs the backend functionality, the backend functionality is quite tricky. The patch size is dramatically lowered this way, I just need to implement the actual item splitting.

Since there are no icons yet, the pixel numbers are obviously wrong. I might have to check this patch on other OS, since it will obviously look different. IIRC, the wizard on windows has a white header. The same color is used in this header, but on linux its all gray.

Also, the this and future button should be hidden. I just left it in for effects. Will post a screenshot soon.

Comment 3

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Screenshot linux

Comment 4

[Philipp Kewisch [:Fallen][📱📆]]

Also there is a Problem with events with long titles. Not quite sure how to solve this: Since the whole phrase is one label, I can only crop at start/end/center, but not just before the quotes.

Berend, if you have an idea, please tell me about it :-)

Comment 5

[Christian Jansen]

I can imagine that two left aligned rows would work fine.
One for the title, the other for the description.

"Event Title"
Is a repeating event
----------------
[Button 1]
[Button 2]
[Button 3]
----------------

Comment 6

[Christian Jansen]

Comment on attachment 320033 [details]
Screenshot linux

The default button should be focused initially (in this case "Edit just this occurrence").

It might also be OK to have different button heights this would help users to focus the "most" important one (see proposal attached).

Please remove the icons until I (or anyone who wants to volunteer ;-) ) has created a set.

We need icons for:

Edit
Delete
Move
Copy

()

Comment 7

[Christian Jansen]

Attachment: mock-up with different button heights

Comment 8

[Philipp Kewisch [:Fallen][📱📆]]

Setting uiwanted to get UI.

What do you mean with move and copy? I haven't implemented anything for those actions, and move is just a special form of edit.

Do you have a suggested height for the button? I'm not sure the proposed height is sufficient, it didn't really catch my eye, but otoh maybe its subliminal :-)

I'll adapt the pixel heights and split the labels after code review.

Comment 9

[Berend Cornelius]

The patch works as advertised: Some remarks:
-As the button "Edit this and all future occurrences" has not yet any implemented  action behind we should probably rather disable it for the time being or maybe raise a messagebox.
- Your implementation "promptOccurrenceModification()" is not really easy to understand: 
- I would call the third parameter "aAction" instead of "aType". I don't know if I like your way to map its value directly to the resource id - but it works fine. What could happen is that somebody who wants to tidy up the resources might think that e.g the resource "buttons.occurrence.delete/edit.label" is not used (because its identifier is concatenated in the source code) and may be deleted.
- In your returned array "futureItem" can either be null or an item or a boolean that  is meant to be dependent on the second parameter "aNeedsFuture" when this is "false". The latter condition has not yet been implemented. I would try to make this easier.
- I am asking myself if your local constants
>    const CANCEL = 0;
>    const MODIFY_OCCURRENCE = 1;
>    const MODIFY_FOLLOWING = 2;
>    const MODIFY_PARENT = 3;
should not be defined in an interface and probably passed as an inout parameter instead of passing "aNeedsFuture", but this is just an idea that you probably also have thought about already...

Comment 10

[Berend Cornelius]

Comment on attachment 320032 [details] [diff] [review]
Fix v1

r=berend

Comment 11

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to comment #9)
> The patch works as advertised: Some remarks:
> -As the button "Edit this and all future occurrences" has not yet any
> implemented  action behind we should probably rather disable it for the time
> being or maybe raise a messagebox.
I chose to hide it for now.

> - Your implementation "promptOccurrenceModification()" is not really easy to
> understand: 
> - I would call the third parameter "aAction" instead of "aType". I don't know
> if I like your way to map its value directly to the resource id - but it works
> fine. What could happen is that somebody who wants to tidy up the resources
> might think that e.g the resource "buttons.occurrence.delete/edit.label" is not
> used (because its identifier is concatenated in the source code) and may be
> deleted.
I renamed the parameter. I see your concern, we have been doing this all along though (i.e calGetString("recurrenceDetailsDay" + day_of_week(byday[0])) and I think if someone wants to clean up the properties (bug 434985), then he has to do some more research than just to grep for the full string.

> - In your returned array "futureItem" can either be null or an item or a
> boolean that  is meant to be dependent on the second parameter "aNeedsFuture"
> when this is "false". The latter condition has not yet been implemented. I
> would try to make this easier.
I could add a third return array parameter with the type and then make that be one of the constants, how's that?

> - I am asking myself if your local constants
> >    const CANCEL = 0;
> >    const MODIFY_OCCURRENCE = 1;
> >    const MODIFY_FOLLOWING = 2;
> >    const MODIFY_PARENT = 3;
> should not be defined in an interface and probably passed as an inout parameter
> instead of passing "aNeedsFuture", but this is just an idea that you probably
> also have thought about already...
aNeedsFuture is more for places like delete all following, where the future item is not needed. This has nothing to do with the returned constants.

I think an interface for UI code is not needed.

Comment 12

[Berend Cornelius]

>I could add a third return array parameter with the type and then make that be
>one of the constants, how's that?
Yes that's better because more understandable.

Comment 13

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: [checked in] Fix - v3

I've checked in this patch since I need it for a future bug, but leaving this bug open to check in the images needed for the dialog and to adjust the button height if needed.

Comment 14

[Stefan Sitter [:ssitter]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.15pre) Gecko/2008052110 Calendar/0.9pre

With this new added dialog the wording is completely wrong when trying to edit a repeating task. 

The previous dialog used the wording "repeating item". The new dialog always uses the wording "repeating event". That is just wrong for tasks.

The dialog doesn't seem to be resizeable and the dimensions seem to be hard coded in css. This might cause issues with some translations.

Comment 15

[Philipp Kewisch [:Fallen][📱📆]]

Christian, whats your decision about the button height? How many px? Also, do you have ETA on the images?

Comment 16

[Christian Jansen]

(In reply to comment #14)
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.15pre)
> Gecko/2008052110 Calendar/0.9pre
> 
> With this new added dialog the wording is completely wrong when trying to edit
> a repeating task. 
> 
> The previous dialog used the wording "repeating item". The new dialog always
> uses the wording "repeating event". That is just wrong for tasks.

That true. We need to differentiate here. I can't recommend to use item, because it is a very abstract, and technical description.
> 
> The dialog doesn't seem to be resizeable and the dimensions seem to be hard
> coded in css. This might cause issues with some translations.
> 

Comment 17

[Christian Jansen]

(In reply to comment #15)
> Christian, whats your decision about the button height? How many px? Also, do
> you have ETA on the images?
> 

I'll work on the images beginning next week.

Comment 18

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix localization issue - v1

Since the string freeze is on monday, I'll fix the localization issue extra. It would be nice to get some icons, or at least the requested button heights.

Comment 19

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix localization issue - v1

Argh, I forgot to save before uploading.

Comment 20

[Martin Schröder [:mschroeder]]

Comment on attachment 328740 [details] [diff] [review]
Fix localization issue - v1

r=mschroeder, but you should adjust the comments (header is mentioned twice).

>       // Set up title and header
>       document.title =
>         calGetString("calendar-occurrence-prompt",
>-                     "windowtitle." + action);
>+                     "windowtitle." + itemType + "." + action);
>       document.getElementById("title-label").value = window.arguments[0].item.title;
> 
>+      // Set up header
>+      document.getElementById("isrepeating-label").value =
>+        calGetString("calendar-occurrence-prompt",
>+                     "header.isrepeating." + itemType + ".label");
>+

Comment 21

[Christian Jansen]

Comment on attachment 320033 [details]
Screenshot linux

Please make "Edit just this occurrence" the default button. The size of the icons will be 20x20px. Please also align the heading with the left button edge.
ui=christian

Comment 22

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: [checked in] Fix localization issue - v2

Patch as checked in. I already prepared the CSS files for the 20x20 px images too.

Comment 23

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Add icons code

If anyone would like to steal review, please go ahead :-)

Comment 24

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Winstripe Icons (Windows,Linux)

Comment 25

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Pinstripe Icons (Mac)

Comment 26

[Martin Schröder [:mschroeder]]

Comment on attachment 333308 [details] [diff] [review]
Add icons code

looks good, r=mschroeder

Comment 27

[Philipp Kewisch [:Fallen][📱📆]]

Finally, all parts are taken care of...


Checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED

Comment 28

[Daniel Boelzle [:dbo]]

The space between the icon and the button's label looks quite close to me; Christian suggested to insert 3px like we do elsewhere, too.

Comment 29

[Stefan Sitter [:ssitter]]

(In reply to comment #28)

I filed Bug 450611 to follow-up the issue.