Community
Participate
Working Groups
Created attachment 194933 [details] Patch change isShowing() to isVisible() Looking at Control.setTabItemFocus() returns false for all controls having a size of 0,x or x,0 at this time. I do not see any reason to mix up showing information with a tab traversal. To ensure the layout of complex forms we collect all invalidation on the layout and async recalculate the layout once (Performance boost about factor 10). Since the traverse can change the visibility of the next control (master-slave) this field can getVisible immediatly and having a size of (0,0) - will be updated with the next layout batch job. -> Focus is anywhere but not in the next control. To fix this bug I suggest to change Control.setTabItemFocus the isShowing request with a isVisible. Have a look at the following example. After shell open the first TAB-traversal does not set the focus to the text2. Every next TAB works well. --------------------- Example ------------ import org.eclipse.swt.SWT; import org.eclipse.swt.events.TraverseEvent; import org.eclipse.swt.events.TraverseListener; import org.eclipse.swt.layout.GridData; import org.eclipse.swt.layout.GridLayout; import org.eclipse.swt.widgets.Composite; import org.eclipse.swt.widgets.Display; import org.eclipse.swt.widgets.Shell; import org.eclipse.swt.widgets.Text; public class TraverseBug { public static void main(String[] args) { new TraverseBug().displayLoop(); } private void displayLoop(){ Display display = new Display(); Shell shell = new Shell(display); createContents(shell); shell.pack(); shell.open(); while (!shell.isDisposed()) { if (!display.readAndDispatch()) display.sleep(); } display.dispose(); } Text text1; Text text2; private void createContents(Composite parent) { text1 = new Text(parent, SWT.BORDER); text1.addTraverseListener(new TraverseListener() { @Override public void keyTraversed(TraverseEvent e) { toggleText2Visible(); } }); text2 = new Text(parent, SWT.BORDER); text2.setVisible(false); //layout parent.setLayout(new GridLayout(2, true)); text1.setLayoutData(new GridData(GridData.FILL_BOTH)); GridData gd = new GridData(GridData.FILL_BOTH); gd.exclude = true; text2.setLayoutData(gd); } private void toggleText2Visible() { boolean visible = !text2.getVisible(); GridData gd = (GridData) text2.getLayoutData(); gd.exclude = !visible; text2.setVisible(visible); text2.getDisplay().asyncExec(new Runnable() { @Override public void run() { text2.getParent().layout(true); } }); } } --------------- END EXAMPLE -----------------------
(In reply to comment #0) > Created attachment 194933 [details] > Patch change isShowing() to isVisible() > Looking at Control.setTabItemFocus() returns false for all controls having a > size of 0,x or x,0 at this time. > I do not see any reason to mix up showing information with a tab traversal. why would a control that is not visible take part in the traverse code ? > Since the traverse can change the visibility of the next control (master-slave) You are changing the children list during traverse, that is special case and you will have to deal with it in your code. > Have a look at the following example. After shell open the first TAB-traversal > does not set the focus to the text2. Every next TAB works well. Does that work for you: text1.addTraverseListener(new TraverseListener() { public void keyTraversed(TraverseEvent e) { //toggleText2Visible(); boolean visible = !text2.getVisible(); GridData gd = (GridData) text2.getLayoutData(); gd.exclude = !visible; text2.setVisible(visible); text2.getParent().layout(new Control[] {text2}, SWT.DEFER);//note, in this case you don't really need to defer the layout... if (visible) { text2.setFocus(); e.doit = false; } } }); > --------------------- Example ------------ > import org.eclipse.swt.SWT; > import org.eclipse.swt.events.TraverseEvent; > import org.eclipse.swt.events.TraverseListener; > import org.eclipse.swt.layout.GridData; > import org.eclipse.swt.layout.GridLayout; > import org.eclipse.swt.widgets.Composite; > import org.eclipse.swt.widgets.Display; > import org.eclipse.swt.widgets.Shell; > import org.eclipse.swt.widgets.Text; > public class TraverseBug { > public static void main(String[] args) { > new TraverseBug().displayLoop(); > } > private void displayLoop(){ > Display display = new Display(); > Shell shell = new Shell(display); > createContents(shell); > shell.pack(); > shell.open(); > while (!shell.isDisposed()) { > if (!display.readAndDispatch()) > display.sleep(); > } > display.dispose(); > } > Text text1; > Text text2; > private void createContents(Composite parent) { > text1 = new Text(parent, SWT.BORDER); > text1.addTraverseListener(new TraverseListener() { > @Override > public void keyTraversed(TraverseEvent e) { > toggleText2Visible(); > } > }); > text2 = new Text(parent, SWT.BORDER); > text2.setVisible(false); > //layout > parent.setLayout(new GridLayout(2, true)); > text1.setLayoutData(new GridData(GridData.FILL_BOTH)); > GridData gd = new GridData(GridData.FILL_BOTH); > gd.exclude = true; > text2.setLayoutData(gd); > } > private void toggleText2Visible() { > boolean visible = !text2.getVisible(); > GridData gd = (GridData) text2.getLayoutData(); > gd.exclude = !visible; > text2.setVisible(visible); > text2.getDisplay().asyncExec(new Runnable() { > @Override > public void run() { > text2.getParent().layout(true); > } > }); > } > } > --------------- END EXAMPLE -----------------------
(In reply to comment #1) I agree invisible controls should not take any focus. Now we have somehow to define what exactly an invisible control describes. From my point of view the size of a control has really nothing to do with its visibility especially in the SWT concepts where usually invisible controls reserve its space on in the container. Otherwise the getVisible() method should also consider the size of a control what nobody would expect. If you are having remote validations on a field (client-server), validation during traversal is the usual way also the validation can change visibility of controls. Furthermore in any RAP project people should take care about how many requests are shipped to the server part and think twice validating on anykey or modify. Separating model and UI makes a direct 'text2.setFocus()' impossible since text1 has no idea about the existence of a text2. If you do not want to change the existing behavior the bug could also be solved in a flag (stylebit) to set 'do not consider size in tab traversals'. This bug is relevant for the Eclipse Scout project. Thank you Andreas > (In reply to comment #0) > > Created attachment 194933 [details] [details] > > Patch change isShowing() to isVisible() > > Looking at Control.setTabItemFocus() returns false for all controls having a > > size of 0,x or x,0 at this time. > > I do not see any reason to mix up showing information with a tab traversal. > > why would a control that is not visible take part in the traverse code ? > > > > Since the traverse can change the visibility of the next control (master-slave) > > You are changing the children list during traverse, that is special case and > you will have to deal with it in your code. > > > Have a look at the following example. After shell open the first TAB-traversal > > does not set the focus to the text2. Every next TAB works well. > > Does that work for you: > text1.addTraverseListener(new TraverseListener() { > public void keyTraversed(TraverseEvent e) { > //toggleText2Visible(); > boolean visible = !text2.getVisible(); > GridData gd = (GridData) text2.getLayoutData(); > gd.exclude = !visible; > text2.setVisible(visible); > text2.getParent().layout(new Control[] {text2}, > SWT.DEFER);//note, in this case you don't really need to defer the layout... > if (visible) { > text2.setFocus(); > e.doit = false; > } > } > }); > > > > --------------------- Example ------------ > > import org.eclipse.swt.SWT; > > import org.eclipse.swt.events.TraverseEvent; > > import org.eclipse.swt.events.TraverseListener; > > import org.eclipse.swt.layout.GridData; > > import org.eclipse.swt.layout.GridLayout; > > import org.eclipse.swt.widgets.Composite; > > import org.eclipse.swt.widgets.Display; > > import org.eclipse.swt.widgets.Shell; > > import org.eclipse.swt.widgets.Text; > > public class TraverseBug { > > public static void main(String[] args) { > > new TraverseBug().displayLoop(); > > } > > private void displayLoop(){ > > Display display = new Display(); > > Shell shell = new Shell(display); > > createContents(shell); > > shell.pack(); > > shell.open(); > > while (!shell.isDisposed()) { > > if (!display.readAndDispatch()) > > display.sleep(); > > } > > display.dispose(); > > } > > Text text1; > > Text text2; > > private void createContents(Composite parent) { > > text1 = new Text(parent, SWT.BORDER); > > text1.addTraverseListener(new TraverseListener() { > > @Override > > public void keyTraversed(TraverseEvent e) { > > toggleText2Visible(); > > } > > }); > > text2 = new Text(parent, SWT.BORDER); > > text2.setVisible(false); > > //layout > > parent.setLayout(new GridLayout(2, true)); > > text1.setLayoutData(new GridData(GridData.FILL_BOTH)); > > GridData gd = new GridData(GridData.FILL_BOTH); > > gd.exclude = true; > > text2.setLayoutData(gd); > > } > > private void toggleText2Visible() { > > boolean visible = !text2.getVisible(); > > GridData gd = (GridData) text2.getLayoutData(); > > gd.exclude = !visible; > > text2.setVisible(visible); > > text2.getDisplay().asyncExec(new Runnable() { > > @Override > > public void run() { > > text2.getParent().layout(true); > > } > > }); > > } > > } > > --------------- END EXAMPLE -----------------------
Sorry Andreas, I can not change a behaviour that has been working over 10 years now. Besides, your code adds a bug, apply your patch and try this code: public class TraverseBug2 { public static void main(String[] args) { Display display = new Display(); Shell shell = new Shell(display); shell.setLayout(new GridLayout(3, true)); Text text1 = new Text(shell, SWT.BORDER); text1.setText("text1"); Text text2 = new Text(shell, SWT.BORDER); text2.setText("text2"); GridData gd = new GridData(GridData.FILL_BOTH); gd.exclude = true; text2.setLayoutData(gd); Text text3 = new Text(shell, SWT.BORDER); text3.setText("text3"); shell.pack(); shell.open(); while (!shell.isDisposed()) { if (!display.readAndDispatch()) display.sleep(); } display.dispose(); } } hit the Tab key, the focus moves from the text1 to text2 (which is not visible). To the user of the UI that is a bug. At this point I can't add any flags, we are passed API freeze. In your snippet you call text2.setVisible(), why can't you call setFocus() ? Sorry, this type of problem you need to fix in the application side. As you mention, your code has a concept of master-slave, change your framework to allow you to do what you need. Closing as not Eclipse.
(In reply to comment #3) > Sorry Andreas, I can not change a behaviour that has been working over 10 years A 10 year existence of a bug does not legitimize it to exist an other 10 years;) > now. Besides, your code adds a bug, apply your patch and try this code: > public class TraverseBug2 { > public static void main(String[] args) { > Display display = new Display(); > Shell shell = new Shell(display); > shell.setLayout(new GridLayout(3, true)); > > Text text1 = new Text(shell, SWT.BORDER); > text1.setText("text1"); > Text text2 = new Text(shell, SWT.BORDER); > text2.setText("text2"); > GridData gd = new GridData(GridData.FILL_BOTH); > gd.exclude = true; > text2.setLayoutData(gd); > Text text3 = new Text(shell, SWT.BORDER); > text3.setText("text3"); > shell.pack(); > shell.open(); > while (!shell.isDisposed()) { > if (!display.readAndDispatch()) > display.sleep(); > } > display.dispose(); > } > } > > hit the Tab key, the focus moves from the text1 to text2 (which is not > visible). To the user of the UI that is a bug. From my point of view this snippet is a bug. The concept of SWT is to set a control invisible to disappear from the screen and remove from the traversal list. The exclude flag in the layout is to free the allocated space in the layout for other controls. To get the expected behavior add the line 'text2.setVisible(false);'. The only reason makes the expected result is the layout has not be called before setting the exclude flag. Something like: Text t1 = new Text(parent, SWT.NONE); final Text t2 = new Text(parent, SWT.NONE); Text t3 = new Text(parent, SWT.NONE); //layout parent.setLayout(new GridLayout(3, true)); t1.setLayoutData(new GridData(GridData.FILL_BOTH)); GridData gd = new GridData(GridData.FILL_BOTH); t2.setLayoutData(gd); t3.setLayoutData(new GridData(GridData.FILL_BOTH)); t2.getDisplay().asyncExec(new Runnable() { @Override public void run() { ((GridData) t2.getLayoutData()).exclude = true; parent.layout(true); } }); } makes the focus traversing the invisible t2 because the size where set once to the t2. > > At this point I can't add any flags, we are passed API freeze. But we could try to find a solution for the next release. > > In your snippet you call text2.setVisible(), why can't you call setFocus() ? One of the main patterns in MVC and M2V architectures is the decoupling of UI and model/controller. In such architectures the value change of a UI (e.g. text) sets the new value to its model. A controller listens to this change executes some business logic and sets same properties of (other) controls (e.g. the visibility of the model part from text2). The model of text2 fires a property change where it's UI component listens to and sets the visibility of the UI control. So there is really no change without braking all patterns to set the focus straight from one UI control to another. The only and less performance killing workaround I see is to ensure with every visibility property event the size of a control is not (0,x) or (x,0). Even that increases the execution time of some property changes by factor 8. > Sorry, this type of problem you need to fix in the application side. As you > mention, your code has a concept of master-slave, change your framework to > allow you to do what you need. > Closing as not Eclipse. I reopend this bug since I have not seen any good reason to call the isShowing() method from the setTabItemFocus. Furthermore I'm really interested in having a sound and simple SWT code base. If needed I could help to solve this issue, create some test cases or doing some manual tests. Thanks Andy
(In reply to comment #3) The one and only way to fix the TraverseBug2 is to consider the GridData.exclude flag in the Control.setTabItemFocus() method. Every other assumption as considering the size is a lucky punch could work for a certain use case. We both agree that considering a concrete layout data (GridData) in the traverse mechanism breaks SWT concepts and is not the way to go. -Andreas
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. If the bug is still relevant, please remove the "stalebug" whiteboard tag.
