Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 455671 - [GTK] Location of Shell gets reset after reparenting
Summary: [GTK] Location of Shell gets reset after reparenting
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: SWT (show other bugs)
Version: 4.4.1   Edit
Hardware: PC Linux
: P3 normal (vote)
Target Milestone: 4.5 M6   Edit
Assignee: Leo Ufimtsev CLA
QA Contact: Leo Ufimtsev CLA
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-12-18 12:09 EST by Alexandra Buzila CLA
Modified: 2015-05-29 05:11 EDT (History)
7 users (show)

See Also:


Attachments
code reproducing the issue (8.73 KB, application/x-zip-compressed)
2014-12-18 12:09 EST, Alexandra Buzila CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Alexandra Buzila CLA 2014-12-18 12:09:54 EST
Created attachment 249533 [details]
code reproducing the issue

The problem was observed for gtk2-2.24.23-6.

Steps to reproduce:
1. Create a shell, a childShell for that shell and a control (e.g. label) inside the shell. Open the shells.
2. Move the childShell to a new location (locationXXX)
3. Set the visibility of the childShell to false
4. Reparent the control from shell to childShell
5. Set the visibility of the childShell to true

As a result, when the childShell is shown again, it will be reset to its default position. The expected behaviour would be to show it in the last location (locationXXX), where it was before it was hidden.

The same problem occurs if the original parent of the control is the shell and on step 4 the control's parent is set to childShell.

I've attached a small program that can be used to reproduce the issue.
Steps: 
	- push the "Invisible" button
	- push "Reparent" button
	- push "Visible" button
	
The error seems to come from gtk (the position of the shell is reset if a control is reparented). I can also share a plain gtk code sample with you, exemplifying the issue - if you need it.
Comment 1 Leo Ufimtsev CLA 2014-12-19 12:52:28 EST
Interesting observation and thank you for the test case. 

I can reproduce the issue just by setting the widget to be invisible [setVisable(false)] and then setting it visible again [setVisable(true)]. 

(I.e, reparenting doesn't seem to be necessary to reproduce the issue).

This is reproducible on gtk2 and gtk3.14, Fedora 21.

Once the critical gtk3 bugs are squashed:
Bug 441566 - [GTK3] Improve support for newer versions of GTK+ 3 (>= 3.10.x)
We will work on fixing this also. (unless someone has time to work on this in the meantime).

Thank you
Comment 2 Alexandra Buzila CLA 2015-01-08 09:55:37 EST
I could not reproduce the issue you described - I tried to switch the visibility of both the label and shells with the same correct result. I only experience the issue when reparenting.

We tested this in plain gtk and the issue comes from there: the window's position gets reset after a reparenting takes place.
Comment 3 Eugen Neufeld CLA 2015-02-05 10:56:06 EST
Hi Leo, 
we would like to help solve this issue. 
Currently our only idea is to track the reparenting, by checking whether a reparenting is happening while one of the affected shells is invisible. This also would mean keeping track of the shell location by ourself. 

Maybe you have some idea how to solve this issue more elegant?

Otherwise I would provide a gerrit commit containing our solution as soon as it is ready.

Cheers,
Eugen
Comment 4 Leo Ufimtsev CLA 2015-02-05 11:02:56 EST
Hello Eugen, 

By all means, please go ahead with patch. The DND stuff has been keeping me occupied.

Gtk is somewhat notorious for not keeping size data. 
e.g in Control:setParent() there is a note about it:
/*
* Restore the original widget size since GTK does not keep it.
*/
resizeHandle(width, height);

I had similar issues in DateTime, and in the end I ended up caching size data (see DateTime:computeSize())

I'd be happy to review/test the patch.
Comment 5 Eclipse Genie CLA 2015-02-27 10:03:50 EST
New Gerrit change created: https://git.eclipse.org/r/42910
Comment 6 Eugen Neufeld CLA 2015-02-27 10:11:08 EST
Hi Leo, 
I implemented a solution that fixes the bahavior for me. 
Please let me know if the patch needs to be refactored etc. 

Cheers,
Eugen
Comment 7 Leo Ufimtsev CLA 2015-02-27 10:15:34 EST
I'll try to test/review today or early next week.
Comment 8 Eclipse Genie CLA 2015-02-27 15:46:00 EST
New Gerrit change created: https://git.eclipse.org/r/42930
Comment 9 Leo Ufimtsev CLA 2015-02-27 15:56:53 EST
(In reply to Eugen Neufeld from comment #6)
> Hi Leo, 
> I implemented a solution that fixes the bahavior for me. 
> Please let me know if the patch needs to be refactored etc. 
> 
> Cheers,
> Eugen

Thank you for the investigation and writing a patch to fix. It is most useful. 

I tried out your patch and it did work. 

My only concern was that we already have the 'oldX/oldY/moved' workaround (1*)
and the newly added workaround has variables: 'posX/posY/updatePosition'  that sort of duplicate the functionality of the existing workaround. 

So as a result we have~: Workaround * Workaround = Workaround^2   :-).

I investigated a little. There seems to be two issues with the existing workaround:
1) We handle a configure-event (*2) even if the shell is invisible, and this messes with the oldX/oldY position. 
2) We don't re-position the shell when the shell is made visible again from being invisible. 

In the interest of maintainability; based on your patch, I merged the functionality of your patch with the old patch. 
I couldn't amend to the patch as I don't have contributor rights, but I made a separate commit:
https://git.eclipse.org/r/#/c/42930/

@Alexandra/Eugene, 
Would you be able to inspect the patch and let me know your thoughts?

Thank you


1* This workaround moves the shell back into place after a minimize/maximize event.

2* A configure event is a GTK event that is sent when a window is moved/resized:
https://developer.gnome.org/gtk3/stable/GtkWidget.html#GtkWidget-configure-event
Comment 10 Eugen Neufeld CLA 2015-02-27 17:24:41 EST
Hi Leo, 
thank you for refactoring the patch. 
I already thought that the new fix is defining variables that are already there. 
But I still don't understand how the oldX/oldY values are working, as those variables are set but never read. 
I will try out the patch on Monday morning. 

Thanks, 
Eugen
Comment 11 Eugen Neufeld CLA 2015-03-02 04:47:38 EST
Hi Leo, 
your patch works perfectly for me. 

Do you have an idea whether this might be part of the Mars release?

Cheers,
Eugen
Comment 12 Leo Ufimtsev CLA 2015-03-02 11:26:41 EST
(In reply to Eugen Neufeld from comment #10)
> Hi Leo, 
> thank you for refactoring the patch. 
> I already thought that the new fix is defining variables that are already
> there. 
> But I still don't understand how the oldX/oldY values are working, as those
> variables are set but never read. 
> I will try out the patch on Monday morning. 
> 
> Thanks, 
> Eugen

Well, they are used in gtk_configure_event, to determine if an SWT.Move event should be triggered:
     if (!moved || oldX != x [0] || oldY != y [0]) {   //<< here
        moved = true;
	oldX = x [0];
	oldY = y [0];
	sendEvent (SWT.Move);
	// widget could be disposed at this point
     }

This then eventually reaches 'setBounds(..)' and moves the shell around.


The submitted patch still allows the user to move the shell when it is invisible, it just blocks 'gtk' from automatically moving it via a config_event when it's invisible. So it shouldn't break anything.

Not sure if it can make it's way into Mars, I'll check with Alex.K
Comment 14 Alexander Kurtakov CLA 2015-03-10 17:49:59 EDT
Definetely Mars content. Pushed to master now.
Comment 15 Sravan Kumar Lakkimsetti CLA 2015-03-18 04:01:24 EDT
Verified in I20150316-2000