search

stamboman / workspacemechanic

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

Review Issue 83 change d90d6246832e #84

Closed GoogleCodeExporter closed 8 years ago

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

https://code.google.com/a/eclipselabs.org/r/stevemash-devel/source/detail?r=d90d
6246832e8c4c5425b39a4d15214de4fdb68f

The KeyboardBindingTask was defaulting to CompositeTask's getId() impl which 
meant that multiple keyboardBindingTasks all shared the same id.  This 
prevented meaningful blocked task functionality.

This patch implements a source specific id in the same way that 
LastModifiedPreferencesFileTask implements its getId()

When reviewing my code changes, please focus on:
* there were no unit tests for the Id.  I thought about trying to add one, 
mocking out the taskRef but not sure if its worth it. Let me know and I'll be 
happy to add.
* I went ahead and updated equals and hashcode to be based off of the Id 
instead of model.equals().  I think this makes sense as it would be possible 
(perhaps silly) for two kbd files with different names, descriptions to contain 
identical key bindings.  Thus the model would be the same (task.equals() 
returns true) but the IDs would be different.  So consistency seems important.  
If you prefer the model to be used for equality then perhaps the Id should be 
based off of some hash of the model.  Thoughts?
* I put the formatting of the id in the public constructor before delegating to 
the package private one.  I was thinking this would make it easier to test, but 
kind of clutters up the public constructor and puts something kind of important 
(id creation) almost as a detail.

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

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

GoogleCodeExporter commented 8 years ago 
Z this is yours to review, please.

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

GoogleCodeExporter commented 8 years ago 
This isn't still pending, is it?

Original comment by konigsb...@gmail.com on 23 Jun 2012 at 1:47

GoogleCodeExporter commented 8 years ago 
Nope you merged it in a while ago.  I just confirmed in your master.  This can 
be closed as fixed.

Original comment by stevemash on 23 Jun 2012 at 3:43

GoogleCodeExporter commented 8 years ago

Original comment by konigsb...@gmail.com on 24 Jun 2012 at 3:35