Closed Bug 449031 Opened 16 years ago Closed 16 years ago

Add meta data API to memory/storage

Categories

(Calendar :: Provider: Local Storage, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dbo, Assigned: dbo)

Details

Attachments

(1 file, 1 obsolete file)

For the purpose of synchronizing remote calendars with the local cache, it's sensible to have meta data stored with items.
Flags: wanted-calendar0.9+
Flags: in-testsuite?
Attached patch patch - v1 (obsolete) — — Splinter Review
patch including unit tests
Assignee: nobody → daniel.boelzle
Status: NEW → ASSIGNED
Attachment #332211 - Flags: review?(philipp)
Daniel,


For whatever reason, your code doesn't store anything in the database, even though the message "calStorageCalendar: stored meta data: id = ..." is displayed and no exception occurs...
I cannot confirm this: modifying the unit test to leave some items and meta data in works well for me. BTW: I am a bit confused, because a quick glance shows me you're using a memory calendar in the caldav opt patch to store meta data.
(In reply to comment #3)
> I cannot confirm this: modifying the unit test to leave some items and meta
> data in works well for me. BTW: I am a bit confused, because a quick glance
> shows me you're using a memory calendar in the caldav opt patch to store meta
> data.
> 

Hi Daniel,

When you adapted my code, you replaced the references to "mDBTwo" with "mDB" for the insert and delete statements but left the other queries untouched. Replacing "mDB" back with "mDBTwo" solves the issue.

Regarding the memory calendar, where do you see this? If you look closely, the memory calendar is setup only when the calendar is its own superCalendar (meaning its not cached), otherwise, the property "mTargetCalendar" gets the value of the calendar passed as parameter to replayChangesOn.
Attached patch patch - v2 — — Splinter Review
Wolfgang, you're right, there's a misalignment with mDb and mDbTwo. However, I am wondering why my unit tests actually pass... seems to be different from "real" application scenario :/

I think using mDb is correct here, since mDbTwo is to be used when querying over secondary tables iterating events/todos.
Attachment #332211 - Attachment is obsolete: true
Attachment #332252 - Flags: review?(philipp)
Attachment #332211 - Flags: review?(philipp)
Comment on attachment 332252 [details] [diff] [review]
patch - v2

>+ *
>+ * @attention
Attention? I might be wrong, but I think the javadoc identifier you are looking for is @note.

>+        this.mInsertMetaData.execute();
>+        this.mInsertMetaData.reset();
>+//         LOG("calStorageCalendar: stored meta data: id = " + id + "; value = " + value);
>+    },
>+
>+    deleteMetaData: function stor_deleteMetaData(id) {
>+        this.mDeleteMetaData(id);
>+//         LOG("calStorageCalendar: deleted meta data: " + id);
Remove commented code here and elsewhere. Also I just noticed mDeleteMetaData() as a function? We usually don't prefix functions with m and since this function does nothing else but to call the other, why not use it right away?

>+        try {
>+            if (query.step()) {
>+                value = query.row.value;
>+            }
>+        } finally {
>+            query.reset();
>+        }
Not quite sure, but doesn't this cause a warning like "try without catch" ?

>+function testMetaData() {
>+    function testMetaData_(cal) {
Some comments as to what exactly you are testing would be nice, but not required.


r=philipp
Attachment #332252 - Flags: review?(philipp) → review+
(In reply to comment #6)
> (From update of attachment 332252 [details] [diff] [review])
> >+ *
> >+ * @attention
> Attention? I might be wrong, but I think the javadoc identifier you are looking
> for is @note.
If we use javadoc... changed that to @note.

> >+        this.mInsertMetaData.execute();
> >+        this.mInsertMetaData.reset();
> >+//         LOG("calStorageCalendar: stored meta data: id = " + id + "; value = " + value);
> >+    },
> >+
> >+    deleteMetaData: function stor_deleteMetaData(id) {
> >+        this.mDeleteMetaData(id);
> >+//         LOG("calStorageCalendar: deleted meta data: " + id);
> Remove commented code here and elsewhere. Also I just noticed 
removed LOGs

mDeleteMetaData()
> as a function? We usually don't prefix functions with m and since this function
> does nothing else but to call the other, why not use it right away?
you could call storage statement wrappers as functions if there's one parameter

> >+        try {
> >+            if (query.step()) {
> >+                value = query.row.value;
> >+            }
> >+        } finally {
> >+            query.reset();
> >+        }
> Not quite sure, but doesn't this cause a warning like "try without catch" ?
no, it's valid


Checked in on HEAD and MOZILLA_1_8_BRANCH => FIXED.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → 0.9
No longer blocks: 437318
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: