Bug 385939 - Omit use of deprecated GTK_TOOLTIPS_TIP_WINDOW and gtk_tooltips_new
Summary: Omit use of deprecated GTK_TOOLTIPS_TIP_WINDOW and gtk_tooltips_new
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: SWT (show other bugs)
Version: 4.2   Edit
Hardware: PC Linux
: P3 normal (vote)
Target Milestone: 4.3 M2   Edit
Assignee: Silenio Quarti CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 340067
  Show dependency tree
 
Reported: 2012-07-25 09:21 EDT by Anatoly Spektor CLA
Modified: 2013-05-24 09:34 EDT (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Anatoly Spektor CLA 2012-07-25 09:21:04 EDT
Build Identifier: I20120608-1400

This patch omits use of GTK_TOOLTIPS_TIP_WINDOW and gtk_tooltips_new in the newer GTK versions, thus avoiding errors and warning when building SWT with GTK_DISABLE_DEPRECATED:


http://fedorapeople.org/gitweb?p=aspektor/public_git/eclipse.platform.swt.git;a=commit;h=d2a46cfbc7487aceddf63ffa5bc46f74e581958b

Reproducible: Always
Comment 1 Anatoly Spektor CLA 2012-07-27 12:24:37 EDT
Please ignore previous patch, as there is incorrect  os_custom.h  and use the patch below:

http://fedorapeople.org/gitweb?p=aspektor/public_git/eclipse.platform.swt.git;a=commit;h=5843ab624f1985a1a773e0a4e88eebad1a012189


Sorry for the confusion.

Regards,

Anatoly
Comment 2 Anatoly Spektor CLA 2012-07-27 12:29:35 EDT
(In reply to comment #1)
> Please ignore previous patch, as there is incorrect  os_custom.h  and use the
> patch below:
> 
> http://fedorapeople.org/gitweb?p=aspektor/public_git/eclipse.platform.swt.git;a=commit;h=5843ab624f1985a1a773e0a4e88eebad1a012189
> 
> 
> Sorry for the confusion.
> 
> Regards,
> 
> Anatoly

The comment above is regarding patch 386145. 

  With this patch everything is ok.

 Pleas ignore it for this patch, and sorry for the confusion again.
Comment 3 Silenio Quarti CLA 2012-08-01 17:21:33 EDT
The changes in Shell do not make sense (not needed). The code path is already not running for GTK_VERSION>=2.12.0 (see the beginning of the method).

The changes in Tooltip are wrong. Aren't we leaking a window everytime we do this kind of code:

tipWindow = OS.gtk_window_new(OS.GTK_WINDOW_POPUP);

I would expect the window to be created once in Tooltip.createHandle(), saved as an instance var and used everywhere else.

This line is really odd. Why are you setting the tooltip window to NULL.

OS.gtk_widget_set_tooltip_window(tipWindow, 0);
Comment 4 Anatoly Spektor CLA 2012-08-03 10:51:53 EDT
(In reply to comment #3)
> The changes in Shell do not make sense (not needed). The code path is
> already not running for GTK_VERSION>=2.12.0 (see the beginning of the
> method).
> 

Agree, I will take out changes to Shell as it is already guarded. Thank you for noticing.

> The changes in Tooltip are wrong. Aren't we leaking a window everytime we do
> this kind of code:
> 
> tipWindow = OS.gtk_window_new(OS.GTK_WINDOW_POPUP);
> 
The question is: wasn't window leaked before when:

 OS.GTK_TOOLTIPS_TIP_WINDOW (handle);   was used?

 

> I would expect the window to be created once in Tooltip.createHandle(),
> saved as an instance var and used everywhere else.
> 

  I absolutely agree that this approach is much better, all I was trying to do is just replicate the old code with the new/not deprecated way.

I'll change my code to create window once.

> This line is really odd. Why are you setting the tooltip window to NULL.
> 
> OS.gtk_widget_set_tooltip_window(tipWindow, 0);

 I don't think that this line is odd, please take a look here:

 http://developer.gnome.org/gtk/2.24/GtkWidget.html#gtk-widget-set-tooltip-window

  All I am doing here is just specifying that default tooltip should be used instead of custom_window.


Thank you for your comments, I'll review my patch taking into account your comments and I'll resubmit it as soon as possible.

Regards,

Anatoly
Comment 5 Anatoly Spektor CLA 2012-08-03 12:33:49 EDT
Regarding Tooltip as it is now, can someone please try calling Tooltip.getVisible() with Tooltip code as it is now in master ?

I am using this test program:

import org.eclipse.swt.SWT;
import org.eclipse.swt.widgets.Display;
import org.eclipse.swt.widgets.Shell;
import org.eclipse.swt.widgets.ToolBar;
import org.eclipse.swt.widgets.ToolItem;
import org.eclipse.swt.widgets.ToolTip;

public class Tooltip_test {

  public static void main(String[] args) {

    Display display = new Display();
    Shell shell = new Shell(display);

    ToolBar bar = new ToolBar(shell, SWT.BORDER);
    ToolTip tip = new ToolTip(shell,0);
    tip.getVisible();
    bar.setBounds(0, 20, 200, 64);
    ToolItem item1 = new ToolItem(bar, 0);
    item1.setToolTipText("ToolItem \n toolTip");


    shell.open();
    while (!shell.isDisposed()) {
      if (!display.readAndDispatch())
        display.sleep();
    }
    display.dispose();
  }
}


And I am getting core dump. 

Question is: Is it a bug in Tooltip or I am trying to test it in a wrong way ?

Thanks in advance,

Anatoly
Comment 6 Anatoly Spektor CLA 2012-08-03 14:19:10 EDT
What I have done:

1. Removed implementation from Shell as it is guarded by 2,12 already as it was pointed out by Silenio.

2. Created instance variable tipWindow and initialized it in createHandle() by creating Tooltip window with default tooltip (that's why '0' passed as second argument)

3.Created helper function "gtk_tooltip_tips_window" inside Tooltip to avoid repetitive version guard code 

4. Replaced GTK_TOOLTIPS_TIP_WINDOW with version guarded helper

5. Changed logic of return  of getVisible() to avoid crash when GTK_WIDGET_VISIBLE receives null as argument. ( see Comment 5)

6. Tested on the test program above as well as on Tooltip tab in Control Example. 

Please find patch here:

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


Regards,

Anatoly
Comment 7 Silenio Quarti CLA 2012-08-03 16:16:40 EDT
(In reply to comment #4)
> > 
> The question is: wasn't window leaked before when:
> 
>  OS.GTK_TOOLTIPS_TIP_WINDOW (handle);   was used?

GTK_TOOLTIPS_TIP_WINDOW just accesses a field in the widget. It does not leak.

> 
>  
> 
> > I would expect the window to be created once in Tooltip.createHandle(),
> > saved as an instance var and used everywhere else.
> > 
> 
>   I absolutely agree that this approach is much better, all I was trying to
> do is just replicate the old code with the new/not deprecated way.
> 
> I'll change my code to create window once.
> 
> > This line is really odd. Why are you setting the tooltip window to NULL.
> > 
> > OS.gtk_widget_set_tooltip_window(tipWindow, 0);
> 
>  I don't think that this line is odd, please take a look here:
> 
>  http://developer.gnome.org/gtk/2.24/GtkWidget.html#gtk-widget-set-tooltip-
> window
> 
>   All I am doing here is just specifying that default tooltip should be used
> instead of custom_window.

Creating a new gtk window is not a replacement for GTK_TOOLTIPS_TIP_WINDOW. We really need the window that is used to display the tooltip.  For example, TooltTip.getVisible() for the non SWT.BALLOON case, uses the tipWindow to determine whether the tooltip is visible or not.  Your latest patch always returns false because it is asking to an unrelated window that is always hidden.

The code in master for ToolTip.getVisible() is not working either with GTK 2.24. I am pretty sure it used to work before GTK 2.12. We need to try RedHat 5.8 to confirm since it has gtk 2.10 by default. Anyways, it has to be fixed as well.

Note that I do not get the segmentation fault you are getting in ToolTip.getVisible(). The tip window should never be null because we call gtk_tooltips_force_window() in createHandle(). Maybe you were running the code with a library compile after applying your patch (GTK_TOOLTIPS_TIP_WINDOW returns NULL if compiled with GTK >= 2.12).
Comment 8 Silenio Quarti CLA 2012-08-03 19:47:29 EDT
I have released changes that fix this problem specifically. Some of the changes should have been there since we added the >= 2.12 code path. All the code related to GTK_TOOLTIPS_TIP_WINDOW only makes sense to the < 2.12 code path.

http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=88dc3a4dc3c68bb6669db018f4d6d63689ecddba

We still need to fix getVisible(), setLocation(), etc. But that is a separate bug. An option is to fake the tooltip the same way we do for the SWT.BALLOON style.

Anatoly, please take a look at the changes and let me know if see any issues.
Comment 9 Anatoly Spektor CLA 2012-08-07 10:50:47 EDT
(In reply to comment #8)
> I have released changes that fix this problem specifically. Some of the
> changes should have been there since we added the >= 2.12 code path. All the
> code related to GTK_TOOLTIPS_TIP_WINDOW only makes sense to the < 2.12 code
> path.
> 
> http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/
> ?id=88dc3a4dc3c68bb6669db018f4d6d63689ecddba
> 
> We still need to fix getVisible(), setLocation(), etc. But that is a
> separate bug. An option is to fake the tooltip the same way we do for the
> SWT.BALLOON style.
> 
> Anatoly, please take a look at the changes and let me know if see any issues.

Thanks for your help Silenio. I didn't find any issues.

 Now we need to create our own tooltip window and avoid using gtk_tooltips_new() for newer GTK+.