Fixed UDA & priority orders

[riodelphino]

Consider priority and UDA orders in taskrc, such as uda.priority.values = H,M,L,.
Note that this only works if the UDA type is set to string.

taskrc:

uda.priority.values = H,M,L,
# uda.priority.type = string

report.list.sort = priority+

Before:

H -> L -> M (Wrong... Alphabetically...)

After:

H -> M -> L (Correct !)

This is my first pull request on GitHub.
Please let me know if there are any issues or if I've made any mistakes.

[riodelphino]

#228

[thehunmonkgroup]

Thanks for the PR. I will tell you up front that we have to be very careful with the sorting code, in particular because it's a real performance bottleneck for large reports -- some of my feedback will be addressing that.

1. Is there a more efficient way to test if a column is a UDA at all? As opposed to pulling and parsing data from all columns in case they are UDA
2. Do integer UDAs already sort correctly? I'm guessing yes. If so, then this patch breaks them.
3. I think it's a bad idea to always call sorted(), the existing code only calls it in the descending order case, for efficiency in the ascending case.

[riodelphino]

@thehunmonkgroup Thank you for your quick reply.
I'm sorry — I'm just an amateur, and I submitted the PR without giving it enough thought.
I really appreciate your valuable feedback and will take some time to review my code carefully.
Please give me a little time.

[riodelphino]

I've updated the code based on your suggestions, in commit 95863fa

Updated

1. Is there a more efficient way to test if a column is a UDA at all? As opposed to pulling and parsing data from all columns in case they are UDA

• Cached UDA configs at the top of the sort() function.
• Added a DEFAULT_COLUMNS set to check whether a column is a UDA.

2. Do integer UDAs already sort correctly? I'm guessing yes. If so, then this patch breaks them.

• Now clearly bypasses numeric, date, and duration types to the default comparator function.
• Also bypasses string UDA which have no .values.

3. I think it's a bad idea to always call sorted(), the existing code only calls it in the descending order case, for efficiency in the ascending case.

• That was my mistake. It's been reverted.

Testing

For the performance concern, I tested with the following settings:

uda.test.values = a,b,c,
uda.test.type = string

and used a report like:

report.all.sort = test-

With around 250 tasks, I didn't notice significant difference in rendering speed.
(Is it too few ?)

---

I'm not entirely sure if I’ve fully understood your suggestions, so please let me know if there are still any issues with my approach.

[riodelphino]

I've made a small refinement to the sort logic in 19a59fa
This commit fixes a case where 'L' and empty values ('') were not sorted correctly.

[thehunmonkgroup]

We're heading in a bit of the wrong direction here. We definitely do not want to keep a hard-coded list of non-UDA values around, that's brittle.

In fact, we shouldn't be parsing the UDA config in this code at all, that should be the job of TaskParser proper -- it can be done once at startup or reload. Also, TaskTable is already passed an instance of of the parsed task configuration in task_config when it's initialized -- there's no need to recreate that.

I've added 6625681 to handle parsing UDA values as part of TaskParser, check that to see how to inspect the new uda_config attribute on the instance.

Also in 921f9fd I fixed a bug you would have introduced by stripping trailing commas from string values. Those are valid values, the value being an empty string.

So refactor your approach to inspect self.task_config.uda_config directly, I believe checking for a non-empty value of values_index would be sufficient to detect if your custom comparator should be used.

[thehunmonkgroup]

Here's a dummy install script that can be used to test the performance of your change:

custom-sort-dummy-install.sh.txt

Make that executable and run it, and it will create about 4200 tasks in a temporary install, and give you instructions about how to execute VIT to access it, with this config:

data.location=$TASK_DIR

uda.test.values = one,two,three,
uda.test.type = string

# Report sorted by priority ascending
report.priorityasc.description=Tasks sorted by priority (Ascending)
report.priorityasc.columns=id,priority,project,due,description
report.priorityasc.filter=priority.not:
report.priorityasc.sort=priority+,id+

# Report sorted by priority descending
report.prioritydesc.description=Tasks sorted by priority (Descending)
report.prioritydesc.columns=id,priority,project,due,description
report.prioritydesc.filter=priority.not:
report.prioritydesc.sort=priority-,id+

# Report sorted by test ascending
report.testasc.description=Tasks sorted by test (Ascending)
report.testasc.columns=id,test,project,due,description
report.testasc.filter=test.not:
report.testasc.sort=test+,id+

# Report sorted by test descending
report.testdesc.description=Tasks sorted by test (Descending)
report.testdesc.columns=id,test,project,due,description
report.testdesc.filter=test.not:
report.testdesc.sort=test-,id+

Once you get your PR sorted, I'd recommend running each of those reports several times with and without your change to check on the performance hit, and well as standard reports which don't use the custom comparator. That will allow us some confidence that the change will not negatively impact performance. Really, it's most important that it doesn't impact performance when sorting on columns other than UDA string type, as I think it's reasonable to give up a little performance on the string type sorting in exchange for it actually sorting correctly. ;)

[riodelphino]

Thank you for the detailed explanation and for even updating the main branch to support my changes — I really appreciate it!
I'll try it again based on your new commits and run the performance tests.
I'm going to need a bit more time to revise the PR.

[riodelphino]

I noticed that you wrote that shell script ! Thank you so much !
I was busy, and also wasn't sure whether to merge or rebase upstream/2.x, so it took me a while.

• Merged the latest upstream/2.x branch and reset my previous modifications.
• Then reapplied my UDA-related fixes in task_list.py.
• I inserted 4,200 dummy tasks using custom-sort-dummy-install.sh.
• export VIT_DIR and TASKRC
• Finally, I measured the report switching/display time for each report.

Results:

upstream/2.x

priorityasc, prioritydesc, testasc, testdesc: around 2 seconds
all, list, long: around 2.8 seconds

My fork

priorityasc, prioritydesc, testasc, testdesc: around 2 seconds
all, list, long: around 2.8 seconds

I measured the time manually using the timer app on my iPhone ^^;
There doesn't seem to be any noticeable performance difference.

Please let me know if there are still any concerns or improvements needed.

[thehunmonkgroup]

Something about the PR seems off, I'm seeing changes listed in vit/config_parser.py that have already been applied to the 2.x branch.

[riodelphino]

I'm sorry for the trouble. I'm still not fully familiar with how GitHub and forks work...

I suspect that the issue may have been caused by accidentally running git merge in the Reset to upstream/2.x commit.

Is the following the correct approach for me to take?

git reset --hard HEAD~2
git rebase upstream/2.x
# git add & commit
# Add my fix again
# git add & commit

Or, should I make new PR with another branch created with current upstream/2.x ?

[thehunmonkgroup]

i don't think you need to reset hard. if you have the latest 2.x code locally, a git rebase -i should allow you to see how the rebasing will work, squash/fixup extra commits, etc.

[thehunmonkgroup]

after the rebase -i you may need to force push the changes: git push -f

[riodelphino]

Thank you.

I tried git rebase -i upstream/2.x and squash Reapply fixes into Reset to upstream/2.x, and rename the commit as Reapply fixes.

But I'm not sure my procedure is right...

[thehunmonkgroup]

The code looks the same, see here:

Another way to approach this is to do a hard reset to a commit before your work started, then merge 2.x to get to the tip of that branch, then manually apply your changes and commit them, then do a force push with git push -f -- clunky but it should work.

Most importantly, look at the 2.x branch, it already contains the code in vit/config_parser.py, so you shouldn't be including that in your PR.