Skip to content
This repository has been archived by the owner on Jun 24, 2021. It is now read-only.

Fix bug JDK-8210386 (merged with Marlin 0.9.3) #206

Closed
wants to merge 1 commit into from
Closed

Fix bug JDK-8210386 (merged with Marlin 0.9.3) #206

wants to merge 1 commit into from

Conversation

bourgesl
Copy link
Contributor

Please review this PR for #205 that consist in merging latest changes from Marlin 0.9.3.

See also the OpenJDK 12 patch under review:
http://mail.openjdk.java.net/pipermail/2d-dev/2018-September/009443.html

laurent

@bourgesl
Copy link
Contributor Author

FYI I adapted the ScaleClipTest class to use JavaFX API:
It fails on openjfx without the fix and passes with the patch.

@kevinrushforth
Copy link
Collaborator

Don't forget to send a review request to openjfx-dev, in which case there will be no need to post a webrev. This PR can serve as the review to get it into openjfx/jfx-dev/rt

@eugener eugener added the enhancement New feature or request label Sep 18, 2018
@bourgesl bourgesl added bug Something isn't working and removed enhancement New feature or request labels Sep 18, 2018
@kevinrushforth
Copy link
Collaborator

Fix for JDK-8210386

@bourgesl
Copy link
Contributor Author

bourgesl commented Oct 3, 2018

Phil, could you have a look or any other reviewer ?

@johanvos
Copy link
Collaborator

johanvos commented Oct 3, 2018

I did a quick review and this patch makes sense -- but I didn't do before/after tests.

@kevinrushforth
Copy link
Collaborator

@johanvos or @prrace can one of you approve so we have a second reviewer?

Copy link

@prrace prrace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a biggy but I see the use of java.awt.Color in the test. The FX Color class doesn't have the same handy API to return an int, but it should be possible. If you think that is just make work, then what about making the import be for the FX Color class and using the fully qualified name for the java.awt.Color usage. All up to you. If you think it is not worth it I'm OK with that.

@bourgesl
Copy link
Contributor Author

bourgesl commented Oct 5, 2018

Phil, you are right.
FX Color class is not equivalent to awt Color and it was disappointing that no method exist to translate Color <=> int (rgba), probably for good reasons (colorspace srgb vs rgb ?).

Kevin, could you create a RFE to enhance FX Color API ?

I will push asap this patch as is, in jfx-dev directly. Kevin, what is the process to ask for a jfx11u backport ?

@kevinrushforth
Copy link
Collaborator

Kevin, could you create a RFE to enhance FX Color API ?

You can file it if you want. I suspect that this isn't something we would work on in isolation, rather more likely as part of an "add image operations to JavaFX" RFE (which I thought we already had, but I can't find). If there is enough interest by someone to do this separately, in a way that makes sense, that could be another option.

I will push asap this patch as is, in jfx-dev directly.

OK (in that case, no need to merge this PR into develop).

Kevin, what is the process to ask for a jfx11u backport ?

Still pending, as we haven't nailed down how this will be done. Johan and I have been discussing the logistics of this, and will come up with something in the next week or so.

@bourgesl bourgesl closed this Oct 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants