Comment 1 Xavier Raynaud 2011-09-12 05:20:08 EDT

Created attachment 203136 [details]
proposed patch

Comment 2 Xavier Raynaud 2011-09-12 05:22:10 EDT

Just one more thing:
I think this widget is excellent, and I vote for moving this widget in jface :)

Comment 3 Francois Chouinard 2011-09-12 08:54:39 EDT

(In reply to comment #2)
> Just one more thing:
> I think this widget is excellent, and I vote for moving this widget in jface :)

Thanks :)

Patrick will be happy to hear that when he comes back.

Comment 4 Francois Chouinard 2011-09-12 16:40:39 EDT

Nice enhancement :-) I like the labelListener acting as a pass-through for item selection on mouse click. Nice touch.

It could be committed "as is" but I have two very minor stylish comments:

- IMO, the tooltip label key ("_TABLEITEM") should be a string constant. It doesn't have to be at the top of the file - a string constant in the constructor would be just fine.

- Maybe a break statement could be added when the column has been identified in the for-loops (for tooltip and double-click)


That's it.

Nice work.

Comment 5 Xavier Raynaud 2011-09-13 02:54:08 EDT

Created attachment 203209 [details]
patch proposal

new patch proposal

Comment 6 Xavier Raynaud 2011-09-13 02:57:07 EDT

Many thanks for your comments.
I've modified the patch according to your suggestions (see attached), and commit it.

Comment 7 Francois Chouinard 2011-09-13 09:12:22 EDT

(In reply to comment #6)
> Many thanks for your comments.
> I've modified the patch according to your suggestions (see attached), and
> commit it.

Patch looks great. Are you OK to commit it or do you want me to do it?

BTW, something I missed in the first review: you might want to add the standard EPL copyright notice in IDoubleClickListener and TooltipProvider and give yourself some credit. You should also add your name to the list of contributors in TmfVirtualTable.

Comment 8 Xavier Raynaud 2011-09-13 09:18:36 EDT

Hi,

My initial intention was to commit it.
Unfortunately, I had some trouble with my account. I requested a password change, but it seems that it does not affect git...

I guess I have to contact admins to solve this issue.

Comment 9 Francois Chouinard 2011-09-13 09:23:16 EDT

Ack.

Comment 10 Xavier Raynaud 2011-09-13 09:33:00 EDT

Problem solved. It's pushed.
I also add copyright notice and credits.

Comment 11 Jeff Johnston 2011-09-21 16:16:38 EDT

Comment on attachment 203209 [details]
patch proposal

Contributor is now comitter.  Less than 250 changed lines.

Comment 12 Francois Chouinard 2012-03-26 13:56:58 EDT

Delivered in 0.9.0