Bug 332145

Comment 1 Beat Schwarzentrub 2010-12-08 11:30:35 EST

There are some open Java bugs that possibly correspond to this problem. (Do a Google search for 
"javax.swing.plaf.basic.BasicTreeUI$Handler.isActualPath(BasicTreeUI.java:3421)").

Maybe the information in the following bug are helpful for a fix in Scout:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6505523

Comment 2 Ivan Motsch 2010-12-09 04:42:00 EST

So far i have not been able to reproduce this behaviour.
I have launched the various tests of the scout james tests with successs.

Can you attach your "hello world" example so we have the same base.

Comment 3 Jeremie Bresson 2011-01-10 08:10:12 EST

Created attachment 186387 [details]
A HelloWorld Project to reproduce the bug.

A HelloWorld Project to reproduce the bug. Simple Scout Application:
* (Standard Log-in/Password: admin/manager)
* A Outline (MyOutline) having on top a TablePage (MyTablePage)
* The TablePage loads 3 rows (hardcoded without any Server Call)
* For each row a NodePage (MyNodePage) is instantiated.
* MyNodePage not create any childPage, but overrides the execCreateChildPage function.

To get the AWT Bug:
* Log in
* Drill down the Table Page.
* Drill down any of the NodePage. It is necessary to click on the triangle and not to select the node in the Tree.

This is one simple way to produce the Bug. I think that it occurs here because the VirtualPage created in the first place does not know that there are no further nodes to be displayed. When the VirtualPage is replaced by a valid instance, there is nothing to display.
It is possible to prevent this to happen by hiding the drill down triangle on the NodePage. The ViratualPage needs to be declared as Leaf in the TablePage. 

  @Override
  protected IPage execCreateVirtualChildPage(ITableRow row) throws ProcessingException {
    return new VirtualPage() {
      @Override
      public boolean isLeaf() {
        return true;
      }
    };
  }

This is not a good fix for the AWT Bug, because there are other configurations that produce the same bug (and they seem to be more random). It also not depends on the SWING look and feel.

Comment 4 Ivan Motsch 2011-05-11 06:25:23 EDT

Thanks a lot for your example. I could perfectly reproduce it.
It occurs in BasicTreeUI due to the fact that after pressing the virtual node, it is resolved.
Swing does basically care about any tree change that occurs between mousePress and mouseRelease.

But NOT so does the DnD support on the sing basic look and feel.
BasicTreeUI.Handler.isActualPath checks for null path but not for potentially changed TreePath and so a null bounds object.
This is a "code style" bug for java swing, that i should post to oracle, however it takes years for such a bugs to even get an answer from oracle.

Therefore we will fix this in e3.7 release of eclipse scout with a workaround. Since the virtual node and its (later) resolved tree node are very related it is acceptable
to call them "equal". This will resolve the swing issue since TreePath with the virtual node and TreePath with the resolved node will then be equal and the bounds can be found in the size cache.

Fixed on SVN trunk and for e3.7

Comment 5 Ivan Motsch 2011-05-11 06:25:42 EDT

Added unit test for this use case.

Comment 6 Matthias Zimmermann 2011-06-28 08:03:39 EDT

shipped with eclipse scout 3.7.0

Comment 7 Lukas Huser 2011-11-09 17:46:00 EST

I would like to re-open this bug (but I think I dont't have the permission to do so(?))

The bugfix for this ticket overrides the hashCode() methods in classes VirtualTreeNode and AbstractTreeNode and returns a constant value.
This is correct (consistent with equals()), but breaks performance of HashMap lookups etc.

This is especially notable when 
a) displaying large amount of data in a table page (> 20'000) rows 
b) drilling down in the tree (and any child node in the tree) of such a large table page

Proposed Solution:
Provide a correct and reasonable (!= constant return value) implementation of hashCode() on VirtualTreeNode and AbstractTreeNode.
There seems to be no straightforward solution to this problem, as there is no shared immutable state between two objects of type VirtualTreeNode and AbstractTreeNode.

The attached patch (org.eclipse.scout.rt.client.patch) does the following:
- create an internal hashCode in VirtualTreeNode on construction, use a simple instance counter 
- when resolving a virtual node to a real node, pass the hashCode to the real node (attaching the real node to the virtual node and passing the hashCode should happen as a single operation to make sure consistency of hashCode and equals() is not broken)
- it is important to pass the hashCode to the real node as soon as possible after construction (certainly before putting it into a HashMap)
-> therefore, attaching the real node to the virtual node has moved from AbstractTree to AbstractPageWithTable

I did some (hand crafted) performance testing:
Test 1: populate a table page with 29'000 rows (through an unconstrained search)
Existing code: ca 34 seconds
Patched: ca 21 seconds
Note that about 10 seconds go away for fetching the data from the database

Test 2: drill down in the tree (double-click on a row in the table)
Existing code: 16 to 18 seconds
Patched: 2 to 5 seconds

Similar values (slightly shorter times) for another test with 17'000 rows.

Comment 8 Lukas Huser 2011-11-09 17:47:17 EST

Created attachment 206743 [details]
Patch hashCode() on VirtualTreeNode and AbstractTreeNode

Comment 9 Lukas Huser 2011-11-09 20:09:00 EST

Well, an artificially created hash code is actually not necessary. We can simply take the default hash code of VirtualTreeNode and pass it to AbstractTreeNode.

I will provide an updated patch.

Comment 10 Lukas Huser 2011-11-09 20:11:45 EST

Created attachment 206747 [details]
Updated patch for hashCode on VirtualTreeNode

Comment 11 Ivan Motsch 2011-11-10 05:22:26 EST

checked the patch: I see you moved the vnode.attachResolvedNode(resolvedNode) from the AbstractTree to the execResolveVirtualNode.

This is basically ok, but existing code that overrides that exec does not yet call the new method vnode.attachResolvedNode(resolvedNode).

Therefore I added a slight double-check in AbstractTree to support for old code:
  @Override
  public ITreeNode resolveVirtualNode(ITreeNode node) throws ProcessingException {
...
        ITreeNode resolvedNode = parentNode.resolveVirtualChildNode(vnode);
//BEGIN DOUBLE CHECK
        if (resolvedNode != vnode && vnode.getResolvedNode() == null) {
          vnode.attachResolvedNode(resolvedNode);
        }
//END DOUBLE CHECK

Patch accepted and applied.
Thank you a lot for your work!

Comment 12 Ivan Motsch 2011-11-10 05:32:45 EST

Created attachment 206775 [details]
Revised patch

Comment 13 Matthias Zimmermann 2011-11-10 09:33:05 EST

reopend ticket for lukas huser

Comment 14 Matthias Zimmermann 2012-02-29 08:00:57 EST

shipped with scout 3.7.2 (indigo sr2)