Bug 531928 - [GTK3] Table editing is broken
Summary: [GTK3] Table editing is broken
Status: VERIFIED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: SWT (show other bugs)
Version: 4.8   Edit
Hardware: PC Linux
: P3 major (vote)
Target Milestone: 4.8 M7   Edit
Assignee: Eric Williams CLA
QA Contact: Eric Williams CLA
URL:
Whiteboard:
Keywords: regression, triaged
: Snippet149_MultipleTableEditorControls 539109 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-03-02 08:55 EST by Simeon Andreev CLA
Modified: 2018-09-22 04:36 EDT (History)
6 users (show)

See Also:


Attachments
Snippet to reproduce the problem with. Creates a table with a combo cell editor. (3.91 KB, text/x-java)
2018-03-02 08:55 EST, Simeon Andreev CLA
no flags Details
Gif that shows the behaviour in PullUpMemberPage. (5.00 MB, image/gif)
2018-03-02 10:36 EST, Simeon Andreev CLA
no flags Details
Gif showing the problem when editing available sites with GTK 3.14. (2.17 MB, image/gif)
2018-03-07 08:42 EST, Simeon Andreev CLA
no flags Details
Git showing the problem when editing available sites with GTK 3.22. (1.74 MB, image/gif)
2018-03-07 08:42 EST, Simeon Andreev CLA
no flags Details
Snippet with a table with Combo and Text as editors. (2.01 KB, text/x-java)
2018-03-07 10:32 EST, Simeon Andreev CLA
no flags Details
JFace snippet, since JFace behaves differently (and correctly) than SWT on GTK 3.22. (3.31 KB, text/x-java)
2018-03-07 11:13 EST, Simeon Andreev CLA
no flags Details
small issue with patch 119090 on 3.14 (89.38 KB, image/png)
2018-03-09 11:45 EST, Andrey Loskutov CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Simeon Andreev CLA 2018-03-02 08:55:33 EST
Created attachment 272975 [details]
Snippet to reproduce the problem with. Creates a table with a combo cell editor.

Steps to reproduce:

1. Run attached snippet TestTableWithCombo.java.
2. Select a cell and choose a new value.
3. Observe that the value of the cell is not updated.
4. Click on the table.
5. Observe that the cell value is updated.

If I run with SWT_GTK3=0, I see changes of the value as expected. Not sure if this should therefore go to SWT.


I've noticed that e.g. refactoring action to pull up methods or members has a similar table with a combo cell editor, but functions correctly. I've been unable to determine why. Code is found in: org.eclipse.jdt.internal.ui.refactoring.PullUpMethodPage


Environment:

Eclipse SDK
Version: Photon (4.8)
Build id: I20171217-2000

RHEL 7.2
GTK 3.14
Comment 1 Simeon Andreev CLA 2018-03-02 09:48:52 EST
Comparing what the pull-up page does vs what the snippet does.

Pull-up:

Thread [main] (Suspended (breakpoint at line 1028 in PullUpMemberPage))	
	PullUpMemberPage$MemberActionCellModifier.modify(Object, String, Object) line: 129	
	ColumnViewer$2.setValue(Object, Object) line: 260	
	ColumnViewer$2(EditingSupport).saveCellEditorValue(CellEditor, ViewerCell) line: 113	
	TableViewerEditor(ColumnViewerEditor).saveEditorValue(CellEditor) line: 433	
	TableViewerEditor(ColumnViewerEditor).applyEditorValue() line: 299	
	ColumnViewerEditor$1.applyEditorValue() line: 152	
	CellEditor$1.run() line: 332	
	SafeRunner.run(ISafeRunnable) line: 42	
	JFaceUtil.lambda$0(ISafeRunnable) line: 44	
	1881949035.run(ISafeRunnable) line: not available	
	SafeRunnable.run(ISafeRunnable) line: 173	
	ComboBoxCellEditor(CellEditor).fireApplyEditorValue() line: 329	
	ComboBoxCellEditor.applyEditorValueAndDeactivate() line: 271	
	ComboBoxCellEditor.focusLost() line: 278	
	ComboBoxCellEditor$3.focusLost(FocusEvent) line: 168	
	TypedListener.handleEvent(Event) line: 144	
	EventTable.sendEvent(Event) line: 86	
	Display.sendEvent(EventTable, Event) line: 5632	
	CCombo(Widget).sendEvent(Event) line: 1348	

Snippet:

