codenameone / CodenameOne

Cross-platform framework for building truly native mobile apps with Java or Kotlin. Write Once Run Anywhere support for iOS, Android, Desktop & Web.
https://www.codenameone.com/
Other
1.72k stars 408 forks source link

Container.snapToGrid not working as expected #1966

Open saeder opened 7 years ago

saeder commented 7 years ago

Container overrides com.codename1.ui.Container.getGridPosX() but apparently doesn't do it well.

Try my demonstration code - the gridPosX doesn't return good values for the third component on the two lines:

public class FormSnapToGridTensileLengthBug extends Form {
    private class ContainerScrollableX extends Container {
        ContainerScrollableX(int aTensileLength) {
            super((new BoxLayout(BoxLayout.X_AXIS)));
            setTensileLength(aTensileLength);
        }
        @Override
        protected int getGridPosX() {
            return super.getGridPosX();
        }
    }

    public FormSnapToGridTensileLengthBug() {
        setTitle("FormSnapToGridTensileLengthBug");
        setScrollable(false);
        setScrollableY(true);
        getContentPane().setTensileLength(0);
        setLayout(new BoxLayout(BoxLayout.Y_AXIS));
        for (int rowIndex = 0; rowIndex < 2; rowIndex++) {
            int tensileLength = rowIndex == 0 ? 0 : -1;
            ContainerScrollableX containerRow = new ContainerScrollableX(tensileLength);
            containerRow.setSnapToGrid(true);
            containerRow.setScrollableX(true);
            containerRow.setTensileDragEnabled(true); // setTensileDragEnabled(true); bad behaviour
            {
                Container containerColumn = new Container(new FlowLayout());
                containerColumn.add(new Button("DummyLeft"));
                containerRow.add(containerColumn);
            }
            {
                Container containerColumn = new Container(new FlowLayout()) {
                    @Override
                    protected Dimension calcPreferredSize() {
                        Dimension dimension = new Dimension(super.calcPreferredSize());
                        dimension.setWidth(containerRow.getWidth());
                        return dimension;
                    }
                };
                containerColumn.add(new Label("row tensileLength " + tensileLength));
                containerRow.add(containerColumn); 
            }
            {
                Container containerColumn = new Container(new FlowLayout());
                containerColumn.add(new Button("DummyRight"));
                containerRow.add(containerColumn);
            }
            add(containerRow);
        }
    }
}
codenameone commented 7 years ago

Again I'm not sure this is a bug. If I understand correctly you scroll on X with tensile set to 0 and can't reach the last element because its grid is out of reach. Snap to grid is off by default and if you turn it on with tensile it works fine. The problem exists when there is no tensile and we have the snap to grid on. In that case you reach the last element but since there is no grid "footing" unless we go outside of the container (which we can't because there is no tensile) then everything fails... You can manually define your grid or just use tensile for this specific case which I think is unavoidable.

saeder commented 7 years ago

Errors are errors. You can always decide an error would not be important enough to fix it immediately.

I tried to create a slider component using the default scrolling. For this use case tensile scrolling would look very wrong. But even if this bug was fixed I couldn't use it because there is another one (#2122) which causes endless animation loops. And there is another one where snap to grid is basically ignored when a tap disturbes an ongoing animation (#1736). The whole area is a mess. I still do not understand why these topics are not important to you. No perceived quality can arise that way.

saeder commented 7 years ago

Oh - I meant #1947 instead of #1736

codenameone commented 7 years ago

I'm unconvinced this is an error. I think you have behavioral expectations that are somewhat outside of the scope of our original design and adapting our code to match these expectations carries the serious risk of introducing big regressions to crucial functionality (scrolling) for a benefit I still don't quite follow.

Internally we don't use scroll the way you expect e.g. SwipeableContainer isn't a scrollable container on the X axis and neither are the swipable tabs. Creating a slider using scrolling is probably not a good use case as evident by the issues you filed. Scrolling is intended to scroll components using platform native effect.

If you have a very specific edge case you can always override the pointer behavior and control the setScrollX value manually so it's totally possible to get the functionality you want.

Furthermore, it should be possible for you to implement your own scrolling logic entirely without us by overriding the methods and implementing them the way you see fit. If some functionality can't be overriden you might have a better chance with submitting an RFE to expose a method as protected so you can override that functionality.

About #1947 I assume you mean when the pointer is released after the tap? If so we need to debug that. The issue wasn't assigned to a milestone if this is the case I'll try to set a milestone.

I don't understand #2122 at all. isFinished() returns true when the destination value is reached. Just fix this:

        while (!motion.isFinished()) {
            int value = motion.getValue();
            integers.add(value);
            try {
                Thread.sleep(1000 / 60);
            } catch (InterruptedException e) {;} // OK
        }
        Log.p(Arrays.toString(integers.toArray()));

With:

        while (!motion.isFinished()) {
            int value = motion.getValue();
            integers.add(value);
            try {
                Thread.sleep(1000 / 60);
            } catch (InterruptedException e) {;} // OK
        }
                int value = motion.getValue();
        integers.add(value);
        Log.p(Arrays.toString(integers.toArray()));
saeder commented 7 years ago

Do not be distracted by my use case. You always ask about use cases, that's why I describe them. But the error also occurs in the base case of a scrollable container as shown in the second row of the example.

With #2122 it is of no use when You correct my demonstration code. I attached another code sample on 1st of June - see FormTensileLoop - which demonstrates the issue I have. The other code sample just shows what is causing the issue in a simplified way because I expected it to be difficult to point out the bug on the real thing - but it is there.

1947 is well described and has demonstration code. A snap to grid animation can be interrupted by a tap and the container does not snap then. How can You possibly not understand that?