Closed Bug 455728 Opened 16 years ago Closed 16 years ago

Consolidate sort mechanisms in different views and trees


(Calendar :: Internal Components, defect)

Not set


(Not tracked)



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



(1 file, 4 obsolete files)

The should be an abstract class handling the possible sort problems for arrays. This should be done in an idl class.
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

Problem: what is the kind of the "array" of this object ? We don't take one ? Or do we want to hack around with nsIArrays ?
Assignee: nobody → fred.jen
Attached patch unifing the sort mechanism (obsolete) — Splinter Review
The two trees are using the same structure of sort algorithms and I moved some functions into a newly created calSortUtil.js. I am not sure if we really need this new file, they could become part of the calUtils, too.
The task tree uses a localeCollator and the unifinder a real status order.
Attachment #340316 - Flags: review?(Berend.Cornelius)
Comment on attachment 340316 [details] [diff] [review]
unifing the sort mechanism

I find that the quite complitated source code has not really become easier to understand with your patch. As far as I se what you majorly did was to extract some sorting functions into an own class, but I am sure that here can be done more consolidation. I admit I find it sometimes hard to predict all upcoming problems in advance that's why I to keep my proposals a bit vague but how about this:
In both trees you have several columns that want to be sorted by different keys, e.g. “”, “priority”, “complete”. Why don't you define these sort keys in the respective xul files as attribute like “sortkey='start'”, “sortkey='priority'” .... directly at the columns? I would suggest to declare these attributes - probably as member variables - in your sorting component so that you have them properly defined somewhere. This way you could move all the logic of “SortEntryComparer” and “getItemSortKey” into your sorting component without hardwiring this to the existing trees. This would also ease your future work at the consolidation of the two trees.
Maybe this idea could even be extended:
If you define an attribute like (again fast-spoken) “item-content” at the tree-columns with attribute values like “property”, “complete”.. you could even consolidate the “getCellText()  functions of both trees in the same way.

For the moment I would like to deny the review.
Attachment #340316 - Flags: review?(Berend.Cornelius) → review-
Attached patch Sort - v3 (obsolete) — Splinter Review
I pushed everything out of the trees. There isn't anything else then the sortItems function.
The sortEntryComparer is independant of items and can be used whenever needed.
The getSortEntry is independant of xul, the only thing that is a bit strange are the names.
I simplified the code for the sortActive and sortDirection tags. The unifinder-code become somehow more complicated for this part, but now the code is the same for both trees. I am not sure if everything is fine here, it is saving the sort entry sometimes and sometimes not, but it doesn't kill anything.

The difference between item-content and sortKey is only done if the content isn't a "real" itemProperty.
Attachment #340316 - Attachment is obsolete: true
Attachment #341088 - Flags: review?(Berend.Cornelius)
I think that this patch is a very good improvement concerning the simplification of the tree implementations. You put almost all the "sorting" logic into the sortingComponent.
I find that you should go even one step further and include all the "sortType" "sortKey" logic to that component, too. These attributes seem redundant for me as the sortType of the column content (duedates, entrydates, calendar...) is already determined by itself. So you could sort the array directly on the item-content. This way you would probably not even need the sortKey and sortType and SortEntryComparer stuff at all as far as I can see. IMO the sorting component could be built around an array of items, because in a calendar-application items are actually the most central data structure. If it is capable to sort also other arrays I wouldn't mind this either of course.
I would call the item-content "itemproperty", because this is what it actually - in most cases - is (but this is probably just my taste).
A patch big as this should probably be tested by Andreas before we integrate it.
I also think that the attribute values for the "item-content" or itemproperty or whatever should be directly derived from the property names of an item ("entrydate", "duedate", "startdate"..) -if possible. This way you would make the implementation more obvious. Of course with the the "completedDate" value we have a somehow "ugly" exception that we have to deal with somehow - maybe by defining a member variable -don't really know.

One good side effect of your implementation is also that you can now put the "getCellText" function into the parent class of the consolidate trees in a followup patch
Attached patch Sort - v4 (obsolete) — Splinter Review
Changed as proposed. You can't sort out the sortKey completly. A good example is the duration col in the task tree. The sortKey and and the content are different objects, so we have to handle it in different ways. It may be possible to implify it again, but I think that we are losing extensibility then.
Attachment #341088 - Attachment is obsolete: true
Attachment #341121 - Flags: review?(Berend.Cornelius)
Attachment #341088 - Flags: review?(Berend.Cornelius)
Comment on attachment 341121 [details] [diff] [review]
Sort - v4

The patch looks good. Yet I am afraid I have to deny the review because
1) you corrupted the sorting indicators at the column headers. This sems to be due to your changes of the "sort-Active" and "sortdirection" attributes that you moved from the columns to the tree.  Please see the following link for a description
2) some columns like "due" and "start" were just empty as Andreas also noticed.

Some other notes about the patch:
Here you did not change the attribute name:

Actually I still cannot really see why you need the sortKey and the sort Type stuff. It would make the code more understandable if you just implement everything with the sorting just based on the "itemproperty" attribute. I wouldn't mind if you put all the logic how to deal with e.g. an item duration in the sorting component. But however I am not insisting on this as your logic is majorly based on the existing implmentation.
Attachment #341121 - Flags: review?(Berend.Cornelius) → review-
Attached patch Sort - v5 (obsolete) — Splinter Review
The sortActive header works.
I didn't sort out the sortKey, maybe we will find that it is really stupid to have it, but at the moment I don't see this.
I can't test if everything persists, because my calendar mustn't be ended normally.
Attachment #341121 - Attachment is obsolete: true
Attachment #341441 - Flags: review?(Berend.Cornelius)
Attached patch Sort - v6Splinter Review
- reverted the changes on modifyItem
Attachment #341441 - Attachment is obsolete: true
Attachment #341468 - Flags: review?(Berend.Cornelius)
Attachment #341441 - Flags: review?(Berend.Cornelius)
Attachment #341468 - Flags: review?(Berend.Cornelius) → review+
Comment on attachment 341468 [details] [diff] [review]
Sort - v6

I applied the patch and found it worked fine. I could see some redundancy e.g the implementation of "selectedColumn", but you can sonsolidate this with the rest of the tree implementations. I know and appreciate that you have invested so much effort into this. I hope that you see through the code now much clearer and got an idea how to bring together also the rest of the implementations.
I helped me definitely. I will try to break out redundancy now in a next step. When the sort and the filter patch are commited I will sort out the treeview and the observer objects. I think that there can be done a lot, I don't even know which code is more useful and sophisticated.
patch  Sort - v6 pushed to comm-central.
As were are about to move all common "calUtils.js" stuff into an own namespace I moved the "calSortUtils.js" content to "calUtils.jsm" in coordination with daniel. As I noted in other bugs already it will be a task for the coming months to find appropriate places for all these functions.
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0
Target Milestone: 1.0 → 1.0b1
You need to log in before you can comment on or make changes to this bug.