brunano21 / workspacemechanic

Automatically exported from code.google.com/p/workspacemechanic
0 stars 0 forks source link

Review Issue 1 change 498605e02e71 #82

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
Purpose of code changes on this branch:

https://code.google.com/a/eclipselabs.org/r/stevemash-devel/source/detail?r=4986
05e02e7196f093339571b827c6bb71f18227

(be sure to pick up all three revs with "1: " as the prefix to the commit 
comment (they are adjacent).  I pathologically pushed before testing...twice.

Hooking into the remove button to try and keep the list of unblocked tasks in 
sync is tricky (as the original author mentioned in the class comment).  
Rebuilding the list of unblocked tasks each time the user clicks new seems 
reasonable and significantly simpler.  I also added a comparator and sort the 
list of unblocked tasks by name, which seems reasonable.  There is a minor side 
effect of the sorting in that now the user doesn't know the order of known 
tasks -- which might make a difference if two tasks have identical IDs.  If 
they have identical IDs then you will already get unintuitive behavior, so I 
don't think this makes it worse.

When reviewing my code changes, please focus on:
Correctness of approach.  Again there were no test cases, and I didn't add any.

After the review, I'll merge this branch into:
Someone will merge into /trunk

Original issue reported on code.google.com by stevemash on 10 Dec 2011 at 4:07

GoogleCodeExporter commented 9 years ago
Agreed that duplicate IDs is bad.

Original comment by konigsb...@gmail.com on 18 Dec 2011 at 7:04

GoogleCodeExporter commented 9 years ago
Updated with feedback from the review: 
https://code.google.com/a/eclipselabs.org/r/stevemash-devel/source/detail?r=fa9e
70f0a8581e51e7c611141be1636446f92dda

Original comment by stevemash on 18 Dec 2011 at 8:28

GoogleCodeExporter commented 9 years ago

Original comment by konigsb...@gmail.com on 22 Dec 2011 at 7:47