322827 - All X-Components are serialized with BEGIN:X, rather than BEGIN:X-FOO

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Joey Minta]

Something like

BEGIN:X-SOMETHING-COOL
X-AWESOME-THING1:yay

END:X-LOCATION

is disappearing when publishing/editing the calendar.

Comment 1

[Joey Minta]

err, that should be 
END:X-SOMETHING-COOL

Comment 2

[Reed Loden [:reed]]

The bugspam monkeys have struck again. They are currently chewing on default assignees for Calendar. Be afraid for your sanity!

Comment 3

[Joey Minta]

Attachment: testcase.js

This is a xpcshell test for this bug.  It shows that even though the X-Component is added, it fails to be serialized.

Comment 4

[Joey Minta]

Looks like we have some weird component-conversion going on here.  If I try simply

var xComp = icsServ.parseICS("BEGIN:X-SOMETHING-COOL\nX-AWESOME-THING1:yay\nEND:X-SOMETHING-COOL");
dump(xComp.serializeToICS()+'\n');

it will throw with BADARG, due to http://lxr.mozilla.org/mozilla/source/calendar/libical/src/libical/icalcomponent.c#299

The fact that this is saying its ICAL_NO_COMPONENT, rather than ICAL_X_COMPONENT is likely the reason that we're failing to serialize.

Comment 5

[Joey Minta]

http://lxr.mozilla.org/mozilla/source/calendar/libical/src/libical/icalcomponent.c#1350
strcmp is giving us garbage here.  |string| is properly X-SOMETHING-COOL, but when  i is 10 (and component_map[i].name is "X") it's returning -45, rather than the 0 that I would expect.  This means we fail to match the right component type.

Comment 6

[Joey Minta]

Attachment: part one (checked in)

This at least will parse the x-comp and spit it back out.  The problem is that the name of the x-prop gets lost.  Someone started implementing this here: http://lxr.mozilla.org/mozilla/source/calendar/libical/src/libical/icalcomponent.c#54
but a quick glance at http://lxr.mozilla.org/mozilla/search?string=x_name will show you it never got past defining the member.  Someone with more c-foo than me is going to need to take care of actually storing that name here: http://lxr.mozilla.org/mozilla/source/calendar/libical/src/libical/icalparser.c#667
and reusing it here: http://lxr.mozilla.org/mozilla/source/calendar/libical/src/libical/icalcomponent.c#306

Comment 7

[Matthew (lilmatt) Willis]

Comment on attachment 233193 [details] [diff] [review]
part one (checked in)

After dusting off my rusty C, this looks right.

We won't try to include the trailing \0 in the comparison.

r1=lilmatt

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 233193 [details] [diff] [review]
part one (checked in)

r=dmose

Comment 9

[Joey Minta]

Comment on attachment 233193 [details] [diff] [review]
part one (checked in)

patch checked in.

Comment 10

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

Attachment: patch libical

This patch does what jminta described in comment 6.
Sorry about the tab mess, that's just libicals fault.

Comment 11

[Daniel Boelzle [:dbo]]

(In reply to comment #10)
> Created an attachment (id=234605) [edit]
> patch libical

Looks good, only minor nits:

> +   icalcomponent* comp = icalcomponent_new_impl(kind);
> +   comp->x_name = icalmemory_strdup(x_name);
> +   return comp;
check for out-of-mem:
if (comp != 0)
    comp->x_name

>     kind_string  = icalcomponent_kind_to_string(kind);
> +   if (kind == ICAL_X_COMPONENT) {
> +       kind_string = impl->x_name;
> +   }
minor opt:
      else
          kind_string  = icalcomponent_kind_to_string(kind);

r1=dbo

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 234605 [details] [diff] [review]
patch libical

>+/** @brief Constructor
>+ */
>+icalcomponent*
>+icalcomponent_new_x (icalcomponent_kind kind, const char* x_name)

Unless I'm misunderstanding something, kind is always going to be ICAL_X_COMPONENT, so there should be no need for the kind argument at all.
r2=dmose with dbo's comments addressed and that fixed.

Comment 13

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

Attachment: updated patch

Updated patch, ready for checkin

Comment 14

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

patch checked in