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

Fix toolbar overflow (JDK-8152395) #318

Merged
merged 12 commits into from Jan 4, 2019

Conversation

@Ciruman
Copy link
Contributor

commented Dec 7, 2018

test (JDK8152395) toolbar overflow control visibility

- if children size changed and they are bigger than toolbar shall be shown
- if children size changed and they are smaller than toolbar shall not be shown

fix (JDK8152395) toolbar overflow control visibility

- if children size is bigger than toolbar shall be shown
- if children size is smaller than toolbar shall not be shown
Ciruman added 3 commits Dec 7, 2018
test (JDK8152395) toolbar overflow control visibility
- if children size changed and they are bigger than toolbar shall be shown
- if children size changed and they are smaller than toolbar shall not be shown
fix (JDK8152395) toolbar overflow control visibility
- if children size is bigger than toolbar shall be shown
- if children size is smaller than toolbar shall not be shown
@kevinrushforth

This comment has been minimized.

Copy link
Collaborator

commented Dec 10, 2018

@kevinrushforth kevinrushforth added the bug label Dec 10, 2018

@kevinrushforth kevinrushforth changed the title Fix jdk8152395 Fix toolbar overflow (JDK-8152395) Dec 10, 2018

@kevinrushforth kevinrushforth requested review from aghaisas and arapte Dec 10, 2018

@@ -0,0 +1,159 @@
package test.robot.javafx.scene;

This comment has been minimized.

Copy link
@kevinrushforth

kevinrushforth Dec 10, 2018

Collaborator

This will need a standard Copyright header

private static final String MODIFIED_STYLE = "-fx-border-width: 1; -fx-border-color: red";

static CountDownLatch startupLatch = new CountDownLatch(1);
static Robot robot;

This comment has been minimized.

Copy link
@aghaisas

aghaisas Dec 12, 2018

robot is not at all used.
The question is why should this test be under test.robot.javafx.scene package?

This comment has been minimized.

Copy link
@Ciruman

Ciruman Dec 12, 2018

Author Contributor

Good point. At first I though a robot test will be necessary. Later I saw it was not needed. But now, I do not know where would make sense to have them. Can you tell me the best place from them? Thanks.

This comment has been minimized.

Copy link
@kevinrushforth

kevinrushforth Dec 12, 2018

Collaborator

Assuming that it even needs to be a system test, then test/javafx/scene/control seems an OK place.

My question is whether this needs to be a system test at all? If you can make it a javafx.controls unit test then that would be best (almost all controls tests can be done that way).

This comment has been minimized.

Copy link
@aghaisas

aghaisas Dec 13, 2018

If you decide to write a unit test - it can be added as a test case in existing test file rt/modules/javafx.controls/src/test/java/test/javafx/scene/control/ToolbarTest.java

else, system test under location suggested by Kevin looks fine.

This comment has been minimized.

Copy link
@Ciruman

Ciruman Dec 14, 2018

Author Contributor

I think the system test is nice to have but it could make sense to have a unit test.

  • I will try to implement one.
  • I will move the test to the test/javafx/scene/control package.

This comment has been minimized.

Copy link
@Ciruman

Ciruman Dec 16, 2018

Author Contributor

@kevinrushforth I am not sure about what you meant with making it a javafx.controls unit test. Did you mean something like test.javafx.scene.control.SplitPaneTest(Because for me SplitPaneTest have system tests)? Or did you mean a junit test at skin level(ToolBarSkinTest)? Can you tell me a similar test to the one we need(with an updated Height/Width property)? If I try to do it in ToolBarSkinTest, I do not know how to trigger correctly the layout of the skin just using the control methods.

This comment has been minimized.

Copy link
@Ciruman

Ciruman Dec 17, 2018

Author Contributor

Tests moved to proposed test class:
rt/modules/javafx.controls/src/test/java/test/javafx/scene/control/ToolbarTest.java

This comment has been minimized.

Copy link
@kevinrushforth

kevinrushforth Dec 19, 2018

Collaborator

To answer your question, by "javafx.controls unit test", I meant a test under modules/javafx.controls/src/test/java/... (as opposed to tests/system/...), which is what your latest change does. Did you verify that the test fails without your fix and passes with your fix?


