Re: RFR: 8282999: Add support for EXT-X-MEDIA tag in HTTP Live Streaming [v2]
On Fri, 26 Apr 2024 22:58:30 GMT, Alexander Matveev wrote: >> - Added support for #EXT-X-MEDIA tag to HTTP Live Streaming. >> - Following audio renditions via #EXT-X-MEDIA tag will be supported (see CSR >> for more details): >> - MP2T streams with one H.264/AVC video track and elementary AAC audio >> stream via #EXT-X-MEDIA tag. >> - fMP4 streams with one H.264/AVC or H.265/HEVC video track and elementary >> AAC audio stream via #EXT-X-MEDIA tag. >> - fMP4 streams with one H.264/AVC or H.265/HEVC video track and fMP4 >> streams with one AAC audio track via #EXT-X-MEDIA tag. >> - Separate audio stream will be playback via separate chain of GStreamer >> elements inside one pipeline. Which means two "javasource" elements will be >> used inside one pipeline and they will be reading data independently of each >> other via two separate HLSConnectionHolders. GStreamer will handle audio and >> video synchronization based on PTS as for other streams. Other solutions >> were considered such as one "javasource" with multiple source pads, but such >> implementation will be more complex and does not provide any benefits. >> - HLSConnectionHolder which handles video stream will also parse all >> information for separate audio stream and then create child >> HLSConnectionHolder for separate audio stream which will be responsible for >> downloading audio segments and seek of audio streams. >> - Parser in HLSConnectionHolder was reworked to make it more readable and >> easy to maintain and extend. >> - JavaDoc updated to point to latest HLS implementation vs old draft. It >> also updated with information on #EXT-X-MEDIA tag. Also, added missing >> information on AAC elementary streams and fMP4 from previous fixes. >> - Fixed and improved debug output in Linux AV plugins. >> - Added new property to "dshowwrapper" to disable PTS reset for each new >> segment, since with #EXT-X-MEDIA tag audio and video segments are not align >> and they can start at different time. >> - Fixed missing PTS on first buffer after seek in MP2T demuxer in >> "dshowwrapper". Without it audio and video synchronization breaks with two >> separate streams. >> - Removed dead code from MediaManager. >> - Added handling for GST_MESSAGE_LATENCY. Based on GStreamer doc we need to >> call gst_bin_recalculate_latency() when such message received. Not sure if >> we really need to do this, but with separate video and audio streams we do >> receive this message when seek is done. Most likely due to video and audio >> is not align perfectly when we seek. For other streams this message is not >> received in most cases. > > Alexander Matveev has updated the pull request with a new target base due to > a merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains two additional > commits since the last revision: > > - Merge remote-tracking branch 'upstream/master' into JDK-8282999 > - 8282999: Add for support EXT-X-MEDIA tag in HTTP Live Streaming 8282999: Add for support EXT-X-MEDIA tag in HTTP Live Streaming [v2] - Added missing header. I do not have Ubuntu 16.04 and this issue does not reproduce on Ubuntu 22.04. - PR Comment: https://git.openjdk.org/jfx/pull/1435#issuecomment-2080234887
Re: RFR: 8282999: Add support for EXT-X-MEDIA tag in HTTP Live Streaming [v3]
> - Added support for #EXT-X-MEDIA tag to HTTP Live Streaming. > - Following audio renditions via #EXT-X-MEDIA tag will be supported (see CSR > for more details): > - MP2T streams with one H.264/AVC video track and elementary AAC audio > stream via #EXT-X-MEDIA tag. > - fMP4 streams with one H.264/AVC or H.265/HEVC video track and elementary > AAC audio stream via #EXT-X-MEDIA tag. > - fMP4 streams with one H.264/AVC or H.265/HEVC video track and fMP4 > streams with one AAC audio track via #EXT-X-MEDIA tag. > - Separate audio stream will be playback via separate chain of GStreamer > elements inside one pipeline. Which means two "javasource" elements will be > used inside one pipeline and they will be reading data independently of each > other via two separate HLSConnectionHolders. GStreamer will handle audio and > video synchronization based on PTS as for other streams. Other solutions were > considered such as one "javasource" with multiple source pads, but such > implementation will be more complex and does not provide any benefits. > - HLSConnectionHolder which handles video stream will also parse all > information for separate audio stream and then create child > HLSConnectionHolder for separate audio stream which will be responsible for > downloading audio segments and seek of audio streams. > - Parser in HLSConnectionHolder was reworked to make it more readable and > easy to maintain and extend. > - JavaDoc updated to point to latest HLS implementation vs old draft. It also > updated with information on #EXT-X-MEDIA tag. Also, added missing information > on AAC elementary streams and fMP4 from previous fixes. > - Fixed and improved debug output in Linux AV plugins. > - Added new property to "dshowwrapper" to disable PTS reset for each new > segment, since with #EXT-X-MEDIA tag audio and video segments are not align > and they can start at different time. > - Fixed missing PTS on first buffer after seek in MP2T demuxer in > "dshowwrapper". Without it audio and video synchronization breaks with two > separate streams. > - Removed dead code from MediaManager. > - Added handling for GST_MESSAGE_LATENCY. Based on GStreamer doc we need to > call gst_bin_recalculate_latency() when such message received. Not sure if we > really need to do this, but with separate video and audio streams we do > receive this message when seek is done. Most likely due to video and audio is > not align perfectly when we seek. For other streams this message is not > received in most cases. Alexander Matveev has updated the pull request incrementally with one additional commit since the last revision: 8282999: Add for support EXT-X-MEDIA tag in HTTP Live Streaming [v2] - Changes: - all: https://git.openjdk.org/jfx/pull/1435/files - new: https://git.openjdk.org/jfx/pull/1435/files/0187b577..84103bd4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1435&range=02 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1435&range=01-02 Stats: 4 lines in 2 files changed: 4 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/1435.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1435/head:pull/1435 PR: https://git.openjdk.org/jfx/pull/1435
Re: RFR: 8330462: StringIndexOutOfBoundException when typing anything into TextField [v24]
On Fri, 26 Apr 2024 22:58:37 GMT, Oliver Kopp wrote: >> Fixes https://bugs.openjdk.org/browse/JDK-8330462. >> >> If the parameter `maxLength` is larger than `Integer.MAX_VALUE - start`, >> then an addition of `start` to it leads to a negative value. This is "fixed" >> by using `Math.max` comparing the `maxLength` and `maxLength + start`. > > Oliver Kopp has updated the pull request incrementally with one additional > commit since the last revision: > > Revert using utility method Regression seems to be fixed. No differences in behavior doing a quick test with with TextField/TextArea and Narrator on Windows 11. - Marked as reviewed by angorya (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/1442#pullrequestreview-2025994582
Re: RFR: 8330462: StringIndexOutOfBoundException when typing anything into TextField [v23]
On Fri, 26 Apr 2024 09:57:34 GMT, Ambarish Rapte wrote: >> Oliver Kopp has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Make use of Utils.clamp function > > Here is a capture from my machine. > I have two copies of source code: One with fix and other without fix. > Please do unmute the video playback, you can listen to Narrator voice output. > > https://github.com/openjdk/jfx/assets/11330676/bd3efcae-c661-4906-8388-fcb373e7e5c7 @arapte OK, I have one more idea: The usage of the utility method in the non-issue related methods. I reverted the change at that place. Could you please recheck if the Narrator works on Windows 11? (I am still unable to compile JavaFX. Now, it's `stdio.h` missing) - PR Comment: https://git.openjdk.org/jfx/pull/1442#issuecomment-2080210927
Re: RFR: 8330462: StringIndexOutOfBoundException when typing anything into TextField [v24]
> Fixes https://bugs.openjdk.org/browse/JDK-8330462. > > If the parameter `maxLength` is larger than `Integer.MAX_VALUE - start`, then > an addition of `start` to it leads to a negative value. This is "fixed" by > using `Math.max` comparing the `maxLength` and `maxLength + start`. Oliver Kopp has updated the pull request incrementally with one additional commit since the last revision: Revert using utility method - Changes: - all: https://git.openjdk.org/jfx/pull/1442/files - new: https://git.openjdk.org/jfx/pull/1442/files/968e3f05..92155148 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1442&range=23 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1442&range=22-23 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jfx/pull/1442.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1442/head:pull/1442 PR: https://git.openjdk.org/jfx/pull/1442
Re: RFR: 8282999: Add support for EXT-X-MEDIA tag in HTTP Live Streaming [v2]
> - Added support for #EXT-X-MEDIA tag to HTTP Live Streaming. > - Following audio renditions via #EXT-X-MEDIA tag will be supported (see CSR > for more details): > - MP2T streams with one H.264/AVC video track and elementary AAC audio > stream via #EXT-X-MEDIA tag. > - fMP4 streams with one H.264/AVC or H.265/HEVC video track and elementary > AAC audio stream via #EXT-X-MEDIA tag. > - fMP4 streams with one H.264/AVC or H.265/HEVC video track and fMP4 > streams with one AAC audio track via #EXT-X-MEDIA tag. > - Separate audio stream will be playback via separate chain of GStreamer > elements inside one pipeline. Which means two "javasource" elements will be > used inside one pipeline and they will be reading data independently of each > other via two separate HLSConnectionHolders. GStreamer will handle audio and > video synchronization based on PTS as for other streams. Other solutions were > considered such as one "javasource" with multiple source pads, but such > implementation will be more complex and does not provide any benefits. > - HLSConnectionHolder which handles video stream will also parse all > information for separate audio stream and then create child > HLSConnectionHolder for separate audio stream which will be responsible for > downloading audio segments and seek of audio streams. > - Parser in HLSConnectionHolder was reworked to make it more readable and > easy to maintain and extend. > - JavaDoc updated to point to latest HLS implementation vs old draft. It also > updated with information on #EXT-X-MEDIA tag. Also, added missing information > on AAC elementary streams and fMP4 from previous fixes. > - Fixed and improved debug output in Linux AV plugins. > - Added new property to "dshowwrapper" to disable PTS reset for each new > segment, since with #EXT-X-MEDIA tag audio and video segments are not align > and they can start at different time. > - Fixed missing PTS on first buffer after seek in MP2T demuxer in > "dshowwrapper". Without it audio and video synchronization breaks with two > separate streams. > - Removed dead code from MediaManager. > - Added handling for GST_MESSAGE_LATENCY. Based on GStreamer doc we need to > call gst_bin_recalculate_latency() when such message received. Not sure if we > really need to do this, but with separate video and audio streams we do > receive this message when seek is done. Most likely due to video and audio is > not align perfectly when we seek. For other streams this message is not > received in most cases. Alexander Matveev has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains two additional commits since the last revision: - Merge remote-tracking branch 'upstream/master' into JDK-8282999 - 8282999: Add for support EXT-X-MEDIA tag in HTTP Live Streaming - Changes: - all: https://git.openjdk.org/jfx/pull/1435/files - new: https://git.openjdk.org/jfx/pull/1435/files/c4ac5e9e..0187b577 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1435&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1435&range=00-01 Stats: 5035 lines in 351 files changed: 3411 ins; 991 del; 633 mod Patch: https://git.openjdk.org/jfx/pull/1435.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1435/head:pull/1435 PR: https://git.openjdk.org/jfx/pull/1435
Re: RFR: 8330462: StringIndexOutOfBoundException when typing anything into TextField [v23]
On Fri, 26 Apr 2024 09:57:34 GMT, Ambarish Rapte wrote: >> Oliver Kopp has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Make use of Utils.clamp function > > Here is a capture from my machine. > I have two copies of source code: One with fix and other without fix. > Please do unmute the video playback, you can listen to Narrator voice output. > > https://github.com/openjdk/jfx/assets/11330676/bd3efcae-c661-4906-8388-fcb373e7e5c7 @arapte Please share your concrete Windows setup with me. I cannot reproduce here. I bet, you are on Windows 11. Reason: Here on my Windows 10 machine (and on @calixtus and on @siedlerchr's machine), the Narrator does not work, but the Narrator works on Windows 11. (from `master` branch) Currently, I can only move on forward blindly. I hope, I find someone with a Windows 11 machine willing to try out the `base` branch of my fork and help me with the values... https://github.com/koppor/jfx/pull/4 --- I could get JFX to get compile on Windows 10, but I have issues to get it to run on Windows 11. You know, this things "java.util.concurrent.ExecutionException: org.gradle.process.internal.ExecException: A problem occurred starting process 'command '/VC/BIN/amd64/cl.exe'' and `rc.exe` not being found etc. (Yes, I followed the instructions, but the binaries are in `Hostx64/x64` instead of `x64`; some tools are only in `Windows Kits\10`. The installation instructions tell about Visual Studio 2022, but the example is windows_tools.properties is for 2019 (https://wiki.openjdk.org/display/OpenJFX/Building+OpenJFX). - PR Comment: https://git.openjdk.org/jfx/pull/1442#issuecomment-2080118102
Re: RFR: 8242553: IntegerSpinner and DoubleSpinner do not wrap around values correctly in some cases [v2]
On Wed, 10 Apr 2024 11:47:28 GMT, drmarmac wrote: >> This PR should fix the issue and cover all relevant cases with new tests. >> >> Note: This involves a small behavior change, as can be seen in >> dblSpinner_testWrapAround_decrement_twoSteps() in SpinnerTest.java:749. With >> this change the wraparound behavior is similar to that of the IntegerSpinner. > > drmarmac has updated the pull request incrementally with one additional > commit since the last revision: > > Use direction-dependent modulo arithmetic in DoubleSpinnerValueFactory > wrap-around logic Created https://bugs.openjdk.org/browse/JDK-8331214 @drmarmac would you like to update the javadoc or provide the description of the new behavior? - PR Comment: https://git.openjdk.org/jfx/pull/1431#issuecomment-2080043462
Re: RFR: 8314215: Trailing Spaces before Line Breaks Affect the Center Alignment of Text [v18]
On Tue, 23 Apr 2024 19:30:49 GMT, John Hendrikx wrote: >> There are a number of tickets open related to text rendering: >> >> https://bugs.openjdk.org/browse/JDK-8314215 >> >> https://bugs.openjdk.org/browse/JDK-8145496 >> >> https://bugs.openjdk.org/browse/JDK-8129014 >> >> They have in common that wrapped text is taking the trailing spaces on each >> wrapped line into account when calculating where to wrap. This looks okay >> for text that is left aligned (as the spaces will be trailing the lines and >> generally aren't a problem, but looks weird with CENTER and RIGHT >> alignments. Even with LEFT alignment there are artifacts of this behavior, >> where a line like `AAA BBB CCC` (note the **double** spaces) gets split up >> into `AAA `, `BBB ` and `CCC`, but if space reduces further, it will wrap >> **too** early because the space is taken into account (ie. `AAA` may still >> have fit just fine, but `AAA ` doesn't, so the engine wraps it to `AA` + `A >> ` or something). >> >> The fix for this is two fold; first the individual lines of text should not >> include any trailing spaces into their widths; second, the code that is >> taking the trailing space into account when wrapping should ignore all >> trailing spaces (currently it is ignoring all but one trailing space). With >> these two fixes, the layout in LEFT/CENTER/RIGHT alignments all look great, >> and there is no more early wrapping due to a space being taking into account >> while the actual text still would have fit (this is annoying in tight >> layouts, where a line can be wrapped early even though it looks like it >> would have fit). >> >> If it were that simple, we'd be done, but there may be another issue here >> that needs solving: wrapped aligned TextArea's. >> >> TextArea don't directly support text alignment (via a setTextAlignment >> method like Label) but you can change it via CSS. >> >> For Left alignment + wrapping, TextArea will ignore any spaces typed before >> a line that was wrapped. In other words, you can type spaces as much as you >> want, and they won't show up and the cursor won't move. The spaces are all >> getting appended to the previous line. When you cursor through these >> spaces, the cursor can be rendered out of the control's bounds. To >> illustrate, if you have the text `AAA BBB CCC`, and the text >> gets wrapped to `AAA`, `BBB`, `CCC`, typing spaces before `BBB` will not >> show up. If you cursor back, the cursor may be outside the control bounds >> because so many spaces are trailing `AAA`. >> >> The above behavior has NOT changed, is pretty standard for wrapped text >> controls,... > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Update copyright years The newly added test passes on all platforms. @hjohn I assigned [JDK-8145496](https://bugs.openjdk.org/browse/JDK-8145496) to you to verify that it is fixed by this PR. If so, please close it as a duplicate of this bug. Since [JDK-8129014](https://bugs.openjdk.org/browse/JDK-8129014) is an old (JDK 7) issue, let's just leave it alone. - PR Comment: https://git.openjdk.org/jfx/pull/1236#issuecomment-2080012431
Re: RFR: 8322619: Parts of SG no longer update during rendering - overlapping - culling - dirty [v3]
On Tue, 23 Jan 2024 11:51:52 GMT, Florian Kirmaier wrote: >> In some situations, a part of the SG is no longer rendered. >> I created a test program that showcases this problem. >> >> Explanation: >> >> This can happen, when a part of the SG, is covered by another Node. >> In this part, one node is totally covered, and the other node is visible. >> >> When the totally covered Node is changed, then it is marked dirty and it's >> parent, recursively until an already dirty node is found. >> Due to the Culling, this totally covered Node is not rendered - with the >> effect that the tree is never marked as Clean. >> >> In this state, a Node is Dirty but not It's parent. Based on my CodeReview, >> this is an invalid state which should never happen. >> >> In this invalid state, when the other Node is changed, which is visible, >> then the dirty state is no longer propagated upwards - because the recursive >> "NGNode.markTreeDirty" algorithm encounters a dirty node early. >> >> This has the effect, that any SG changes in the visible Node are no longer >> rendered. Sometimes the situation repairs itself. >> >> Useful parameters for further investigations: >> -Djavafx.pulseLogger=true >> -Dprism.printrendergraph=true >> -Djavafx.pulseLogger.threshold=0 >> >> PR: >> This PR ensures the dirty flag is set to false of the tree when the culling >> is used. >> It doesn't seem to break any existing tests - but I'm not sure whether this >> is the right way to fix it. >> It would be great to have some feedback on this solution - maybe guiding me >> to a better solution. >> >> I could write a test, that just does the same thing as the test application, >> but checks every frame that these nodes are not dirty - but maybe there is a >> better way to test this. > > Florian Kirmaier has updated the pull request incrementally with one > additional commit since the last revision: > > reverted accidental change in the .idea folder I analysed the bug and can confirm the fix. I summarized it in the following table. First column is the node structure, the others the steps: update nodes, render, update nodes, render, ... | Nodes (L - left and R - right) / Steps| 1st -> update Circles | 2nd -> render | 3rd -> update Lines | 4th -> render | |-|-|-|-|-| |HBox |dirty |clearDirty |dirty |clearDirty |HBox >StackPane-L |dirty |clearDirty |dirty |clearDirty |HBox >StackPane-L > Group-L|dirty |clearDirty |dirty |clearDirty |HBox >StackPane-L > Group-L >Line-L| | |dirty |clearDirty |HBox >StackPane-L > Group-L >Circle-L |dirty |clearDirty | |HBox >StackPane-R |dirty |clearDirty *1 | | *3 |HBox >StackPane-R > Group-R|dirty |dirty |dirty |HBox >StackPane-R > Group-R > Line-R | | |dirty *2 |dirty |HBox >StackPane-R > Group-R > Circle-R |dirty |dirty |dirty |dirty *1: > no culling bits for first dirty region (Circle-L) -> skip children -> clearDirty() is not called on children > no root path for the second dirty region (Circle-R) -> no rendering and root.clearDirtyTree() stops on StackPane-R (the children stay dirty: Group-R and Circle-R) *2: > Line-R.markTreeDirty() skips marking of further parents because Group-R is already dirty -> the StackPane-R stays clean *3: > the dirty regions are not collected because the StackPane-R is clean -> Line-R is not rendered and the dirty-flag is not reset --- The error is that ``clearDirty()`` is not called in the second step on the children of StackPane-R (see *1). Subsequent changes in Line-R and Circle-R are not propagated to the root node because Group-R is already dirty (see *2) and therefore StackPane-R remains clean. The last changes are therefore ignored in the next rendering phase (see *3). --- We can simply say: **the dirty flags must be deleted on all nodes after rendering**. If this is true, the test can simply check this instead of screenshots or similar. - PR Comment: https://git.openjdk.org/jfx/pull/1310#issuecomment-2079676517
Re: RFR: 8314215: Trailing Spaces before Line Breaks Affect the Center Alignment of Text [v18]
On Wed, 24 Apr 2024 15:23:39 GMT, Kevin Rushforth wrote: >> John Hendrikx has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Update copyright years > > One meta comment (not directly related to this review). > >> > since it's been modified, please change the year >> >> Sorry, I stopped doing that, I thought this was automated. > > It is optional to update copyrights in a PR for a modified file (new files do > need a correctly formatted copyright file with the right year). Ambarish > periodically runs a script to update them. If you do update copyrights, you > need to make sure that the year is right (if, for example, the PR was started > in one year and finished the next yet). I tend to not update them for PRs > that I expect to backport, since it increases the likelihood of not being > able to use the `/backport` command. > @kevinrushforth This is ready to integrate, but I know the headful tests > don't run automatically. Would it be wise to run these once more before > integrating? Yes, that would be a good idea. I'll do that and report results here. > Also, I re-read the issues https://bugs.openjdk.org/browse/JDK-8145496 and > https://bugs.openjdk.org/browse/JDK-8129014, and I'm positive they're > duplicates that would be fixed by this solution. I think it's better to close JDK-8145496 as a duplicate. As for JDK-8129014, it is already closed (as not an issue), so we could either leave it alone or reopen it and reclose it as a Duplicate. - PR Comment: https://git.openjdk.org/jfx/pull/1236#issuecomment-2079670636
Re: RFR: 8330462: StringIndexOutOfBoundException when typing anything into TextField [v23]
On Thu, 25 Apr 2024 21:41:14 GMT, Oliver Kopp wrote: >> Fixes https://bugs.openjdk.org/browse/JDK-8330462. >> >> If the parameter `maxLength` is larger than `Integer.MAX_VALUE - start`, >> then an addition of `start` to it leads to a negative value. This is "fixed" >> by using `Math.max` comparing the `maxLength` and `maxLength + start`. > > Oliver Kopp has updated the pull request incrementally with one additional > commit since the last revision: > > Make use of Utils.clamp function Sure thing! I will work on that! And I will forget about the TextField. TextArea FTW! - PR Comment: https://git.openjdk.org/jfx/pull/1442#issuecomment-2079659598
Re: RFR: 8330462: StringIndexOutOfBoundException when typing anything into TextField [v23]
On Fri, 26 Apr 2024 09:57:34 GMT, Ambarish Rapte wrote: > Here is a capture from my machine. > I have two copies of source code: One with fix and other without fix. > Please do unmute the video playback, you can listen to Narrator voice output. @arapte Thank you for doing this and attaching it. This matches what I see and is, I think, what Andy is seeing as well. @koppor This clearly shows that the current version of the fix in this PR introduces a regression. You will need to fix it before we can continue the review. While testing and debugging this problem, use a manual program rather than a Robot-based automated test. Speaking of tests, we cannot reliably test a11y using automated tests, so I wouldn't spend any more time trying to get your new `TextAreaTest_Narrator_8330462` test working. The automated test in this PR that verifies the correctness of the new `getEndIndex` method will have to do (along with manual testing of the a11y functionality). Once you figure out what the problem is with the `getEndIndex` method, you might want to add additional cases to your test. - PR Comment: https://git.openjdk.org/jfx/pull/1442#issuecomment-2079640408
Re: RFR: 8314215: Trailing Spaces before Line Breaks Affect the Center Alignment of Text [v18]
On Wed, 24 Apr 2024 15:23:39 GMT, Kevin Rushforth wrote: >> John Hendrikx has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Update copyright years > > One meta comment (not directly related to this review). > >> > since it's been modified, please change the year >> >> Sorry, I stopped doing that, I thought this was automated. > > It is optional to update copyrights in a PR for a modified file (new files do > need a correctly formatted copyright file with the right year). Ambarish > periodically runs a script to update them. If you do update copyrights, you > need to make sure that the year is right (if, for example, the PR was started > in one year and finished the next yet). I tend to not update them for PRs > that I expect to backport, since it increases the likelihood of not being > able to use the `/backport` command. @kevinrushforth This is ready to integrate, but I know the headful tests don't run automatically. Would it be wise to run these once more before integrating? Also, I re-read the issues https://bugs.openjdk.org/browse/JDK-8145496 and https://bugs.openjdk.org/browse/JDK-8129014, and I'm positive they're duplicates that would be fixed by this solution. Shall I add these to this PR? - PR Comment: https://git.openjdk.org/jfx/pull/1236#issuecomment-2079108947
Re: RFR: 8320563: Remove D3D9 code paths in favor of D3D9Ex [v3]
On Fri, 26 Apr 2024 07:53:59 GMT, Lukasz Kostyra wrote: >> JFX minimum requirements guarantee 9Ex availability, so old non-Ex paths are >> no longer needed. >> >> In multiple parts (ex. Mesh, Graphics, etc.) where the Device is acquired I >> changed the type to explicitly use `IDirect3DDevice9Ex`. Technically it >> doesn't matter much (`IDirect3DDevice9Ex` inherits `IDirect3DDevice` - it >> was leveraged to transparently use the Ex device in the backend) but now we >> don't have the non-Ex device, so that keeps it a bit more consistent and >> clear IMO. >> >> Verified by running tests on Windows 11, did not notice any regressions. >> Unfortunately I have no way to test this on older systems. > > Lukasz Kostyra has updated the pull request incrementally with one additional > commit since the last revision: > > Change pd3dEx to pd3d9 LGTM - Marked as reviewed by mstrauss (Committer). PR Review: https://git.openjdk.org/jfx/pull/1445#pullrequestreview-2024655439
Re: RFR: 8330462: StringIndexOutOfBoundException when typing anything into TextField [v23]
On Thu, 25 Apr 2024 21:41:14 GMT, Oliver Kopp wrote: >> Fixes https://bugs.openjdk.org/browse/JDK-8330462. >> >> If the parameter `maxLength` is larger than `Integer.MAX_VALUE - start`, >> then an addition of `start` to it leads to a negative value. This is "fixed" >> by using `Math.max` comparing the `maxLength` and `maxLength + start`. > > Oliver Kopp has updated the pull request incrementally with one additional > commit since the last revision: > > Make use of Utils.clamp function Here is a capture from my machine. I have two copies of source code: One with fix and other without fix. Please do unmute the video playback, you can listen to Narrator voice output. https://github.com/openjdk/jfx/assets/11330676/bd3efcae-c661-4906-8388-fcb373e7e5c7 - PR Comment: https://git.openjdk.org/jfx/pull/1442#issuecomment-2079046989
Integrated: 8273657 : TextField: all text content must be selected initially
On Thu, 25 Apr 2024 14:26:06 GMT, Karthik P K wrote: > The text was not getting selected on adding the `TextField` to the scene > initially, subsequently removing and adding the `TextField` to the scene > selects the entire text present in the `TextField`. > > Made changes in the `TextFieldBehavior` constructor to select the text on > adding the `TextField`. > > Added unit test to validate the fix This pull request has now been integrated. Changeset: c23ac747 Author:Karthik P K URL: https://git.openjdk.org/jfx/commit/c23ac74714a88649a65d455a06254e97cd6ecb3f Stats: 25 lines in 2 files changed: 25 ins; 0 del; 0 mod 8273657: TextField: all text content must be selected initially Reviewed-by: angorya - PR: https://git.openjdk.org/jfx/pull/1446
Re: RFR: 8320563: Remove D3D9 code paths in favor of D3D9Ex [v3]
On Fri, 26 Apr 2024 07:53:59 GMT, Lukasz Kostyra wrote: >> JFX minimum requirements guarantee 9Ex availability, so old non-Ex paths are >> no longer needed. >> >> In multiple parts (ex. Mesh, Graphics, etc.) where the Device is acquired I >> changed the type to explicitly use `IDirect3DDevice9Ex`. Technically it >> doesn't matter much (`IDirect3DDevice9Ex` inherits `IDirect3DDevice` - it >> was leveraged to transparently use the Ex device in the backend) but now we >> don't have the non-Ex device, so that keeps it a bit more consistent and >> clear IMO. >> >> Verified by running tests on Windows 11, did not notice any regressions. >> Unfortunately I have no way to test this on older systems. > > Lukasz Kostyra has updated the pull request incrementally with one additional > commit since the last revision: > > Change pd3dEx to pd3d9 Marked as reviewed by nlisker (Reviewer). - PR Review: https://git.openjdk.org/jfx/pull/1445#pullrequestreview-2024451797
Re: RFR: 8320563: Remove D3D9 code paths in favor of D3D9Ex [v3]
> JFX minimum requirements guarantee 9Ex availability, so old non-Ex paths are > no longer needed. > > In multiple parts (ex. Mesh, Graphics, etc.) where the Device is acquired I > changed the type to explicitly use `IDirect3DDevice9Ex`. Technically it > doesn't matter much (`IDirect3DDevice9Ex` inherits `IDirect3DDevice` - it was > leveraged to transparently use the Ex device in the backend) but now we don't > have the non-Ex device, so that keeps it a bit more consistent and clear IMO. > > Verified by running tests on Windows 11, did not notice any regressions. > Unfortunately I have no way to test this on older systems. Lukasz Kostyra has updated the pull request incrementally with one additional commit since the last revision: Change pd3dEx to pd3d9 - Changes: - all: https://git.openjdk.org/jfx/pull/1445/files - new: https://git.openjdk.org/jfx/pull/1445/files/4a9605fc..d915c19f Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1445&range=02 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1445&range=01-02 Stats: 4 lines in 2 files changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jfx/pull/1445.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1445/head:pull/1445 PR: https://git.openjdk.org/jfx/pull/1445
Re: RFR: 8320563: Remove D3D9 code paths in favor of D3D9Ex [v2]
> JFX minimum requirements guarantee 9Ex availability, so old non-Ex paths are > no longer needed. > > In multiple parts (ex. Mesh, Graphics, etc.) where the Device is acquired I > changed the type to explicitly use `IDirect3DDevice9Ex`. Technically it > doesn't matter much (`IDirect3DDevice9Ex` inherits `IDirect3DDevice` - it was > leveraged to transparently use the Ex device in the backend) but now we don't > have the non-Ex device, so that keeps it a bit more consistent and clear IMO. > > Verified by running tests on Windows 11, did not notice any regressions. > Unfortunately I have no way to test this on older systems. Lukasz Kostyra has updated the pull request incrementally with two additional commits since the last revision: - Cleanup leftover "Ex" suffixes in variable names - Remove leftover disableD3D9Ex Prism setting - Changes: - all: https://git.openjdk.org/jfx/pull/1445/files - new: https://git.openjdk.org/jfx/pull/1445/files/2dbed83d..4a9605fc Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1445&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1445&range=00-01 Stats: 7 lines in 3 files changed: 0 ins; 3 del; 4 mod Patch: https://git.openjdk.org/jfx/pull/1445.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1445/head:pull/1445 PR: https://git.openjdk.org/jfx/pull/1445
Re: RFR: 8320563: Remove D3D9 code paths in favor of D3D9Ex
On Thu, 25 Apr 2024 15:20:54 GMT, Nir Lisker wrote:
>> JFX minimum requirements guarantee 9Ex availability, so old non-Ex paths are
>> no longer needed.
>>
>> In multiple parts (ex. Mesh, Graphics, etc.) where the Device is acquired I
>> changed the type to explicitly use `IDirect3DDevice9Ex`. Technically it
>> doesn't matter much (`IDirect3DDevice9Ex` inherits `IDirect3DDevice` - it
>> was leveraged to transparently use the Ex device in the backend) but now we
>> don't have the non-Ex device, so that keeps it a bit more consistent and
>> clear IMO.
>>
>> Verified by running tests on Windows 11, did not notice any regressions.
>> Unfortunately I have no way to test this on older systems.
>
> modules/javafx.graphics/src/main/native-prism-d3d/D3DPipeline.cc line 237:
>
>> 235: }
>> 236:
>> 237: int getMaxSampleSupport(IDirect3D9Ex *d3d9, UINT adapter) {
>
> Minor: In some cases you also change the name of the variable to add the "Ex"
> suffix., like in
>
> D3DContext::D3DContext(IDirect3D9Ex *pd3dEx, UINT adapter)
> ^
>
> Here and In `PiplineManager.h` it's left as `IDirect3D9Ex * pd3d9;` without
> "Ex".
> I don't mind it either way (I would probably not bother changing them
> myself), but perhaps we should remain consistent.
You're right. Most places use it as `pd3d9` so I'll leave it this way (remove
the `Ex` suffix from other spots).
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1445#discussion_r1580608891
Re: RFR: 8320563: Remove D3D9 code paths in favor of D3D9Ex
On Tue, 23 Apr 2024 10:33:58 GMT, Lukasz Kostyra wrote: > JFX minimum requirements guarantee 9Ex availability, so old non-Ex paths are > no longer needed. > > In multiple parts (ex. Mesh, Graphics, etc.) where the Device is acquired I > changed the type to explicitly use `IDirect3DDevice9Ex`. Technically it > doesn't matter much (`IDirect3DDevice9Ex` inherits `IDirect3DDevice` - it was > leveraged to transparently use the Ex device in the backend) but now we don't > have the non-Ex device, so that keeps it a bit more consistent and clear IMO. > > Verified by running tests on Windows 11, did not notice any regressions. > Unfortunately I have no way to test this on older systems. I also noticed there's a property in Prism settings called `disableD3D9Ex` - I will also remove it as it is now redundant. - PR Comment: https://git.openjdk.org/jfx/pull/1445#issuecomment-2078809190
Re: RFR: 8330462: StringIndexOutOfBoundException when typing anything into TextField [v23]
On Thu, 25 Apr 2024 21:41:14 GMT, Oliver Kopp wrote: >> Fixes https://bugs.openjdk.org/browse/JDK-8330462. >> >> If the parameter `maxLength` is larger than `Integer.MAX_VALUE - start`, >> then an addition of `start` to it leads to a negative value. This is "fixed" >> by using `Math.max` comparing the `maxLength` and `maxLength + start`. > > Oliver Kopp has updated the pull request incrementally with one additional > commit since the last revision: > > Make use of Utils.clamp function On branch `main` `rm -rf build && ./gradlew sdk && ./gradlew --info -PFULL_TEST=true systemTests:test --tests "test.com.sun.glass.ui.win.TextAreaTest_Narrator_8330462" || ./gradlew --info -PFULL_TEST=true -PTEST_ONLY=true systemTests:test --tests "test.com.sun.glass.ui.win.TextAreaTest_Narrator_8330462"` With [Carnac](https://github.com/Code52/carnac/) and [ShareX](https://getsharex.com/) I tried to capture what happens on **`main`**. Showing especially interactions with Space and Cursor-left. https://github.com/openjdk/jfx/assets/1366654/f25bb801-565c-4d50-9471-c4391962049d - PR Comment: https://git.openjdk.org/jfx/pull/1442#issuecomment-2078773098
