432582 - Consolidate task and unifinder tree view into a common item tree view

Status: NEW

(Calendar :: Calendar Frontend, defect)

Description

[Philipp Kewisch [:Fallen][📱📆]]

Currently, we have two similar implementations of the tree view in

calendar-task-view.xml
calendar-unifinder.js

These two tree implementations could be refactored into one implementation that can be inherited from. The methods that come to mind are:


get*Properties
add/remove/clear methods
sortItem
getItemRow
getItemFromEvent
setSelectedItems (?)
All static methods (isContainer, isSorted, toggleOpenState ...)
setTree

Comment 1

[Fred Jendrzejewski]

I take thisone. I'll try to get the unifinder-tree in a first step in a similiar binding like the calendar-task-tree. After this we can look which parts can be put in a common calendar-item-tree.

Comment 2

[Philipp Kewisch [:Fallen][📱📆]]

I'm not sure a binding is really needed here, this bug is more about a common tree view implementation.

Note that the way events are displayed may change in tb3, so I'd wait a bit before making an unifinder binding.

Comment 3

[Fred Jendrzejewski]

The thing is, that the two trees are having a common sense, one is a tree for events the other for tasks. So there is like always for the two a lot in common (handlers, columns, items as base). But the functionality is implemented really differently, so it is hard maintain them together. Let's say that you can implement subitems in the task-tree, at the moment I am not sure at all if you can do this easily for events too. I talked about this with Berend and he a similiar opinion.

But if the mechanism will change maybe I'll stop work.

Comment 4

[Berend Cornelius]

When I compare the code around the two trees (or actually three trees!) I find that this seems far too much work to do in one patch. I also cannot not deliver a complete concept ad hoc how the final implementation should look like. 
My approach would be to first extract and consolidate the sorting (btw. the task tree does not yet use a locale collator as it seems!) and filtering functionality first. Probably they can be encapsulated - maybe with defined interfaces - don't know yet.
After having done so you would get a clearer picture about how to go on, simply because of the reduced code size.
I think what seems confusing at first is that the calendar task-tree is implemented with xbl used by two different implementations, whereas the calendar-Unifinder is only using xul and js. You should probably stick to xbl here.
In the final steps you can create a parent task-tree that contains all common functionality - probably in xbl and make derivations of it with special functionality, where your override certain methods, like "isContainer" or "getParentIndex".
My suggestions contain some "Maybes" and "Probablies" but I guess that's the way  it goes - sometimes you just have to jump into the cold water...

Comment 5

[Fred Jendrzejewski]

I will try to step into this. As there seems to be nothing like a filtering at the moment, I will try to get the searches on a same base. Not sure, where this code should live at the beginning I will put it in a calendar-tree-sort.js.
Let's see what I get done.

Comment 6

[Fred Jendrzejewski]

Attachment: Sort out the code for filters and sorts

I sorted out us much code as possible. Some points about the patch:
- I think that the parts concerning the sort are an improvement, the code is shorter, unified and both  trees have new "features", the task tree has the localecollator and the unifinder supports the status in a nice way. The only strange thing is, that the sort is slow in the task-tree and I have no idea why. But if we unify those codes even more, I think that it will be resolved.
- I don't like the filter part at all. I tried to sort out the code from the other files and unify the code as much as possible. The thing is: the task-tree has one times a filter, one times not. I weren't capable to test the lightning part until now. So this is really strange and I am not 100% sure how to do it, I will relook the code, maybe I have some spontaneous new ideas.

Comment 7

[Fred Jendrzejewski]

Attachment: Sort out the code for filters and sorts

I didn't change anything on the sort part, so comments are the same. But I think that I have now an idea how a filter mechanism generally could work.
Every tree has a "filter"-object. Every filter-object has two filter mechanism
1.) A date filter. It looks on the dates of each event and filters on them. It is chosen by the getDates function and applied by the setDates function
2.) A property filter, looking on other properties. There is a "property-filter"-bag called propertyFilters and the actual filter can be chosen by propertyFilters[filter].
- I am not sure who describe the createFilter function in this context. It creates the filter on the calICalendar, but the propertyFilter is still needed afterwards, as the filtermechanism of the calendar lacks of power.

I think that this is a pretty general mechanism that could work, now it only needs to become nicer, as the names aren't very intelligently chosen.

Comment 8

[Berend Cornelius]

I think with this patch you somehow “overshot the mark”. Maybe I expressed myself a bit unclear, for which I apologize because you invested so much work into it already, but my (and I think basically also Philipp's) idea was merely to extract all **common** functionality around filtering and sorting (and elsewhere) from both trees and encapsulate this in **abstract** classes. I had in mind to make these classes universally reusable – probably even for other widgets. In your sorting and filtering implementations you directly refer to XUL sub elements of the two trees which are actually implementation details that should actually rather stay where they currently are. 
Probably it is possible to set up the filter class with defined functions like (fast spoken)
- 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...)

As I said maybe these functions should be defined in an idl file...

For the Sorting component equivalent
All the compareDateTime, compareNumber... functions should be extracted in an own class that can than - optimalwize - be used to sort any kind of lists or Arrays.

Comment 9

[Fred Jendrzejewski]

Let's say that the two patches for bug #432728 and #455733 are near to the final form. So they do not really really reduce the code size, they only make the code more similiar. So there are two possibilities.
1.) Move the calendar-unifinder into a xbl and the two trees laterly. The good thing is, we get rid of all those global variables. The problem is the complexity of the code, so it may become complicated to move the handlers.
2.) Create a common treeview and treeobserver, let the two trees inherit from those and move the unifinder after this to an xbl. This simplifies the code more and it will become easier to create a common xbl. But we have to move a lot of functions out of the unifinderTaskView if we want that this makes any sense and they would become global variables, not really cool neither.

I tend to the first choice, we don't make the code really shorter, but we the code of the two trees becomes more similiar.

Comment 10

[Philipp Kewisch [:Fallen][📱📆]]

Not having read too much of this bug, my opinion may not be as firm as needed, but I'd tend to the second option. I'm fine with moving that code out of the binding, it would simplify code a lot and only require changing things once if problems show up.

Comment 11

[Fred Jendrzejewski]

Okay, if I'll wait for the review of the two patches and after this I will start to move the treeview out of the binding.
Berend any commands on that ?

Comment 12

[Berend Cornelius]

Comment on attachment 338873 [details] [diff] [review]
Sort out the code for filters and sorts

denying review because of comment #8

Comment 13

[Martin Schröder [:mschroeder]]

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