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

8237602: TabPane doesn't respect order of TabPane.getTabs() list #201

Closed
wants to merge 2 commits into from

Conversation

@arapte
Copy link
Member

@arapte arapte commented Apr 29, 2020

Issue:
When tabs are permuted as mentioned in the issue description as,

  1. TabPane.getTabs().setAll(tab0, tab1)
  2. TabPane.getTabs().setAll(tab0, tab1, tab2, tab3);
    the tab headers do not get permuted in same order as TabPane.getTabs().

=> tab headers should be shown in order as tab0, tab1, tab2, tab3.
=> but are show in order as tab2, tab3, tab0, tab1

Cause:
Newly added tabs(tab2, tab3) are not inserted at correct index. The index Change.getFrom() (0) used from Change does not remain valid after the tabs to be moved(tab0, tab1) are removed from tabsToAdd list.

Fix:
Use the index of first newly added tab, from TabPane.getTabs(), which would be always reliable.

Verification:
No existing tests fail due to this change.
Added a system test which fails without and pass with fix.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8237602: TabPane doesn't respect order of TabPane.getTabs() list

Reviewers

  • Kevin Rushforth (kcr - Reviewer)
  • Jeanette Winzenburg (fastegal - Author)

Download

$ git fetch https://git.openjdk.java.net/jfx pull/201/head:pull/201
$ git checkout pull/201

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 29, 2020

👋 Welcome back arapte! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request.

@arapte arapte changed the title [WIP] 8237602: TabPane doesn't respect order of TabPane.getTabs() list 8237602: TabPane doesn't respect order of TabPane.getTabs() list Apr 29, 2020
@arapte arapte marked this pull request as ready for review Apr 29, 2020
@openjdk openjdk bot added the rfr label Apr 29, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 29, 2020

Webrevs

@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Apr 29, 2020

didn't look closely, just wondering why you need a system test for this?

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Apr 29, 2020

The following is wrong in your Description:

=> tab headers should be shown in order as tab2, tab3, tab0, tab1.
=> But should are show in order as tab0, tab1, tab2, tab3

Based on what I see in the bug report (and what I would expect), I'm pretty sure you meant to say:

The tab headers should be shown in order as tab0, tab1, tab2, tab3.
But are shown in order as tab2, tab3, tab0, tab1.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Apr 29, 2020

/reviewers 2

@openjdk
Copy link

@openjdk openjdk bot commented Apr 29, 2020

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@arapte
Copy link
Member Author

@arapte arapte commented Apr 30, 2020

didn't look closely, just wondering why you need a system test for this?

The system test file TabPanePermuteGetTabsTest was added to address exact similar kind of issue JDK-8222457. The current issue JDK-8237602 is just another case of same type. So it seems good to add a new test with similar existing tests.
I myself had fixed the previous issue, Can't remember why did I not consider of a unit test then 🤔.

@arapte
Copy link
Member Author

@arapte arapte commented Apr 30, 2020

The following is wrong in your Description:

Thanks Kevin, the description is corrected now. 👍

@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Apr 30, 2020

The system test file TabPanePermuteGetTabsTest was added to address exact similar kind of issue JDK-8222457. The current issue JDK-8237602 is just another case of same type. So it seems good to add a new test with similar existing tests.
I myself had fixed the previous issue, Can't remember why did I not consider of a unit test then 🤔.

yeah, if we don't document everything :))) Would you consider moving both into a unit test? Have one locally, could send it to you ..

Also a minor quirk as to the naming: it should not name it "permutation" because it isn't from the perspective of the list change: the notification is a "replace". Btw, the pattern in the listener (of collecting up additions/removals) is a bit smelly - will fail as soon as you get disjoint additions (on the bright side: they don't happen in the current implementation of ObservableList but are allowed to), just something to keep in mind :)

@arapte
Copy link
Member Author

@arapte arapte commented Apr 30, 2020

Would you consider moving both into a unit test? Have one locally, could send it to you ..

A ready test 👍 , That's very nice of you. Yes, We can move all the four tests to unit test. But I would prefer to do it as another TEST_BUG. Does that sound good ?
I have created a follow on issue to do this JDK-8244195

Btw, the pattern in the listener (of collecting up additions/removals) is a bit smelly

I did try to change whole implementation, but it looked like a big change for the issue we have. The fix is very minimal and local.

it should not name it "permutation"

Shall think of a new name when we move the tests..

@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Apr 30, 2020

A ready test 👍 , That's very nice of you. Yes, We can move all the four tests to unit test. But I would prefer to do it as another TEST_BUG. Does that sound good ?

not quite complete, just nearly - attached the unit test to the issue JDK-8244195 you created

