Re: RFR: 8090547: Allow for transparent backgrounds in WebView [v7]
On Wed, 8 Sep 2021 21:10:27 GMT, Kevin Rushforth wrote: > I looked at the generated javadoc and this will read better without a > paragraph break. When you remove the paragraph break, please also remove the blank line to make it clearer to future readers of the code that it is intended to be part of the same paragraph as the previous sentence. - PR: https://git.openjdk.java.net/jfx/pull/563
Re: RFR: 8090547: Allow for transparent backgrounds in WebView [v7]
On Wed, 8 Sep 2021 16:15:36 GMT, Jose Pereda wrote:
>> Currently, `WebPage` has already a public `setBackgroundColor()` method, but
>> the class is not public. Therefore, public API is needed in `WebView` to
>> allow developers access to it.
>>
>> In line with the `fontSmoothingType` property, this PR provides public
>> support for setting the background color of a WebPage, by adding a
>> `pageFill` property, and a CSR is required.
>>
>> The color for the background, that can be opaque, transparent or with any
>> level of opacity, can be set via code or via CSS using `-fx-page-fill`.
>>
>> Unit tests and a system test are provided.
>
> Jose Pereda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Address feedback from reviewers
I left a few minor comments, but otherwise this all looks good to me.
Once you make the one requested change to remove the paragraph break, you can
also update the CSR with that change and the two other changes I requested in
the CSR, and then move the CSR to proposed.
modules/javafx.web/src/main/java/com/sun/webkit/WebPage.java line 97:
> 95:
> 96: private int fontSmoothingType;
> 97: private int backgroundIntRgba = 0x;
Maybe add back the comment about this being Color.WHITE?
modules/javafx.web/src/main/java/com/sun/webkit/WebPage.java line 2589:
> 2587: private static int getIntRgba(Color color) {
> 2588: if (color == null) {
> 2589: return -1;
Maybe: `return 0x;` ? or else assign `color = Color.WHITE;` and fall
through?
modules/javafx.web/src/main/java/javafx/scene/web/WebView.java line 707:
> 705: * level of transparency.
> 706: *
> 707: * However, if the HTML content being loaded sets its own
I looked at the generated javadoc and this will read better without a paragraph
break.
-
PR: https://git.openjdk.java.net/jfx/pull/563
Re: RFR: 8089398: [ChoiceBox, ComboBox] throws NPE on setting value on null selectionModel [v6]
On Tue, 27 Jul 2021 12:41:00 GMT, Marius Hanl wrote: >> This PR fixes multiple NPEs in Choice-and ComboBox, when the selection model >> is null. >> >> ChoiceBox: >> - Null check in **valueProperty()** listener >> >> ComboBox: >> - Null check in **editableProperty()** listener >> - Null check in **valueProperty()** listener >> - Null check in **ComboBoxListViewSkin#updateValue()** >> - Null check in **ComboBoxListViewSkin#layoutChildren()** >> - 2x Null check in **ComboBoxListViewSkin#createListView()** >> >> ~~The tests checks, that no NPE is printed to the console. >> Some checks, that the set value is still displayed (either in the ComboBox >> button cell or the ChoiceBox display label)~~ > > Marius Hanl has updated the pull request incrementally with one additional > commit since the last revision: > > Fixed another NPE on skin creation when the cbx selection model is null Marked as reviewed by aghaisas (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/557
Re: RFR: 8089398: [ChoiceBox, ComboBox] throws NPE on setting value on null selectionModel [v6]
On Tue, 27 Jul 2021 12:41:00 GMT, Marius Hanl wrote: >> This PR fixes multiple NPEs in Choice-and ComboBox, when the selection model >> is null. >> >> ChoiceBox: >> - Null check in **valueProperty()** listener >> >> ComboBox: >> - Null check in **editableProperty()** listener >> - Null check in **valueProperty()** listener >> - Null check in **ComboBoxListViewSkin#updateValue()** >> - Null check in **ComboBoxListViewSkin#layoutChildren()** >> - 2x Null check in **ComboBoxListViewSkin#createListView()** >> >> ~~The tests checks, that no NPE is printed to the console. >> Some checks, that the set value is still displayed (either in the ComboBox >> button cell or the ChoiceBox display label)~~ > > Marius Hanl has updated the pull request incrementally with one additional > commit since the last revision: > > Fixed another NPE on skin creation when the cbx selection model is null The latest patch does not apply cleanly due to the change in copyright headers. It was fixed in - [JDK-8270960](https://bugs.openjdk.java.net/browse/JDK-8270960) - PR: https://git.openjdk.java.net/jfx/pull/557
RFR: 8273485: Deadlock when also using Swing and exiting Fullscreen on Mac
When using Swing it's possible to generate a Deadlock. It's related to the nested eventloop started in enterFullScreenExitingLoop - and the RenderLock aquired when using setView in Scene. Sample Programm and Threaddump are added to the ticket. Removing the nested loop fixes the Problem. I hope this doesn't have any side effect - so far i don't know of any. - Commit messages: - JDK-8273485 Changes: https://git.openjdk.java.net/jfx/pull/622/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=622=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8273485 Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/622.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/622/head:pull/622 PR: https://git.openjdk.java.net/jfx/pull/622
Integrated: 8273138: BidirectionalBinding fails to observe changes of invalid properties
On Sun, 29 Aug 2021 04:12:22 GMT, Michael Strauß wrote:
> This PR fixes a bug that was introduced in #454.
>
> Since this fix might impact the performance considerations in the original
> PR, I ran a performance benchmark against the previous `ChangeListener`-based
> implementation, which still shows better performance for the new
> implementation:
>
>
> @State(Scope.Benchmark)
> public class BindingBenchmark {
> DoubleProperty property1 = new SimpleDoubleProperty();
> DoubleProperty property2 = new SimpleDoubleProperty();
>
> public BindingBenchmark() {
> property2.bindBidirectional(property1);
> }
>
> @Benchmark
> public void benchmark() {
> for (int i = 0; i < 1000; ++i) {
> property1.set((i % 2 == 0) ? 12345.0 : 54321.0);
> }
> }
> }
>
>
> | Benchmark | Mode | Cnt | Score | Error | Units |
> |---|--|-|---|---||
> | ChangeListener | thrpt | 5 | 7.463 | 0.040 | ops/s |
> | InvalidationListener (fixed) | thrpt | 5 | 15.095 | 0.092 | ops/s |
This pull request has now been integrated.
Changeset: 26d6438e
Author:Michael Strauß
Committer: Nir Lisker
URL:
https://git.openjdk.java.net/jfx/commit/26d6438ea267d703039facac7dab67175b863b46
Stats: 54 lines in 2 files changed: 54 ins; 0 del; 0 mod
8273138: BidirectionalBinding fails to observe changes of invalid properties
Reviewed-by: kcr, arapte
-
PR: https://git.openjdk.java.net/jfx/pull/614
Re: RFR: 8090547: Allow for transparent backgrounds in WebView [v4]
On Mon, 6 Sep 2021 11:05:42 GMT, Jose Pereda wrote:
>> I don't doubt that the area should be cleared. What I was questioning is
>> whether this was the best place to do it? I'd be OK with it if you document
>> it.
>
> The two options I see so far to clear the area are the one I've committed
> (change in `WCGraphicsPrismContext::setClip`) and the one I've mentioned
> above (`WCPageBackBufferImpl::copyArea`).
> After some more testing, I see that the latter doesn't work when there is
> full transparency, so I will discard it.
>
> Back to my original commit, it could be moved up to `WebPage::paint2GC`,
> before the call to `gc.setClip(rq.getClip());`, and that would also remove
> the need of passing down the opacity level to `WCGraphicsContext`. I also see
> now that the `clearRect()` call is required for full transparency only.
>
> This would be the change:
>
> WCRectangle clip = rq.getClip();
> if (clip != null) {
> if (isBackgroundTransparent()) {
> // As backbuffer is enabled, new clips are drawn over
> the old rendered frames
> // regardless the alpha channel. While that works
> fine for alpha > 0,
> // for alpha == 0 we need to clear the old frame or
> it will still be visible.
>
> gc.clearRect((int) clip.getX(), (int) clip.getY(),
> (int) clip.getWidth(), (int) clip.getHeight());
> }
> gc.setClip(clip);
> }
This seems like the best approach to me in that it moves the clear to a drawing
method rather than as a side effect of setting the clip.
-
PR: https://git.openjdk.java.net/jfx/pull/563
Re: RFR: 8090547: Allow for transparent backgrounds in WebView [v5]
On Sun, 29 Aug 2021 06:36:40 GMT, Michel Jung
wrote:
>> Jose Pereda has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Color to int32 conversion and more changes based on feedback
>
> modules/javafx.web/src/main/java/com/sun/webkit/WebPage.java line 629:
>
>> 627: try {
>> 628: log.fine("setBackgroundColor int32: " + backgroundColor +
>> 629:" for all frames");
>
> I don't know JavaFX's PlatformLogger but this should probably be:
>
> Suggestion:
>
> log.fine("setBackgroundColor int32: {} for all frames",
> backgroundColor);
>
> or:
>
> Suggestion:
>
> if(log.isTraceEnabled()) {
> log.fine("setBackgroundColor int32: {} for all frames",
> backgroundColor);
> }
>
>
> Even though this probably isn't something that's called very often :)
Or just wrap the call in a test for `if (log.isLoggable(Level.FINE))`, which is
similar to what was done earlier. In any case, this is just modifying the
contents of the String and is otherwise preexisting code, so I'll leave it up
to @jperedadnr as to whether he wants to change it.
-
PR: https://git.openjdk.java.net/jfx/pull/563
Re: RFR: 8273485: Deadlock when also using Swing and exiting Fullscreen on Mac
On Wed, 8 Sep 2021 10:37:40 GMT, Florian Kirmaier wrote: > When using Swing it's possible to generate a Deadlock. > It's related to the nested eventloop started in enterFullScreenExitingLoop - > and the RenderLock aquired when using setView in Scene. > Sample Programm and Threaddump are added to the ticket. > > Removing the nested loop fixes the Problem. > I hope this doesn't have any side effect - so far i don't know of any. I am almost certain that this proposed fix will cause problems. At the very least we will now have mismatched enter / exit nested loop calls. The nested loop for full screen on Mac was done for a reason, so you will need to track down what that reason is and either verify that it is no longer needed or else solve that some other way. Also, you don't explain why removing the nested event loop is the right fix for the deadlock. Finally, once you address the above two points, you will need to turn the test program attached to the JBS bug into an automated test. - Changes requested by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/622
Re: RFR: 8089398: [ChoiceBox, ComboBox] throws NPE on setting value on null selectionModel [v6]
On Wed, 8 Sep 2021 06:32:06 GMT, Ajit Ghaisas wrote: > The latest patch does not apply cleanly due to the change in copyright > headers. @aghaisas If you fetch the PR branch and merge master (which is what I usually do when testing a PR), git is able to auto-resolve it; this is why Skara didn't flag this PR as having a merge conflict (which it will if there is). - PR: https://git.openjdk.java.net/jfx/pull/557
Re: RFR: 8090547: Allow for transparent backgrounds in WebView [v6]
On Mon, 6 Sep 2021 09:28:20 GMT, Jose Pereda wrote:
>> modules/javafx.web/src/main/java/com/sun/webkit/WebPage.java line 97:
>>
>>> 95:
>>> 96: private int fontSmoothingType;
>>> 97: private Color backgroundColor = Color.WHITE;
>>
>> This might be a problem, since there are code paths that bypass
>> `setBackgroundColor(Color)`. I might recommend storing the converted 32-bit
>> color, and then checking that for transparency. Either that or you will need
>> to derive a `Color` from a 32-bit int in the cases that set a 32-bit int
>> color directly. The former is probably easier.
>
> In my first commit there was already a method to get the color out of the
> 32-bit int (which was still referred as hash value at that time):
>
> private static Color getColorFromHash(int hash) {
> String hexString = Integer.toHexString(hash);
> int length = hexString.length();
> return Color.valueOf("#" + "0".repeat(8 - length) + hexString);
> }
>
> If we keep it in `WebPage` (renaming it accordingly to `getColorFromInt32`
> for instance), we could do:
>
>
> public void setBackgroundColor(Color backgroundColor) {
> setBackgroundColor(getColorInt32Value(backgroundColor));
> }
>
> public void setBackgroundColor(int backgroundColor) {
>this.backgroundColor = getColorFromInt32(backgroundColor);
>lockPage();
> ...
> }
> ```
> which looks a little bit ugly.
>
> The other option, as you mention, is finding out if the 32-bit int has alpha
> 0 or 1, which can be done storing only the int value, not the color, so this
> looks cleaner, we don't really need to hold a reference to the Color after
> all:
>
>
> private int backgroundColor = -1; // Color.WHITE
>
> public void setBackgroundColor(Color backgroundColor) {
> setBackgroundColor(getColorInt32Value(backgroundColor));
> }
>
> public void setBackgroundColor(int backgroundColor) {
>this.backgroundColor = backgroundColor;
>lockPage();
> ...
> }
>
> private boolean isBackgroundTransparent() {
> return (backgroundColor & 0x00FF) == 0;
> }
>
> private boolean isBackgroundOpaque() {
> return (backgroundColor & 0x00FF) == 255;
> }
Yes, the second approach does look cleaner, although I think the initial
`backgroundColor` would be better defined as a hex constant, `0x` for
clarity.
-
PR: https://git.openjdk.java.net/jfx/pull/563
Re: RFR: 8273324: IllegalArgumentException: fromIndex(0) > toIndex(-1) after clear and select TableCell [v3]
On Tue, 7 Sep 2021 10:40:01 GMT, Michael Strauß wrote: >> This PR fixes the exception thrown by the sample code in >> [8273324](https://bugs.openjdk.java.net/browse/JDK-8273324), while retaining >> the incorrect behavior in the scenario described. > > Michael Strauß has updated the pull request incrementally with one additional > commit since the last revision: > > fixed case of multi-selection across several rows The updated fix and new test both look good. - Marked as reviewed by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/617
Re: RFR: 8089398: [ChoiceBox, ComboBox] throws NPE on setting value on null selectionModel [v6]
On Wed, 8 Sep 2021 12:20:57 GMT, Kevin Rushforth wrote: > > The latest patch does not apply cleanly due to the change in copyright > > headers. > > @aghaisas If you fetch the PR branch and merge master (which is what I > usually do when testing a PR), git is able to auto-resolve it; this is why > Skara didn't flag this PR as having a merge conflict (which it will if there > is). Hmm.. interesting. I updated my master branch and then tried to apply this PR diff manually. The patch did not apply and hence I commented the same. I do see that Skara did not flag the merge conflict for this PR. - PR: https://git.openjdk.java.net/jfx/pull/557
Re: RFR: 8273324: IllegalArgumentException: fromIndex(0) > toIndex(-1) after clear and select TableCell [v3]
On Tue, 7 Sep 2021 10:40:01 GMT, Michael Strauß wrote: >> This PR fixes the exception thrown by the sample code in >> [8273324](https://bugs.openjdk.java.net/browse/JDK-8273324), while retaining >> the incorrect behavior in the scenario described. > > Michael Strauß has updated the pull request incrementally with one additional > commit since the last revision: > > fixed case of multi-selection across several rows Looks good to me - Marked as reviewed by jpereda (Committer). PR: https://git.openjdk.java.net/jfx/pull/617
Integrated: 8273324: IllegalArgumentException: fromIndex(0) > toIndex(-1) after clear and select TableCell
On Fri, 3 Sep 2021 18:19:45 GMT, Michael Strauß wrote: > This PR fixes the exception thrown by the sample code in > [8273324](https://bugs.openjdk.java.net/browse/JDK-8273324), while retaining > the incorrect behavior in the scenario described. This pull request has now been integrated. Changeset: a272c4f6 Author:Michael Strauß Committer: Kevin Rushforth URL: https://git.openjdk.java.net/jfx/commit/a272c4f6bd08fee8928c78e17428574aec485cfd Stats: 53 lines in 2 files changed: 50 ins; 0 del; 3 mod 8273324: IllegalArgumentException: fromIndex(0) > toIndex(-1) after clear and select TableCell Reviewed-by: jpereda, kcr - PR: https://git.openjdk.java.net/jfx/pull/617
Re: RFR: 8090547: Allow for transparent backgrounds in WebView [v7]
> Currently, `WebPage` has already a public `setBackgroundColor()` method, but > the class is not public. Therefore, public API is needed in `WebView` to > allow developers access to it. > > In line with the `fontSmoothingType` property, this PR provides public > support for setting the background color of a WebPage, by adding a `pageFill` > property, and a CSR is required. > > The color for the background, that can be opaque, transparent or with any > level of opacity, can be set via code or via CSS using `-fx-page-fill`. > > Unit tests and a system test are provided. Jose Pereda has updated the pull request incrementally with one additional commit since the last revision: Address feedback from reviewers - Changes: - all: https://git.openjdk.java.net/jfx/pull/563/files - new: https://git.openjdk.java.net/jfx/pull/563/files/941ba714..02061b9e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=563=06 - incr: https://webrevs.openjdk.java.net/?repo=jfx=563=05-06 Stats: 61 lines in 4 files changed: 9 ins; 35 del; 17 mod Patch: https://git.openjdk.java.net/jfx/pull/563.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/563/head:pull/563 PR: https://git.openjdk.java.net/jfx/pull/563
