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

RESOLVED FIXED

Status

Calendar
General
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: gekacheka, Assigned: Michiel van Leeuwen (email: mvl+moz@))

Tracking

({dataloss, regression})

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

12 years ago
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
(Reporter)

Updated

12 years ago
Keywords: regression
Summary: ICS provider: escaped characters not unescaped from description, location → ICS provider: escaped characters \n \, \; \\ not unescaped from description, location

Comment 1

12 years ago
Is this a duplicate of/related to Bug 265971?

Comment 2

12 years ago
(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.
(Reporter)

Comment 3

12 years ago
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

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

Comment 5

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

Updated

12 years ago
Keywords: dataloss

Comment 6

12 years ago
Created attachment 202903 [details] [diff] [review]
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.
Attachment #202903 - Flags: first-review?(mvl)
(Assignee)

Comment 7

12 years ago
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.
(Assignee)

Comment 8

12 years ago
Created attachment 203021 [details] [diff] [review]
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?
Attachment #203021 - Flags: first-review?(dmose)

Comment 9

12 years ago
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.
Attachment #203021 - Flags: first-review?(dmose) → first-review-
(Assignee)

Comment 10

12 years ago
Created attachment 203160 [details] [diff] [review]
updated alternative

This patch implements the other solution.
Attachment #203021 - Attachment is obsolete: true
Attachment #203160 - Flags: first-review?(dmose)

Updated

12 years ago
OS: Windows 2000 → All
Hardware: PC → All
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?
Attachment #203160 - Flags: first-review?(dmose) → first-review-

Updated

12 years ago
Attachment #202903 - Attachment is obsolete: true
Attachment #202903 - Flags: first-review?(mvl)
(Assignee)

Comment 12

12 years ago
Created attachment 203445 [details] [diff] [review]
patch v4

Patch updated to review comments
Attachment #203160 - Attachment is obsolete: true
Attachment #203445 - Flags: first-review?(dmose)
Comment on attachment 203445 [details] [diff] [review]
patch v4

The last comment from my previous review doesn't appear to have been addressed.
Attachment #203445 - Flags: first-review?(dmose) → first-review-
(Assignee)

Comment 14

12 years ago
Created attachment 203455 [details] [diff] [review]
patch v4.1

I did miss one spot. patch updated
Attachment #203445 - Attachment is obsolete: true
Attachment #203455 - Flags: first-review?(dmose)
Comment on attachment 203455 [details] [diff] [review]
patch v4.1

r=dmose
Attachment #203455 - Flags: first-review?(dmose) → first-review+

Updated

12 years ago
Assignee: mostafah → mvl
(Assignee)

Comment 16

12 years ago
patch checked in
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 17

12 years ago
*** Bug 331602 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.