Fixed the EDT violation

[sergeyCodenameOne]

No description provided.

[shannah]

Can you explain how this fixes the EDT violation? It is still accessing the UI hierarchy off the EDT and it isn't guarding possible NPEs anymore.

In addition, the behaviour of the safetyGetComponentAt() method looks different than the Container.getComponentAt() method which means that this change may result in regressions.

[shai-almog]

This is redundant. instanceof will return false for the case of null.

[shai-almog]

@shannah please review. The EDT violation here triggered a layout off the EDT which caused an issue if a revalidate was just in progress. Specifically this stack trace caused a failure (notice this happened in a test case with no peer components!):

calcPreferredSize:2878, Container (com.codename1.ui)
calcScrollSize:1442, Component (com.codename1.ui)
getScrollDimension:1429, Component (com.codename1.ui)
isScrollableX:2950, Container (com.codename1.ui)
isScrollable:2723, Component (com.codename1.ui)
respondsToPointerEvents:4308, Component (com.codename1.ui)
getComponentAt:2699, Container (com.codename1.ui)
getComponentAt:2701, Container (com.codename1.ui)
onTouchEvent:431, CodenameOneView (com.codename1.impl.android)
dispatchTouchEvent:485, AndroidAsyncView (com.codename1.impl.android)

[shai-almog]

@shannah test case for the bug:

        Form      form        = new Form(new BorderLayout());
        Container container   = new Container(BoxLayout.x());
        Container left        = new Container(new BorderLayout());
        Container leftCenter  = new Container(BoxLayout.y());
        Container right       = new Container(new BorderLayout());
        Container rightCenter = new Container(BoxLayout.y());
        Button    button      = new Button("BUTTON");
        
        button.addActionListener((ev) -> 
        {
            leftCenter.removeAll();
            rightCenter.removeAll();
            
            for (int i = 0; i < 1000; i++)
            {
                leftCenter.add(new Label(String.valueOf(i)));
                rightCenter.add(new Label(String.valueOf(i)));
            }
            
            container.revalidate();
        });
        
        container.setScrollableX(true);
        
        left.getAllStyles().setBgColor(0x01c056);
        left.getAllStyles().setBgTransparency(255);
        
        right.getAllStyles().setBgColor(0xd5305a);
        right.getAllStyles().setBgTransparency(255);
        
        left.add(BorderLayout.NORTH, new Label("------------------------------------------------------------------------------------------------------------------------------------"));
        right.add(BorderLayout.NORTH, new Label("------------------------------------------------------------------------------------------------------------------------------------"));
        
        left.add(BorderLayout.CENTER, leftCenter);
        right.add(BorderLayout.CENTER, rightCenter);
        
        right.add(BorderLayout.SOUTH, button);
        
        container.addAll(left, right);
        
        form.add(BorderLayout.CENTER, container);
        
        form.show();

You press the button fast on an Android device and eventually it breaks.

[shannah]

Tricky situation. I'd be more inclined to attack this problem at the points where the UI is mutated as a result of calling a simple getter.

E.g. In Container.getScrollableDimension() on line 1428, it calculates the scroll size and caches it in the scrollSize variable.

if (!scrollSizeRequestedByUser && (scrollSize == null || shouldCalcScrollSize)) {
            scrollSize = calcScrollSize();
            shouldCalcScrollSize = false;
        }

If we add a check to make sure we're on the EDT before performing this mutation, then it alone would probably fix this particular issue.

e.g.

public Dimension getScrollDimension() {
        if (!CN.isEdt()) {
            if (scrollSize != null) {
                return scrollSize;
            } else {
                 return bounds.getSize();
             }
        }
        if (!scrollSizeRequestedByUser && (scrollSize == null || shouldCalcScrollSize)) {
            scrollSize = calcScrollSize();
            shouldCalcScrollSize = false;
        }
        return scrollSize;
    }

If there end up being other places where a simple accessor triggers a mutation, we can apply similar fixes in those places as well.

[shai-almog]

It calculates scroll size because it's null and this is just one case where this is failing. The null pointer exception and other problematic hacks are also causing issues here. I think this approach is better since it circumvents the problem as a special case.

We have a special case here that only makes sense for the case of peer component. A lot of the logic of getComponentAt(x, y) is designed for Codename One components (e.g. draggable and scrollable itself are irrelevant here). So why pay the price of EDT checks etc. We can rely on the positions of the peer component in this case since they're already calculated and we don't have to worry about other EDT issues coming up later to bite us.

If this fails it will only impact the peer components. When the old solution failed it impacted everything...

[shannah]

The logic probably needs to be tweaked then as it will result in events that should be handled by a peer component to not be.

The behaviour needs to match as exactly as possible the behaviour of getComponentAt()

[shai-almog]

We can do that iteratively. Right now this fixes a major bug that impacts all apps (even without a peer component). I'll merge it once the optimizations are in place so it will also provide a small performance improvement.

[shannah]

I can see right now several cases that will break. I'll adapt the test cases tomorrow.

[shannah]

I think it is worth trying to just ensure that getComponentAt can be thread safe. The issue here is just because it calls isScrollableY() which triggers some mutations, but that could easily be avoided.

GetComponentAt() is full of pain and sharp edges based on lots of heuristics built up over the years. If we use a method here that almost works the same but not exactly there will be regressions when dealing with the behaviour of any lightweight component that is displayed in front of a peer component.

[shai-almog]

I disagree with that.
If you touch getComponentAt(x, y) you break a huge amount of apps whether they use peer components or not. We went down this path multiple times... You impact their performance whether they use peer components or not.

In this case we only impact this one use case of peer component. So this is a very limited change that will only impact that. There are edge cases that might break but they are very niche. The risk is far lower.

[shannah]

Peer components need to work just like everything else though.

If you like we can copy and paste getComponentAt() exactly but just make it thread safe.

I'd probably want to make the same changes to getComponentAt as this isn't the only place where it is used off edt

[shai-almog]

PeerComponent isn't like everything else. We don't care if a component is scrollable. We don't care if it's draggable. We just want to deliver it the event when appropriate. This is an opportunity to throw away a lot of the junk/legacy in getComponentAt() for a cleaner break and better performance.

If there are other cases that use getComponentAt off the EDT we need to implement similar fixes for those cases. I'm guessing similar code exists in all the ports. The JS port probably wouldn't be a problem as it's effectively single threaded. There probably won't be a similar bug. But for the iOS port we'll probably need to replicate this code. We can move it into CodenameOne under the impl package to keep it generic between the ports.

[shannah]

Here is a simple test case that breaks it.

Form hi = new Form("Hi World", new LayeredLayout());
        hi.setScrollableY(false);
        BrowserComponent bc = new BrowserComponent();
        bc.setURL("https://www.codenameone.com");
        hi.add(bc);
        
        Container list = new Container(BoxLayout.y());
        list.getStyle().setBgTransparency(0xff);
        list.getStyle().setBgColor(0xffffff);
        list.setScrollableY(true);
        for (int i=0; i<50; i++) {
            Label l = new Label("Label "+i);
            l.setFocusable(false);
            l.setEnabled(false);
            list.add(l);
        }
        Container grid = GridLayout.encloseIn(1, new Container(), list);
        grid.setScrollableY(false);
        hi.add(grid);
        
        
        hi.show();

Correct behaviour:

You should be able to scroll the list in the bottom half of the screen without the webpage scrolling behind it.

Incorrect Behaviour:

When you try to scroll the list in the bottom half of the screen, the webpage will scroll behind it.