Thread [main] (Suspended (breakpoint at line 164 in TestTableWithCombo$CellModifier))	
	TestTableWithCombo$CellModifier.modify(Object, String, Object) line: 164	
	ColumnViewer$2.setValue(Object, Object) line: 260	
	ColumnViewer$2(EditingSupport).saveCellEditorValue(CellEditor, ViewerCell) line: 113	
	TableViewerEditor(ColumnViewerEditor).saveEditorValue(CellEditor) line: 433	
	TableViewerEditor(ColumnViewerEditor).applyEditorValue() line: 299	
	ColumnViewerEditor$1.applyEditorValue() line: 152	
	CellEditor$1.run() line: 332	
	SafeRunnable$1.run(ISafeRunnable) line: 126	
	SafeRunnable.run(ISafeRunnable) line: 173	
	ComboBoxCellEditor(CellEditor).fireApplyEditorValue() line: 329	
	ComboBoxCellEditor.applyEditorValueAndDeactivate() line: 271	
	ComboBoxCellEditor.focusLost() line: 278	
	ComboBoxCellEditor$3.focusLost(FocusEvent) line: 168	
	TypedListener.handleEvent(Event) line: 144	
	EventTable.sendEvent(Event) line: 86	
	Display.sendEvent(EventTable, Event) line: 5632	
	CCombo(Widget).sendEvent(Event) line: 1348	

Then they both do a refresh on the viewer.

I debugged the refresh for the snippet, it goes all the way down to the label provider, and sets correct text in the cell.

So maybe some SWT update of some sort is missing.
Comment 2 Simeon Andreev CLA 2018-03-02 09:52:44 EST
What strikes me as odd in the snippet, is that the cell modification is triggered first when the shell loses focus. This is not the case for the pull-up action.
Comment 3 Simeon Andreev CLA 2018-03-02 10:36:29 EST
Created attachment 272976 [details]
Gif that shows the behaviour in PullUpMemberPage.

Actually never mind. I was debugging Eclipse with SWT_GKT3=0 due to left-over settings.

The same behaviour can be seen for the pull-up action. See attached gif.
Comment 4 Eclipse Genie CLA 2018-03-05 04:07:05 EST
New Gerrit change created: https://git.eclipse.org/r/118642
Comment 5 Simeon Andreev CLA 2018-03-05 08:02:36 EST
OK, I think the problem here is that with GTK3, the combo is not "on top" of the table cell.

So the combo and the table viewer behave in the same manner, in respect to the point in time when the table cell is updated with the combo content.

What is missing is that the combo is not at all visible, even when the cell (TableItem) is selected.

If the CCombo were to be drawn above the TableItem, I imagine it should show the correct value.
Comment 6 Simeon Andreev CLA 2018-03-05 11:20:49 EST
I believe this is a very new regression, I observed that with Eclipse 4.8 from 20171702 (17 December 2017) the CCombo in the table cell behaved fine.
Comment 7 Simeon Andreev CLA 2018-03-07 06:56:40 EST
Maybe coming from Bug 511133. With the patch added to 4.7.3, I stop seeing the combo. Could be something else though, since this patch seems to depend on previous work.
Comment 8 Simeon Andreev CLA 2018-03-07 08:41:15 EST
I think there is a general problem here on GTK 3.14, one that is fixed on GTK 3.22.

I e.g. cannot see when I'm editing available sites from Window -> Preferences -> Install/Update -> Available Software Sites.

I'm attaching gifs of the problem, it seems to be exactly the same issue as in the snippet here.

Whether its combo or not, it looks like the table cell is painted *over* the table editor.
Comment 9 Simeon Andreev CLA 2018-03-07 08:42:09 EST
Created attachment 273020 [details]
Gif showing the problem when editing available sites with GTK 3.14.
Comment 10 Simeon Andreev CLA 2018-03-07 08:42:44 EST
Created attachment 273021 [details]
Git showing the problem when editing available sites with GTK 3.22.
Comment 11 Simeon Andreev CLA 2018-03-07 08:45:57 EST
See attached Eclipse48_GTK314_EditAvailableSites.gif and Eclipse48_GTK322_EditAvailableSites.gif.

In GTK 3.14, the table cell is painted over the cell editor (bad). So we don't see what we are editing. After editing though, the value is changed.

In GTK 3.22, the editor is painted over the table cell (good). So we see all actions done on the editor. E.g. selecting items in a combo, or typing in a text field.
Comment 12 Simeon Andreev CLA 2018-03-07 09:08:13 EST
Hi Eric,

we observed that, with GTK 3.22.10, org.eclipse.swt.widgets.Control.setVisible(boolean) will execute the code block:

/*
 * Raise this widget's GdkWindow if the reparentOnVisibility
 * flag has been set and visible is true. See bug 511133.
 */
