Closed
Bug 408652
Opened 17 years ago
Closed 16 years ago
Task <-> event conversion: no description text
Categories
(Calendar :: Lightning Only, defect)
Calendar
Lightning Only
Tracking
(Not tracked)
VERIFIED
FIXED
0.8
People
(Reporter: andreas.treumann, Assigned: Fallen)
References
Details
Attachments
(1 file, 2 obsolete files)
23.82 KB,
patch
|
dbo
:
review+
|
Details | Diff | Splinter Review |
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•17 years ago
|
Flags: wanted-calendar0.8+
Assignee | ||
Updated•16 years ago
|
Assignee: nobody → philipp
Assignee | ||
Comment 1•16 years ago
|
||
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)
Updated•16 years ago
|
Status: NEW → ASSIGNED
Version: Trunk → unspecified
Comment 2•16 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•16 years ago
|
||
Philipp, I think you should rename calIInternalShallowCopy e.g. to calIItemBaseInternal, and add your new method to that interface.
Comment 4•16 years ago
|
||
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•16 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•16 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•16 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.
Comment 8•16 years ago
|
||
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?
Comment 9•16 years ago
|
||
(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.
Comment 10•16 years ago
|
||
(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.
Assignee | ||
Comment 11•16 years ago
|
||
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)
Assignee | ||
Comment 12•16 years ago
|
||
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 | ||
Comment 13•16 years ago
|
||
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)
Comment 14•16 years ago
|
||
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+
Assignee | ||
Comment 15•16 years ago
|
||
(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.
Assignee | ||
Comment 16•16 years ago
|
||
Checked in on HEAD and MOZILLA_1_8_BRANCH -> FIXED
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.8
Comment 17•16 years ago
|
||
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•16 years ago
|
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•