Consolidate filter mechanisms in different views and trees

VERIFIED FIXED in 1.0b1

Status

--
enhancement
VERIFIED FIXED
10 years ago
9 years ago

People

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

Tracking

Trunk
1.0b1

Details

Attachments

(1 attachment, 9 obsolete attachments)

(Assignee)

Description

10 years ago
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...)
(Assignee)

Comment 1

10 years ago
Created attachment 339271 [details] [diff] [review]
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

10 years ago
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.
(Assignee)

Comment 3

10 years ago
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.
(Assignee)

Comment 4

10 years ago
Created attachment 339936 [details] [diff] [review]
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
Attachment #339271 - Attachment is obsolete: true
(Assignee)

Comment 5

10 years ago
Created attachment 339949 [details] [diff] [review]
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.
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
(Assignee)

Comment 8

10 years ago
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

10 years ago
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.
(Assignee)

Comment 10

10 years ago
Created attachment 341140 [details] [diff] [review]
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.
Attachment #339949 - Attachment is obsolete: true
Attachment #341140 - Flags: review?(Berend.Cornelius)
Attachment #339949 - Flags: review?(Berend.Cornelius)

Comment 11

10 years ago
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
(Assignee)

Comment 12

10 years ago
Created attachment 341295 [details] [diff] [review]
fixed the reported bug

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)

Comment 13

10 years ago
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.
(Assignee)

Comment 14

10 years ago
Created attachment 341428 [details] [diff] [review]
filter - v4

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)
(Assignee)

Comment 15

10 years ago
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.
(Assignee)

Comment 16

10 years ago
Created attachment 341436 [details] [diff] [review]
filter - v5
Attachment #341428 - Attachment is obsolete: true
Attachment #341436 - Flags: review?(Berend.Cornelius)
Attachment #341428 - Flags: review?(Berend.Cornelius)

Comment 17

10 years ago
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"
(Assignee)

Comment 18

10 years ago
Created attachment 341464 [details] [diff] [review]
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
Attachment #341436 - Attachment is obsolete: true
Attachment #341464 - Flags: review?(Berend.Cornelius)
Attachment #341436 - Flags: review?(Berend.Cornelius)

Comment 19

10 years ago
Created attachment 342046 [details] [diff] [review]
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
(Assignee)

Comment 20

10 years ago
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 ?
(Assignee)

Comment 21

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

Comment 22

10 years ago
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

10 years ago
Once more:

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

Comment 24

10 years ago
> 2.) What do you mean with unbitrotted ?

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

Comment 25

10 years ago
Created attachment 342402 [details] [diff] [review]
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.
Attachment #341464 - Attachment is obsolete: true
Attachment #342402 - Flags: review?(Berend.Cornelius)
Attachment #341464 - Flags: review?(Berend.Cornelius)

Comment 26

10 years ago
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+

Comment 27

10 years ago
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!

Updated

10 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Attachment #342046 - Attachment is obsolete: true
Target Milestone: --- → 1.0

Comment 28

10 years ago
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.