private void organizeOverflow(double length, boolean hasOverflow) {
double x;

This comment has been minimized.

Copy link
@aghaisas

aghaisas Dec 13, 2018

This declaration can be moved to it's first use.

This comment has been minimized.

Copy link
@Ciruman

Ciruman Dec 17, 2018

Author Contributor

I reorganized the methods close to its first use.

This comment has been minimized.

Copy link
@aghaisas

aghaisas Dec 21, 2018

Thanks.
My comment was for variable 'x' declaration. It is declared on line 578, but initialized on line 589.

This comment has been minimized.

Copy link
@Ciruman

Ciruman Dec 21, 2018

Author Contributor

Done.

@Ciruman

This comment has been minimized.

Copy link
Contributor Author

commented Dec 19, 2018

Is it correct now? Should I do something else?

@kevinrushforth

This comment has been minimized.

Copy link
Collaborator

commented Dec 19, 2018

You still need to send out a review request email to trigger the formal review.

Also, can you answer the following question:

Did you verify that the test fails without your fix and passes with your fix?

@Ciruman

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2018

You still need to send out a review request email to trigger the formal review.

Also, can you answer the following question:

Did you verify that the test fails without your fix and passes with your fix?

It does.

With the fix there is no error but without the fix it fails with the following error:

test.javafx.scene.control.ToolbarTest > overflowShownInHorizontalAfterChildrenReziseTest FAILED
    java.lang.AssertionError: 
        at org.junit.Assert.fail(Assert.java:91)
        at org.junit.Assert.assertTrue(Assert.java:43)
        at org.junit.Assert.assertTrue(Assert.java:54)
        at test.javafx.scene.control.ToolbarTest.assertOverflowShown(ToolbarTest.java:309)
        at test.javafx.scene.control.ToolbarTest.testOverflowVisibility(ToolbarTest.java:275)
        at test.javafx.scene.control.ToolbarTest.overflowShownInHorizontalAfterChildrenReziseTest(ToolbarTest.java:249)

test.javafx.scene.control.ToolbarTest > overflowShownInVerticalAfterChildrenReziseTest FAILED
    java.lang.AssertionError: 
        at org.junit.Assert.fail(Assert.java:91)
        at org.junit.Assert.assertTrue(Assert.java:43)
        at org.junit.Assert.assertTrue(Assert.java:54)
        at test.javafx.scene.control.ToolbarTest.assertOverflowShown(ToolbarTest.java:309)
        at test.javafx.scene.control.ToolbarTest.testOverflowVisibility(ToolbarTest.java:275)
        at test.javafx.scene.control.ToolbarTest.overflowShownInVerticalAfterChildrenReziseTest(ToolbarTest.java:257)

16 tests completed, 2 failed

Is it enough like this or do we need a better error message? I think the test method name is clear: overflowShownIn{Vertical|Horizontal}AfterChildrenReziseTest. But If the feedback could be better, please, let me know.

I will send the email tomorrow, right now, I do not have access to the Zeiss emails.

@Before public void setup() {
tk = (StubToolkit)Toolkit.getToolkit();//This step is not needed (Just to make sure StubToolkit is loaded into VM)
toolBar = new ToolBar();
node1 = new Rectangle();
node2 = new Rectangle(2.0,4.0);
toolBarWithItems = new ToolBar(node1,node2);

root = new StackPane();
scene = new Scene(root);

This comment has been minimized.

Copy link
@aghaisas

aghaisas Dec 20, 2018

scene & stage creation + stage.setScene() can be moved to private method initializeToolBar(), Also we need not have them as class members.

This comment has been minimized.

Copy link
@aghaisas

aghaisas Dec 21, 2018

Thanks for making the change.
You have not addressed the second half my earlier comment - scene and stage need not be class members.

This comment has been minimized.

Copy link
@aghaisas

aghaisas Dec 26, 2018

Have you missed fixing this?
I am OK with the changes and will approve once you fix this comment.

@kevinrushforth

This comment has been minimized.

Copy link
Collaborator

commented Dec 20, 2018

I'll review it when I get a chance. To answer your question:

16 tests completed, 2 failed

Is it enough like this or do we need a better error message?

This is sufficient.

@Ciruman

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2018

I was considering squashing all changes in a single commit because in the documentation recommends this. But if I do this now, I do not know what will happens with the comments.

@kevinrushforth

This comment has been minimized.

Copy link
Collaborator

commented Dec 20, 2018

I was considering squashing all changes in a single commit because in the documentation recommends this. But if I do this now, I do not know what will happens with the comments.

This is only recommended for the initial creation of the PR. Once you have a PR, the recommendation is to not squash the commits (so that reviewers who have already started reviewing can look at the delta changes).

}

private void setFixSize(Node node, double size) {
if(node instanceof Region) {

This comment has been minimized.

Copy link
@aghaisas

aghaisas Dec 21, 2018

There should be a space between if and (

((Region) node).setMinSize(Pane.USE_PREF_SIZE, Pane.USE_PREF_SIZE);
((Region) node).setMaxSize(Pane.USE_PREF_SIZE, Pane.USE_PREF_SIZE);
((Region) node).setPrefSize(size, size);
} else if(node instanceof Rectangle) {

This comment has been minimized.

Copy link
@aghaisas

aghaisas Dec 21, 2018

There should be a space between if and (


private void assertOverflowNotShown() {
Pane pane = (Pane) toolBar.queryAccessibleAttribute(AccessibleAttribute.OVERFLOW_BUTTON);
if(pane!=null) {

This comment has been minimized.

Copy link
@aghaisas

aghaisas Dec 21, 2018

There should be a space between if and (.
Also around comparison operator

Correct version would be : if (pane != null)

This comment has been minimized.

Copy link
@aghaisas

aghaisas Dec 21, 2018

I think, you can use assertNotNull(pane); instead of if condition. This is already done in assertOverflowShown() method.

This comment has been minimized.

Copy link
@Ciruman

Ciruman Dec 21, 2018

Author Contributor

I think, you can use assertNotNull(pane); instead of if condition. This is already done in assertOverflowShown() method.

I do not think so. If the overflow button is not present, it is also correct. In assertOverflowShown, the overflow button must be present and visible

This comment has been minimized.

Copy link
@kevinrushforth

kevinrushforth Dec 21, 2018

Collaborator

Right, the "if not null" test needs to remain, or it could be changed to:

    assertFalse(pane != null && pane.isVisible());

Either seems fine.

This comment has been minimized.

Copy link
@Ciruman

Ciruman Dec 21, 2018

Author Contributor

You are right. overflowMenu is always initialized, and never should be null. I will leave it in two asserts and if it fails will be easier to find why.

@kevinrushforth
Copy link
Collaborator

left a comment

Once you address Ajit's comments this looks fine to me. Unless Ajit has any concerns, this seems a low impact fix that can go in with a single reviewer (so you don't need to wait for me to formally review it).


private void assertOverflowNotShown() {
Pane pane = (Pane) toolBar.queryAccessibleAttribute(AccessibleAttribute.OVERFLOW_BUTTON);
if(pane!=null) {

This comment has been minimized.

Copy link
@kevinrushforth

kevinrushforth Dec 21, 2018

Collaborator

Right, the "if not null" test needs to remain, or it could be changed to:

    assertFalse(pane != null && pane.isVisible());

Either seems fine.

@Ciruman

This comment has been minimized.

Copy link
Contributor Author

commented Dec 21, 2018

BTW, modules/javafx.controls/src/test/java/test/javafx/scene/control/ToolbarTest.java should be modules/javafx.controls/src/test/java/test/javafx/scene/control/ToolBarTest.java should I rename it? The control name is ToolBar.

@kevinrushforth

This comment has been minimized.

Copy link
Collaborator

commented Dec 21, 2018

BTW, .../ToolbarTest.java should be .../ToolBarTest.java should I rename it?

No. This would be an unrelated change that should not be done as part of fixing this bug.

@Ciruman

This comment has been minimized.

Copy link
Contributor Author

commented Dec 27, 2018

@aghaisas do we have something left? Did I address all your comments?

@aghaisas

This comment has been minimized.

Copy link

commented Dec 28, 2018

Have you missed fixing this?
I am OK with the changes and will approve once you fix this comment.

@Ciruman, I guess, you did not see my last comment made 2 days ago?

@Ciruman

This comment has been minimized.

Copy link
Contributor Author

commented Dec 28, 2018

@aghaisas you are right, I did not see the comments. I will fix them ASAP.
Thanks!

@aghaisas

This comment has been minimized.

Copy link

commented Jan 3, 2019

This is ready to be committed now.

Have you signed Oracle Contributor Agreement?
This is pre-requisit for the submission as listed at - https://github.com/johanvos/openjfxorg/blob/master/CONTRIBUTING.md

@Ciruman

This comment has been minimized.

Copy link
Contributor Author

commented Jan 3, 2019

This is ready to be committed now.

Have you signed Oracle Contributor Agreement?
This is pre-requisit for the submission as listed at - https://github.com/johanvos/openjfxorg/blob/master/CONTRIBUTING.md

Yes, I have. I have already contributed in the past(with mercurial patches or emails).

@kevinrushforth

This comment has been minimized.

Copy link
Collaborator

commented Jan 3, 2019

@aghaisas Thank you for checking, since this is a requirement. I can confirm that he has signed the OCA and is listed on the OCA signatory list.

@aghaisas aghaisas merged commit 74c5279 into javafxports:develop Jan 4, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Ciruman Ciruman deleted the Ciruman:fix_JDK8152395 branch Jan 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.