Integrated: 8208169: can not print selected pages of web page
On Fri, 15 May 2020 16:36:29 GMT, Bhawesh Choudhary wrote: > Print function of WebEngine.java ignores page range setting and prints given > number of pages starting from first page, > which is the root cause of this issue. To fix it, put check for page ranges > and if it available, use it for printing > pages otherwise print all pages as usual. This pull request has now been integrated. Changeset: 8440b64b Author:bhawesh Committer: Kevin Rushforth URL: https://git.openjdk.java.net/jfx/commit/8440b64b Stats: 159 lines in 2 files changed: 0 ins; 155 del; 4 mod 8208169: can not print selected pages of web page Reviewed-by: prr, kcr - PR: https://git.openjdk.java.net/jfx/pull/222
Re: RFR: 8208169: can not print selected pages of web page [v5]
On Tue, 16 Jun 2020 07:59:02 GMT, Bhawesh Choudhary wrote: >> Print function of WebEngine.java ignores page range setting and prints given >> number of pages starting from first page, >> which is the root cause of this issue. To fix it, put check for page ranges >> and if it available, use it for printing >> pages otherwise print all pages as usual. > > Bhawesh Choudhary has updated the pull request incrementally with one > additional commit since the last revision: > > Formatting changes Marked as reviewed by prr (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/222
Integrated: 8247163: JFXPanel throws exception on click when no Scene is set
On Tue, 9 Jun 2020 10:39:29 GMT, Florian Kirmaier wrote: > Fixing exception when clicking on JFXPanel when no Scene is set. > > quick test: `./gradlew -PFULL_TEST=true -PUSE_ROBOT=true :systemTests: test > --tests *javafx.embed.swing.JFXPan* --info` > > It's a regression from my previous PR: https://github.com/openjdk/jfx/pull/25 This pull request has now been integrated. Changeset: 1727dfa4 Author:Florian Kirmaier Committer: Kevin Rushforth URL: https://git.openjdk.java.net/jfx/commit/1727dfa4 Stats: 22 lines in 2 files changed: 0 ins; 20 del; 2 mod 8247163: JFXPanel throws exception on click when no Scene is set Reviewed-by: kcr - PR: https://git.openjdk.java.net/jfx/pull/248
Result: New OpenJFX Reviewer: Nir Lisker
Voting for Nir Lisker [1] to OpenJFX Reviewer [2] is now closed. Yes: 5 Veto: 0 Abstain: 0 According to the Bylaws definition of Three-Vote Consensus, this is sufficient to approve the nomination. -- Kevin [1] https://openjdk.java.net/census#nlisker [2] https://mail.openjdk.java.net/pipermail/openjfx-dev/2020-June/026609.html
Re: RFR: 8238954: Improve performance of tiled snapshot rendering [v11]
> Issue JDK-8088198, where an exception would be thrown when trying to capture > a snapshot whose final dimensions would be > larger than the running platform's maximum supported texture size, was > addressed in openjfx14. The fix, based around > the idea of capturing as many tiles of the maximum possible size and > re-compositing the final snapshot out of these, is > currently only attempted after the original, non-tiled, strategy has already > failed. This was decided to avoid any risk > of regressions, either in terms of performances and correctness, while still > offering some relief to the original > issue. This follow-on issue aims to propose a fix to the original issue, > that is able to correctly decide on the best > snapshot strategy (tiled or not) to adopt before applying it and ensure best > performances possible when tiling is > necessary while still introducing no regressions compared to the original > solution. Frederic Thevenet has updated the pull request incrementally with one additional commit since the last revision: Changed wrong value for image size (for these tests we *don't* want it to be dividable by 2 nor 3) - Changes: - all: https://git.openjdk.java.net/jfx/pull/112/files - new: https://git.openjdk.java.net/jfx/pull/112/files/82746316..4752d83e Webrevs: - full: https://webrevs.openjdk.java.net/jfx/112/webrev.10 - incr: https://webrevs.openjdk.java.net/jfx/112/webrev.09-10 Stats: 10 lines in 1 file changed: 0 ins; 0 del; 10 mod Patch: https://git.openjdk.java.net/jfx/pull/112.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/112/head:pull/112 PR: https://git.openjdk.java.net/jfx/pull/112
Re: RFR: 8247163: JFXPanel throws exception on click when no Scene is set [v2]
On Tue, 16 Jun 2020 07:34:13 GMT, Florian Kirmaier wrote: >> Fixing exception when clicking on JFXPanel when no Scene is set. >> >> quick test: `./gradlew -PFULL_TEST=true -PUSE_ROBOT=true :systemTests: test >> --tests *javafx.embed.swing.JFXPan* --info` >> >> It's a regression from my previous PR: https://github.com/openjdk/jfx/pull/25 > > Florian Kirmaier has updated the pull request incrementally with one > additional commit since the last revision: > > 8247163: > Added space based on code review Marked as reviewed by kcr (Lead). - PR: https://git.openjdk.java.net/jfx/pull/248
Re: RFR: 8193800: TreeTableView selection changes on sorting [v4]
On Wed, 17 Jun 2020 11:46:07 GMT, Ambarish Rapte wrote:
>> modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java
>> line 472:
>>
>>> 471: countSelectedIndexChangeEvent++;
>>> 472: assertEquals(selectedItemBefore,
>>> treeTableView.getTreeItem(sm.getSelectedIndex()));
>>> 473: });
>>
>> If this assertion ever fails, I don't think it will cause a test failure,
>> since the event handling code will swallow
>> the exception. The same is true of the other listeners. I don't know if it
>> would be possible to use the
>> UncaughtExceptionHandler as is done in other tests -- see
>> [JDK-8244531](https://bugs.openjdk.java.net/browse/JDK-8244531) -- but that
>> might be worth exploring. Another
>> possibility is to wrap all the listeners in a try/catch and keep a list of
>> `Throwable`s that are caught.
>
> Hi Kevin, I tested these listeners by adding assert that always fail. It
> shows the failures correctly. Also in this
> file there are other tests which assert inside a listener. I confirmed that
> those tests also show failures correctly. I
> could not find out what makes it work though.
I can confirm that the test fails if an assertion hits. The reason is that this
ChangeListener throws any exception to
the code that causes the change, so it will propagate to the test thread.
>> modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java
>> line 431:
>>
>>> 430: for (int j = 0; j < FIRST_LEVEL_COUNT - 1; j++) {
>>> 431: TreeItem tj = new TreeItem<>("" + i + j);
>>> 432: tj.setExpanded(true);
>>
>> The tree item strings will not be unique. This won't affect whether the test
>> passes or fails, since `TreeItem` does not
>> override `equals`, but it might be better if the strings were unique (in
>> case there ever was an error it would be
>> easier to understand).
>
> Hi Kevin, I verified the the tree item strings again. They seem to be unique.
> A total of 8800 of tree items get created
> each with unique string. Could you please recheck.
Confirmed. This was my mistake.
-
PR: https://git.openjdk.java.net/jfx/pull/244
Re: RFR: 8193800: TreeTableView selection changes on sorting [v5]
On Wed, 17 Jun 2020 11:38:45 GMT, Ambarish Rapte wrote: >> Issue: >> In TreeTableView, in case of Multiple selection mode, if nested items are >> selected, then TreeTableView does not >> retain/update the selection correctly when the tree items are >> permuted(either by `sort()` or by reordering using >> `setAll()`). Cause: >> >> 1. For permutation, the current implementation uses `TreeModificationEvent` >> to update the selection. >> 2. The indices from these TreeModificationEvents are not reliable. >> 3. It uses the non public `TreeTablePosition` constructor to create >> intermediate `TreeItem` positions, this constructor >> results in another unexpected TreeModificationEvent while one for sorting is >> already being processed. 4. In case of >> sorting, there can be multiple intermediate TreeModificationEvents >> generated, and for each TreeModificationEvent, the >> selection gets updated and results in selection change events being >> generated. 5. Each time a TreeItem is expanded or >> collapsed, the selection must be shifted, but shifting is not necessary in >> case of permutation. All these issues >> combine in wrong update of the selection. Fix: >> >> 1. On each TreeModificationEvent for permutation, for updating the >> selection, use index of TreeItem from the >> TreeTableView but not from the TreeModificationEvent. 2. Added a new non >> public TreeTablePosition constructor, which is >> almost a copy constructor but accepts a different row. 3. In case of >> sorting, send out the set of selection change >> events only once after the sorting is over. 4. In case of setAll, send out >> the set of selection change events same as >> before.(setAll results in only one TreeModificationEvent, which effectively >> results in only one set of selection change >> events). `shiftSelection()` should not be called in case of permutation i.e. >> call `if (shift != 0)` >> Verification: >> The change is very limited to updating of selection of TreeTableView items >> when the TreeItems are permuted, so the >> change should not cause any other failures. Added unit tests which fail >> before and pass after the fix. > > Ambarish Rapte has updated the pull request incrementally with one additional > commit since the last revision: > > Remove the un-required flag Marked as reviewed by kcr (Lead). - PR: https://git.openjdk.java.net/jfx/pull/244
Re: [Rev 04] RFR: 8238954: Improve performance of tiled snapshot rendering
On Mon, 15 Jun 2020 15:55:25 GMT, Frederic Thevenet wrote: >> Overall, this looks quite good. In particular the tiled rendering, as >> implemented by the `renderTile` method, should be >> reasonably efficient. >> My only high-level comment is that I'm somewhat skeptical of >> `computeOptimumTileSize` to determine the size and >> direction of tiling. I note that in the case of an image that is tiled in >> both X and Y, there are at most 4 distinct >> tile sizes if it doesn't fit evenly. In the case where only one of X or Y is >> tiled, there are at most 2 distinct tile >> sizes. Here is an example: +---+---+ . +---+ | >>| | . | | >> | M | M | . | R | >> | | | . | | >> +---+---+ . +---+ >> | | | . | | >> | M | M | . | R | >> | | | . | | >> +---+---+ . +---+ >> . .. . >> +---+---+ . +---+ >> | | | . | | >> | M | M | . | R | >> | | | . | | >> +---+---+ . +---+ >> | B | B | . | C | >> +---+---+ . +---+ >> >> Where `M` represents the middle set of tiles each with a size of `tileW x >> tileH`. `R` is the right hand column of >> tiles, `B` is bottom row, and `C` is corner. >> Recognizing this, I wonder if it might be better to always use the maximum >> tile size, but fill all of the middle tiles >> of that size first, and then pick up the right and/or bottom edges as >> needed. This will minimize thrashing (no more >> than 3 changes of tile size), while avoiding the more complicated logic that >> tries to keep the tiles all the same size >> at the cost of smaller tiles, and which has to fall back to using uneven >> tiles anyway. If you do it this way, there is >> also no need to have code that switches the order of the inner loop. It will >> naturally handle that. Either way, I'd >> like to see some additional system tests that cover all of the cases of X >> and Y fitting/not-fitting exactly (and if you >> stick with your current approach, X or Y as the inner loop). I left a >> couple inline comments as well. > >> [...] I'd like to see some additional system tests that cover all of the >> cases of X and Y fitting/not-fitting exactly >> (and if you stick with your current approach, X or Y as the inner loop). > > What kind of tests do you have in mind? More specifically do you mean simply > adding tests that expand on the existing > `doTestSnapshotScaleNodeDefer`and `doTestSnapshotScaleNodeImm` (which > basically just prove that taking a snapshot > returns a non-null image of the expected size)? Or do you think we need to > include a test that proves the snapshot > produced by tiling is entirely faithful to the original, pixel-wise? I went ahead and wrote a bunch of tests that: 1. Setup a scene to display an `ImageView` of a selected dimensions chosen to trigger tiling in different ways when taking snapshots. 2. Fill up the image with noise. 3. Take a snapshot and do a pixel-wise comparison with the original image. I've added the new tests to the existing `Snapshot2Test.java`. - PR: https://git.openjdk.java.net/jfx/pull/112
Re: [Rev 09] RFR: 8238954: Improve performance of tiled snapshot rendering
> Issue JDK-8088198, where an exception would be thrown when trying to capture > a snapshot whose final dimensions would be > larger than the running platform's maximum supported texture size, was > addressed in openjfx14. The fix, based around > the idea of capturing as many tiles of the maximum possible size and > re-compositing the final snapshot out of these, is > currently only attempted after the original, non-tiled, strategy has already > failed. This was decided to avoid any risk > of regressions, either in terms of performances and correctness, while still > offering some relief to the original > issue. This follow-on issue aims to propose a fix to the original issue, > that is able to correctly decide on the best > snapshot strategy (tiled or not) to adopt before applying it and ensure best > performances possible when tiling is > necessary while still introducing no regressions compared to the original > solution. Frederic Thevenet has updated the pull request incrementally with one additional commit since the last revision: Removed trailing whitespaces - Changes: - all: https://git.openjdk.java.net/jfx/pull/112/files - new: https://git.openjdk.java.net/jfx/pull/112/files/2951d538..82746316 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/112/webrev.09 - incr: https://webrevs.openjdk.java.net/jfx/112/webrev.08-09 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jfx/pull/112.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/112/head:pull/112 PR: https://git.openjdk.java.net/jfx/pull/112
Re: [Rev 03] RFR: 8193800: TreeTableView selection changes on sorting
On Mon, 15 Jun 2020 23:41:08 GMT, Kevin Rushforth wrote: >> Ambarish Rapte has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Correcting the selection change events generated post sorting > > modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java > line 472: > >> 471: countSelectedIndexChangeEvent++; >> 472: assertEquals(selectedItemBefore, >> treeTableView.getTreeItem(sm.getSelectedIndex())); >> 473: }); > > If this assertion ever fails, I don't think it will cause a test failure, > since the event handling code will swallow > the exception. The same is true of the other listeners. I don't know if it > would be possible to use the > UncaughtExceptionHandler as is done in other tests -- see > [JDK-8244531](https://bugs.openjdk.java.net/browse/JDK-8244531) -- but that > might be worth exploring. Another > possibility is to wrap all the listeners in a try/catch and keep a list of > `Throwable`s that are caught. Hi Kevin, I tested these listeners by adding assert that always fail. It shows the failures correctly. Also in this file there are other tests which assert inside a listener. I confirmed that those tests also show failures correctly. I could not find out what makes it work though. - PR: https://git.openjdk.java.net/jfx/pull/244
Re: [Rev 03] RFR: 8193800: TreeTableView selection changes on sorting
On Mon, 15 Jun 2020 23:26:33 GMT, Kevin Rushforth wrote:
>> Ambarish Rapte has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Correcting the selection change events generated post sorting
>
> modules/javafx.controls/src/main/java/javafx/scene/control/TreeTableView.java
> line 1815:
>
>> 1814: boolean isSortTreeOfSelectedItems() {
>> 1815: return sortTreeOfSelectedItems;
>> 1816: }
>
> This method / state attribute isn't really needed. The value is never set to
> anything other than `true`. I recommend
> removing it.
Corrected in the next commit..
> modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java
> line 431:
>
>> 430: for (int j = 0; j < FIRST_LEVEL_COUNT - 1; j++) {
>> 431: TreeItem tj = new TreeItem<>("" + i + j);
>> 432: tj.setExpanded(true);
>
> The tree item strings will not be unique. This won't affect whether the test
> passes or fails, since `TreeItem` does not
> override `equals`, but it might be better if the strings were unique (in case
> there ever was an error it would be
> easier to understand).
Hi Kevin, I verified the the tree item strings again. They seem to be unique. A
total of 8800 of tree items get created
each with unique string. Could you please recheck.
-
PR: https://git.openjdk.java.net/jfx/pull/244
Re: [Rev 04] RFR: 8193800: TreeTableView selection changes on sorting
> Issue: > In TreeTableView, in case of Multiple selection mode, if nested items are > selected, then TreeTableView does not > retain/update the selection correctly when the tree items are permuted(either > by `sort()` or by reordering using > `setAll()`). Cause: > > 1. For permutation, the current implementation uses `TreeModificationEvent` > to update the selection. > 2. The indices from these TreeModificationEvents are not reliable. > 3. It uses the non public `TreeTablePosition` constructor to create > intermediate `TreeItem` positions, this constructor > results in another unexpected TreeModificationEvent while one for sorting is > already being processed. 4. In case of > sorting, there can be multiple intermediate TreeModificationEvents generated, > and for each TreeModificationEvent, the > selection gets updated and results in selection change events being > generated. 5. Each time a TreeItem is expanded or > collapsed, the selection must be shifted, but shifting is not necessary in > case of permutation. All these issues > combine in wrong update of the selection. Fix: > > 1. On each TreeModificationEvent for permutation, for updating the selection, > use index of TreeItem from the > TreeTableView but not from the TreeModificationEvent. 2. Added a new non > public TreeTablePosition constructor, which is > almost a copy constructor but accepts a different row. 3. In case of sorting, > send out the set of selection change > events only once after the sorting is over. 4. In case of setAll, send out > the set of selection change events same as > before.(setAll results in only one TreeModificationEvent, which effectively > results in only one set of selection change > events). `shiftSelection()` should not be called in case of permutation i.e. > call `if (shift != 0)` > Verification: > The change is very limited to updating of selection of TreeTableView items > when the TreeItems are permuted, so the > change should not cause any other failures. Added unit tests which fail > before and pass after the fix. Ambarish Rapte has updated the pull request incrementally with one additional commit since the last revision: Remove the un-required flag - Changes: - all: https://git.openjdk.java.net/jfx/pull/244/files - new: https://git.openjdk.java.net/jfx/pull/244/files/e733a6c1..c70178cf Webrevs: - full: https://webrevs.openjdk.java.net/jfx/244/webrev.04 - incr: https://webrevs.openjdk.java.net/jfx/244/webrev.03-04 Stats: 6 lines in 1 file changed: 0 ins; 5 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/244.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/244/head:pull/244 PR: https://git.openjdk.java.net/jfx/pull/244
Re: [Rev 08] RFR: 8238954: Improve performance of tiled snapshot rendering
> Issue JDK-8088198, where an exception would be thrown when trying to capture > a snapshot whose final dimensions would be > larger than the running platform's maximum supported texture size, was > addressed in openjfx14. The fix, based around > the idea of capturing as many tiles of the maximum possible size and > re-compositing the final snapshot out of these, is > currently only attempted after the original, non-tiled, strategy has already > failed. This was decided to avoid any risk > of regressions, either in terms of performances and correctness, while still > offering some relief to the original > issue. This follow-on issue aims to propose a fix to the original issue, > that is able to correctly decide on the best > snapshot strategy (tiled or not) to adopt before applying it and ensure best > performances possible when tiling is > necessary while still introducing no regressions compared to the original > solution. Frederic Thevenet has updated the pull request incrementally with one additional commit since the last revision: Added tiled snapshots tests - Changes: - all: https://git.openjdk.java.net/jfx/pull/112/files - new: https://git.openjdk.java.net/jfx/pull/112/files/8f0c2d22..2951d538 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/112/webrev.08 - incr: https://webrevs.openjdk.java.net/jfx/112/webrev.07-08 Stats: 170 lines in 1 file changed: 168 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/112.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/112/head:pull/112 PR: https://git.openjdk.java.net/jfx/pull/112
Re: RFR: 8220484: JFXPanel renders a slanted image with a hidpi monitor scale of 125% or 175%
On Wed, 3 Jun 2020 15:46:36 GMT, Oliver Schmidtmer wrote: > In edge cases where monitor scaling of 1.25 or 1.75 is active, Math.ceil and > Math.round produce different results and > EmbeddedScene#getPixels in JFXPanel#paintComponent causes an off-by-one error > on the line width and therefore sheared > rendering. The changes were already proposed by the submitter in JBS-8220484. > > OCA is signed and send. In 2D, we normally use sun.java2d.pipe.Region.clipRound as it also checks for -ve/+ve max INTEGER but I guess that is internal class to FX so it's ok to use Math.ceil. Approval pending test creation. - PR: https://git.openjdk.java.net/jfx/pull/246
Re: 11-dev backport request: JDK-8247360: Add missing license file for Microsoft DirectShow Samples
approved On Wed, Jun 17, 2020 at 1:58 AM Kevin Rushforth wrote: > Hi Johan, > > I request approval to backport the following to 11-dev for 11.0.8: > > JDK-8247360 [1] : Add missing license file for Microsoft DirectShow Samples > > -- Kevin > > [1] https://bugs.openjdk.java.net/browse/JDK-8247360 > > >
