459434 - Consolidate task and unifinder tree observer into a common item tree observer

Status: NEW

(Calendar :: Internal Components, enhancement)

Description

[Fred Jendrzejewski]

The code of should be moved on a common base, as described in bug #432582.
I am opening this as this could become once again a bigger patch.

Comment 1

[Fred Jendrzejewski]

Attachment: A first approach

I didn't find any real bug until now, so I ask for review. I tried to move as much code as possible together, so the two trees became once again a little bit more similiar.
I am not sure if everything is fine with the new refresh mechanism in the task tree, and if the calendars are really cleanly added and removed, my impression was yes.

Comment 2

[Andreas Treumann]

I checked the patch and the 'find' function doesn't work. But this functionality is also broken in the nightly build.

Comment 3

[Berend Cornelius]

I won't have time to review the patch by Wednesday but glancing over it I wonder if it would not be better to introduce the parent binding already at this stage and implement the observer in it. This would give you the opportunity to overwrite certain methods in the children bindings and avoid the "TreeType" logic that you are using. But as I said I was just glancing over the patch.

Comment 4

[Fred Jendrzejewski]

Actually I am wondering too, if I may should introduce the parent binding now. But I think that this could become a pretty complex thread, that should be broken down. The code is still pretty complex and there are a lot of differences, so it may be more intelligent to get more incremental steps done. This has also the advantange that I can do every part proberly.I thought something like:
- unify the observers (they can also live in a calendar-item-tree.js)
- unify the tree view (this will be the worst part) (they would live in the calendar-item-tree.js too)
- put both trees in a common binding.
- clean it intelligently up.

Andreas : Is there already a bug report where I can attach the patch ?

Comment 5

[Andreas Treumann]

Fred: I filed a new bug report, see https://bugzilla.mozilla.org/show_bug.cgi?id=459803

Comment 6

[Berend Cornelius]

Fred, I was looking at your code for quite some time and my conclusion is:
On one hand I cannot fine anything in detail  that's wrong about it, so from this  point of view your patch is Ok and my testing did also did not bring any new insights. But on the other hand I must say that your code has not really  simplified the code, in fact I find it quite had to keep track of the implementation.
I hope that I am not wrong with this but I still can't see a reason why you don't introduce the parent binding at this stage already. This way you could keep all implementations in one place. 
I don't know how familiar you are with xbl, but the major thing you have to keep in mind with the inheritance chains is that <content> tags are not inherited while properties, methods and fields are inherited, so that you can overwrite them wherever you wish to. So you could keep your <content> elements where they are and step by step build up your parent implementations.
I am afraid I could not reach you and I will be on holiday till including next monday.

Here is just one nit:
>    onModifyItem: function eTO_onModifyItem(aNewItem, aOldItem) {
>        this.onDeleteItem(aOldItem);
>        this.onAddItem(aNewItem);
>    },

please remove ",", same in the calTasktreeObserver

Comment 7

[Fred Jendrzejewski]

