Closed Bug 319909 Opened 19 years ago Closed 16 years ago

Failure to properly serialize/unserialize ics ATTACH properties

Categories

(Calendar :: Internal Components, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jminta, Assigned: fred.jen)

References

(Blocks 1 open bug, )

Details

(Keywords: dataloss)

Attachments

(5 files, 9 obsolete files)

488 bytes, text/plain
Details
11.43 KB, image/png
Details
40.04 KB, image/png
Details
67.26 KB, patch
Fallen
: review+
Fallen
: ui-review+
Details | Diff | Splinter Review
1.12 KB, patch
Fallen
: review+
Details | Diff | Splinter Review
If an vevent or vtodo has an ATTACH property specified, we only keep a small portion of the data around on a round-trip.  Specifically, any parameters that may be specified on the property are lost.

This is probably part of the larger problem that calIAttachment looks rather incomplete.

Testcase to follow.  See also bug 168680.
Attached file testcase
The testcase contains 2 attachments, taken directly from examples given in RFC2445.  On a round-trip test in sunbird-20051211, the first attachment is completely lost, since it only contains a parameter.  The second attachment's url remains, but the FMTTYPE is lost.
Keywords: testcase
Attached patch first attempt (obsolete) — Splinter Review
With this patch, I get no dataloss using the testcase, when editing the event in lightning.  There are substantial changes here to the calIAttachment idl, but that's largely because it was previously unfinished.  We can use bug 168680 to actually expose UI for people to use this feature at some point in the future (or an extension can do it first).  The point is that this gets the backend support for attachments in place.
Assignee: base → jminta
Status: NEW → ASSIGNED
Attachment #205792 - Flags: second-review?(dmose)
Attachment #205792 - Flags: first-review?(mvl)
Blocks: 298358
Comment on attachment 205792 [details] [diff] [review]
first attempt

The interface looks pretty low-level. It would be pretty hard to create the kind off attachments as in the testcase. You need to know all kinds of ical internals to be able to create them. Shouldn't we make the interface be more suited to the task of adding an attachment, instead of trying to reflect the ical standard?
(In reply to comment #3)
> (From update of attachment 205792 [details] [diff] [review] [edit])
> The interface looks pretty low-level. It would be pretty hard to create the
> kind off attachments as in the testcase. You need to know all kinds of ical
> internals to be able to create them. Shouldn't we make the interface be more
> suited to the task of adding an attachment, instead of trying to reflect the
> ical standard?
> 

What sort of exposures are you looking for here?  I'm not entirely convinced that we need to really support adding attachments in any form other than adding a sound to an alarm.  Also, the set/getProperty methods are pretty flexible.
Comment on attachment 205792 [details] [diff] [review]
first attempt

After chatting about this in IRC, I'm going to take another cut at this.
Attachment #205792 - Attachment is obsolete: true
Attachment #205792 - Flags: second-review?(dmose)
Attachment #205792 - Flags: first-review?(mvl)
Attached patch new calIAttachment v2 (obsolete) — Splinter Review
Version 2 includes an easier way to set the format-type of the attachment.  This is the only other part of an ATTACH property that the rfc mentions in any detail.  I couldn't think of anything else to expose that might make this easier to use for creating/editing attachments.  Then again, I'm not entirely sure that I want to see this really exposed to the user at all, but that's another argument...

Just for comparison, this is fairly closely based on calIAttendee.
Attachment #207075 - Flags: second-review?(dmose)
Attachment #207075 - Flags: first-review?(mvl)
Comment on attachment 207075 [details] [diff] [review]
new calIAttachment v2

looks ok. r=mvl
Attachment #207075 - Flags: first-review?(mvl) → first-review+
This only supports URI-based attachments, not inline ones.  I've got some ideas on how to implement this, so after discussion with Joey on IRC I'm gonna take this one.
Assignee: jminta → dmose
Status: ASSIGNED → NEW
Attachment #207075 - Flags: second-review?(dmose)
Attachment #207075 - Flags: first-review+
No longer blocks: lightning-0.1
Target Milestone: --- → Lightning 0.2
Flags: blocking0.3+
Attachments don't block 0.3.  I think we can do alarms decently well without them.
Flags: blocking0.3+ → blocking0.3-
Keywords: relnote
Whiteboard: [cal relnote]
Target Milestone: Lightning 0.3 → Sunbird 0.5
Do we want to take Joey's patch as is for now, and add the inline support later?
Blocks: 168680
Cleaning up incorrectly assigned bugs; search for dmosecleanup to get rid of this bugmail.
Assignee: dmose → nobody
jminta:
Your rev2 (unbitrotted) appears to work on ICS calendars, but as evidenced by the "// XXX write me?" comment in calStorageCalendar::writeAttachments, it doesn't work for storage.

Suggestions?  I think having a calAttachments table is a bit overkill for this, but I could be convinced otherwise.
So for reference, if you import an ICS file with ATTACHments into a storage calendar, there's dataloss.  We drop them on the floor on the way in.
Target Milestone: Sunbird 0.5 → ---
Whiteboard: [cal relnote]
Blocks: 382951
Flags: wanted-calendar0.9+
So i started to work on it, because i really miss this feature.	Unfortunately, i'll surely need some help as i am a freshman.

I got the patch in the code, but one part is still missing:

@@ -572,16 +605,20 @@ calItemBase.prototype = {
 
         this.mapPropsToICS(icalcomp, this.icsBasePropMap);
 
         if (this.mOrganizer)
             icalcomp.addProperty(this.mOrganizer.icalProperty);
         for (var i = 0; i < this.mAttendees.length; i++)
             icalcomp.addProperty(this.mAttendees[i].icalProperty);
 
+        for each (att in this.mAttachments) {
+            icalcomp.addProperty(att.icalProperty);
+        }
+
         if (this.mGeneration) {
             var genprop = icalProp("X-MOZILLA-GENERATION");
             genprop.value = String(this.mGeneration);
             icalcomp.addProperty(genprop);
         }
 
         if (this.mRecurrenceInfo) {
             var ritems = this.mRecurrenceInfo.getRecurrenceItems({});


I can't find out where to but it now. The result is a an error, when the createTodo() from calUtils is called.

So if someone is interested in this feature and giving me sometimes some help, i will try my best to get this done.
(In reply to comment #14)
>          for (var i = 0; i < this.mAttendees.length; i++)
>              icalcomp.addProperty(this.mAttendees[i].icalProperty);
> 
> +        for each (att in this.mAttachments) {
> +            icalcomp.addProperty(att.icalProperty);
> +        }
> +

Hi Fred, in general the above looks correct to me (although I haven't had a look at the calIAttachment implementation, how it serializes etc). But please mind that this feature requires enhancing the storage provider to properly read and write multiple attachments, too.

> I can't find out where to but it now. The result is a an error, when the
> createTodo() from calUtils is called.
I don't understand that. Could you please explain in more detail?
> > I can't find out where to but it now. The result is a an error, when the
> >  createTodo() from calUtils is called.
> I don't understand that. Could you please explain in more detail?

The patch is rejected. So i have to merge it in the code by hand. I can do this for the hole patch, except the part of the patch (calAttachment v2) that i posted. I can't find out where to put this part.

The result of the changes that i did until now is an error, when the function "createTodo" is called and the Tasks aren't loaded anymore.

I think, the first thing before the storage problem will be to merge in the old patch.

I hope that i explained it better now.
+        for each (att in this.mAttachments) {
+            icalcomp.addProperty(att.icalProperty);
+        }
+

Looks like this needs to go in 
http://lxr.mozilla.org/mozilla1.8/source/calendar/base/src/calItemBase.js#711
Attached patch Possible storage support (obsolete) — Splinter Review
I implemented storage support. I am posting this only for reasons that my work can be somehow observed. I didn't test it very much (not at all), but Sunbird runs now without making any displayed errors. The things i'd like to do for testing is:
1.) Change the front-end to work with this implementation.
2.) Look for an intelligent way to work with base64 strings and how to export files.
Hello, some problems that i can't interpret until now:
1.)When i want to use the command 
"attachment.uri = "
i get the following error message:

reserved slot index out of range

So my storage isn't working ? 
2.)The other thing that doesn't work is
atts = item.getAttachments({});
for each ( att in atts){...}

att is always null.
3.) I can normally start sunbird and create a new profile. But i don't get any information about database updates and i have never seen one.
Attached patch calAttach - v3 (obsolete) — Splinter Review
The calAttachment is storaged now. I tested the interface on the possibility to delete and save attachments for any item extensivly. I implemented the encoding property as this one is important for the export and the import. I tested neighter import nor export at the moment. I need some opinions about the new class and about the export/import.
1.) It has absolutly no sense to export the uri of a local file. I also think that the "ATTACH" property is used for local files and the "URL" property for distant files.
So i think that any attachments should be exported as a base64 uri.
2.) What should be done this base64 imported uris ? Should we suppose to save them locally during the import?
3.) How do we open attachments? Should we open it with a dialog that is similar to the Thunderbird dialog.

Bye,
Fred
Attachment #327421 - Attachment is obsolete: true
Attached image A first possible frontend (obsolete) —
It is possible to
1.) Attach files
2.) Delete them
Attachment #328140 - Flags: ui-review?(christian.jansen)
So export seems to work, but local uri aren't transformed.
When i want to import a calendar it never works, because he can't handle line 544 of the patch:
writeAttachments(item, olditem){
...
ap.uri = att.uri.spec;

...}
He never gets any data into the uri. And i just can't find the reason for it.
(In reply to comment #20)
> Created an attachment (id=328138) [details]
> calAttach - v3
> 
> The calAttachment is storaged now. I tested the interface on the possibility to
> delete and save attachments for any item extensivly. I implemented the encoding
> property as this one is important for the export and the import. I tested
> neighter import nor export at the moment. I need some opinions about the new
> class and about the export/import.
> 1.) It has absolutly no sense to export the uri of a local file. I also think
> that the "ATTACH" property is used for local files and the "URL" property for
> distant files.
The URL property is not used for remote files. We do this now, but it is not supposed to. the URL property should be used if there is an alternative representation of the event (i.e a html page).

To attach a remote url to an event, the ATTACH property needs to be used. For local urls, the file could be attached base64 encoded as you suggested and I think this is the way to go. I'm eager to hear Daniel's opinion on this though.

If we stay with the terms "Link" for linked remote urls and "File" for local urls, then I don't think we need additional informative UI. We should make sure that if the user adds a link with a file:// url, then the file should be attached in binary form.



> So i think that any attachments should be exported as a base64 uri.
> 2.) What should be done this base64 imported uris ? Should we suppose to save
> them locally during the import?
This should be provider dependant. The ics provider will store the attachment as is, the storage provider can use a BLOB (see http://www.sqlite.org/datatype3.html) to save binary attachments.

Other providers like gdata don't support attachments. There should be a calendar property, (i.e calendar.getProperty("capabilities.attachments.supported") !== false) that disables options to add attachments in the dialog.



> 3.)  How do we open attachments? Should we open it with a dialog that is similar
> to the Thunderbird dialog.
Yes, I think a such dialog should be used. Similar UI as in thunderbird should also be used to show which attachments have been added to the event.

Serialization of the attachment should be done similar to how thunderbird acts when an attachment is added. I'm not sure how it actually is, but for example if the user attaches a file in the event dialog, then changes the attached file locally, and then saves the event, the attachment should also be changed.

I was initially thinking it would be great to store just the url when running and just serialize when writing the attachments, but after some thinking I came to the conclusion this is unintuitive.
Assignee: nobody → fred.jen
Status: NEW → ASSIGNED
Keywords: uiwanted
Version: Trunk → unspecified
> The URL property is not used for remote files. We do this now, but it is not
> supposed to. the URL property should be used if there is an alternative
> representation of the event (i.e a html page).
So this has to be represented in an intuitive way in the UI, if we really want this feature.
> Other providers like gdata don't support attachments. There should be a
> calendar property, (i.e
> calendar.getProperty("capabilities.attachments.supported") !== false) that
> disables options to add attachments in the dialog.
This is theoretically already built in.
>Yes, I think a such dialog should be used. Similar UI as in thunderbird should
>also be used to show which attachments have been added to the event.
I used the thundbird code as an example for the hole file-attaching, so it is already very similiar.
> I was initially thinking it would be great to store just the url when running
> and just serialize when writing the attachments, but after some thinking I came
>to the conclusion this is unintuitive.
The advantages of only storing the local url are the followings:
- always the actual version
- no copies of the file on the same computer, this would be a waste of resources.(always getting the newest version, storing the copy, load it from the db)

And i don't see how to do it as well with a base64 stored file.
Comment on attachment 328140 [details]
A first possible frontend

We should use the same wording and UI as Thunderbird does.

For the menu it would be:

Attach
  Files(s)...
  Web Pages

Attachment List Box:

IMO the File URLs are a not needed. Can't we show the file, only?

In case of an URL we should continue to show the whole URL.

We should also offer an Context menu for:

Open,Delete, Select All
See attachment.


Overall I think this UI needs some fine tuning.
Attachment #328140 - Flags: ui-review?(christian.jansen) → ui-review-
Attached patch calAttach - v3 (obsolete) — Splinter Review
We can store attachments, delete them, import and export them. But there is still a very big problem.
We get handle the VALUE=BINARY property, so we can't handle properly base64 url's and this is the only really interesting point in the hole work. It has to be done in the libical and i very very far from calling me a good c-programmer.
Attachment #328138 - Attachment is obsolete: true
The following things are possible:
- attach a file
- attach a url
- delete the attachments with a context menu or the keyboard
- attach a file by clicking the listbox
- open the file by doubleclicking the item

Problems:
-resizing of the box after adding an attachment
- should the box always be visible? at the moment it is only visible after adding the first attachment, this surely has to changed
- what are reasonable commands for the popup menu? I think delete and open are sufficient
- do we need the possibility to delete all attachments at once?
Attachment #328140 - Attachment is obsolete: true
With the frontend patch applied, I get a listbox that has no border which extends past the dialog (probably one too many flex attributes), with the whole filename as an url.

Also, I think we should make it look more like the tb attachments dialog (display an icon, more than one column). I have attached a mockup (just for the attachments part, not for the toolbar) of how I picture it. Any comments welcome!  To stay consistant, I think an icon should also be displayed for URLs.

...

I just took a closer look at the code and saw that you are actually setting an image for the link. This doesn't seem to work out for me (linux), no image is displayed. Generally I think you are on the right track!


(Fred btw, for your future patches, please put all files into one instead of splitting up :-)
> Also, I think we should make it look more like the tb attachments dialog
> (display an icon, more than one column). I have attached a mockup (just for the
> attachments part, not for the toolbar) of how I picture it. Any comments
> welcome!  To stay consistant, I think an icon should also be displayed for
> URLs.
Looks good with two attachments. I'd suggest the following, if it is possible to implement it:
- one row is always displayed if there is an attachment or not, so it is possible to use maybe a drag and drop/to doubleclick/to use the context menu/to no that it is there
- 2 attachments per row with 3 rows max. then it should become scrollable. Maybe three rows from the beginning are fine too.
- only the name of the file is displayed, if needed it is cropped and the full name is part of the tooltip - same for urls

About the backend:
Does anyone have a good example how to proceed to implement the VALUE=BINARY property ? Would be great if someone could do the hole part, but i could try if i had an example.
hello, so here is my proposition how to handle import of base64 strings and export of locale files:
Import : The import of the attachment shouldn't doesn't fail silently, but an errormessage should be raised.
Export: It would be optimal to have some kind of tags; So we'd add the filenames as tags. Actually this doesn't seems to be possible. I am not sure if we could use the relationship property maybe ?
I'll try to get done all the strings until monday, this should be possible.
Attached patch calAttach - front endback (obsolete) — Splinter Review
This patch is almost everythin I can do. The issues are the followings:

Frontend:
- moz-icon doesn't work -> no icons
- I have to remove the listboxbody statement from the css and I can't get the class to be applied for the attendee listboxes
- the formattype of an attachment isn't set
- I don't use "Select All" in the popup because it seems not really logic why I should introduce this, if I only need delete all

Backend:
- What do we do with binary files ?

My suggestion :
The frontend and the backend works well for webpages. So this can be used IMHO. And we have to find an intelligent solution how we want to handle binary content laterly. If we want to support it only partly, or later or full.

I hope this hole bunch of work will be useful one day,
Fred
Attachment #328307 - Attachment is obsolete: true
Attachment #328308 - Attachment is obsolete: true
Attachment #329112 - Flags: review?(philipp)
Attached patch calAttach - v4 (obsolete) — Splinter Review
This patch is almost everythin I can do. The issues are the followings:

Frontend:
- moz-icon doesn't work -> no icons
Works, just needs some more libs installed.

- I have to remove the listboxbody statement from the css and I can't get the
class to be applied for the attendee listboxes
Fixed

- the formattype of an attachment isn't set
Didn't fix this since its not really important for URLs.

- I don't use "Select All" in the popup because it seems not really logic why I
should introduce this, if I only need delete all
I agree.