if (reparentOnVisibility && OS.GTK3) {
	OS.gdk_window_raise(gtk_widget_get_window(topHandle));
}

On GTK 3.14.13, however, it will not do so.

We assume this is the problem, but we have no idea why the call might be missing. Any ideas?

Best regards and thanks,
Simeon
Comment 13 Andrey Loskutov CLA 2018-03-07 09:35:57 EST
(In reply to Simeon Andreev from comment #12)
> Hi Eric,
> 
> we observed that, with GTK 3.22.10,
> org.eclipse.swt.widgets.Control.setVisible(boolean) will execute the code
> block:
> 
> /*
>  * Raise this widget's GdkWindow if the reparentOnVisibility
>  * flag has been set and visible is true. See bug 511133.
>  */
> if (reparentOnVisibility && OS.GTK3) {
> 	OS.gdk_window_raise(gtk_widget_get_window(topHandle));
> }
> 
> On GTK 3.14.13, however, it will not do so.
> 
> We assume this is the problem, but we have no idea why the call might be
> missing. Any ideas?

A bit more context + stack:

To reproduce, open Window -> Preferences -> Install/Update -> Available software sites and try to click into the first column of any entry.
On GTK 3.22 a text editor appears and with a breakpoint at line 5625 in Control (4.8 head) we see this stack on GTK3.22:

at org.eclipse.swt.widgets.Control.setVisible(Control.java:5625)
at org.eclipse.swt.custom.ControlEditor.setEditor(ControlEditor.java:245)
at org.eclipse.swt.custom.TableEditor.setEditor(TableEditor.java:238)
at org.eclipse.swt.custom.TableEditor.setEditor(TableEditor.java:254)
at org.eclipse.jface.viewers.TableViewerEditor.setEditor(TableViewerEditor.java:115)
at org.eclipse.jface.viewers.ColumnViewerEditor.activateCellEditor(ColumnViewerEditor.java:204)
at org.eclipse.jface.viewers.ColumnViewerEditor.handleEditorActivationEvent(ColumnViewerEditor.java:422)
at org.eclipse.jface.viewers.ColumnViewer.triggerEditorActivationEvent(ColumnViewer.java:677)
at org.eclipse.jface.viewers.ColumnViewer.handleMouseDown(ColumnViewer.java:652)
at org.eclipse.jface.viewers.ColumnViewer.access$0(ColumnViewer.java:648)
at org.eclipse.jface.viewers.ColumnViewer$1.mouseDown(ColumnViewer.java:97)
at org.eclipse.swt.widgets.TypedListener.handleEvent(TypedListener.java:193)
at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:86)
at org.eclipse.swt.widgets.Display.sendEvent(Display.java:5637)
at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1348)
at org.eclipse.swt.widgets.Display.runDeferredEvents(Display.java:4891)
at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:4472)
at org.eclipse.jface.window.Window.runEventLoop(Window.java:818)

