Project

General

Profile

Bug #2893

Mouse clicks on rows are not 100% accurate

Added by Stanislav Lomany over 8 years ago. Updated over 7 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Stanislav Lomany
Target version:
FWD - Cleanup and Stabilization for GUI
Start date:
Due date:
% Done:

100%

billable:
No
vendor_id:
GCD
case_num:

proper_position.png (8.49 KB) Stanislav Lomany, 07/14/2016 03:02 PM

Related issues

Related to User Interface - Feature #2564: implement GUI BROWSE widget Closed

History

#1 Updated by Stanislav Lomany almost 8 years ago

  • Status changed from New to WIP

That is a general issue with AbstractWidget.translate(Widget<O>, MouseEvent). It also can be seen e.g. in radio sets. The returned value gets ~ +6/+6 relatively to the actual pointer position.

#2 Updated by Greg Shah almost 8 years ago

Could this be the same problem as described in #3142?

#3 Updated by Stanislav Lomany almost 8 years ago

Most likely it is.

#4 Updated by Stanislav Lomany almost 8 years ago

This screen illustrates what is the proper hot spot for Java2D and standard Ubuntu cursor. It is +1/+2 relatively to the very tip of the pointer.

#5 Updated by Stanislav Lomany almost 8 years ago

Created task branch 2893a from trunk revision 11069.

#6 Updated by Stanislav Lomany almost 8 years ago

The browse issue I've noted in the daily report is not an issue.

So please review task branch 2893a revision 11070.

#7 Updated by Greg Shah almost 8 years ago

  • Start date deleted (12/01/2015)
  • Target version set to Milestone 16

Code Review Task Branch 2893a Revision 11070

I'm OK with the change.

Hynek: please review.

#8 Updated by Hynek Cihlar almost 8 years ago

Code Review Task Branch 2893a Revision 11070.

The translation of parent's insets should be made only for BorderedPanelGuiImpl, see AbstractWidget.screenPhysicalLocation(). Otherwise the change is OK.

#9 Updated by Greg Shah almost 8 years ago

The translation of parent's insets should be made only for BorderedPanelGuiImpl, see AbstractWidget.screenPhysicalLocation().

Please hide this knowledge in a method instead of doing an instanceof BorderedPanelGuiImpl on the parent.

#10 Updated by Hynek Cihlar almost 8 years ago

Greg Shah wrote:

The translation of parent's insets should be made only for BorderedPanelGuiImpl, see AbstractWidget.screenPhysicalLocation().

Please hide this knowledge in a method instead of doing an instanceof BorderedPanelGuiImpl on the parent.

Also, it would make sense to reuse screenPhysicalLocation() instead of duplicating the same logic in translate().

So instead of this:

      // get widget physical location wich is relative to it container. 
      NativePoint p = widget.physicalLocation();

      // get inner containers and calculate position
      Widget<O> parent = widget.parent();

      while (!(parent instanceof TopLevelWindow<?>))
      {
         p.translate(parent.physicalLocation());
         parent = parent.parent();
      }

      // mouse position relative to widget position
      return new NativePoint(evt.getX() - p.x, evt.getY() - p.y);

you would get this:
      NativePoint p = topLevelWindow().physicalLocation().translate(screenPhysicalLocation());
      // mouse position relative to widget position
      return new NativePoint(evt.getX() - p.x, evt.getY() - p.y);

The advantage is, aside from the code deduplication itself, that once the TODO is resolved in screenPhysicalLocation() it will not brake translate().

#11 Updated by Stanislav Lomany almost 8 years ago

translate function has two parameters:

translate(Widget<O> widget, MouseEvent evt)

In the new code the first parameter is not used. Considering that in practice this function always takes this as the first parameter, should I remove it?

#12 Updated by Stanislav Lomany almost 8 years ago

Also, should I skip regression testing because it is GUI-changes only?

#13 Updated by Greg Shah almost 8 years ago

In the new code the first parameter is not used. Considering that in practice this function always takes this as the first parameter, should I remove it?

Yes.

Also, should I skip regression testing because it is GUI-changes only?

MAJIC testing can be skipped. But the manual GUI testing will be very important.

#14 Updated by Stanislav Lomany almost 8 years ago

Task branch 2893a rebased from trunk rev 11071.
I did some GUI testing. Please review.

#15 Updated by Greg Shah almost 8 years ago

Code Review Task Branch 2893a Revision 11073

I'm fine with the changes.

Have you done enough manual GUI testing to confirm that this is safe to merge to trunk?

#16 Updated by Stanislav Lomany almost 8 years ago

I've checked testcases for widgets and menus and ran some customer GUI app scenarios.

#17 Updated by Greg Shah almost 8 years ago

OK, merge it to trunk.

#18 Updated by Stanislav Lomany almost 8 years ago

2893a was merged into the trunk as bzr revision 11072.

#19 Updated by Stanislav Lomany almost 8 years ago

  • Status changed from WIP to Review

#20 Updated by Greg Shah almost 8 years ago

  • % Done changed from 0 to 100
  • Status changed from Review to Closed

#21 Updated by Greg Shah over 7 years ago

  • Target version changed from Milestone 16 to Cleanup and Stabilization for GUI

Also available in: Atom PDF