So actually I wanted to brake down what is coming in some way. The problem is, that everything is extremly related. So I can go forward and put everything in common binding and unify it step by step. But the resulting patch will be a monster. I looked for a way to get the changes incrementally in the tree, as the hole work will need weeks to be done intelligently. I also think that we win already by cutting done code that was written twice into one object. This holds especially for the all the mechanisms of the trees. So I wanted to try to get always only one tree-view, tree-observer and one kind of event handling for the trees. This gets in a binding afterwards and we can start creative work in smaller pieces on the binding and on every mechanism (event handling, setting items, make the tree persistant...).
We surely have the possibility to create an item and an event tree now, but the way how the event-tree is working is extremly far away from the way how the task-tree works, so I am not sure that I get the event tree into a binding. If the ways they are working are almost similiar I have much much confidence in my capabilities, because I have a working binding (the task-tree) with this mechanism already in front of me .
It is surely hard to keep track of the changes, but we may create a wikipage or a meta-bug for this. And surely everything isn't perfectly improved if it gets in the tree, but it is already in a simplified way in the tree.
And I think that I have everything in one place and that I am putting more and more stuff there. The treeview and the observers will live in the calendar-item-tree.js (or the inheriting objects in the oppropiate js-file, I don't think that it is so important now). This file will be existing with a binding too. 
At all, I am fearing the complexity of introducing a binding now and I can't see the big advantage of doing the simplification of every code piece now and not when I only have one.

Comment 8

[Berend Cornelius]

But when you create a parent binding now you don't need to put *all* of your implementation into that binding. You will have three bindings altogether: one parent binding and two children bindings; the parent binding will only contain the methods, fields and properties that are used by both children in the same way the other functions may be implemented empty there. Whatever is task-related remains in the task-tree and whatever is strictly event-related is implemented within the event tree by overwriting the according parent methods, properties and fields. I also think that you even won't have to change your code very much to do so. What is currently implemented in your observer implementation could be shifted to the parent binding - and adapted in several details of course.

Comment 9

[Fred Jendrzejewski]

Attachment: observer - v2

- created a calendar-item-tree and a calendar-event-tree
- moved all observer code in one object
- simplified the modify/add/remove mechanism in the observer itself
- unified the load and unload mechanism and moved a lot of this code in the calendar-item-tree binding.

Comment 10

[Fred Jendrzejewski]

I am wondering if the observer shouldn't be a standalone object using the prototype mechanism and being defined in the calendar-item-tree.js. 
This would make the code easier usable for extension developers. It is ugly to overwrite a hole object if you want to change only one of its properties.

Comment 11

[Andreas Treumann]

The 'Events in the Next 31 Days' shows only events in the next 30 days. This works in lightning 0.9. The 'Events in the Next 14 Days' filter works as expected.

Comment 12

[Fred Jendrzejewski]

Hi, I tested it once again and it is adding always the right duration.
I can see an event that is defined for the 21.nov 6:00. This is actually in 31 days or am I wrong? (Sry, I am never sure about this) The calculation isn't done only by the day, but on the hours and min.

Comment 13

[Berend Cornelius]

Comment on attachment 343719 [details] [diff] [review]
observer - v2

The patch looks quite good and I look forward to check it in.
Here some nits  that I have noticed:

> <calendar-event-tree id="unifinder-search-results-tree"
You removed the "flex attribute here.

As I found out travelling by keyboard throught the event tree throws an error "aEvent is undefined" probably deriving from

> onselect="unifinderSelect(event); calendarController.onSelectionChanged()"

> +            <method name="refresh">
> +                <body><![CDATA[
> +                    switch (this.mItemType) {
> +                        case "event":
> +                            refreshEventTree();
> +                            break;
> +                        case "task":
> +                            refreshTaskTree();
> +                            break;
> +                    }
> +                ]]></body>
> +            </method>
> +            <method name="isTreeType">
> +                <parameter name="aItem"/>
> +                <body><![CDATA[
> +                    switch (this.mItemType) {
> +                        case "event":
> +                            return isEvent(aItem);
> +                        case "task":
> +                            return isToDo(aItem);
> +                    }
> +                ]]></body>
> +            </method>

As I said already I believe that you should just overwrite the "refresh" methods in the children trees and put your needed implementation there -> that's what inheritance is actually about. When I grep over the patch I think you would not need the "mItemType" field either. It would make the code clearer if you invested some to overwrite the methods where you make use of it.
Just like Fred I could not reproduce Andreas issue in comment #11. Maybe we wait for Andreas comment tomorrow.

I am happy to approve the patch with the comments being addressed. At least the first two issues that I have discovered should be resolved. Thank you again for your endurance!

Comment 14

[Andreas Treumann]

I checked it again and with hour and minutes calculation it works correct.

Comment 15

[Stefan Sitter [:ssitter]]

I think the filters should include all events of the end day too, i.e. 'Events in the Next 31 Days' lists all events from Now to Now+31 23:59:59.

Comment 16

[Fred Jendrzejewski]

Stefan : I thought about this too, but if you are searching for the next hour you expect only the coming 60 min and not more. So IMO we should stay consistant.

And something broke the tree with applying any patches, that observers are notified anymore and I get an error:
JavaScript error: , line 0: uncaught exception: [Exception... "Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsISupports.QueryInterface]"  nsresult: "0x80004002 (NS_NOINTERFACE)"  location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: no]

Comment 17

[Fred Jendrzejewski]

Attachment: strange things are going on here

I unbitrotted the patch but now there are some bugs that I can't handle at all.
1.) In the unifinder he tries to load the event-filter-menulist and can't find it.
2.) He can't handle any changes on tasks.

Comment 18

[Martin Schröder [:mschroeder]]

Fred, please change the status to NEW if you no longer work on this issue.
Thanks.