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

Jump to bottom

8270107: Open source FXMediaPlayer test app #613

Closed
wants to merge 1 commit into from
Closed

8270107: Open source FXMediaPlayer test app #613

wants to merge 1 commit into from

Conversation

sashamatveev
Copy link
Member

@sashamatveev sashamatveev commented Aug 27, 2021 •
edited by openjdk[bot] bot

  • Added FXMediaPlayer test application.
  • This app uses all media APIs and very handy in verifying media builds during development.
  • It can be compiled and run by running "ant" and "ant run" in tests/manual/media/FXMediaPlayer.

Progress

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

Issue

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/613/head:pull/613
$ git checkout pull/613

Update a local copy of the PR:
$ git checkout pull/613
$ git pull https://git.openjdk.java.net/jfx pull/613/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 613

View PR using the GUI difftool:
$ git pr show -t 613

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/613.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Aug 27, 2021

👋 Welcome back almatvee! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Ready for review label Aug 27, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Aug 27, 2021

Webrevs

@kevinrushforth kevinrushforth self-requested a review Aug 27, 2021
@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Aug 27, 2021

By way of background this is a lightly cleaned up version of a closed-source test program that we are open sourcing in order to facilitate testing various media features.

Even though it is a test program, I'd like a second pair of eyes on it.

/reviewers 2

@openjdk
Copy link

@openjdk openjdk bot commented Aug 27, 2021

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

@johanvos
Copy link
Collaborator

@johanvos johanvos commented Aug 28, 2021

That's great. I agree a global media sample would be an excellent improvement for testing (regression on) media.

I notice there are a number of netbeans-impl files, which is ok for me, unless we want to avoid ide-specific impl files?
It would be nice if this sample can leverage openjfx-global properties, i.e. I see it now has java source level set to 1.9.

Rather than requesting many changes (e.g. bump java version) before it is in the repository, I think it would be good to do some sanity tests on the different platforms and then include it. I plan to do that later this weekend.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Aug 28, 2021

I agree with getting it in now (after testing) and then improving it after it is in the repo.

Btw, the nbproject/ files were derived from those in the apps/toys/ directory, for example, apps/toys/Hello/nbproject/, and similarly are used to build using ant. So they aren't really IDE files any longer, even though that's where they originated back in the FX 2 time frame.

A good cleanup effort would be to rewrite them to remove the NetBeans project structure, but that would be a larger effort than just this one manual test program.

@swpalmer
Copy link
Collaborator

@swpalmer swpalmer commented Aug 29, 2021

kevinrushforth
kevinrushforth approved these changes Sep 2, 2021
View changes
Copy link
Member

@kevinrushforth kevinrushforth left a comment

Looks good. Tested on all three platforms.

@kevinrushforth kevinrushforth requested review from johanvos and aghaisas Sep 7, 2021
@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Sep 14, 2021

@aghaisas or @johanvos can one of you be the second reviewer on this?

@johanvos
Copy link
Collaborator

@johanvos johanvos commented Sep 14, 2021

I partially tested it, will do the remainder now.

johanvos
johanvos approved these changes Sep 14, 2021
View changes
Copy link
Collaborator

@johanvos johanvos left a comment

tested on linux and mac, works great.
Good addition!

@openjdk
Copy link

@openjdk openjdk bot commented Sep 14, 2021

@sashamatveev This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8270107: Open source FXMediaPlayer test app

Reviewed-by: kcr, jvos

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 8 new commits pushed to the master branch:

  • f987b18: 8089398: [ChoiceBox, ComboBox] throws NPE on setting value on null selectionModel
  • a272c4f: 8273324: IllegalArgumentException: fromIndex(0) > toIndex(-1) after clear and select TableCell
  • 26d6438: 8273138: BidirectionalBinding fails to observe changes of invalid properties
  • 5552213: Merge
  • 24d0600: 8273343: Create release notes for JavaFX 17
  • 78ae4a8: 8269871: CellEditEvent: must not throw NPE in accessors
  • 2267b11: 8273071: SeparatorSkin: must remove child on dispose
  • e931501: 8269081: Tree/ListViewSkin: must remove flow on dispose

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ 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 Ready to be integrated label Sep 14, 2021
@sashamatveev
Copy link
Member Author

@sashamatveev sashamatveev commented Sep 15, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Sep 15, 2021

Going to push as commit 51265c0.
Since your change was applied there have been 8 commits pushed to the master branch:

  • f987b18: 8089398: [ChoiceBox, ComboBox] throws NPE on setting value on null selectionModel
  • a272c4f: 8273324: IllegalArgumentException: fromIndex(0) > toIndex(-1) after clear and select TableCell
  • 26d6438: 8273138: BidirectionalBinding fails to observe changes of invalid properties
  • 5552213: Merge
  • 24d0600: 8273343: Create release notes for JavaFX 17
  • 78ae4a8: 8269871: CellEditEvent: must not throw NPE in accessors
  • 2267b11: 8273071: SeparatorSkin: must remove child on dispose
  • e931501: 8269081: Tree/ListViewSkin: must remove flow on dispose

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Sep 15, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Ready to be integrated rfr Ready for review labels Sep 15, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 15, 2021

@sashamatveev Pushed as commit 51265c0.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

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

@johanvos johanvos

@kevinrushforth kevinrushforth

@aghaisas aghaisas

Assignees
No one assigned
Labels
integrated Pull request has been integrated
Milestone
No milestone
4 participants
@sashamatveev @kevinrushforth @johanvos @swpalmer