But this breakpoint is not hit on 3.14 and the editor does not appear :-(
What is wrong with 3.14?
Comment 14 Andrey Loskutov CLA 2018-03-07 09:42:19 EST
Note: before patch in bug 511133 table editing was broken for the *scrolled* table (one could edit entries *only* if table was not scrolled), after scrolling in the table editor always appeared at the *original* location *before* scrolling. This was on 3.22 and 3.14. Now on 3.22 editing seem to be OK, but 3.14 has a bad regression.

We have an application targeting RHEL 7.2 and thus we have a big problem: with 4.7.3 the table editing is broken after scrolling in the table, and with 4.8 it is broken completely.

We can patch SWT, but we need a direction where to search for the root cause.
Comment 15 Eric Williams CLA 2018-03-07 09:50:55 EST
This is interesting, I'll investigate.
Comment 16 Simeon Andreev CLA 2018-03-07 10:01:40 EST
Sorry for the misinformation, seems like Andrey and I had different examples in mind. Following the reproduction steps by Andrey I also see a call to Text.setVisible that reaches gdk_window_raise.
Comment 17 Eric Williams CLA 2018-03-07 10:04:51 EST
Just so we're clear then:

3.14: no call to gdk_window_raise, and thus Table editing is broken
3.22: everything is OK

correct?
Comment 18 Simeon Andreev CLA 2018-03-07 10:32:13 EST
Created attachment 273024 [details]
Snippet with a table with Combo and Text as editors.

Ugh, sorry for the spam.

But this looks to be worse than we thought. Even on GTK 3.22, if I put more than one TableEditor things go south. The second editor is not visible (compare to SWT_GTK3=0). See attached snippet.

I see the same problem if I use 2 combos or 2 text fields, in any two cell.
Comment 19 Eric Williams CLA 2018-03-07 10:39:16 EST
(In reply to Simeon Andreev from comment #18)
> Created attachment 273024 [details]
> Snippet with a table with Combo and Text as editors.
> 
> Ugh, sorry for the spam.
> 
> But this looks to be worse than we thought. Even on GTK 3.22, if I put more
> than one TableEditor things go south. The second editor is not visible
> (compare to SWT_GTK3=0). See attached snippet.
> 
> I see the same problem if I use 2 combos or 2 text fields, in any two cell.

I'm assuming this is on 4.8? Is this a regression from bug 511133 or is it separate?
Comment 20 Eric Williams CLA 2018-03-07 11:07:29 EST
(In reply to Eric Williams from comment #19)
> I'm assuming this is on 4.8? Is this a regression from bug 511133 or is it
> separate?

I ran the snippet and there are all sorts of things going wrong here.

master (after bug 511133):
3.22: Combo shows up (is always visible/never goes away), Text box does not.

3.14: neither the Combo nor the Text widgets show up at all.

GTK2: same behaviour as before the fix, see below.

pre-bug 511133:
3.22: second table editor appears (Text widget for column 1) but doesn't disappear. It doesn't go away when hitting enter, and it doesn't seem to change the text. The Combo shows up in column 0 but also never goes away (i.e. it's always visible)

3.14: same as 3.22

GTK2: same as GTK3.14 and 3.22.


It seems the second table editor (for column 1) is broken by the fixes for bug 511133. Why...I am not sure. I can also confirm that the update sites editing issue happens on GTK3.14 but not on 3.22...even though the gdk_window_raise() is triggered on 3.14 just as it is on 3.22.
Comment 21 Simeon Andreev CLA 2018-03-07 11:13:54 EST
Created attachment 273025 [details]
JFace snippet, since JFace behaves differently (and correctly) than SWT on GTK 3.22.

Sorry for the delays and inaccurate / scattered information, but we are working on Eclipse 4.7.3 and Eclipse 4.8, mixed with GTK 3.14 and GTK 3.22, and even more mixed with JFace and SWT. Its a nightmare to check anything.

(In reply to Eric Williams from comment #17)
> Just so we're clear then:
> 
> 3.14: no call to gdk_window_raise, and thus Table editing is broken
> 3.22: everything is OK
> 
> correct?

(In reply to Eric Williams from comment #19)
> (In reply to Simeon Andreev from comment #18)
> > Created attachment 273024 [details]
> > Snippet with a table with Combo and Text as editors.
> > 
> > Ugh, sorry for the spam.
> > 
> > But this looks to be worse than we thought. Even on GTK 3.22, if I put more
> > than one TableEditor things go south. The second editor is not visible
> > (compare to SWT_GTK3=0). See attached snippet.
> > 
> > I see the same problem if I use 2 combos or 2 text fields, in any two cell.
> 
> I'm assuming this is on 4.8? Is this a regression from bug 511133 or is it
> separate?

I'll try to answer both questions here.

1. GTK 3.14

a) In SWT, the TableItems paint over the editors. Its difficult to see what I'm doing in the editors, as shown in the gifs.

b) In JFace, the behaviour looks to be the same as in SWT.

2. GTK 3.22

a) In SWT, using multiple TableEditors, only the first behaves correctly. The rest behave like in GTK 3.14.

b) In JFace, using ICellModifiers for each column results in cell editors which work as expected.

Eclipse UI and our product use JFace from what I can see, so I think the SWT problem in 3.22 is not that much of a big deal. In 3.14 however we need some sort of a fix.


3. Control.setVisible()

a) For SWT, on GTK 3.14 and on GTK 3.22, clicking on the editors does not result in Control.setVisible() calls.

b) For JFace, on GTK 3.14 and on GTK 3.22, the setVisible() method is called by the activation of the JFace cell editor. Both on 3.14 and on 3.22 the calls reach gdk_window_raise().


4. Whether Bug 511133 causes a regression

I think the cell editors didn't work since before Bug 438505. The fix for Bug 438505 introduces *something* that doesn't work when the table is scrolled horizontally. This is the case, since the cell editors "move" with the horizontal scrolling. Looking at the code, with the fixedHandle, this is not surprising.