Btw, the pattern in the listener (of collecting up additions/removals) is a bit smelly

I did try to change whole implementation, but it looked like a big change for the issue we have. The fix is very minimal and local.

good with me, as long as it covers all possible modifications to the tabs list

have a nice weekend (holiday here in Berlin tomorrow, so I might be off, except that with corona there's not so much too do, except working :)

@kevinrushforth kevinrushforth self-requested a review May 1, 2020
Copy link
Member

@kevinrushforth kevinrushforth left a comment

I think the change is fine, although I still need to run it. I have a couple questions / comments.

Copy link
Collaborator

@kleopatra kleopatra left a comment

tests are passing, what else can we achieve with a fix :) Looks good!

@openjdk
Copy link

@openjdk openjdk bot commented May 18, 2020

@arapte This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type /integrate in a new comment to proceed. After integration, the commit message will be:

8237602: TabPane doesn't respect order of TabPane.getTabs() list

Reviewed-by: kcr, fastegal
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /issue command.

Since the source branch of this PR was last updated there have been 23 commits pushed to the master branch:

  • bb24322: 8244112: Skin implementations: must not violate contract of dispose
  • dbb6437: 8244647: Wrong first layout pass of Scrollbar controls on touch supported devices
  • 7b06190: 8242548: Wrapped labeled controls using -fx-line-spacing cut text off
  • 435671e: 8202296: Monocle MouseInput doesn't send keyboard modifiers in events.
  • c14cc44: 8244417: support static build for Windows
  • b14e085: 8244735: Error on iOS passing keys with unicode values greater than 255
  • b0d66d0: 8242508: Upgrade to Visual Studio 2019 version 16.5.3
  • 0f87d20: 8244487: One Windows 10 SDK file missing from FX build
  • 4ec163d: 8242001: ChoiceBox: must update value on setting SelectionModel, part2
  • 236e2d6: 8244421: Wrong scrollbar position on touch enabled devices
  • ... and 13 more: https://git.openjdk.java.net/jfx/compare/91d4c8b56efba2a6ea121292dec5dfd79060e307...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate bb2432241bcf0ec1add651b995d4ada6676b4cc3.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label May 18, 2020
@arapte
Copy link
Member Author

@arapte arapte commented May 18, 2020

/integrate

@openjdk openjdk bot closed this May 18, 2020
@openjdk openjdk bot added integrated and removed ready labels May 18, 2020
@openjdk
Copy link

@openjdk openjdk bot commented May 18, 2020

@arapte The following commits have been pushed to master since your change was applied:

  • bb24322: 8244112: Skin implementations: must not violate contract of dispose
  • dbb6437: 8244647: Wrong first layout pass of Scrollbar controls on touch supported devices
  • 7b06190: 8242548: Wrapped labeled controls using -fx-line-spacing cut text off
  • 435671e: 8202296: Monocle MouseInput doesn't send keyboard modifiers in events.
  • c14cc44: 8244417: support static build for Windows
  • b14e085: 8244735: Error on iOS passing keys with unicode values greater than 255
  • b0d66d0: 8242508: Upgrade to Visual Studio 2019 version 16.5.3
  • 0f87d20: 8244487: One Windows 10 SDK file missing from FX build
  • 4ec163d: 8242001: ChoiceBox: must update value on setting SelectionModel, part2
  • 236e2d6: 8244421: Wrong scrollbar position on touch enabled devices
  • 0385563: 8243255: Font size is large in JavaFX app with enabled Monocle on Raspberry Pi
  • 2e90076: 8242507: Add support for Visual Studio 2019
  • 39d9c3b: 8244110: NPE in MenuButtonSkinBase change listener
  • 99f7747: 8241999: ChoiceBox: incorrect toggle selected for uncontained
  • 1cae6a8: 8176499: Dependence on java.util.Timer freezes screen when OS time resets backwards
  • 2b9eb52: 8237504: Update copyright header for files modified in 2020
  • 3130fc8: 8198402: ToggleButton.setToggleGroup causes memory leak when button is removed via ToggleGroup.getToggles()
  • 8ad5805: 8191758: Match WebKit's font weight rendering with JavaFX
  • 66c3b38: 8227425: Add support for e-paper displays on i.MX6 devices
  • e30049f: 8242077: Add information about HTTP/2 and HttpClient usage in WebEngine
  • e0ffca3: 8242505: Some WebKit tests might fail because Microsoft libraries are not loaded
  • ceb3fce: 8087555: [ChoiceBox] uncontained value not shown
  • 818ac00: 8175358: Memory leak when moving MenuButton into another Scene

Your commit was automatically rebased without conflicts.

Pushed as commit 6e03930.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants