Task <-> event conversion: no description text

VERIFIED FIXED in 0.8

Status

VERIFIED FIXED
11 years ago
11 years ago

People

(Reporter: andreas.treumann, Assigned: Fallen)

Tracking

unspecified
Dependency tree / graph
Bug Flags:
wanted-calendar0.8 +

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

11 years ago
STEPS TO REPRODUCE:
===================

- Create a event with a description
- Drag this event to the Task mode icon to convert this event
- New task dialog comes up

RESULT:
=======

- description box is empty

EXPECTED RESULT:
================

- the description text should be visible

REPRODUCIBLE:
=============

- always

Updated

11 years ago
Flags: wanted-calendar0.8+
(Assignee)

Updated

11 years ago
Assignee: nobody → philipp
(Assignee)

Comment 1

11 years ago
Created attachment 297538 [details] [diff] [review]
Fix DND

This fixes. Not sure if it fixes bug 412746 though, havent tested that.

Daniel, what do you say about the calICalendar interface addition?
Attachment #297538 - Flags: review?(michael.buettner)
Status: NEW → ASSIGNED
Version: Trunk → unspecified

Comment 2

11 years ago
Comment on attachment 297538 [details] [diff] [review]
Fix DND

I don't quite like that idea and recommend to use the existing "private" cloneShallow() from calIInternalShallowCopy/calInternalInterfaces.idl (implemened both at calEvent and calTodo) which should already offer what you need.

>+    copyItemBase: function (aItemBase) {
>+        var itemComp = aItemBase.icalComponent;
>+
>+        this.setItemBaseFromICS(itemComp);
>+        this.importUnpromotedProperties(itemComp, this.itemBasePromotedProps);
>+    },
seems to be unused in the patch?

Comment 3

11 years ago
Philipp, I think you should rename calIInternalShallowCopy e.g. to calIItemBaseInternal, and add your new method to that interface.
Comment on attachment 297538 [details] [diff] [review]
Fix DND

>-                newItem.wrappedJSObject.setItemBaseFromICS(
>-                    item.icalComponent);
>+                item.cloneItemBaseInto(newItem, null);
I also used cloneItemBaseInto() in my first iteration of bug 388018 (which introduced this feature). During the review we decided to use the raw ical string to initialize the new item from the old one. I'm not entirely certain why we decided it that way. Moving review to Daniel, as he was the driving force to not use cloneItemBaseInto in the first place.
Attachment #297538 - Flags: review?(michael.buettner) → review?(daniel.boelzle)
(Assignee)

Comment 5

