Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed problem in 3 Layouts where Dimension reused between calls to getPreferredSize and getScrollSize #2229

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ThomasH99
Copy link
Contributor

Border, Box and Flow layouts (contrary to all other layouts) reuse the same Dimension for calls to getPreferredSize and getScrollSize. This creates an issue since the same dim instance gets used both as preferredSize and scrollSize which creates side effects when preferredSize is forced to (0,0) in setHidden(true). (Took me several days to debug why a hidden container with BoxLayout kept becoming visible after being set hidden)

(Sorry for the Commit history, still struggling with using Git correctly)

This reverts commit 7b4b53f.
This reverts commit 7b4b53f.
@codenameone
Copy link
Collaborator

I'm not sure if the command code in the span button is still there, it might be problematic. Moving the field into the method is probably wrong and could have a huge impact on performance. It might make sense to have two fields instead of one though.

@ThomasH99
Copy link
Contributor Author

Since there are no comments explaining the code it's difficult to me to know if there are good reasons for the current implementation. getPreferredSize gets recalculated on each call, so it shouldn't add a performance penalty. Also, since since the more recent layouts do it the same was as my change I reasoned the old layouts just never got updated. In any case, the current implementation causes the really tricky side effect I described, so something needs to be done.

@codenameone
Copy link
Collaborator

No, preferred size is always cached. There are a lot of dependencies on this as this is pretty deep in.

@ThomasH99
Copy link
Contributor Author

I meant that it is recalculated on each call to Layout.getpreferredSize() but, I won't pretend to an expert on this But as I said, most layouts e.g. LayeredLayout, does create a new Dimension in each call to getPreferredSize, so I simply assumed that that was also a valid implementation and it fixes the issue. Anyway, I hope the real issue that led me to make this change was clear? If not, let me know, and I'll be happy to create an example to reproduce the issue.

@codenameone
Copy link
Collaborator

That's a pathway that is called when layout is forced. getPreferredSize in component always caches

@ThomasH99
Copy link
Contributor Author

Here's the code to reproduce the issue. As you can see when you click the button to hide the Container it doesn't work. Hopefully this helps illustrate the issue. The reason is what I tried to describe above.

NB. You need to click the button several times to see issue, first time it behaves correctly, but not after that.

Form hideable = new Form("Hiding Container doesn't work", BoxLayout.y());
hideable.setLayout(new BorderLayout());
Container hideableContainer = new Container(BoxLayout.y());
hideableContainer.setScrollableY(true);
//NB. Clicking "Hide list" works the *first* time, but not after that, so click several times to see the issue
hideable.add(BorderLayout.NORTH, new Button(Command.create("Hide list",null,(e)->{
    hideableContainer.setHidden(!hideableContainer.isHidden());
    hideable.revalidate();
})));
hideable.add(BorderLayout.SOUTH, hideableContainer);
hideableContainer.addAll(new Label("label1"),new Label("label2"),new Label("label3"));
hideable.show();

@JrmyDev
Copy link
Contributor

JrmyDev commented Oct 2, 2017

I think it the same problem I mentionned here : #2228

@ThomasH99
Copy link
Contributor Author

Any update on this (the issue with setHidden not working is currently preventing me from building my app for a device)? Could you reproduce the issue based my example above?

@codenameone
Copy link
Collaborator

I suggest moving that to an issue. Adding a scrollable container to the north is a mistake as the border layout always gives the full requested height to the north/south containers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants