Re: RFR: 8212034: Potential memory leaks in jpegLoader.c in error case
On Wed, 27 Nov 2019 11:58:18 GMT, Ambarish Rapte wrote: > Memory allocated in initDecompressor() and decompressIndirect() is not freed > in error case. > In error case, > 1. Allocated memory should be freed. > 2. Appropriate de-initialization jpeg library calls should be added. > > Verified that, > 1. All unit and systems tests pass on three platforms, and > 2. Memory consumption with and without fix is similar by comparing memory > before and after showing 10 jpeg images for 100 times. > > > > Commits: > - 7af932b7: 8212034: Memory leaks in jpegLoader.c in error case > > Changes: https://git.openjdk.java.net/jfx/pull/54/files > Webrev: https://webrevs.openjdk.java.net/jfx/54/webrev.00 > Issue: https://bugs.openjdk.java.net/browse/JDK-8212034 > Stats: 62 lines in 1 file changed: 36 ins; 14 del; 12 mod > Patch: https://git.openjdk.java.net/jfx/pull/54.diff > Fetch: git fetch https://git.openjdk.java.net/jfx pull/54/head:pull/54 In general, this makes sense. I need to look into more detail that the additional calls for freeing resources (in case of errors) don't cause e.g. segmentation violations and lead to a crash -- which would be worse than throwing an Exception. I expect memory consumption to be similar before and after this patch if you don't run into errors, but did you check memory consumption before/after this patch in case of errors? PR: https://git.openjdk.java.net/jfx/pull/54
Re: RFR: 8212034: Potential memory leaks in jpegLoader.c in error case
On Wed, 27 Nov 2019 11:58:18 GMT, Ambarish Rapte wrote: > Memory allocated in initDecompressor() and decompressIndirect() is not freed > in error case. > In error case, > 1. Allocated memory should be freed. > 2. Appropriate de-initialization jpeg library calls should be added. > > Verified that, > 1. All unit and systems tests pass on three platforms, and > 2. Memory consumption with and without fix is similar by comparing memory > before and after showing 10 jpeg images for 100 times. > > > > Commits: > - 7af932b7: 8212034: Memory leaks in jpegLoader.c in error case > > Changes: https://git.openjdk.java.net/jfx/pull/54/files > Webrev: https://webrevs.openjdk.java.net/jfx/54/webrev.00 > Issue: https://bugs.openjdk.java.net/browse/JDK-8212034 > Stats: 62 lines in 1 file changed: 36 ins; 14 del; 12 mod > Patch: https://git.openjdk.java.net/jfx/pull/54.diff > Fetch: git fetch https://git.openjdk.java.net/jfx pull/54/head:pull/54 modules/javafx.graphics/src/main/native-iio/jpegloader.c line 1345: > 1344: free(cinfo->err); > 1345: free(cinfo); > 1346: ThrowByName(env, "java/io/IOException", buffer); jerr_mgr is also allocated via malloc, but not freed. Do you want to free that too? PR: https://git.openjdk.java.net/jfx/pull/54
Re: RFR: 8212034: Potential memory leaks in jpegLoader.c in error case
On Wed, 27 Nov 2019 11:58:18 GMT, Ambarish Rapte wrote: > Memory allocated in initDecompressor() and decompressIndirect() is not freed > in error case. > In error case, > 1. Allocated memory should be freed. > 2. Appropriate de-initialization jpeg library calls should be added. > > Verified that, > 1. All unit and systems tests pass on three platforms, and > 2. Memory consumption with and without fix is similar by comparing memory > before and after showing 10 jpeg images for 100 times. > > > > Commits: > - 7af932b7: 8212034: Memory leaks in jpegLoader.c in error case > > Changes: https://git.openjdk.java.net/jfx/pull/54/files > Webrev: https://webrevs.openjdk.java.net/jfx/54/webrev.00 > Issue: https://bugs.openjdk.java.net/browse/JDK-8212034 > Stats: 62 lines in 1 file changed: 36 ins; 14 del; 12 mod > Patch: https://git.openjdk.java.net/jfx/pull/54.diff > Fetch: git fetch https://git.openjdk.java.net/jfx pull/54/head:pull/54 This will need two reviewers. PR: https://git.openjdk.java.net/jfx/pull/54
Re: RFR: 8234916: [macos 10.15] Garbled text running with native-image
On Wed, 27 Nov 2019 17:06:13 GMT, Jose Pereda wrote: > Running on MacOS Catalina, when doing static builds of `libjavafx_font.a` and > linking against this in a JavaFX app compiled with GraalVM native-image, if > default fonts are used, the rendered text is garbled, and a warning message > is printed: > > CoreText note: Client requested name ".SFUI-Regular", it will get > TimesNewRomanPSMT rather than the intended font. All system UI font access > should be through proper APIs such as CTFontCreateUIFontForLanguage() or > +[UIFont systemFontOfSize:]. > CoreText note: Set a breakpoint on CTFontLogSystemFontNameRequest to debug. > > On Mac OS, when a map with all the regular fonts is created, we also add two > system fonts: [System > regular](https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/native-font/MacFontFinder.c#L178), > and [System > bold](https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/native-font/MacFontFinder.c#L187). > These will be the default fonts to be used if the project doesn't specify a > font. > > On runtime, when a font is > [created](https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/java/com/sun/javafx/font/coretext/CTFontStrike.java#L90), > `CTFontCreateWithName` is > [used](https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/native-font/coretext.c#L391), > which according to Apple guidelines should be used only for the regular > fonts (those that are not system fonts). In fact the above warning doesn't > happen when creating any of those fonts. > > Following the warning message, one of the options to create system fonts is > to use `CTFontCreateUIFontForLanguage`, which according to the documentation > in `CTFont.h`: >> Returns the special UI font for the given language and UI type. > > and matches what we already do in `MacFontFinder` to create such fonts. (The > other option will require the use of UIKit.) > > > This PR modifies `CTFontCreateWithName` in `coretext.c` to detect if the font > is a system font in the first place, else use the same existing mechanism. > > As an aside, the constants `kCTFontSystemFontType` and > `kCTFontEmphasizedSystemFontType` are deprecated, and this PR uses now the > new ones, but in both cases their int value is the same (2 and 3, in that > order). As a follow-up PR, we could replace these deprecated constants. > > > > Commits: > - 693dfa78: Use CTFontCreateUIFontForLanguage for system fonts > > Changes: https://git.openjdk.java.net/jfx/pull/55/files > Webrev: https://webrevs.openjdk.java.net/jfx/55/webrev.00 > Issue: https://bugs.openjdk.java.net/browse/JDK-8234916 > Stats: 9 lines in 1 file changed: 8 ins; 0 del; 1 mod > Patch: https://git.openjdk.java.net/jfx/pull/55.diff > Fetch: git fetch https://git.openjdk.java.net/jfx pull/55/head:pull/55 This seems simple enough that one reviewer should be sufficient (unless Phil thinks otherwise). PR: https://git.openjdk.java.net/jfx/pull/55
Re: [Approved] RFR: 8234916: [macos 10.15] Garbled text running with native-image
On Wed, 27 Nov 2019 17:06:13 GMT, Jose Pereda wrote: > Running on MacOS Catalina, when doing static builds of `libjavafx_font.a` and > linking against this in a JavaFX app compiled with GraalVM native-image, if > default fonts are used, the rendered text is garbled, and a warning message > is printed: > > CoreText note: Client requested name ".SFUI-Regular", it will get > TimesNewRomanPSMT rather than the intended font. All system UI font access > should be through proper APIs such as CTFontCreateUIFontForLanguage() or > +[UIFont systemFontOfSize:]. > CoreText note: Set a breakpoint on CTFontLogSystemFontNameRequest to debug. > > On Mac OS, when a map with all the regular fonts is created, we also add two > system fonts: [System > regular](https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/native-font/MacFontFinder.c#L178), > and [System > bold](https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/native-font/MacFontFinder.c#L187). > These will be the default fonts to be used if the project doesn't specify a > font. > > On runtime, when a font is > [created](https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/java/com/sun/javafx/font/coretext/CTFontStrike.java#L90), > `CTFontCreateWithName` is > [used](https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/native-font/coretext.c#L391), > which according to Apple guidelines should be used only for the regular > fonts (those that are not system fonts). In fact the above warning doesn't > happen when creating any of those fonts. > > Following the warning message, one of the options to create system fonts is > to use `CTFontCreateUIFontForLanguage`, which according to the documentation > in `CTFont.h`: >> Returns the special UI font for the given language and UI type. > > and matches what we already do in `MacFontFinder` to create such fonts. (The > other option will require the use of UIKit.) > > > This PR modifies `CTFontCreateWithName` in `coretext.c` to detect if the font > is a system font in the first place, else use the same existing mechanism. > > As an aside, the constants `kCTFontSystemFontType` and > `kCTFontEmphasizedSystemFontType` are deprecated, and this PR uses now the > new ones, but in both cases their int value is the same (2 and 3, in that > order). As a follow-up PR, we could replace these deprecated constants. > > > > Commits: > - 693dfa78: Use CTFontCreateUIFontForLanguage for system fonts > > Changes: https://git.openjdk.java.net/jfx/pull/55/files > Webrev: https://webrevs.openjdk.java.net/jfx/55/webrev.00 > Issue: https://bugs.openjdk.java.net/browse/JDK-8234916 > Stats: 9 lines in 1 file changed: 8 ins; 0 del; 1 mod > Patch: https://git.openjdk.java.net/jfx/pull/55.diff > Fetch: git fetch https://git.openjdk.java.net/jfx pull/55/head:pull/55 Approved by prr (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/55
RFR: 8234916: [macos 10.15] Garbled text running with native-image
Running on MacOS Catalina, when doing static builds of `libjavafx_font.a` and linking against this in a JavaFX app compiled with GraalVM native-image, if default fonts are used, the rendered text is garbled, and a warning message is printed: CoreText note: Client requested name ".SFUI-Regular", it will get TimesNewRomanPSMT rather than the intended font. All system UI font access should be through proper APIs such as CTFontCreateUIFontForLanguage() or +[UIFont systemFontOfSize:]. CoreText note: Set a breakpoint on CTFontLogSystemFontNameRequest to debug. On Mac OS, when a map with all the regular fonts is created, we also add two system fonts: [System regular](https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/native-font/MacFontFinder.c#L178), and [System bold](https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/native-font/MacFontFinder.c#L187). These will be the default fonts to be used if the project doesn't specify a font. On runtime, when a font is [created](https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/java/com/sun/javafx/font/coretext/CTFontStrike.java#L90), `CTFontCreateWithName` is [used](https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/native-font/coretext.c#L391), which according to Apple guidelines should be used only for the regular fonts (those that are not system fonts). In fact the above warning doesn't happen when creating any of those fonts. Following the warning message, one of the options to create system fonts is to use `CTFontCreateUIFontForLanguage`, which according to the documentation in `CTFont.h`: > Returns the special UI font for the given language and UI type. and matches what we already do in `MacFontFinder` to create such fonts. (The other option will require the use of UIKit.) This PR modifies `CTFontCreateWithName` in `coretext.c` to detect if the font is a system font in the first place, else use the same existing mechanism. As an aside, the constants `kCTFontSystemFontType` and `kCTFontEmphasizedSystemFontType` are deprecated, and this PR uses now the new ones, but in both cases their int value is the same (2 and 3, in that order). As a follow-up PR, we could replace these deprecated constants. Commits: - 693dfa78: Use CTFontCreateUIFontForLanguage for system fonts Changes: https://git.openjdk.java.net/jfx/pull/55/files Webrev: https://webrevs.openjdk.java.net/jfx/55/webrev.00 Issue: https://bugs.openjdk.java.net/browse/JDK-8234916 Stats: 9 lines in 1 file changed: 8 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/55.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/55/head:pull/55 PR: https://git.openjdk.java.net/jfx/pull/55
Re: [Integrated] RFR: 8200224: Multiple press event when JFXPanel gains focus
Changeset: 1d670f18 Author:Florian Kirmaier Committer: Kevin Rushforth Date: 2019-11-27 16:20:12 + URL: https://git.openjdk.java.net/jfx/commit/1d670f18 8200224: Multiple press event when JFXPanel gains focus Reviewed-by: kcr, psadhukhan ! modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java + tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
Re: [Rev 05] RFR: 8200224: Multiple press event when JFXPanel gains focus
The pull request has been updated with additional changes. Added commits: - fa465c55: JDK-8200224 Changes: - all: https://git.openjdk.java.net/jfx/pull/25/files - new: https://git.openjdk.java.net/jfx/pull/25/files/24385eb8..fa465c55 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/25/webrev.05 - incr: https://webrevs.openjdk.java.net/jfx/25/webrev.04-05 Issue: https://bugs.openjdk.java.net/browse/JDK-8200224 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/25.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/25/head:pull/25 PR: https://git.openjdk.java.net/jfx/pull/25
Re: [Rev 04] RFR: 8200224: Multiple press event when JFXPanel gains focus
On Wed, 27 Nov 2019 15:13:33 GMT, Kevin Rushforth wrote: > On Wed, 27 Nov 2019 05:31:27 GMT, Prasanta Sadhukhan > wrote: > >> On Tue, 26 Nov 2019 13:06:36 GMT, Florian Kirmaier >> wrote: >> >>> The pull request has been updated with additional changes. >>> >>> >>> >>> Added commits: >>> - 24385eb8: JDK-8200224 >>> - e0829ad3: JDK-8200224 >>> - c190384f: JDK-8200224 >>> - 17b458b1: JDK-8200224 >>> >>> Changes: >>> - all: https://git.openjdk.java.net/jfx/pull/25/files >>> - new: https://git.openjdk.java.net/jfx/pull/25/files/44774dfb..24385eb8 >>> >>> Webrevs: >>> - full: https://webrevs.openjdk.java.net/jfx/25/webrev.04 >>> - incr: https://webrevs.openjdk.java.net/jfx/25/webrev.03-04 >>> >>> Issue: https://bugs.openjdk.java.net/browse/JDK-8200224 >>> Stats: 141 lines in 2 files changed: 123 ins; 11 del; 7 mod >>> Patch: https://git.openjdk.java.net/jfx/pull/25.diff >>> Fetch: git fetch https://git.openjdk.java.net/jfx pull/25/head:pull/25 >> >> modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 449: >> >>> 448: requestFocus(); >>> 449: // This fixes JDK-8087914 without causing JDK-8200224 >>> 450: // It is safe, because in JavaFX only the method >>> "setFocused(true)" is called, >> >> We actually do not clutter source code with bugids, it will be good if it >> can be removed with proper comments maybe something like "extra simulated >> mouse pressed event is removed by making the JavaFX scene focused" > > In general, we no longer reference bug IDs in source code, so I agree with > the recommendation to reword the comment. > > @FlorianKirmaier - can you reword as suggested? Once you are done, you can > integrate it (using the `/integrate` command), and I will sponsor it. There > will likely be a delay due to the US Thanksgiving holiday. Done PR: https://git.openjdk.java.net/jfx/pull/25
Re: [Rev 04] RFR: 8200224: Multiple press event when JFXPanel gains focus
On Wed, 27 Nov 2019 05:31:27 GMT, Prasanta Sadhukhan wrote: > On Tue, 26 Nov 2019 13:06:36 GMT, Florian Kirmaier > wrote: > >> The pull request has been updated with additional changes. >> >> >> >> Added commits: >> - 24385eb8: JDK-8200224 >> - e0829ad3: JDK-8200224 >> - c190384f: JDK-8200224 >> - 17b458b1: JDK-8200224 >> >> Changes: >> - all: https://git.openjdk.java.net/jfx/pull/25/files >> - new: https://git.openjdk.java.net/jfx/pull/25/files/44774dfb..24385eb8 >> >> Webrevs: >> - full: https://webrevs.openjdk.java.net/jfx/25/webrev.04 >> - incr: https://webrevs.openjdk.java.net/jfx/25/webrev.03-04 >> >> Issue: https://bugs.openjdk.java.net/browse/JDK-8200224 >> Stats: 141 lines in 2 files changed: 123 ins; 11 del; 7 mod >> Patch: https://git.openjdk.java.net/jfx/pull/25.diff >> Fetch: git fetch https://git.openjdk.java.net/jfx pull/25/head:pull/25 > > modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 449: > >> 448: requestFocus(); >> 449: // This fixes JDK-8087914 without causing JDK-8200224 >> 450: // It is safe, because in JavaFX only the method >> "setFocused(true)" is called, > > We actually do not clutter source code with bugids, it will be good if it can > be removed with proper comments maybe something like "extra simulated mouse > pressed event is removed by making the JavaFX scene focused" In general, we no longer reference bug IDs in source code, so I agree with the recommendation to reword the comment. @FlorianKirmaier - can you reword as suggested? Once you are done, you can integrate it (using the `/integrate` command), and I will sponsor it. There will likely be a delay due to the US Thanksgiving holiday. PR: https://git.openjdk.java.net/jfx/pull/25
Re: [Rev 03] RFR: 8130738: Add tabSize property to Text and TextFlow
The pull request has been updated with additional changes. Added commits: - af959665: 8130738: Add tabSize property to Text and TextFlow Changes: - all: https://git.openjdk.java.net/jfx/pull/32/files - new: https://git.openjdk.java.net/jfx/pull/32/files/254c40de..af959665 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/32/webrev.03 - incr: https://webrevs.openjdk.java.net/jfx/32/webrev.02-03 Issue: https://bugs.openjdk.java.net/browse/JDK-8130738 Stats: 57 lines in 6 files changed: 25 ins; 26 del; 6 mod Patch: https://git.openjdk.java.net/jfx/pull/32.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/32/head:pull/32 PR: https://git.openjdk.java.net/jfx/pull/32
Re: RFR: 8211308: Support HTTP/2 in WebView
On Wed, 16 Oct 2019 21:43:15 GMT, Kevin Rushforth wrote: > On Wed, 16 Oct 2019 18:10:00 GMT, Kevin Rushforth wrote: > >> On Wed, 16 Oct 2019 17:58:32 GMT, Arunprasad Rajkumar >> wrote: >> >>> On Fri, 11 Oct 2019 11:21:08 GMT, Robin Westberg >>> wrote: >>> On Fri, 11 Oct 2019 07:01:48 GMT, Arunprasad Rajkumar wrote: > On Fri, 11 Oct 2019 06:44:09 GMT, Johan Vos wrote: > >> On Fri, 11 Oct 2019 06:18:38 GMT, Arunprasad Rajkumar >> wrote: >> >>> On Fri, 11 Oct 2019 06:07:14 GMT, Arunprasad Rajkumar >>> wrote: >>> The goal of this enhancement is to use new [HttpClient APIs](https://docs.oracle.com/en/java/javase/11/docs/api/java.net.http/java/net/http/HttpClient.html) available from JDK 11. Reference: [1] https://openjdk.java.net/groups/net/httpclient/intro.html [2] https://docs.oracle.com/en/java/javase/11/docs/api/java.net.http/java/net/http/HttpClient.html Though this uses JDK 11 HttpClient APIs, it needs latest JDK 12 to work correctly due to the dependency on following issues, [JDK-8218546](https://bugs.openjdk.java.net/browse/JDK-8218546) Unable to connect to https://google.com using java.net.HttpClient [JDK-8218662](https://bugs.openjdk.java.net/browse/JDK-8218662) Allow 204 responses with Content-Length:0 [JDK-8203850](https://bugs.openjdk.java.net/browse/JDK-8203850) java.net.http HTTP client should allow specifying Origin and Referer headers Task List - [x] simple GET requests - [x] Runtime setting to fallback to legacy client - [ ] Runtime settings to use *only* HTTP/1.1 - [x] sync requests - [x] Error Handling & Propagation - [x] POST with form data - [x] AccessController association for HttpClient.sendAsync / send - [x] Redirection - [ ] Check for possibilities to write unit tests - [ ] Sync request handling from WebCore java platform layer - [x] Make use of singleton instance of direct ByteBuffer instead of using allocator pool - [x] gzip, deflate encoding support HTTP/2 Test pages - http://www.http2demo.io - https://http2.akamai.com/demo - https://http2.golang.org - https://google.com Redirection Test - https://www.httpwatch.com/httpgallery/redirection/#showExample7 More details are available at https://github.com/javafxports/openjdk-jfx/pull/247. Commits: - 1798a661: 8211308: Support HTTP/2 in WebView Changes: https://git.openjdk.java.net/jfx/pull/14/files Webrev: https://webrevs.openjdk.java.net/jfx/14/webrev.00 Issue: https://bugs.openjdk.java.net/browse/JDK-8211308 Stats: 1161 lines in 14 files changed: 876 ins; 217 del; 68 mod Patch: https://git.openjdk.java.net/jfx/pull/14.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/14/head:pull/14 >>> >>> Still few changes need to be done as [suggested >>> by](https://github.com/javafxports/openjdk-jfx/pull/247#pullrequestreview-283699613) >>> @kevinrushforth. >> >> Good work. Should the title be prefixed with WIP until it's ready for >> review, so that Skara will send the RFR when it is ready for review? > > I was wondering why @skara had sent the RFR when the PR is still in draft > stage. Actually @skara should consider the "Draft" attribute associated > with the PR. Good point, I've created https://bugs.openjdk.java.net/browse/SKARA-129 to track this. >>> >>> @jfx team, now it is ready for a fresh review :) >> >> @guruhb please also review this. > > I note that this change needs two reviewers (so should not be integrated > until there are two approved reviews). I did a fair bit of testing yesterday on all three platforms, and it all looks good to me. I verified the behavior on JDK 11 LTS (not enabled by default) and on JDK 13 (is enabled by default). I have one more things to check next week. @guruhb can you also review? PR: https://git.openjdk.java.net/jfx/pull/14
RFR: 8212034: Potential memory leaks in jpegLoader.c in error case
Memory allocated in initDecompressor() and decompressIndirect() is not freed in error case. In error case, 1. Allocated memory should be freed. 2. Appropriate de-initialization jpeg library calls should be added. Verified that, 1. All unit and systems tests pass on three platforms, and 2. Memory consumption with and without fix is similar by comparing memory before and after showing 10 jpeg images for 100 times. Commits: - 7af932b7: 8212034: Memory leaks in jpegLoader.c in error case Changes: https://git.openjdk.java.net/jfx/pull/54/files Webrev: https://webrevs.openjdk.java.net/jfx/54/webrev.00 Issue: https://bugs.openjdk.java.net/browse/JDK-8212034 Stats: 62 lines in 1 file changed: 36 ins; 14 del; 12 mod Patch: https://git.openjdk.java.net/jfx/pull/54.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/54/head:pull/54 PR: https://git.openjdk.java.net/jfx/pull/54
Re: RFR: 8196587: Remove use of deprecated finalize method from JPEGImageLoader
On Tue, 26 Nov 2019 15:16:41 GMT, Kevin Rushforth wrote:
> On Tue, 26 Nov 2019 15:16:38 GMT, Ambarish Rapte wrote:
>
>> The finalize() method is deprecated in JDK9. See [Java 9 deprecated
>> features](https://www.oracle.com/technetwork/java/javase/9-deprecated-features-3745636.html).
>> And so the JPEGImageLoader.finalize() method should be removed.
>>
>> The change is,
>> 1. Remove finalize method from JPEGImageLoader class.
>>
>> 2. Instance of JPEGImageLoader is created and used in ImageStorage class.
>> JPEGImageLoader.dispose() should be called after it's use over. This would
>> be a common call for the other (GIF, PNG, BMP) ImageLoader classes, which
>> have empty dispose() method.
>>
>> 3. JPEGImageLoader.load() method almost always calls the dispose() method
>> after the image is loaded. In normal scenario JPEGImageLoader is disposed
>> here. The calls to dispose() added in ImageStorage seem logically correct
>> place to add and should be added.
>>
>> Verification:
>> Verified :graphics:test and system tests on all three platforms.
>> Verified that JPEGImageLoader.dispose() is always initiated by
>> JPEGImageLoader.load()
>>
>>
>>
>> Commits:
>> - b48c8087: 8196587: Remove use of deprecated finalize method from
>> JPEGImageLoader
>>
>> Changes: https://git.openjdk.java.net/jfx/pull/50/files
>> Webrev: https://webrevs.openjdk.java.net/jfx/50/webrev.00
>> Issue: https://bugs.openjdk.java.net/browse/JDK-8196587
>> Stats: 28 lines in 2 files changed: 14 ins; 12 del; 2 mod
>> Patch: https://git.openjdk.java.net/jfx/pull/50.diff
>> Fetch: git fetch https://git.openjdk.java.net/jfx pull/50/head:pull/50
>
> I still need to double-check all calls to dispose, but I think this is
> essentially the right solution, and is ready to be submitted for review. I
> added one comment inline.
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/iio/ImageStorage.java
> line 273:
>
>> 272: } else {
>> 273: throw new ImageStorageException("No loader for image
>> data");
>> 274: }
>
> Now that this is moved inside a try/catch, this `ImageStorageException` will
> get wrapped in another `ImageStorageException` if it is ever thrown. You
> probably want to catch `ImageStorageException` below and re-throw it without
> wrapping it.
I will change the catch as below and update with next commit.
} catch (ImageStorageException ise) {
throw ise;
} catch (IOException e) {
throw new ImageStorageException(e.getMessage(), e);
}
PR: https://git.openjdk.java.net/jfx/pull/50
Re: RFR: 8130738: Add tabSize property to Text and TextFlow
On Wed, 27 Nov 2019 01:30:22 GMT, Kevin Rushforth wrote: > On Wed, 27 Nov 2019 01:17:42 GMT, Scott Palmer wrote: > >> On Tue, 26 Nov 2019 18:48:38 GMT, Kevin Rushforth wrote: >> >>> On Tue, 26 Nov 2019 18:40:10 GMT, Scott Palmer wrote: >>> On Thu, 7 Nov 2019 14:56:58 GMT, Kevin Rushforth wrote: > On Wed, 6 Nov 2019 19:11:48 GMT, Scott Palmer > wrote: > >> Added tabSize property to Text and TextFlow and -fx-tab-size CSS >> attribute to both. TextFlow's tab size overrides that of contained Text >> nodes. >> >> >> >> Commits: >> - 68d221c7: 8130738: TextFlow's tab width is static >> >> Changes: https://git.openjdk.java.net/jfx/pull/32/files >> Webrev: https://webrevs.openjdk.java.net/jfx/32/webrev.00 >> Issue: https://bugs.openjdk.java.net/browse/JDK-8130738 >> Stats: 324 lines in 8 files changed: 260 ins; 0 del; 64 mod >> Patch: https://git.openjdk.java.net/jfx/pull/32.diff >> Fetch: git fetch https://git.openjdk.java.net/jfx pull/32/head:pull/32 > >> I should clamp so it reads back as 1 - Fixed. > > This has implications for binding, etc., so I suspect this is not what we > want. I'll look into this more when I review it. > > NOTE: This will need a CSR in addition to two reviewers. You can file the > CSR any time, but I suggest leaving it in Draft state until Phil and I > have had a first pass look at the API. The indenting was wrong in that section and I had to make some minor changes, so I corrected the bad indenting. Kevin indicated, "In your specific case, reformatting the methods in the StyleableProperties nested class that are adjacent to code you add or modify seems fine, as long as you only change the indentation and not the line wrapping." I didn't intend to trigger an update to this pull request...I find Git awkward to work with (Mercurial makes more sense to me) so I'm making mistakes as I bumble around. I do expect an update to address the way I've clamped to 1. I agree with Kevin that it is probable wrong. Just waiting for feedback. >>> >>> I'll take a preliminary look today. I also plan to change the bug title to >>> make it more clear that it is an enhancement request, since it currently >>> reads like a bug report (I'll also change the CSR and PR title to match). >> >> I just noticed that Text.toString is missing any indication of the tabSize. >> (Found by accident when I search for wrappingWidth to see how the javadocs >> were done.) >> Is there any rule as to where in the order of properties ", tabSize = X" >> should be in the string? I assume it should only be included when not set >> to the default value, much like lineSpacing? > > Yes, I think your idea of including `tabSize` in `Text.toString` when != > default is a good one. Should this PR also add a `tabSize` property to controls such as `TextArea`? Or should that be a different PR after this one is merged? PR: https://git.openjdk.java.net/jfx/pull/32
Re: [Rev 02] RFR: 8130738: TextFlow's tab width is static
On Wed, 27 Nov 2019 00:58:22 GMT, Kevin Rushforth wrote:
> On Tue, 26 Nov 2019 17:26:33 GMT, Scott Palmer wrote:
>
>> The pull request has been updated with a complete new set of changes
>> (possibly due to a rebase).
>>
>>
>>
>> Commits:
>> - 254c40de: Merge remote-tracking branch 'upstream/master'
>> - a670c4f8: 8130738: TextFlow's tab width is static
>> - 68d221c7: 8130738: TextFlow's tab width is static
>>
>> Changes: https://git.openjdk.java.net/jfx/pull/32/files
>> Webrev: https://webrevs.openjdk.java.net/jfx/32/webrev.02
>> Issue: https://bugs.openjdk.java.net/browse/JDK-8130738
>> Stats: 325 lines in 8 files changed: 261 ins; 0 del; 64 mod
>> Patch: https://git.openjdk.java.net/jfx/pull/32.diff
>> Fetch: git fetch https://git.openjdk.java.net/jfx pull/32/head:pull/32
>
> I did a first pass review focusing mostly on the public API. Once we get that
> solidified I'll look at the implementation and tests more closely. Also, once
> the public API is done, you can update the CSR with the API.
>
> modules/javafx.graphics/src/main/docs/javafx/scene/doc-files/cssref.html line
> 3035:
>
>> 3034:
>> 3035:
>> 3036: -fx-text-alignment
>
> the `` should be on the next line (once you do, you can remove the
> trailing white space that will then be present).
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java
> line 655:
>
>> 654: if (spaces < 1)
>> 655: spaces = 1;
>> 656: if (tabSize != spaces) {
>
> Please surround the statement with curly braces (our coding style is to
> always surround the target of a conditional in curly braces unless it is on
> the same line).
>
> modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1289:
>
>> 1288: * @since 14
>> 1289: */
>> 1290: public final int getTabSize() {
>
> The javadoc property support only requires one of the
> setXxxx/getXxxx/Property methods to have a javadoc comment block. The
> javadoc tool takes care of documenting the property itself and all three
> methods. See `wrappingWidth` for a good example.
>
> modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1302:
>
>> 1301: * @since 14
>> 1302: */
>> 1303: public final void setTabSize(int spaces) {
>
> Same comment as on the set method. You can move the comment about values
> being clamped to 1 to the property method (which will then be the only method
> with javadoc comments). If we use my recommendation of clamping on usage, I
> recommend the following language, borrowed from `Node::opacity`:
>
> Values less than 1 are treated as 1.
>
> modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1895:
>
>> 1894: }
>> 1895: @Override public void set(int v) { super.set((v <
>> 1) ? 1 : v); }
>> 1896: @Override protected void invalidated() {
>
> For mutable properties, we usually clamp on usage, so that we don't have
> problems binding to the value. This preserves the invariant that `set(val);
> get() == val` for all values. If that is what we end up doing, then this
> overridden method should be removed.
>
> modules/javafx.graphics/src/main/java/javafx/scene/text/TextFlow.java line
> 515:
>
>> 514: * @since 14
>> 515: */
>> 516: public final void setTabSize(int spaces) {
>
> Same comments about the javadoc comments as for `Text.java`
>
> modules/javafx.graphics/src/main/java/javafx/scene/text/TextFlow.java line
> 528:
>
>> 527: }
>> 528: @Override public void set(int v) { super.set((v < 1) ?
>> 1 : v); }
>> 529: @Override protected void invalidated() {
>
> Same clamp-on-use comment as in `Text.java`
>
> modules/javafx.graphics/src/test/java/test/com/sun/javafx/pgstub/StubTextLayout.java
> line 200:
>
>> 199: if (spaces < 1)
>> 200: spaces = 1;
>> 201: if (tabSize != spaces) {
>
> Either surround with braces or put onto the same line as the `if` test.
>
> modules/javafx.graphics/src/test/java/test/javafx/scene/text/TextTest.java
> line 271:
>
>> 270: }
>> 271: }
>> 272: }
>
> In addition to the above test, I recommend some (very basic) tests of the
> setter / getting / property methods.
>
> modules/javafx.graphics/src/test/java/test/javafx/scene/text/TextTest.java
> line 251:
>
>> 250: tk.firePulse();
>> 251: assertEquals(text.getTabSize(),8);
>> 252: // initial width with default 8-space tab
>
> This is backwards. The expected value should be the first argument.
Hmm ... so you are saying the clamping is the responsibility of client code
(internal as well as external)?
PR: https://git.openjdk.java.net/jfx/pull/32