11 years ago
(In reply to comment #2)
> (From update of attachment 297538 [details] [diff] [review])
> I don't quite like that idea and recommend to use the existing "private"
> cloneShallow() from calIInternalShallowCopy/calInternalInterfaces.idl
> (implemened both at calEvent and calTodo) which should already offer what you
> need.
I'm a bit confused. I don't use cloneShallow, nor would using it work out (I think at least). while cloneShallow() does return a base item, its initializer is still either an event or a todo.


(In reply to comment #3)
> Philipp, I think you should rename calIInternalShallowCopy e.g. to
> calIItemBaseInternal, and add your new method to that interface.
Here you write I should use that internal interface with my "new" method. all I did was adapt the old method. I don't see why my method should be part of an internal interface when its used to do something quite external.


> seems to be unused in the patch?
Yes, unused. First approach I forgot to remove.

Comment 6

11 years ago
Comment on attachment 297538 [details] [diff] [review]
Fix DND

(In reply to comment #5)
sorry for causing confusion

(In reply to comment #4)
> (From update of attachment 297538 [details] [diff] [review])
> >-                newItem.wrappedJSObject.setItemBaseFromICS(
> >-                    item.icalComponent);
> >+                item.cloneItemBaseInto(newItem, null);
> I also used cloneItemBaseInto() in my first iteration of bug 388018 (which
> introduced this feature). During the review we decided to use the raw ical
> string to initialize the new item from the old one. I'm not entirely certain
> why we decided it that way. Moving review to Daniel, as he was the driving
> force to not use cloneItemBaseInto in the first place.

Ah, yes Mickey I remember why I rejected that approach:
If you'd use cloneItemBaseInto(), you'll e.g. fill up a VEVENT with all properties of a VTODO, e.g. the VEVENT might contain PERCENTAGE or COMPLETED afterwards, which unfortunate. To circumvent that, we decided to use setItemBaseFromIcs which only pulls out those properties that are really common to both VEVENT and VTODO.

I think that code should stay as is; r-.
Attachment #297538 - Flags: review?(daniel.boelzle) → review-
(Assignee)

Comment 7

11 years ago
If I leave that code as is, then this bug is WONTFIX. I don't think thats correct though. There are a few other properties that are common but not covered. Obviously the DESCRIPTION, but also X-Props, and maybe bug 412746.

Would it be a solution to extend setItemBaseFromICS to include all common props and X-Props? Or should we have a convertToInterface() method that accepts a target interface and allows a task to specify custom conversion to an event, and throw NS_ERROR_NOT_IMPLEMENTED for any other interface? The latter would be quite general, allowing us to extend in case we support VJOURNAL or such.
The previous event dialog already did all the work to convert event<->task including descriptions etc. Maybe the code is still around in CVS and can be reused?
(In reply to comment #8)
> The previous event dialog already did all the work to convert event<->task
> including descriptions etc. Maybe the code is still around in CVS and can be
> reused?
The previous event dialog used the cloneItemBaseInto() mechanism, which we agree is not a good solution, see comment #4 and comment #6.
(In reply to comment #7)
> If I leave that code as is, then this bug is WONTFIX. I don't think thats
> correct though. There are a few other properties that are common but not
> covered. Obviously the DESCRIPTION, but also X-Props, and maybe bug 412746.
I don't think any X-props are relevant here since it's just a conversion dialog opener for the user.

> Would it be a solution to extend setItemBaseFromICS to include all common props
> and X-Props? Or should we have a convertToInterface() method that accepts a
> target interface and allows a task to specify custom conversion to an event,
> and throw NS_ERROR_NOT_IMPLEMENTED for any other interface? The latter would be
> quite general, allowing us to extend in case we support VJOURNAL or such.
I've coded that, but I fear this has a performance hit since the item base needs to iterate the (unpromoted) properties and the subclasses (event/todo) again.

After all, I think we should simply define the set of common properties between events and tasks that we want to take over when setting up the conversion. I can imagine this is just a few lines of code, but leaves us more control over what's taken over.
Created attachment 299821 [details] [diff] [review]
Custom conversion - v3

This does custom conversion, taking care of event/task conversion as on the spreadsheet at http://spreadsheets.google.com/pub?key=plCAueWeXt4ibydy7QatMFw
Attachment #297538 - Attachment is obsolete: true
Attachment #299821 - Flags: review?(daniel.boelzle)
The patch also takes care of making the default hour for tasks and events the next full hour, as specified on the wiki. There are some bugs on that, Andreas probably has the whole scoop.
(Assignee)

Updated

11 years ago
Blocks: 408966
Created attachment 299835 [details] [diff] [review]
Custom conversion - v3b

Updated patch, also fixes bug 412287, which was a one-liner.
Attachment #299821 - Attachment is obsolete: true
Attachment #299835 - Flags: review?(daniel.boelzle)
Attachment #299821 - Flags: review?(daniel.boelzle)
(Assignee)

Updated

11 years ago
Blocks: 412287
(Assignee)

Updated

11 years ago
Blocks: 408671
Comment on attachment 299835 [details] [diff] [review]
Custom conversion - v3b

looks good to me, r=dbo

some comments:
>+                var index = 0;
>+                while (index < numAddresses) {
...
>+                    index++;
>+                }
please use a for-loop

>+        // XXX It would be great if nsPlainTextParser could take care of this.
>+        function htmlToPlainText(html) {
...
>+                        switch (p1) {
>+                            case "nbsp": return " ";
>+                            case "amp": return "&";
>+                            case "lt": return "<";
>+                            case "gt": return ">";
>+                            case "quot": return '\"';
>+                        }
>+                        return " ";
can we go with U+FFFD here? e.g. Components.interfaces.nsIConverterInputStream.DEFAULT_REPLACEMENT_CHARACTER.

>+    copyItemBase: function iC_copyItemBase(aItem, aTarget) {
>+        const copyProps = ["SUMMARY", "LOCATION", "CATEGORIES", "DESCRIPTION",
>+                           "URL", "CLASS", "PRIORITY", "STATUS"];
>+
>+        for each (var prop in copyProps) {
>+            aTarget.setProperty(prop, aItem.getProperty(prop));
>+        }
You may add the following, although we yet have no UI for them: ATTACH, COMMENT, CONTACT, RESOURCES. Because the latter may occur multiple times in the component, I'd go for enumerating the properties.

>+    taskFromEvent: function iC_taskFromEvent(aEvent) {
...
>+            // Alarms
>+            item.alarmOffset = (aEvent.alarmOffset ?
>+                                aEvent.alarmOffset.clone() :
>+                                null);
>+            item.alarmRelated = aEvent.alarmRelated;
>+            item.alarmLastAck = (aEvent.alarmLastAck ?
>+                                 aEvent.alarmLastAck.clone() :
>+                                 null);
...

>+    eventFromTask: function iC_eventFromTask(aTask) {
...
>+        // Alarms
>+        item.alarmOffset = (aTask.alarmOffset ?
>+                            aTask.alarmOffset.clone() :
>+                            null);
>+        item.alarmRelated = aTask.alarmRelated;
>+        item.alarmLastAck = (aTask.alarmLastAck ?
>+                             aTask.alarmLastAck.clone() :
>+                             null);
I spotted the same block of code; up to you to spend a separate function or not. May pay off when we have more types of alarms in the future.
Attachment #299835 - Flags: review?(daniel.boelzle) → review+
(In reply to comment #14)
> (From update of attachment 299835 [details] [diff] [review])
> >+                        switch (p1) {
> >+                            case "nbsp": return " ";
> >+                            case "amp": return "&";
> >+                            case "lt": return "<";
> >+                            case "gt": return ">";
> >+                            case "quot": return '\"';
> >+                        }
> >+                        return " ";
> can we go with U+FFFD here? e.g.
> Components.interfaces.nsIConverterInputStream.DEFAULT_REPLACEMENT_CHARACTER.
I don't think returning default_replacement_character is correct here, since we don't really want a bunch of questionmark characters in the description. The better solution would be to do real conversion from html to text, but I'd like to leave that for a different bug.

> You may add the following, although we yet have no UI for them: ATTACH,
> COMMENT, CONTACT, RESOURCES. Because the latter may occur multiple times in the
> component, I'd go for enumerating the properties.
Also a candidate for a different bug, its not always wanted to keep those properties.

> I spotted the same block of code; up to you to spend a separate function or
> not. May pay off when we have more types of alarms in the future.
Can be revised in the multi alarms patch.

Checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.8
VERIFIED

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.9) Gecko/20071031 Lightning/0.8pre (2008020218) Thunderbird/2.0.0.9 ID:2007103104
(Reporter)

Updated

11 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.