Bug 511133 removes this. I guess you could argue that this causes a regression, but to me the fix for Bug 438505 looks to be incomplete. From my perspective the editors didn't work as expected before and after the change for Bug 511133.
Comment 22 Eric Williams CLA 2018-03-07 12:24:10 EST
(In reply to Simeon Andreev from comment #21)
> Sorry for the delays and inaccurate / scattered information, but we are
> working on Eclipse 4.7.3 and Eclipse 4.8, mixed with GTK 3.14 and GTK 3.22,
> and even more mixed with JFace and SWT. Its a nightmare to check anything.

I know the feeling. :)

 
> 1. GTK 3.14
> 
> a) In SWT, the TableItems paint over the editors. Its difficult to see what
> I'm doing in the editors, as shown in the gifs.
> 
> b) In JFace, the behaviour looks to be the same as in SWT.

Understood.

 
> 2. GTK 3.22
> 
> a) In SWT, using multiple TableEditors, only the first behaves correctly.
> The rest behave like in GTK 3.14.
> 
> b) In JFace, using ICellModifiers for each column results in cell editors
> which work as expected.

Okay, so this is interesting -- usually it's JFace that is broken and SWT that works. IIRC in Jface the visibility of the editor widget is toggled on and off as the user clicks and selects. Some SWT snippets create a new table editor widget every time, I'll have to look into this.

 
> Eclipse UI and our product use JFace from what I can see, so I think the SWT
> problem in 3.22 is not that much of a big deal. In 3.14 however we need some
> sort of a fix.

I'll investigate the 3.22. As for 3.14...I strongly urge you to upgrade to 3.22. It's very hard for me to justify spending time on fixing bugs that happen only on 3.14. If pushing the agreed upon patch for bug 527729 helps with this, then I'd be happy to do that as soon as master is open for M7.


> 3. Control.setVisible()
> 
> a) For SWT, on GTK 3.14 and on GTK 3.22, clicking on the editors does not
> result in Control.setVisible() calls.

This is probably the crux of the issue: the visibility isn't being toggled properly for some reason.

> b) For JFace, on GTK 3.14 and on GTK 3.22, the setVisible() method is called
> by the activation of the JFace cell editor. Both on 3.14 and on 3.22 the
> calls reach gdk_window_raise().
> 
> 
> 4. Whether Bug 511133 causes a regression
> 
> I think the cell editors didn't work since before Bug 438505. The fix for
> Bug 438505 introduces *something* that doesn't work when the table is
> scrolled horizontally. This is the case, since the cell editors "move" with
> the horizontal scrolling. Looking at the code, with the fixedHandle, this is
> not surprising.
> 
> Bug 511133 removes this. I guess you could argue that this causes a
> regression, but to me the fix for Bug 438505 looks to be incomplete. From my
> perspective the editors didn't work as expected before and after the change
> for Bug 511133.

Okay -- bug 511133 will stay in for M6 as reverting doesn't seem to do anyone any favours.
Comment 23 Thomas Schindl CLA 2018-03-08 02:59:30 EST
JFace only shows one TableEditor at a time, so that might explain it?
Comment 24 Simeon Andreev CLA 2018-03-08 04:49:32 EST
Unfortunately our current release goes with RHEL 7.2 and GTK 3.14. The product was developed and tested on this platform. It will be used for at least an year, with RHEL 7.2.

Due to our current release situation, the severity of the problem here was overlooked.

I've opened RHEL support case for Eclipse 4.6: https://access.redhat.com/support/cases/#/case/02050832

The behaviour in the ticket is the one we observe with Eclipse 4.7.3. The patch from Eric for Bug 511133 removes the "floating" editor, but then no editor is shown at all (this bug).

We'll be looking into this, it has been decided that the problem blocks our release. In worst case scenario we will be setting SWT_GTK3=0 for our product.

Unfortunately we see the problem with Adwaita too (some of the gifs here were made with Adwaita). So Bug 527729 doesn't help us.
Comment 25 Simeon Andreev CLA 2018-03-08 06:18:47 EST
OK, I think I've reduced the problem to the bare minimum:

