455733 - Consolidate filter mechanisms in different views and trees

Status: VERIFIED FIXED

(Calendar :: Internal Components, enhancement)

Description

[Fred Jendrzejewski]

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...)

Comment 1

[Fred Jendrzejewski]

Attachment: filter-v1, strange bug

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?

Comment 2

[Berend Cornelius]

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.

Comment 3

[Fred Jendrzejewski]

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.

Comment 4

[Fred Jendrzejewski]

Attachment: XPCOM cuts away some properties

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

Comment 5

[Fred Jendrzejewski]

Attachment: A working approach

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.

Comment 6

[Michiel van Leeuwen (email: mvl+moz@)]

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 7

[Michiel van Leeuwen (email: mvl+moz@)]

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.

Comment 8

[Fred Jendrzejewski]

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 9

[Berend Cornelius]

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.

Comment 10

[Fred Jendrzejewski]

Attachment: all filters are sorted out

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.

Comment 11

[Andreas Treumann]

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

Comment 12

[Fred Jendrzejewski]

Attachment: fixed the reported bug

I wanted to set the month of a duration and this isn't defined.

Comment 13

[Andreas Treumann]

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.

Comment 14

[Fred Jendrzejewski]

Attachment: filter - v4

The endOfMonth cuts off the last day of a month, so I had to fix this and now it works for me.

Comment 15

[Fred Jendrzejewski]

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.

Comment 16

[Fred Jendrzejewski]

Attachment: filter - v5

Comment 17

[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"

Comment 18

[Fred Jendrzejewski]

Attachment: filter - v6

- 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

Comment 19

[Berend Cornelius]

Attachment: unbitrotted patch

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

Comment 20

[Fred Jendrzejewski]

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 ?

Comment 21

[Fred Jendrzejewski]

The code is now on rev 542 and the task tree works fine in lightning. It filters everything as expected.

Comment 22

[Andreas Treumann]

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.

Comment 23

[Andreas Treumann]

Once more:

the progress bar of the '% Complete' column is gone.

Comment 24

[Berend Cornelius]

> 2.) What do you mean with unbitrotted ?

"unbitrotted" means that I modified your patch so that it would apply against the newest revision again.

Comment 25

[Fred Jendrzejewski]

Attachment: filter - v8

- 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.

Comment 26

[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.

Comment 27

[Berend Cornelius]

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!

Comment 28

[Andreas Treumann]

comment #23 is bug 459352.

Checked in lightning and sunbird build 20081208 -> VERIFIED.