307948 - ICS provider: escaped characters \n \, \; \\ not unescaped from description, location

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[gekacheka]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20050910 Mozilla Sunbird/0.2+

RFC2445 describes the escaped characters for text values:  
  newlines \n
  commas   \,
  semicolon \;
  backslash \\

When these are entered in the event title, they are ok.
When these are entered in the location or description, after sunbird is
restarted, the backslash notation appears in the value instead of the character
.  This does not happen for the local storage provider.

Reproducible: Always

Steps to Reproduce:
1. create new ics calendar file if necessary (remote file://temp/escapetest.ics)
2. select escapetest.ics as default calendar, and create event.
3. enter location "New Orleans, LA"
4. enter description with newlines:
     semicolon ';'
     comma ','
     backslash '\'
5. click ok.
     location and description appear without escape backslashes.
6. exit and restart sunbird
     location and description now appear with escape backslashes.
     

Actual Results:  
location and description appear with escape backslashes, and \n appears where
newline should be.

Expected Results:  
location and description appear without escape backslashes

Comment 1

[Robin Edrenius]

Is this a duplicate of/related to Bug 265971?

Comment 2

[Stefan Sitter [:ssitter]]

(In reply to comment #1)
> Is this a duplicate of/related to Bug 265971?

I also think this is a duplicate of bug 265971. But in my opinion bug 265971 should be closed as a duplicate of this bug, as this bug contains a better error description.

Comment 3

[gekacheka]

This bug is not the same as bug 265971.

In this bug, actual backslash characters appear in the text other than the title.
The special characters such as newline and comma are encoded correctly with backslashes when saved to ICS (rfc2445), but they are not decoded correctly to remove the backslashes when loaded from ICS.  The encoded form appears in the displayed text.  So this bug is about DECODING the text fields other than title.

In bug 265971, the newlines are correctly decoded in the title, but are displayed as blocks rather than causing line breaks.  There are no backslashes displayed in the title.  So bug 265971 is about DISPLAYING newlines that are correctly decoded in the title.

Comment 4

[Robin Edrenius]

*** Bug 315722 has been marked as a duplicate of this bug. ***

Comment 5

[Joey Minta]

*** Bug 314855 has been marked as a duplicate of this bug. ***

Comment 6

[Joey Minta]

Attachment: get escaped value

prop.stringValue returns icalproperty_get_value_as_string, which does not remove any escaping characters.  However, calling values from the ical component with icalcomp[name] does return 'normal' text.  This is the method already used for promoted properties, such as title, which explains why they do not exhibit the bug.

Comment 7

[Michiel van Leeuwen (email: mvl+moz@)]

That patch will only work for properties that are defined in calIIcalComponent. Not all properties are in there. So I think this patch won't always work.

Comment 8

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: alternative patch

Another problem with the previous patch was that it didn't escape backslashed when serializing.

This is an alternative approach. Is changes the interface of calIIcalProperty somewhat, but to the way it was already used everywhere. Avoiding .stringValue like the previous patch means we have to avoid it everywhere.
But i'm not totally happy about this patch, because it makes text values somewhat special. Why should it be special?

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 203021 [details] [diff] [review]
alternative patch

After discussion on IRC, we've come up with a design that's a bit cleaner.

Comment 10

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: updated alternative

This patch implements the other solution.

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 203160 [details] [diff] [review]
updated alternative

Looks good; just needs a few minor tweaks.

>+    /**
>+     * The value of the property as string.
>+     * The exception for properties of TEXT or X- type, those will be unescaped
>+     * when getting, and also expects an unescaped string when setting.
>+     * Datetime, numeric and other non-text types are represented as ical string
>+     */
>+    attribute AUTF8String value;

Maybe add a blank line in between these two attributes for readability?

>+    /**
>+     * The value of the property in (escaped) ical format.
>+     */
>+    attribute AUTF8String valueAsIcalString;
>+

[...]

> NS_IMETHODIMP
>-calIcalProperty::GetStringValue(nsACString &str)
>+calIcalProperty::GetValue(nsACString &str)
>+{
>+    icalvalue_kind kind = icalproperty_kind_to_value_kind(icalproperty_isa(mProperty));
>+
>+    const char *icalstr;
>+    if (kind == ICAL_TEXT_VALUE) {
>+        icalvalue *v = icalproperty_get_value(mProperty);
>+        icalstr = icalvalue_get_text(v);
>+    } else if (kind == ICAL_X_VALUE) {
>+        icalvalue *v = icalproperty_get_value(mProperty);
>+        icalstr = icalvalue_get_x(v);
>+    } else {
>+        icalstr = icalproperty_get_value_as_string(mProperty);
>+    }
>+
>+    if (!icalstr) {
>+        if (icalerrno == ICAL_BADARG_ERROR) {
>+            str.Truncate();
>+            str.SetIsVoid(PR_TRUE);
>+            return NS_OK;
>+        }

Can you add some comments here explaining what this SetIsVoid strategy does and why it makes sense to use it here?       

>@@ -595,28 +597,28 @@ calItemBase.prototype = {
>                                      .getService(Components.interfaces.calIICSService);
>             var alarmComp = icssvc.createIcalComponent("VALARM");
> 
>             var duration = Components.classes["@mozilla.org/calendar/duration;1"]
>                                      .createInstance(Components.interfaces.calIDuration);
>             duration.isNegative = true;
>             duration[this.getProperty("alarmUnits")] = this.getProperty("alarmLength");
> 
>             var triggerProp = icssvc.createIcalProperty("TRIGGER");
>-            triggerProp.stringValue = duration.icalString;
>+            triggerProp.value = duration.icalString;

Can you add a comment above this line explaining why it's better to use .value here, even though .valueAsIcalString would be the obvious choice?

Comment 12

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: patch v4

Patch updated to review comments

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 203445 [details] [diff] [review]
patch v4

The last comment from my previous review doesn't appear to have been addressed.

Comment 14

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: patch v4.1

I did miss one spot. patch updated

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 203455 [details] [diff] [review]
patch v4.1

r=dmose

Comment 16

[Michiel van Leeuwen (email: mvl+moz@)]

patch checked in

Comment 17

[bugzilla]

*** Bug 331602 has been marked as a duplicate of this bug. ***