Bugzilla – Full Text Bug Listing
| Summary: | [GTK3] TabFolder cannot be traversed with Tab key; TabFolder#getChildren() is empty | ||
|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Markus Keller <markus.kell.r> |
| Component: | SWT | Assignee: | Markus Keller <markus.kell.r> |
| Status: | VERIFIED FIXED | QA Contact: | |
| Severity: | major | ||
| Priority: | P3 | CC: | akurtakov, arunkumar.thondapu, daniel_megert, ericwill, isabellerichard89, Lars.Vogel, lufimtse, malaperle, markus.kell.r, patrick.tasse, peter, piotr.findeisen |
| Version: | 4.5.1 | Flags: | arunkumar.thondapu:
review+
|
| Target Milestone: | 4.5.2 | ||
| Hardware: | PC | ||
| OS: | Linux | ||
| See Also: |
https://git.eclipse.org/r/59782 https://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=fb8bba31430f2cf26f6c4a4e51ed776390dfab95 |
||
| Whiteboard: | |||
| Bug Depends on: | 454936 | ||
| Bug Blocks: | 474628 | ||
|
Description
Markus Keller
It seems _getChildren in Composite is not correctly traversing the g_list in GTK3. I will investigate further. *** Bug 481297 has been marked as a duplicate of this bug. *** Bug 481297 has been closed as duplicate of this one. Can this be updated as affecting 4.5.1? Will this be solved in 4.5 maintenance? I must say impossibility to TAB in File Search strikes me several times a day. (In reply to Piotr Findeisen from comment #3) > Bug 481297 has been closed as duplicate of this one. Can this be updated as > affecting 4.5.1? > > Will this be solved in 4.5 maintenance? I must say impossibility to TAB in > File Search strikes me several times a day. Most likely, yes. This is a pretty major bug so once it's fixed it will probably be backported. (In reply to Piotr Findeisen from comment #3) > I must say impossibility to TAB in > File Search strikes me several times a day. You may want to go back to GTK2 until this is fixed. Set environment variable SWT_GTK3 to 0 before starting Eclipse. (In reply to Markus Keller from comment #5) > You may want to go back to GTK2 until this is fixed. > Set environment variable SWT_GTK3 to 0 before starting Eclipse. My Eclipse was freezing in "gtk os call" until i removed SWT_GTK3=0. (In reply to Piotr Findeisen from comment #6) > My Eclipse was freezing in "gtk os call" until i removed SWT_GTK3=0. Please file a separate bug and attach stackdumps if you think your GTK2 libraries are installed correctly. GTK2 is still supposed to work. (In reply to Eric Williams from comment #1) > It seems _getChildren in Composite is not correctly traversing the g_list in > GTK3. I will investigate further. See bug 454936. The TabFolder class needs an implementation comment that points readers to the long "Architecture Fix" comment in TabItem. (In reply to Markus Keller from comment #7) > See bug 454936. The TabFolder class needs an implementation comment that > points readers to the long "Architecture Fix" comment in TabItem. Yes this is the issue. There is something wrong with the way we store Widgets in the widget table, because g_object_get_qdata() in Display.getWidget() doesn't return the widgets correctly when called by Composite._getChildren() (at least in the case of TabFolder/GtkNotebook). I've managed to get the focus to switch properly with some hacky workarounds in Control.traverseGroup(). The focus switches properly once the proper TabFolder child widgets are populated into the list. More investigation to follow. Leo, Alex: It looks like bug 454936 was quite incomplete. I also think the ASCII-art in TabItem is not correct: * GTK3+: * swtFixed * |-- GtkNoteBook * |-- tabLabel1 * |-- tabLabel2 * |-- swtFixed (child1) //child now child of Notebook. * |-- swtFixed (child2) Shouldn't it be like this? * GTK3+: * swtFixed * |-- GtkNoteBook * |-- tabLabel1 * |-- tabLabel2 * |-- pageHandle (tabItem1) * |-- child1 //child now child of Notebook.pageHandle. * |-- pageHandle (tabItem2) * |-- child1 Please make sure there's an SWT test case that verifies the result of TabFolder#getChildren(). It's currently always empty on GTK3. TabFolder#minimumSize(int, int, boolean) is also wrong. E.g. change Snippet76 like this to create buttons of different heights: button.setText ("Page " + i + (i > 0 ? "\nLine 2": "")); And also have a look at TabFolder#releaseChildren(..) again. That method calls super.releaseChildren(), which calls Composite#_getChildren() -- and the latter doesn't make sense any more in GTK3. (In reply to Markus Keller from comment #9) > Leo, Alex: It looks like bug 454936 was quite incomplete. I also think the > ASCII-art in TabItem is not correct: > > * GTK3+: > * swtFixed > * |-- GtkNoteBook > * |-- tabLabel1 > * |-- tabLabel2 > * |-- swtFixed (child1) //child now child of Notebook. > * |-- swtFixed (child2) > > > Shouldn't it be like this? > > * GTK3+: > * swtFixed > * |-- GtkNoteBook > * |-- tabLabel1 > * |-- tabLabel2 > * |-- pageHandle (tabItem1) > * |-- child1 //child now child of Notebook.pageHandle. > * |-- pageHandle (tabItem2) > * |-- child1 Technically these are the same, no? The swtFixed (child 1, 2) in Leo's original diagram represent the pageHandles (TabItems) which are children of TabFolder, (or GtkNotebook). The children of these pageHandles are the controls of those individual TabItems. > which calls Composite#_getChildren() -- and > the latter doesn't make sense any more in GTK3. In the sense that the logic of calling it is incorrect, or because on GTK3 _getChildren() does not function as expected? (In reply to Eric Williams from comment #10) > Technically these are the same, no? No, I think the GTK3 diagram shows what went wrong in the implementation. As I read the comment, "swtFixed" refers to Control#fixedHandle. The problem on GTK3 is that the TabItem#control objects are NOT children of that TabFolder#fixedHandle in GTK3, but they are children of each pageHandle. The current GTK3 diagram doesn't show that. > > which calls Composite#_getChildren() -- and > > the latter doesn't make sense any more in GTK3. > > In the sense that the logic of calling it is incorrect, or because on GTK3 > _getChildren() does not function as expected? It doesn't make sense to dispose of the TabItems and then try to dispose all _getChildren () again. You have to verify that disposing the TabItems also properly disposes of the TabItem#control objects and also properly releases the GTK3 objects (I don't know the protocol there). Then remove the unnecessary traversal of _getChildren (). New Gerrit change created: https://git.eclipse.org/r/59753 (In reply to Markus Keller from comment #11) > No, I think the GTK3 diagram shows what went wrong in the implementation. As > I read the comment, "swtFixed" refers to Control#fixedHandle. The problem on > GTK3 is that the TabItem#control objects are NOT children of that > TabFolder#fixedHandle in GTK3, but they are children of each pageHandle. The > current GTK3 diagram doesn't show that. I understand now: you are correct, that diagram should be updated. > It doesn't make sense to dispose of the TabItems and then try to dispose all > _getChildren () again. You have to verify that disposing the TabItems also > properly disposes of the TabItem#control objects and also properly releases > the GTK3 objects (I don't know the protocol there). Then remove the > unnecessary traversal of _getChildren (). It seems there are more issues at play here, which warrant a separate investigation/fix of TabFolder/TabItem in general. The reason this focus bug happens is because computeTabList() in Composite is called on itself as there are no overriding functions. When Control.traverseGroup() calls Shell.computeTabList(), it traverses down the Widget tree until it reaches the children of a TabFolder. The bug happens when it gets to TabFolder, at which point none of the children are returned (because of _getChildren() not working as expected). We can override computeTabList() in TabFolder to correctly return the child controls to Control.traverseGroup(), which will fix the focus issue in GTK3. (In reply to Eric Williams from comment #13) > We can override computeTabList() in TabFolder to correctly return the child > controls to Control.traverseGroup(), which will fix the focus issue in GTK3. I've actually found a much cleaner solution: in Composite.computeTabList() we can simply call _getChildren() with TabFolder.clientHandle() as an argument. New Gerrit change created: https://git.eclipse.org/r/59782 (In reply to Eric Williams from comment #14) No. An "if (... instanceof ...)" in a superclass that checks for a subclass is always architecturally wrong. You should not try to fix one effect of the problem, but you should try to understand where the problem was introduced and then find a better fix for the original problem. The reason for the focus bug and the wrong minimumSize(..) is that _getChildren() is broken. Here's a fix that reverts parts of bug 454936 and fixes _getChildren(): https://git.eclipse.org/r/59782 TODOs: - move the description of the version-dependent architecture from TabItem#setControl(Control) to the beginning of TabFolder - add regression tests Looks like the final fix should also be backported to 4.4.2, see bug 453827 comment 11. (In reply to Markus Keller from comment #16) > (In reply to Eric Williams from comment #14) > No. An "if (... instanceof ...)" in a superclass that checks for a subclass > is always architecturally wrong. You should not try to fix one effect of the > problem, but you should try to understand where the problem was introduced > and then find a better fix for the original problem. > > The reason for the focus bug and the wrong minimumSize(..) is that > _getChildren() is broken. Here's a fix that reverts parts of bug 454936 and > fixes _getChildren(): Fair point -- this is a much better fix. > TODOs: > - move the description of the version-dependent architecture from > TabItem#setControl(Control) to the beginning of TabFolder > - add regression tests Will you be including these in this patch or should a separate bug be filed for the test cases + comment cleanup? (In reply to Markus Keller from comment #16) > - add regression tests This patch solves the bug for me, but one of the Composite getChildren() tests fail: Composite c1 = new Composite(composite, 0); assertArrayEquals(":b:", new Control[]{c1}, composite.getChildren()); The expected result was 1 but getChildren() in this case returned 0. Gerrit change https://git.eclipse.org/r/59782 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=fb8bba31430f2cf26f6c4a4e51ed776390dfab95 (In reply to Eric Williams from comment #19) Test_org_eclipse_swt_widgets_TabFolder#test_getChildren() doesn't make much sense for TabFolder. It just tests the corner case where children of TabFolder are created but not registered under a TabItem using #setControl(Control). Fixed this in TabFolder#_getChildren(), moved implementation note to TabFolder, and added another test for getChildren(). Arun: OK to backport to 4.5.2? Bug 454936 introduced this accessibility issue. (In reply to Markus Keller from comment #21) > (In reply to Eric Williams from comment #19) > Test_org_eclipse_swt_widgets_TabFolder#test_getChildren() doesn't make much > sense for TabFolder. It just tests the corner case where children of > TabFolder are created but not registered under a TabItem using > #setControl(Control). > > Fixed this in TabFolder#_getChildren(), moved implementation note to > TabFolder, and added another test for getChildren(). > > Arun: OK to backport to 4.5.2? Bug 454936 introduced this accessibility > issue. +1 to backport to 4.5.2 and as mentioned in comment 17, should be backported to 4.4.2 as well... thanks Markus and Eric! Could someone backport this to 4.5? I would do it but I am not the author of the original patch or a committer so I can't push it to gerrit :) It will be a good fix for SWTBot too. (In reply to Marc-Andre Laperle from comment #23) > Could someone backport this to 4.5? I would do it but I am not the author of > the original patch or a committer so I can't push it to gerrit :) It will be > a good fix for SWTBot too. Pushed the patch to R4_5_maintenance branch via http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?h=R4_5_maintenance&id=a7bc9ddae7b09350e5d1aae1fce1a716bff1161f Today's M-build should have the fix. (In reply to Arun Thondapu from comment #24) > (In reply to Marc-Andre Laperle from comment #23) > > Could someone backport this to 4.5? I would do it but I am not the author of > > the original patch or a committer so I can't push it to gerrit :) It will be > > a good fix for SWTBot too. > > Pushed the patch to R4_5_maintenance branch via > http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/ > ?h=R4_5_maintenance&id=a7bc9ddae7b09350e5d1aae1fce1a716bff1161f > > Today's M-build should have the fix. I tested it and it works. Many thanks! Resolving as it is fixed now in 4.5.2 but the backport to 4.4.2+ is still pending. Verified in M20160203-1000. *** Bug 476169 has been marked as a duplicate of this bug. *** |