Comment 1 Scott Kovatch 2011-01-04 12:13:27 EST

Fixed > 20100103. Same behavior on Carbon, Cocoa, and Win32 now; getClientArea now returns a rectangle with origin 0, 0.

Comment 2 Peter Severin 2011-01-08 07:09:10 EST

Scott, any chance for this to be fixed in 3.6?

Comment 3 Scott Kovatch 2011-01-08 19:01:11 EST

I can clone this for 3.6.2, but it needs to go in ASAP. I think builds are underway now for the 3.6.2 candidate.

Comment 4 Silenio Quarti 2011-01-10 10:44:41 EST

I believe the previous behaviour was correct. The client area is not guaranteed to stay unchanged as you scroll (this is also true on GTK tree widget for the x coordinate).

With these changes, client area is inconsistent with the coordinate system of the widget. Mouse events,  bounds of items and children controls, display coordinate mapping and drawing done with GC are all wrong.   Note that when running the code below on windows, the bounds of the item and the coordinates of the mouse down event change as you scroll the tree, but the client area remains unchanged.

If you want to keep client area at 0,0 as the user scrolls a tree/table on cocoa, we would have to fix all the other places.  My vote is remove these changes.


import org.eclipse.swt.SWT;
import org.eclipse.swt.graphics.*;
import org.eclipse.swt.layout.*;
import org.eclipse.swt.widgets.*;


public class PR333375 {
public static void main(String[] args) {
	final Display display = new Display();
	Shell shell = new Shell(display);
	shell.setLayout(new FillLayout());

	final Tree tree = new Tree(shell, SWT.NONE);
	for (int i = 0; i < 10; i++) {
		TreeItem item = new TreeItem(tree, SWT.NONE);
		item.setText("Item xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" + i);
	}
	
	tree.addListener(SWT.Paint, new Listener() {
		public void handleEvent(Event event) {
			TreeItem item = tree.getItem(3);
			GC gc = event.gc;
			gc.setForeground(display.getSystemColor(SWT.COLOR_RED));
			Rectangle rect = item.getBounds();
			Rectangle client = tree.getClientArea();
			gc.drawRectangle(rect);
			System.out.println(rect + " " + client);
		}
	});

	tree.addListener(SWT.MouseDown, new Listener() {
		public void handleEvent(Event event) {
			System.out.println("mouse down=" + event.x + " " + event.y);
		}
	});
	
	
	shell.setSize(200, 200);
	shell.open();
	while (!shell.isDisposed()) {
		if (!display.readAndDispatch())
			display.sleep();
	}
	display.dispose();
}
}

Comment 5 Silenio Quarti 2011-01-10 10:49:42 EST

I meant to say:

Note that when running the code below on windows, the bounds of the item change as you scroll the tree, but the client area remains unchanged. The x,y coordinate of the mouse event does not change. On cocoa,  
it is the reverse.

Comment 6 Alexander Nyßen 2011-01-10 11:55:08 EST

