Closed Bug 455733 Opened 13 years ago Closed 13 years ago

Consolidate filter mechanisms in different views and trees

Categories

(Calendar :: Internal Components, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: fred.jen, Assigned: fred.jen)

Details

Attachments

(1 file, 9 obsolete files)

There should be a abstract class of filters with the functions:
- setDateFilter(aStartDate, aEndDate); //(I find that other date ranges than
the current exsting ones should be at least possible. If we offer them at the
UI is another question that can be dealt with in another issue)
- isWithinCalendarMonth(aMonthIndex);
- isDateWithinRange(aDate);
- setItemPropertyFilter(all||started...)
Attached patch filter-v1, strange bug (obsolete) — Splinter Review
Applying this patch, i get a strange message:

Error: [Exception... "Illegal operation on WrappedNative prototype object"  nsresult: "0x8057000c (NS_ERROR_XPC_BAD_OP_ON_WN_PROTO)"  location: "JS frame :: chrome://calendar/content/calendar-task-tree.xml :: anonymous :: line 926"  data: no]

I don't really understand it. And should we have a possibility so set functions like getDate() in calendar-unifinder as a dateFilter of calIFilter?
I had a look at the code. It seems to be working fine when you replace the line in question with 
>                  for each (var item in this.mTaskArray) {
>                      var takeit = savedThis.mFilter.propertyFilter(item);
>                      if (takeit){
>                        savedThis.mTaskArray.push(item);
>                      }
>                  }

I currently don't know why "filter()" is not working as it should. Does anybody have an idea?. But your class seems to work fine.
We have another problem. And I am not sure if it will be easy to be resolved.
Setting the propertyFilter to a new function raises an Error as the propertyFilter is read-only. This is properly due to the idl file. Here I defined it as a boolean function. If I define it as an attribute I can't call it.
So it must be some kind of object ? But what kind of object ?
A suggestion: You can set an attribute called "propertyFilterBag", this is just a normal object (so what is the type in the idl). And you can set the propertyFilter. So if you want to filter the items, you just call the chosen function of the propertyFilterBag.
Attached patch XPCOM cuts away some properties (obsolete) — Splinter Review
Normally I can set the propertyFilter now. But there is a bug in calendar-task-view.
The itemFilter of the object is cut off, when it is sent to the set cF_set_propertyFilter of calIFilter, so it is never changed
Attachment #339271 - Attachment is obsolete: true
Attached patch A working approach (obsolete) — Splinter Review
The filters are working all with the calIFilter interface. I don't know what else to put in this interface at the moment, the only thing would be to set the getDateForFilter function as a dateFilter attribute, but I am not sure if this makes any sense.
Attachment #339936 - Attachment is obsolete: true
Attachment #339949 - Flags: review?(Berend.Cornelius)
Why are you using an .idl interface? I think it's not necessary to have the xpcom layer in between. The functions are not meant to be overridden, they will never be called from outside javascript, there are no components that need to call the filters. I'd say that the best would be to cut the xpcom layer, and just make the sorting a separate js util file.
(the xpcom layer does have a performance cost, so use them wisely!)
Comment on attachment 339949 [details] [diff] [review]
A working approach

