Comment 1 Arun Thondapu 2012-10-15 05:42:47 EDT

Thanks for the patch Anatoly!
I was just wondering if it is a better idea to create a helper method for this case since the same code seems to be repeating in multiple places.

Another minor detail - for changing the line

if (textRenderer != 0) OS.gtk_cell_renderer_get_size (textRenderer, handle, null, null, null, null, null);

it is probably better to use

if (textRenderer != 0) {
    if (OS.GTK_VERSION >= OS.VERSION (3, 0, 0)) {
      ....
    } else {
      ...
    }
}

because it will involve only one if check when textRenderer is null as opposed to the change in the patch which will always involve both if checks.

Let me know if you think otherwise.

Comment 2 Anatoly Spektor 2012-10-15 09:53:42 EDT

(In reply to comment #1)
> Thanks for the patch Anatoly!
> I was just wondering if it is a better idea to create a helper method for
> this case since the same code seems to be repeating in multiple places.
> 
> Another minor detail - for changing the line
> 
> if (textRenderer != 0) OS.gtk_cell_renderer_get_size (textRenderer, handle,
> null, null, null, null, null);
> 
> it is probably better to use
> 
> if (textRenderer != 0) {
>     if (OS.GTK_VERSION >= OS.VERSION (3, 0, 0)) {
>       ....
>     } else {
>       ...
>     }
> }
> 
> because it will involve only one if check when textRenderer is null as
> opposed to the change in the patch which will always involve both if checks.
> 
> Let me know if you think otherwise.

Thanks for your comments Arun. I totally agree that helper function would suit better in this case, I just didn't notice that all of them are "widgets"

I have put my implimentation into helper function. This solves second performance issue as well.

When you have time, please take a look at this revised patch:

http://fedorapeople.org/cgit/aspektor/public_git/eclipse.platform.swt.git/commit/?h=gtk3_cell_renderer

Comment 3 Arun Thondapu 2012-10-15 10:38:25 EDT

Need null checks before assigning to width and height arrays in Widget.gtk_cell_renderer_get_preferred_size() as they are not necessarily non-null parameters. Apart from that, the patch looks fine according to GTK+ 3 documentation.

Comment 4 Anatoly Spektor 2012-10-15 10:53:56 EDT

(In reply to comment #3)
> Need null checks before assigning to width and height arrays in
> Widget.gtk_cell_renderer_get_preferred_size() as they are not necessarily
> non-null parameters. Apart from that, the patch looks fine according to GTK+
> 3 documentation.

Thanks for the fast response. I have added null checks for both width and height arrays in Widget.gtk_cell_renderer_get_preferred_size(). 

Revised patch is here: http://fedorapeople.org/cgit/aspektor/public_git/eclipse.platform.swt.git/commit/?h=gtk3_cell_renderer

Comment 5 Arun Thondapu 2012-10-15 13:49:26 EDT

Thanks for the revised patch, merged to master - http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=b76a3210e5e0d07a0578d9ae225b51140c25db20