search

jmchamberlain / workspacemechanic

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

Review Issue 57 change 7b20977c503e #81

Closed GoogleCodeExporter closed 8 years ago

GoogleCodeExporter commented 8 years ago 
Purpose of code changes on this branch:
Issue 57 
(https://code.google.com/a/eclipselabs.org/r/stevemash-devel/source/detail?r=7b2
0977c503e1341fbd150ba72e687daadb4ef5b) shows a problem where blocked task IDs 
are not displayed correctly.  It appears that these use to be stored separated 
by the File.pathSeparator and then later this was switched to json.  Prior to 
the switch, the preference panel was implemented using the PathEditor to 
display one line per blocked Id, and the PathEditor would split on the 
File.pathSeparator.  After the switch to json, eclipse on windows broke as 
PathEditor had no File.pathSeparators at all, and on Linux if using an URL task 
sources, the ":" would incorrectly be split on causing the incorrect results in 
the bug attachment.

This code drops the PathEditor-- doesn't seem appropriate anyways, and just 
implements a ListEditor, delegating the parsing to the BlockedTaskIdsParser

Code is in my clone:
https://code.google.com/a/eclipselabs.org/r/stevemash-devel/source/detail?r=7b20
977c503e1341fbd150ba72e687daadb4ef5b

When reviewing my code changes, please focus on:
There were no unit tests before, and I'm not sure the idiomatic way to add 
tests for eclipse preference panels.  There are a couple of ways we could mock 
things and extract TaskIdsListEditor, but not sure its worth it.

After the review, I'll merge this branch into:
Nothing as I'm not a committer, but presumably someone will merge to /trunk

Original issue reported on code.google.com by stevemash on 10 Dec 2011 at 3:56

GoogleCodeExporter commented 8 years ago 
Also as an FYI I tested this change on Linux and Windows to verify the expected 
behavior.

Original comment by stevemash on 10 Dec 2011 at 4:02

GoogleCodeExporter commented 8 years ago 
I appreciate your thorough description of the problem, and its solution. 
Comments provided, and they're minor. Once you apply those changes I'll pull it 
in.

As for testing, yeah, these are tough. Consider though that if I added tests to 
begin with :) we wouldn't be in this mess.

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

GoogleCodeExporter commented 8 years ago 
Updated to reflect the feedback from the review 
https://code.google.com/a/eclipselabs.org/r/stevemash-devel/source/detail?r=d9e0
917f2a3d20f943af361d34419667005e39b3

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

GoogleCodeExporter commented 8 years ago

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