>Index: content/calendar-task-tree.xml
              onGetResult: function(aCalendar, aStatus, aItemType, aDetail, aCount, aItems) {
>-                 for (var i=0; i<aCount; i++) {
>-                     if (!savedThis.mFilterFunction ||
>-                         savedThis.mFilterFunction(aItems[i])) {
>-                         refreshListener.mTaskArray.push(aItems[i]);
>-                     }
>-                 }
>+                 refreshListener.mTaskArray = [ aItem for each (aItem in aItems) ];

onGetResult can be called with batches of events. You need to add to the array, instead of replacing it.
Status: NEW → ASSIGNED
Hi, there are two onGetResult functions.
The function that you mention always overwrote the hole old array, the other function appends the elements.
I didn't change the functionality, I only rewrote it.
Comment on attachment 339949 [details] [diff] [review]
A working approach

Thinking more about the implementation I think mvl is right: An idl interface is not really necessary, because it is not likely that we are going to add another component that will implement this interface. I know that I was the one who put this idea on the table(butI was also very vague with it...).  What I also had in mind with this suggestion was to find a way to declare all these filter constants “next7days”, “notstarted” that are commonly used by both implementations.. If you use these “strings in your filter component you could get rid of “getDatesForFilter()” and “getDates()” in your tree implementations. This would give us the possibility to extend both of our trees with additional filters if desired one day. In this context it would be nice if we find a way to define more flexible filters, so that the filter component would be also able to deal with filters like “next19days” maybe by putting more logic into the string. This sounds a bit awkward at first sight but we have done such generic games in other places too. But I am not sure myself about this...Maybe Philipp has an opinion about it. All these string filters could probably be defined by means of member variables in the filter component.
Your implementation of "isDateWithinRange()" should probably use "checkIfInRange()" in calUtils.js. This function is virtually **the very central** function of our project and considers all different timezones. Please consider that enddates are dealt with as “exclusive” whereas startdates are “inclusive”.
You are exporting jsDates to your filter component. Actually we wanted to get rid of this and come to use calIDateTime objects only.

>+            var filter = createFilter();
>+            this.mFilter = filter
Please assign directly to this.mFilter.
Attached patch all filters are sorted out (obsolete) — Splinter Review
So I abstracted every filter as much as possible.
It should be possible to set a propertyFilter and a dateFilter independently.
I am not sure if this may be a to strange implementation but it should be nice for the end-user.
The user can use now predefined datefilters, propertyfilters or even a search within the text of properties of an item.
Attachment #339949 - Attachment is obsolete: true
Attachment #341140 - Flags: review?(Berend.Cornelius)
Attachment #339949 - Flags: review?(Berend.Cornelius)
I checked the last patch and every time if I choose the 'Events in this Calendar Month' filter I get an error console out put:

Error: uncaught exception: [Exception... "Cannot modify properties of a WrappedNative"  nsresult: "0x80570034 (NS_ERROR_XPC_CANT_MODIFY_PROP_ON_WN)"  location: "JS frame :: chrome://calendar/content/calFilter.js :: getDatesForFilter :: line 193"  data: no]

And the filter doesn't work.

Reloading the calendar view when the 'Events in this Calendar Month' is selected leads to two exceptions:

Error: [Exception... "Cannot modify properties of a WrappedNative"  nsresult: "0x80570034 (NS_ERROR_XPC_CANT_MODIFY_PROP_ON_WN)"  location: "JS frame :: chrome://calendar/content/calFilter.js :: getDatesForFilter :: line 193"  data: no]
Source File: chrome://calendar/content/calFilter.js
Line: 193

Error: [Exception... "Cannot modify properties of a WrappedNative"  nsresult: "0x80570034 (NS_ERROR_XPC_CANT_MODIFY_PROP_ON_WN)"  location: "JS frame :: chrome://calendar/content/calFilter.js :: getDatesForFilter :: line 193"  data: no] STACK: 1: [file:///export/atr/moz_cc/sbird-cc-debug_Linux/mozilla/dist/xpi-stage/lightning/components/calCompositeCalendar.js -> file:///export/atr/moz_cc/sbird-cc-debug_Linux/mozilla/dist/xpi-stage/lightning/js/calUtils.js:1260] notifyFunc
2: [file:///export/atr/moz_cc/sbird-cc-debug_Linux/mozilla/dist/xpi-stage/lightning/components/calCompositeCalendar.js -> file:///export/atr/moz_cc/sbird-cc-debug_Linux/mozilla/dist/xpi-stage/lightning/js/calUtils.js:1263] calListenerBag_notify
3: [file:///export/atr/moz_cc/sbird-cc-debug_Linux/mozilla/dist/xpi-stage/lightning/components/calCompositeCalendar.js:376] cCC_refresh
4: [null:0] null
5: [chrome://calendar/content/calendar-common-sets.js:270] cC_doCommand

Source File: file:///export/atr/moz_cc/sbird-cc-debug_Linux/mozilla/dist/xpi-stage/lightning/components/calCompositeCalendar.js -> file:///export/atr/moz_cc/sbird-cc-debug_Linux/mozilla/dist/xpi-stage/lightning/js/calUtils.js
Line: 1260
Attached patch fixed the reported bug (obsolete) — Splinter Review
I wanted to set the month of a duration and this isn't defined.
Attachment #341140 - Attachment is obsolete: true
Attachment #341295 - Flags: review?(Berend.Cornelius)
Attachment #341140 - Flags: review?(Berend.Cornelius)
I checked patch 341295 again and the exception is gone.

But I found a new bug:

STR:
====

- create an event on October 30th.
- create a second event on October 31th.
- choose again the 'Events in this
Calendar Month' filter

Result:
=======

the last event in the unifinder is October 30th, the event on Oktober 31th isn't visible.
Attached patch filter - v4 (obsolete) — Splinter Review
The endOfMonth cuts off the last day of a month, so I had to fix this and now it works for me.
Attachment #341295 - Attachment is obsolete: true
Attachment #341428 - Flags: review?(Berend.Cornelius)
Attachment #341295 - Flags: review?(Berend.Cornelius)
So actually here is a problem where I am not really sure how to solve it:
Let's consider that I use the "today" filter. So I want to see every item starting with item.startDate and item.endDate of today, the time isn't interesting at all. So I only want to set filter.startDate = filter.endDate = toDay with the property isDate = true. Normally, I want to check this with isCheckIfInRange. But this will throw a false, as it converts the filter.startDate and filter.endDate into something having a time date 0h 0min and my item won't be into this range.
I can check if startDate or endDate are Dates and write additional code, but this is ugly too.
Attached patch filter - v5 (obsolete) — Splinter Review
Attachment #341428 - Attachment is obsolete: true
Attachment #341436 - Flags: review?(Berend.Cornelius)
Attachment #341428 - Flags: review?(Berend.Cornelius)
The patch looks very good.
I explicitly like that you provided the availability to set date filters to certain ranges. You could probably add some more documentation about the available filters and the underlying concepts beneath the header of the source file.

> // XXX: why do I need this ?
>          if(!this.mFilter) {
>            this.mFilter = new calFilter();
>          }
I don't know either!

>   isItemWithinRange: function cF_isDateWithinRange(aItem) {
This is not used anymore.

As we discussed on irc fred delivers a new patch reverting his changes around "modifyItem"
Attached patch filter - v6 (obsolete) — Splinter Review
- removed the changes on modifyItem
- added some documentation
- added a new function calling the propertyFilter and checkIfInRange
- replaced propertyFilter by isItemInFilters for filtering in the trees
Attachment #341436 - Attachment is obsolete: true
Attachment #341464 - Flags: review?(Berend.Cornelius)
Attachment #341436 - Flags: review?(Berend.Cornelius)
Attached patch unbitrotted patch (obsolete) — Splinter Review
The patch looks very good. The documentation is also quite understandable

One minor issue:
>+function getDatesForFilter(aFilter) {
>+    var EndDate = createDateTime();
>+    var StartDate = createDateTime();
>+    var Duration = createDuration();
>+    var oneDay = createDuration();
>+    oneDay.days = 1;

The definitions are dispensable at this place because they are defined later on again.

Yet I noticed that the filter of the task view is not applied correctly after creating a new task.
I unbitrotted your patch. See my attachment
Two things:
1.) I can't recreate your problem, I will move the code to the newest tree code, but the filter works fine for lightning for me.
2.) What do you mean with unbitrotted ?
The code is now on rev 542 and the task tree works fine in lightning. It filters everything as expected.
I checked the last patch and found this issue in the task mode:

The 'All' filter doesn't work correctly after creating:

- a task with due date = yesterday or tomorrow
- a task with a start date in 8 days

In this cases all the other tasks aren't visible if I select the 'All' filter in the task view.

After deleting this special tasks the filter works again as expected and all tasks are visible.
Once more:

the progress bar of the '% Complete' column is gone.
> 2.) What do you mean with unbitrotted ?

"unbitrotted" means that I modified your patch so that it would apply against the newest revision again.
Attached patch filter - v8Splinter Review
- the filters are working now
- I removed the fixAllDayFilter function

- the problem with the progress bar seems to be already in the trunk, I will look on this closer, but i don't think that it is introduced by the patch.
Attachment #341464 - Attachment is obsolete: true
Attachment #342402 - Flags: review?(Berend.Cornelius)
Attachment #341464 - Flags: review?(Berend.Cornelius)
Comment on attachment 342402 [details] [diff] [review]
filter - v8

patch looks very good now. Also Andreas tested it and it found his approval. r=berend.
Attachment #342402 - Flags: review?(Berend.Cornelius) → review+
patch filter v. 8 pushed to comm-central

http://hg.mozilla.org/comm-central/rev/cffc7b7eb6cf

I thank you for your effort and your great endurance!
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Attachment #342046 - Attachment is obsolete: true
Target Milestone: --- → 1.0
comment #23 is bug 459352.

Checked in lightning and sunbird build 20081208 -> VERIFIED.
Status: RESOLVED → VERIFIED
Target Milestone: 1.0 → 1.0b1
You need to log in before you can comment on or make changes to this bug.