Community
Participate
Working Groups
Since the current web connector doesn't allow one to map the task priority to Mylyns internal priority code, all tasks end up with the default P3 priority. Since the Priority sort is only for the first level, it means that the tasks end up being show in pretty much random order. It would be nice if the tasks were sorted on a second level by the id/summary.
Created attachment 81834 [details] Use summary code to sort tasks if the priorities are equal Why the element2.getSummary() call?
Created attachment 81835 [details] mylyn/context/zip
George, can you please update your patch and include extended test cases into org.eclipse.mylyn.tasks.tests.TableSorterTest ? Also, if you are interested, it may worth to look at some optimizations. For example, getSortableSummaryFromElement() combine task key and summary for the AbstractTask, and then that string is split back into key and summary by TaskKeyComparator, which I think adds significant amount of computation for large task list.
Eugene, I did look at at TableSorterTest and noted that it was only concerned about the whether the two elements compared properly. It doesn't currently care how the elements are sorted. I'll try and come up with a sort sequence test case. I'll see how I can optimize the code as well.
Created attachment 83022 [details] Improved patch Added JUnit test. Sped up performance by by-passing the string building/splitting I created the ITaskListView interface so I could fake out TaskListView for the JUnit test
Created attachment 83023 [details] mylyn/context/zip
Rob: please review.
I'll review this Tuesday George.
Created attachment 83408 [details] updated patch George, I've clean up the api slightly so that sorting direction/column are held by the TaskListTableSorter. It is all working nicely with the exception that perhaps in your optimization of TaskKeyComparator, when it gets to the sorting by summary it isn't working as it did previously. With your patch applied I'm now seeing 4: foo, 44:foo2, 5: bar, 55:bar2. This should sort as 4:foo, 5:bar, 44:foo2, 55:bar2.
Created attachment 83476 [details] Proper sorting Sigh, this ended up being ugly. I misunderstood the PATTERN used to split up the task string. I'm not too happy with what I ended up doing to support the "XXX-NN", plus I ended up changing the compare() signature.
Created attachment 83477 [details] mylyn/context/zip
George, your last patch doesn't contain code but rather the comment text? Might be a bugzilla bug? Anyway, if you don't mind could you try re-attaching your patch?
Created attachment 83706 [details] Patch, again See if it gets through the system this time.
Created attachment 83707 [details] mylyn/context/zip
This is looking great so far. Just one outstanding issue related to sorting of LocalTasks. I can create an npe if sorting a few local tasks by priority: public void testLocalTaskSort() { final TaskListTableSorter sorter = new TaskListTableSorter(TaskListView.getFromActivePerspective()); AbstractTask task1 = new LocalTask("1", "task1"); AbstractTask task2 = new LocalTask("2", "task2"); AbstractTask task3 = new LocalTask("3", "task3"); AbstractTask[] tasks = { task1, task2, task3 }; sorter.sort(new EmptyViewer(), tasks); } We'll need to guard against nulls within the comparator. Once we have this covered by unit tests I think we're done here! java.lang.NullPointerException at org.eclipse.mylyn.internal.tasks.ui.views.TaskKeyComparator.compare(TaskKeyComparator.java:32) at org.eclipse.mylyn.internal.context.ui.TaskListInterestSorter.compareKeys(TaskListInterestSorter.java:189) at org.eclipse.mylyn.internal.context.ui.TaskListInterestSorter.comparePrioritiesAndKeys(TaskListInterestSorter.java:181) at org.eclipse.mylyn.internal.context.ui.TaskListInterestSorter.compare(TaskListInterestSorter.java:113) at org.eclipse.jface.viewers.ViewerComparator$1.compare(ViewerComparator.java:187) at java.util.Arrays.mergeSort(Arrays.java:1270) at java.util.Arrays.sort(Arrays.java:1210)
Created attachment 83728 [details] NPE fix Don't try to compare using a null task value
Great! Patch applied, ip log update. Flagged as noteworthy since this is such a nice yet subtle feature.
Marking resolved.
Rob: how is this going to be explicit in the UI? Don't we need to change the "Sort by -> Priority" label to be "Sort by -> Priority then Summary"? Either that or we can add a rule to each manual sorter that it defaults to Summary if it finds two equal entries. I suggest the latter.
+1 for using summary as second level sorting without cluttering sorting description
(In reply to comment #19) > Rob: how is this going to be explicit in the UI? Don't we need to change the > "Sort by -> Priority" label to be "Sort by -> Priority then Summary"? Either > that or we can add a rule to each manual sorter that it defaults to Summary if > it finds two equal entries. I suggest the latter. I like the default to summary option. At some point we could investigate using a dialog where sort order can be configured but I think that might be over kill at this point.
Sounds fine.