public static void main(String[] args) {
	Display display = new Display();
	Shell shell = new Shell(display);
	shell.setLayout(new FillLayout());
	shell.setSize(400, 200);
	shell.setText("Bug 531928");

	table(shell);

	shell.open();

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

private static void table(Shell shell) {
	Composite composite = new Composite(shell, SWT.NONE);
	composite.setLayout(new FillLayout(SWT.VERTICAL));

	Table table = new Table(composite, SWT.NONE);
	table.setLayoutData(new GridData(GridData.FILL_BOTH));

	TableColumn c1 = new TableColumn(table, SWT.CENTER);
	c1.setText("column");
	c1.setWidth(120);

	table.setHeaderVisible(true);
	table.setLinesVisible(true);

	Text text1 = new Text(table, SWT.BORDER);
	text1.setBounds(2, 1, 300, 20);
	text1.setText("HERE1");
	Text text2 = new Text(table, SWT.BORDER);
	text2.setBounds(2, 22, 300, 20);
	text2.setText("HERE2");
}

In GTK 3.22, I see "text1" and can edit it. I don't see "text2", but I can see that the mouse pointer changes to a text cursor when I mouse over it.

In GTK 3.14, I don't see either of the text fields, but the mouse cursor changes when I mouse over them.

Note that the SWT I'm using is for 4.7.3 + patch for Bug 511133 + removed extra entries from patch for Bug 438505 (as indicated in comment 511133#c23).
Comment 26 Simeon Andreev CLA 2018-03-08 07:06:05 EST
Changing the value of org.eclipse.swt.internal.gtk.OS.GTK_VERSION doesn't seem to change the situation.

So I don't think the improved behaviour comes from a magic switch. At least not from one in the Java part of SWT.
Comment 27 Simeon Andreev CLA 2018-03-08 07:51:13 EST
Here is the minimal snippet with GTK3 to add two text fields in a table, in the respective cells:

#include <gtk/gtk.h>
 
int main(int argc, char* argv[])
{
        gtk_init(&argc, &argv);
        GtkWidget *window, *entry, *table;
        window = gtk_window_new(GTK_WINDOW_TOPLEVEL);
        g_signal_connect(window, "delete-event", G_CALLBACK(gtk_main_quit), NULL);
 
        table = gtk_table_new(2, 1, 0);

        entry = gtk_entry_new();
        gtk_entry_set_text((GtkEntry *) entry, "TEXT1");
        gtk_table_attach(GTK_TABLE(table), entry, 0,1,0,1, GTK_FILL, GTK_FILL, 0,0);
 
        entry = gtk_entry_new();
        gtk_entry_set_text((GtkEntry *) entry, "TEXT2");
        gtk_table_attach(GTK_TABLE(table), entry, 0,1,1,2, GTK_FILL, GTK_FILL, 0,0);

        gtk_container_add(GTK_CONTAINER(window), table);
 
        gtk_widget_show_all(window);
        gtk_main();
        return 0;
}

Compiled with:

gcc `pkg-config --cflags gtk+-3.0` -o example example.cpp `pkg-config --libs gtk+-3.0`

SWT doesn't take the approach of calling "gtk_table_attach" as far as I can tell. Strikes me as odd, since GTK2 seems to also have the method, and the API documentation states:

> Widgets can be added to a table using gtk_table_attach() or the more convenient (but slightly less flexible) gtk_table_attach_defaults().

Maybe it didn't work at some point, but to me it seems that this should be used instead of setting the bounds of the editor to draw it at a specific cells location?

With SWT_GTK3=0 the SWT snippet works fine. I guess newer implementation in GTK3 broke this. I've no idea why GTK 3.22 would suddenly make parts of it work again, maybe some deviation in implementation.
Comment 28 Eric Williams CLA 2018-03-08 11:38:19 EST
(In reply to Simeon Andreev from comment #24)
> Unfortunately we see the problem with Adwaita too (some of the gifs here
> were made with Adwaita). So Bug 527729 doesn't help us.

I meant this in terms of making it easier for you to adopt GTK3.22/RHEL7.4.


(In reply to Simeon Andreev from comment #25)
> Note that the SWT I'm using is for 4.7.3 + patch for Bug 511133 + removed
> extra entries from patch for Bug 438505 (as indicated in comment 511133#c23).

It's possible that there is some other fix in 4.8 that "helped" fix this issue inadvertently. There have been several tree/drawing fixes in 4.8 so it's possible one of these fixes it.


(In reply to Simeon Andreev from comment #27)
> Here is the minimal snippet with GTK3 to add two text fields in a table, in
> the respective cells:
> 
> #include <gtk/gtk.h>
>  
> int main(int argc, char* argv[])
> {
>         gtk_init(&argc, &argv);
>         GtkWidget *window, *entry, *table;
>         window = gtk_window_new(GTK_WINDOW_TOPLEVEL);
>         g_signal_connect(window, "delete-event", G_CALLBACK(gtk_main_quit),
> NULL);
>  
>         table = gtk_table_new(2, 1, 0);
> 
>         entry = gtk_entry_new();
>         gtk_entry_set_text((GtkEntry *) entry, "TEXT1");
>         gtk_table_attach(GTK_TABLE(table), entry, 0,1,0,1, GTK_FILL,
> GTK_FILL, 0,0);
>  
>         entry = gtk_entry_new();
>         gtk_entry_set_text((GtkEntry *) entry, "TEXT2");
>         gtk_table_attach(GTK_TABLE(table), entry, 0,1,1,2, GTK_FILL,
> GTK_FILL, 0,0);
> 
>         gtk_container_add(GTK_CONTAINER(window), table);
>  
>         gtk_widget_show_all(window);
>         gtk_main();
>         return 0;
> }
> 
> Compiled with:
> 
> gcc `pkg-config --cflags gtk+-3.0` -o example example.cpp `pkg-config --libs
> gtk+-3.0`
> 
> SWT doesn't take the approach of calling "gtk_table_attach" as far as I can
> tell. Strikes me as odd, since GTK2 seems to also have the method, and the
> API documentation states:
> 
> > Widgets can be added to a table using gtk_table_attach() or the more convenient (but slightly less flexible) gtk_table_attach_defaults().
> 
> Maybe it didn't work at some point, but to me it seems that this should be
> used instead of setting the bounds of the editor to draw it at a specific
> cells location?
> 
> With SWT_GTK3=0 the SWT snippet works fine. I guess newer implementation in
> GTK3 broke this. I've no idea why GTK 3.22 would suddenly make parts of it
> work again, maybe some deviation in implementation.

I agree, the way Table editing is implemented is...less than ideal. We have a similar issue with Menus when attaching them to widgets. This is definitely something to be investigated in future releases. That said, usually GTK API that allows attaching one widget to another results in less flexibility, so there is a trade-off.
Comment 29 Simeon Andreev CLA 2018-03-08 11:54:09 EST
(In reply to Eric Williams from comment #28)
> It's possible that there is some other fix in 4.8 that "helped" fix this
> issue inadvertently. There have been several tree/drawing fixes in 4.8 so
> it's possible one of these fixes it.

No, the only difference is GTK 3.14 or GTK 3.22. We are using the same platform code.

We've only determined that as soon as gtk_widget_set_parent_window is called for editors in a table/tree, the editors are no longer painted over the table/tree, on GTK 3.14.

On GTK 3.22, *one* of the editors manages to paint over the tree/table, which is apparently sufficient for JFace trees/tables.

Inspection with the GTK inspector doesn't help either so far...
Comment 30 Eric Williams CLA 2018-03-08 12:02:23 EST
(In reply to Simeon Andreev from comment #29)
> (In reply to Eric Williams from comment #28)
> > It's possible that there is some other fix in 4.8 that "helped" fix this
> > issue inadvertently. There have been several tree/drawing fixes in 4.8 so
> > it's possible one of these fixes it.
> 
> No, the only difference is GTK 3.14 or GTK 3.22. We are using the same
> platform code.

Ah I see. It could be some difference in event ordering, the gap between 3.14 and 3.22 is quite wide (about 2 years).

 
> We've only determined that as soon as gtk_widget_set_parent_window is called
> for editors in a table/tree, the editors are no longer painted over the
> table/tree, on GTK 3.14.
> 
> On GTK 3.22, *one* of the editors manages to paint over the tree/table,
> which is apparently sufficient for JFace trees/tables.
> 
> Inspection with the GTK inspector doesn't help either so far...

And by JFace, you are referring to the snippet in comment 21, correct?
Comment 31 Simeon Andreev CLA 2018-03-09 02:34:09 EST
(In reply to Eric Williams from comment #30)
> And by JFace, you are referring to the snippet in comment 21, correct?

Yes.
Comment 32 Simeon Andreev CLA 2018-03-09 03:06:56 EST
(In reply to Eric Williams from comment #30)
> Ah I see. It could be some difference in event ordering, the gap between
> 3.14 and 3.22 is quite wide (about 2 years).

I don't see a draw event for the combo / text editor.
Comment 33 Andrey Loskutov CLA 2018-03-09 10:16:15 EST
OK, after a long debug session with Simeon we've found out, that to fix the table editing on GTK 3.14 we must revert the patch from bug 511133 *and* fix the x coordinate of the TreeItem/TableItem like below in getBoundsInPixels():

r.x -= parent.getClientAreaInPixels().x;
return r;

The reason why editors were not shown or were displaced on 3.14 was also that the gtk_tree_view_get_cell_area() returned "wrong" (not translated) x coordinate, but y coordinate was OK!

We believe that the root cause is probably somewhere else (not directly in getBoundsInPixels()) - may be scrolling for some reason doesn't update some internal GTK data for the x axis (either in SWT or in GTK itself).

However, applying this on GTK 3.22 did not work: the editing will be broken.

So probably the original patch for bug 511133 must be guarded with if(GTK >= 3.22), and for 3.14 we additionally need either the "r.x -= parent.getClientAreaInPixels().x;" patch above or *something* which fixes the x coordinate for table/tree editors.
Comment 34 Eclipse Genie CLA 2018-03-09 10:53:49 EST
New Gerrit change created: https://git.eclipse.org/r/119090
Comment 35 Andrey Loskutov CLA 2018-03-09 11:45:31 EST
Created attachment 273066 [details]
small issue with patch 119090 on 3.14

(In reply to Eclipse Genie from comment #34)
> New Gerrit change created: https://git.eclipse.org/r/119090

Beside things already mentioned on the patch, looks good on 3.22, but on 3.14 for rows containing images & checkboxes we should consider their sizes too, see attached picture (otherwise the editor will be shorter by their width).
Comment 36 Eric Williams CLA 2018-03-09 12:23:21 EST
I posted a more in depth comment on the gerrit review, but I do not think the proposed fix is a good one.

Snippet149 is the issue you are experiencing: it's broken after bug 511133 and seems to use similar logic to the one provided here.

I am seeing the pattern: snippets where the TableEditor widget is set once and never touched again don't seem to work after bug 511133. JFace and other SWT snippets (88, 124) either toggle the editor's visibility, or create/dispose the TableEditor widgets in a selectionListener. These cases seem to work fine.

Since this is a regression on my part I'll investigate a fix for M7.
Comment 37 Eclipse Genie CLA 2018-03-13 13:04:33 EDT
New Gerrit change created: https://git.eclipse.org/r/119353
Comment 38 Eclipse Genie CLA 2018-03-27 13:54:17 EDT
New Gerrit change created: https://git.eclipse.org/r/120290
Comment 39 Eric Williams CLA 2018-03-27 14:11:01 EDT
I've been doing some work regarding Table/Tree editing and here are my findings:

Bug 511133 broke these cases:
-Table/Tree editing on GTK3.10 - GTK3.18 (3.20+ is fine)
-Right click a file in the Project Explorer -> Properties -> Resources, the permissions table is broken on GTK3.8+
-Pure SWT table/tree editing on GTK3.8+ from snippet attachment 273024 [details] / comment 8


(In reply to Eclipse Genie from comment #38)
> New Gerrit change created: https://git.eclipse.org/r/120290

This patch reverts portions of the fix for bug 511133 and uses gtk_container_propagate_draw() to make sure children of Table/Tree get proper draw events: it fixes all cases on GTK3.20+. 

GTK3.10 - GTK3.18 has a bug in GTK3 which was fixed in 3.20, so these versions will remain broken. I cannot justify spending the time to fix these cases as these versions of GTK3 are obsolete. However @ Andrey/Simeon: I've posted a comment in the old gerrit with advice to help your situation on GTK3.14 / RHEL 7.2. Since you are patching your SWT manually on your end I think it will be a feasible solution.
Comment 41 Eric Williams CLA 2018-03-28 10:05:30 EDT
(In reply to Eclipse Genie from comment #40)
> Gerrit change https://git.eclipse.org/r/120290 was merged to [master].
> Commit:
> http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/
> ?id=4b0412b94e47be8f2c5480568a36ec71809a148f

Patch in master now. Table/Tree editing behaviour fixed on GTK3.20+, and Snippet149 works again.
Comment 42 Eric Williams CLA 2018-03-28 10:05:38 EDT
*** Bug 531885 has been marked as a duplicate of this bug. ***
Comment 43 Simeon Andreev CLA 2018-04-10 02:41:08 EDT
Hi Eric,

we've noticed (on GTK 3.22) that the file permission property page (right click on any file in the Project Explorer -> properties) doesn't work on our (patched) 4.7.2 platform. I've checked, the table itself uses multiple SWT editors, which don't work as described in comment 21.

In the latest SWT, the table works. I assumed all I had to do is revert our patches and take https://git.eclipse.org/r/#/c/120290/. However this doesn't seem to be enough (I did compile the native code for this) to make the permissions table work. I also see regressions in JFace tables: only the first table editor I click works, from that point on the rest of the cell editors misbehave.

Do you know if there are other patches that are required before this one?

Best regards and thanks,
Simeon
Comment 44 Simeon Andreev CLA 2018-04-10 05:41:19 EDT
Never mind, I overlooked the exposed event handling in Tree/Table while merging. The patch works as expected.
Comment 45 Eric Williams CLA 2018-05-08 10:27:16 EDT
Verified in I20180507-2205.
Comment 46 Ralf S. CLA 2018-09-22 04:36:16 EDT
*** Bug 539109 has been marked as a duplicate of this bug. ***