(In reply to comment #5)
> I meant to say:
> 
> Note that when running the code below on windows, the bounds of the item change
> as you scroll the tree, but the client area remains unchanged. The x,y
> coordinate of the mouse event does not change. On cocoa,  
> it is the reverse.

Well, that pretty much looks like an inconsistency to me...

Comment 7 Scott Kovatch 2011-01-10 11:56:18 EST

(In reply to comment #4)
> I believe the previous behaviour was correct. The client area is not guaranteed
> to stay unchanged as you scroll (this is also true on GTK tree widget for the x
> coordinate).
> 
> With these changes, client area is inconsistent with the coordinate system of
> the widget. Mouse events,  bounds of items and children controls, display
> coordinate mapping and drawing done with GC are all wrong.   Note that when
> running the code below on windows, the bounds of the item and the coordinates
> of the mouse down event change as you scroll the tree, but the client area
> remains unchanged.

I plead ignorance on this one, then. I forgot that a Tree and Table are also Scrollable, so the scroll view is considered part of the control. GIven that, it makes sense that the client area can change, as it's a window onto the underlying NSTableView.

Comment 8 Silenio Quarti 2011-01-10 12:31:40 EST

(In reply to comment #6)
> (In reply to comment #5)
> > I meant to say:
> > 
> > Note that when running the code below on windows, the bounds of the item change
> > as you scroll the tree, but the client area remains unchanged. The x,y
> > coordinate of the mouse event does not change. On cocoa,  
> > it is the reverse.
> 
> Well, that pretty much looks like an inconsistency to me...

This is not an inconsistency for me. It is a difference between platforms. The whole coordinate system (any API in the toolkit that takes or returns x,y) was consistent before this change and now it is not. 

Is there anything you cannot do because of this platform difference?

Comment 9 Scott Kovatch 2011-01-10 12:37:21 EST

I have undone the previous change to Scrollable.

Comment 10 Alexander Nyßen 2011-01-10 12:38:50 EST

(In reply to comment #8)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > I meant to say:
> > > 
> > > Note that when running the code below on windows, the bounds of the item change
> > > as you scroll the tree, but the client area remains unchanged. The x,y
> > > coordinate of the mouse event does not change. On cocoa,  
> > > it is the reverse.
> > 
> > Well, that pretty much looks like an inconsistency to me...
> 
> This is not an inconsistency for me. It is a difference between platforms. The
> whole coordinate system (any API in the toolkit that takes or returns x,y) was
> consistent before this change and now it is not. 
> 
> Is there anything you cannot do because of this platform difference?

Well, I see the problem that the DropTargetEvent's location does not seem to fit into this. If you replace dragOver in my first snippet with the following implementation, you will notice that when you start to scroll the event location is always outside the tree's bounds and client area on Cocoa, while on windows its inside of both (or am I doing something wrong here?).

public void dragOver(DropTargetEvent event) {
				// System.out.println("drag over " + event);
				event.feedback = DND.FEEDBACK_SCROLL | DND.FEEDBACK_EXPAND;
				System.out.println("BOUNDS: " + tree.getBounds());
				System.out.println("CLIENT AREA: " + tree.getClientArea());
				Point location = tree.toControl(event.x, event.y);
				System.out.println("location: (" + location.x + ", "
						+ location.y + ")");
				if (location.x < 0 || location.y < 0
						|| location.x >= tree.getBounds().width
						|| location.y >= tree.getBounds().height) {
					System.out.println("OUTSIDE BOUNDS");
				} else {
					System.out.println("INSIDE BOUNDS");
				}
				if (location.x < 0 || location.y < 0
						|| location.x >= tree.getClientArea().width
						|| location.y >= tree.getClientArea().height) {
					System.out.println("OUTSIDE CLIENT AREA");
				} else {
					System.out.println("INSIDE CLIENT AREA");
				}
			}

Comment 11 Alexander Nyßen 2011-01-10 12:41:31 EST

(In reply to comment #10)
> (In reply to comment #8)
> > (In reply to comment #6)
> > > (In reply to comment #5)
> > > > I meant to say:
> > > > 
> > > > Note that when running the code below on windows, the bounds of the item change
> > > > as you scroll the tree, but the client area remains unchanged. The x,y
> > > > coordinate of the mouse event does not change. On cocoa,  
> > > > it is the reverse.
> > > 
> > > Well, that pretty much looks like an inconsistency to me...
> > 
> > This is not an inconsistency for me. It is a difference between platforms. The
> > whole coordinate system (any API in the toolkit that takes or returns x,y) was
> > consistent before this change and now it is not. 
> > 
> > Is there anything you cannot do because of this platform difference?
> 
> Well, I see the problem that the DropTargetEvent's location does not seem to
> fit into this. If you replace dragOver in my first snippet with the following
> implementation, you will notice that when you start to scroll the event
> location is always outside the tree's bounds and client area on Cocoa, while on
> windows its inside of both (or am I doing something wrong here?).
> 
> public void dragOver(DropTargetEvent event) {
>                 // System.out.println("drag over " + event);
>                 event.feedback = DND.FEEDBACK_SCROLL | DND.FEEDBACK_EXPAND;
>                 System.out.println("BOUNDS: " + tree.getBounds());
>                 System.out.println("CLIENT AREA: " + tree.getClientArea());
>                 Point location = tree.toControl(event.x, event.y);
>                 System.out.println("location: (" + location.x + ", "
>                         + location.y + ")");
>                 if (location.x < 0 || location.y < 0
>                         || location.x >= tree.getBounds().width
>                         || location.y >= tree.getBounds().height) {
>                     System.out.println("OUTSIDE BOUNDS");
>                 } else {
>                     System.out.println("INSIDE BOUNDS");
>                 }
>                 if (location.x < 0 || location.y < 0
>                         || location.x >= tree.getClientArea().width
>                         || location.y >= tree.getClientArea().height) {
>                     System.out.println("OUTSIDE CLIENT AREA");
>                 } else {
>                     System.out.println("INSIDE CLIENT AREA");
>                 }
>             }

Forget it, I have seen it myself. The following will be ok:

if (location.x < 0
						|| location.y < 0
						|| location.x >= tree.getClientArea().x
								+ tree.getClientArea().width
						|| location.y >= tree.getClientArea().y
								+ tree.getClientArea().height) {
					System.out.println("OUTSIDE CLIENT AREA");
				} else {
					System.out.println("INSIDE CLIENT AREA");
				}

Comment 12 Alexander Nyßen 2011-01-10 12:45:17 EST

While I still think this is not nice, I may live with it. I can work around it in GEF properly by no longer assuming that the client area has to be fixed to (0,0).

Comment 13 Alexander Nyßen 2011-01-10 13:01:54 EST

I have changed the related GEF code to take into account that client area may change during scrolling (bug #332956). I suppose to then close this one as WORKSFORME, as well as the related clone for 3.6.2 (bug #333805).

Comment 14 Scott Kovatch 2011-01-10 13:02:40 EST

> Forget it, I have seen it myself. The following will be ok:
> 
> if (location.x < 0
>                         || location.y < 0
>                         || location.x >= tree.getClientArea().x
>                                 + tree.getClientArea().width
>                         || location.y >= tree.getClientArea().y
>                                 + tree.getClientArea().height) {
>                     System.out.println("OUTSIDE CLIENT AREA");
>                 } else {
>                     System.out.println("INSIDE CLIENT AREA");
>                 }

You also want:

if (location.x < tree.getClientArea().x || location.y < tree.getClientArea().y || ....

for the same reasons.

The way the code is written now, you can only compare the mouse location to the client area, because toControl (and, indirectly, Display#map()) isn't taking the scrollView into account. toControl is giving you a client area-relative location. Is that what it's supposed to do?

Comment 15 Alexander Nyßen 2011-01-10 13:11:05 EST

(In reply to comment #14)
> > Forget it, I have seen it myself. The following will be ok:
> > 
> > if (location.x < 0
> >                         || location.y < 0
> >                         || location.x >= tree.getClientArea().x
> >                                 + tree.getClientArea().width
> >                         || location.y >= tree.getClientArea().y
> >                                 + tree.getClientArea().height) {
> >                     System.out.println("OUTSIDE CLIENT AREA");
> >                 } else {
> >                     System.out.println("INSIDE CLIENT AREA");
> >                 }
> 
> You also want:
> 
> if (location.x < tree.getClientArea().x || location.y < tree.getClientArea().y
> || ....
> 
> for the same reasons.
> 

Thank's for the hint. I have done it exactly this way in my fix for bug #332956. Just forget the snippet here, it was written a bit overhasty :-).

> The way the code is written now, you can only compare the mouse location to the
> client area, because toControl (and, indirectly, Display#map()) isn't taking
> the scrollView into account. toControl is giving you a client area-relative
> location. Is that what it's supposed to do?

Yes, I think that's ok. We had a check whether the location is inside the client area, which was 

Rectangle area = tree.getClientArea();
if (pt.x < 0 || pt.y < 0 || pt.x >= area.width || pt.y >= area.height)
  return null;

before, and which I have now adopted to the following:

Rectangle area = tree.getClientArea();
if (pt.x < area.x || pt.y < area.y || pt.x >= area.x + area.width || pt.y >= area.y + area.height)
  return null;

Comment 16 Silenio Quarti 2011-01-10 13:31:33 EST

(In reply to comment #14)
> You also want:
> 
> if (location.x < tree.getClientArea().x || location.y < tree.getClientArea().y
> || ....
> 
> for the same reasons.
> 
> The way the code is written now, you can only compare the mouse location to the
> client area, because toControl (and, indirectly, Display#map()) isn't taking
> the scrollView into account. toControl is giving you a client area-relative
> location. Is that what it's supposed to do?

Yes, that is correct. Every coordinate pair is relative to the client area.

Comment 17 Felipe Heidrich 2011-08-05 13:49:28 EDT

*** Bug 353907 has been marked as a duplicate of this bug. ***

Comment 18 Felipe Heidrich 2011-08-05 13:50:38 EDT

*** Bug 353905 has been marked as a duplicate of this bug. ***