Backend:
- What do we do with binary files ?
As discussed on irc, we should disable/hide those options. I chose to disable since it doesn't make sense to have just one menuitem. Is this ok from a UI perspective? Of course its strange to have a disabled "File(s)..." menuitem, but
 I think we can get away with it.

I'm going to check this in (given review) tomorrow in the course of the day, before the string freeze.
Attachment #207075 - Attachment is obsolete: true
Attachment #329112 - Attachment is obsolete: true
Attachment #329346 - Flags: ui-review?(christian.jansen)
Attachment #329346 - Flags: review?(fred.jen)
Attachment #329112 - Flags: review?(philipp)
Flags: in-testsuite?
Some Questions:
- A function can have an attributes like mValue ? This is nice.
- The moz-icon can be set to attachment.uri.spec or am I overlooking something ?
- a planned with christian that the rows for the attachments are open if the attachments are enabled, so a drag and drop can be implemented one day and it is possible to use the context menu. So the last line of updateAttachment and its call in addAttachment isn't needed

But as those are UI questions I can't see any problem and I am interested in Christians opinion. I don't know why but I am not authorized to edit the attachment, but my r+ is sure :)
Comment on attachment 329346 [details] [diff] [review]
calAttach - v4

(In reply to comment #34)
> Some Questions:
> - A function can have an attributes like mValue ? This is nice.
Yes, a function is an object too, so you can assign attributes freely.


> - The moz-icon can be set to attachment.uri.spec or am I overlooking something
I didn't really look into how moz-icon works, I'll do so and then probably pass the uri spec.

> ?
> - a planned with christian that the rows for the attachments are open if the
> attachments are enabled, so a drag and drop can be implemented one day and it
> is possible to use the context menu. So the last line of updateAttachment and
> its call in addAttachment isn't needed
I was planning to mimic thunderbird here. The attachments listbox is initially closed and only shown when there are attachments. The only thing I've done differently is that the listbox is hidden when the last attachment is removed. Thunderbird leaves the listbox there.

With DND I think we can get away with just showing the listbox after a certain timeout when the user drags a file over the dialog, and allowing to drop the file on the whole dialog instead of only on the listbox.

r=fred based on previous comment.
Attachment #329346 - Flags: review?(fred.jen) → review+
Comment on attachment 329346 [details] [diff] [review]
calAttach - v4

Hi,
some feedback after a short UI-Review (on Mac)

- Attached URLs provides always an / at to the end. This should be removed.

- Would it be possible to offer a two column layout (like suggested in
https://bugzilla.mozilla.org/attachment.cgi?id=328319)?

- It would be good if one could resize the attachment box hight by adding a
splitter between Details and attachment.

- The context menu is ok, IMO Attach File(s) should be hidden if the Provider
can't handle attachments.

But these things can be fixed in a spin-off bug. ui=christian
Attachment #329346 - Flags: ui-review?(christian.jansen) → ui-review+
Attached patch calAttach - v5Splinter Review
(In reply to comment #36)
> (From update of attachment 329346 [details] [diff] [review])
> Hi,
> some feedback after a short UI-Review (on Mac)
> 
> - Attached URLs provides always an / at to the end. This should be removed.
Done

> 
> - Would it be possible to offer a two column layout (like suggested in
> https://bugzilla.mozilla.org/attachment.cgi?id=328319)?
Not that easy, I think we would need to create two listboxes that have no margin/padding/border between each other and then add the items into the two listboxes to make it look like one listbox. -> Followup bug

> 
> - It would be good if one could resize the attachment box hight by adding a
> splitter between Details and attachment.
I misunderstood this a bit on the phone, for now I added a separator between the two, making it a splitter is something also for a followup.

> 
> - The context menu is ok, IMO Attach File(s) should be hidden if the Provider
> can't handle attachments.
Removed attach files from the UI since no provider supports this at the moment.
Attachment #329346 - Attachment is obsolete: true
Attachment #329404 - Flags: ui-review+
Attachment #329404 - Flags: review+
Checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: relnote, uiwanted
Resolution: --- → FIXED
Target Milestone: --- → 0.9
We forgot to change an entity label at one point, this fixes.

r=dbo as discussed.
Attachment #329447 - Flags: review+
Attachment #329447 - Attachment description: Fix regression - v1 → [checked in] Fix regression - v1
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.