Integrated: 8316003: Update FileChooserSymLinkTest.java to HTML instructions
On Mon, 11 Sep 2023 13:28:40 GMT, Alexey Ivanov wrote: > To demonstrate the functionality of HTML formatting in `PassFailJFrame`, I > updated the instructions for the > `test/jdk/javax/swing/JFileChooser/FileChooserSymLinkTest.java` test. The > commands to create the symbolic links are marked up with monospace font, > which makes them stand out. The test instructions for single- and > multi-selection are emphasised with bold text. > > This change depends on #15660 and > [JDK-8294158](https://bugs.openjdk.org/browse/JDK-8294158). This pull request has now been integrated. Changeset: c587211b Author:Alexey Ivanov URL: https://git.openjdk.org/jdk/commit/c587211bf8c60a7a1f6cc63770c38ede6cb4e173 Stats: 43 lines in 1 file changed: 13 ins; 0 del; 30 mod 8316003: Update FileChooserSymLinkTest.java to HTML instructions Reviewed-by: prr - PR: https://git.openjdk.org/jdk/pull/15661
Integrated: 8294156: Allow PassFailJFrame.Builder to create test UI
On Mon, 11 Sep 2023 15:36:56 GMT, Alexey Ivanov wrote:
> This enhances the `Builder` pattern added in
> [JDK-8294535](https://bugs.openjdk.org/browse/JDK-8294535) with a new method
> `testUI` which allows passing a lambda expression or a method reference to
> create *the test UI window*.
>
> The `PassFailJFrame` will automatically call the method on the EDT to create
> the UI, add it to the internal list of windows, install the window closing
> listener and finally position and show both the instructions and test UI.
>
> Alternatively, you can pass an already created window.
>
> The `main` method of a manual test could look as simple as a sequence of
> calls:
>
>
> public static void main(String[] args) throws Exception {
> PassFailJFrame.builder()
> .instructions(INSTRUCTIONS)
> .testUI(() -> createTestUI())
> .build()
> .awaitAndCheck();
> }
>
> where `createTestUI` returns a test UI window.
This pull request has now been integrated.
Changeset: 42b9ac8a
Author:Alexey Ivanov
URL:
https://git.openjdk.org/jdk/commit/42b9ac8a07b540f4d7955a778923d24a876451cc
Stats: 442 lines in 1 file changed: 365 ins; 38 del; 39 mod
8294156: Allow PassFailJFrame.Builder to create test UI
Reviewed-by: azvegint, prr
-
PR: https://git.openjdk.org/jdk/pull/15665
Re: RFR: 8316003: Update FileChooserSymLinkTest.java to HTML instructions [v4]
> To demonstrate the functionality of HTML formatting in `PassFailJFrame`, I > updated the instructions for the > `test/jdk/javax/swing/JFileChooser/FileChooserSymLinkTest.java` test. The > commands to create the symbolic links are marked up with monospace font, > which makes them stand out. The test instructions for single- and > multi-selection are emphasised with bold text. > > This change depends on #15660 and > [JDK-8294158](https://bugs.openjdk.org/browse/JDK-8294158). Alexey Ivanov has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains eight commits: - Merge master - Amend indentation of nested elements - Revert "Revert changes to FileChooserSymLinkTest.java" This reverts commit 7753b3f3e2d3a0df5fde672ad41833b6a9c6e247. - Wrap createUI call It reverts the accidental change where I unwrapped call to createUI - Revert changes to FileChooserSymLinkTest.java - Refactor configuration of a text component for instructions Each text component (JTextArea or JEditorPane) are configured in its own dedicated method. This avoids unnecessary type casts. - Improve instruction formatting - Enable HTML display if instructions start with Instructions for FileChooserSymLinkTest are rendered in HTML. - Changes: https://git.openjdk.org/jdk/pull/15661/files Webrev: https://webrevs.openjdk.org/?repo=jdk=15661=03 Stats: 43 lines in 1 file changed: 13 ins; 0 del; 30 mod Patch: https://git.openjdk.org/jdk/pull/15661.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15661/head:pull/15661 PR: https://git.openjdk.org/jdk/pull/15661
Re: RFR: 8294156: Allow PassFailJFrame.Builder to create test UI [v4]
> This enhances the `Builder` pattern added in
> [JDK-8294535](https://bugs.openjdk.org/browse/JDK-8294535) with a new method
> `testUI` which allows passing a lambda expression or a method reference to
> create *the test UI window*.
>
> The `PassFailJFrame` will automatically call the method on the EDT to create
> the UI, add it to the internal list of windows, install the window closing
> listener and finally position and show both the instructions and test UI.
>
> Alternatively, you can pass an already created window.
>
> The `main` method of a manual test could look as simple as a sequence of
> calls:
>
>
> public static void main(String[] args) throws Exception {
> PassFailJFrame.builder()
> .instructions(INSTRUCTIONS)
> .testUI(() -> createTestUI())
> .build()
> .awaitAndCheck();
> }
>
> where `createTestUI` returns a test UI window.
Alexey Ivanov has updated the pull request with a new target base due to a
merge or a rebase. The pull request now contains 16 commits:
- Merge master
- Add instructions on how to use PassFailJFrame
- 8294156: Allow creating several test windows
The windows can be positioned in advance, or
a call back PositionWindows interface can be used
to define their positions after the instruction UI
frame is positioned on the screen.
- Remove trailing whitespace
- Mark the Builder class final
It's not supposed to be extended.
- Amend message in window closing handler
The WindowClosingHandler is used both for the instruction
frame and for test UI windows.
Also add blank lines to separate different parts of the class.
- Use invokeOnEDT in getInstructionFrameBounds
- Use functional style for disposing of windows
Also clean up the doc for disposeWindows().
- Use statically imported invokeAndWait and isEDT
- Build functional test UI
The Builder now provides a method to create test UI,
PassFailJFrame automatically invokes the method reference
on the EDT and then shows both the instruction frame and
the test window.
- ... and 6 more: https://git.openjdk.org/jdk/compare/14090ef6...eff09d0d
-
Changes: https://git.openjdk.org/jdk/pull/15665/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=15665=03
Stats: 442 lines in 1 file changed: 365 ins; 38 del; 39 mod
Patch: https://git.openjdk.org/jdk/pull/15665.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/15665/head:pull/15665
PR: https://git.openjdk.org/jdk/pull/15665
Integrated: 8294158: HTML formatting for PassFailJFrame instructions
On Mon, 11 Sep 2023 13:11:10 GMT, Alexey Ivanov wrote: > This enhancement provides HTML formatting for instructions in the manual test > framework, `PassFailJFrame`. > > Some tests could benefit from rich-formatted instructions, especially when > the instructions are long. > > See a sample usage in #15661. This pull request has now been integrated. Changeset: 14090ef6 Author:Alexey Ivanov URL: https://git.openjdk.org/jdk/commit/14090ef6039ff2f3064f397a75219b2bc715cc27 Stats: 40 lines in 1 file changed: 35 ins; 0 del; 5 mod 8294158: HTML formatting for PassFailJFrame instructions Reviewed-by: azvegint, prr - PR: https://git.openjdk.org/jdk/pull/15660
Re: RFR: JDK-8314731 : Add support for the alt attribute in the image type input HTML tag [v2]
On Tue, 24 Oct 2023 18:54:17 GMT, ScientificWare wrote:
>>> You should probably pass `altAtt` as the `description` parameter, yet I'm
>>> unsure if it's ever used in this context.
>>
>> As I say, I'm unsure if it's used… The description could be exposed to
>> Accessibility API. By default, `ImageIcon` uses the URL as the description
>> if it isn't provided:
>>
>> https://github.com/openjdk/jdk/blob/99de9bb83ff70fe81c89751516a86a94c8f552be/src/java.desktop/share/classes/javax/swing/ImageIcon.java#L231-L233
>>
>> Now that you handle the `alt` attribute, you can pass the description if
>> it's provided.
>
> 
>
> Did you have this in mind ?
>
>
>
> src="file:oracle_logo_50x50.jpg" alt="Logo Oracle JPG">
>
> src="file:oracle_logo_50x50.jpg">
>
> src="file:none_oracle_logo_50x50.jpg" alt="Logo Oracle JPG">
>
> src="file:none_oracle_logo_50x50.jpg">
>
> src="files:none_oracle_logo_50x50.jpg" alt="Logo Oracle JPG">
>
> src="files:none_oracle_logo_50x50.jpg">
>
>
>
>
>
>
>
> String srcAtt = (String) attr.getAttribute(HTML.Attribute.SRC);
> String altAtt = (String) attr.getAttribute(HTML.Attribute.ALT);
> if (altAtt == null) {
> altAtt = srcAtt;
> }
> JButton button;
> try {
> URL base =
> ((HTMLDocument)getElement().getDocument()).getBase();
> @SuppressWarnings("deprecation")
> URL srcURL = new URL(base, srcAtt);
> ImageIcon icon = new ImageIcon(srcURL, altAtt);
> button = new JButton(icon);
> } catch (MalformedURLException e) {
> button = new JButton(altAtt == null ? srcAtt : altAtt);
> }
Yes, something similar:
String srcAtt = (String) attr.getAttribute(HTML.Attribute.SRC);
String altAtt = (String) attr.getAttribute(HTML.Attribute.ALT);
JButton button;
try {
URL base = ((HTMLDocument)getElement().getDocument()).getBase();
@SuppressWarnings("deprecation")
URL srcURL = new URL(base, srcAtt);
ImageIcon icon = altAtt != null
? new ImageIcon(srcURL, altAtt)
: new ImageIcon(srcURL);
button = icon.getImageLoadStatus() == MediaTracker.COMPLETE
? new JButton(icon)
: new JButton(altAtt != null ? altAtt : srcAtt);
} catch (MalformedURLException e) {
button = new JButton(altAtt != null ? altAtt : srcAtt);
}
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15319#discussion_r1370719498
Re: RFR: JDK-8314731 : Add support for the alt attribute in the image type input HTML tag [v2]
On Tue, 24 Oct 2023 19:04:05 GMT, ScientificWare wrote: >>> > What purpose does this distinction serve? >>> >>> Definitely an application developer and because it's the context of this >>> HTML implementation, may I preserve this distinction ? >> >> Okay, let it be. However, I still think it's not the right thing. >> >> There was nothing else but the `src` attribute, now there is. >> >>> > This actually raises another question: what if altAtt isn't provided? >>> > Should it fallback to srcURL? >>> >>> Yes, it's a logical error on my part. >> >> Are you going to address it? > >  > > > > > > src="file:oracle_logo_50x50.jpg" alt="Logo Oracle JPG"> > > src="file:oracle_logo_50x50.jpg"> > > src="file:none_oracle_logo_50x50.jpg" alt="Logo Oracle JPG"> > > src="file:none_oracle_logo_50x50.jpg"> > > src="files:none_oracle_logo_50x50.jpg" alt="Logo Oracle JPG"> > > src="files:none_oracle_logo_50x50.jpg"> > > > > > > - Left before the Patch > - Right with your suggestion and if the distinction is preserved. Yes, that's what I meant. A button with the plain source is better than a button without a title at all. - PR Review Comment: https://git.openjdk.org/jdk/pull/15319#discussion_r1370710100
Re: RFR: 8169475 : WheelModifier.java fails by timeout [v2]
On Mon, 23 Oct 2023 17:45:38 GMT, Alexey Ivanov wrote: > The updated test failed once on Linux when I ran the test on Friday… I've run this test on all the platforms a few times more, no failures detected. So I consider that failure a one-off glitch. - PR Comment: https://git.openjdk.org/jdk/pull/16281#issuecomment-1777839177
Re: RFR: JDK-8314731 : Add support for the alt attribute in the image type input HTML tag [v2]
On Mon, 23 Oct 2023 20:48:31 GMT, ScientificWare wrote: > > What purpose does this distinction serve? > > Definitely an application developer and because it's the context of this HTML > implementation, may I preserve this distinction ? Okay, let it be. However, I still think it's not the right thing. There was nothing else but the `src` attribute, now there is. > > This actually raises another question: what if altAtt isn't provided? > > Should it fallback to srcURL? > > Yes, it's a logical error on my part. Are you going to address it? - PR Review Comment: https://git.openjdk.org/jdk/pull/15319#discussion_r1370634475
Re: RFR: 8316017: Refactor timeout handler in PassFailJFrame [v2]
On Tue, 19 Sep 2023 18:35:12 GMT, Phil Race wrote: >> Alexey Ivanov has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Refactor failure reason >> >> Ensure the failure reason doesn't change after it's set. >> Removed 'failed' and 'timeout' fields: >> if failureReason is set, the test fails. > > test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 329: > >> 327: timer.stop(); >> 328: testFailedReason = FAILURE_REASON >> 329:+ "Timeout User did not perform >> testing."; > > If the test was explicitly failed by the user (ie failed==true), why does the > reason say that there was a time out ? @prrace Eventually, I refactored handling of `failureReason`. I removed `failed` and `timeout` fields; the `failureReason` is set via a synchronised method to ensure there cannot be data race. If `failureReason` is not `null`, the test fails. After `failureReason` is set to a non-`null` value, it cannot be changed. The timeout handler relies on this behaviour: it calls `setFailureReason` unconditionally; if `failureReason` is already set, the timeout reason is ignored. - PR Review Comment: https://git.openjdk.org/jdk/pull/15668#discussion_r136854
Re: RFR: 8316017: Refactor timeout handler in PassFailJFrame [v2]
> Refactored timeout handling in PassFailJFrame: > > 1. Managing the timer and formatting the time left is inside `TimeoutHandler` > class. > 2. The class handles timer events and updates the label accordingly. > > This is implemented on top of #15665. Alexey Ivanov has updated the pull request incrementally with one additional commit since the last revision: Refactor failure reason Ensure the failure reason doesn't change after it's set. Removed 'failed' and 'timeout' fields: if failureReason is set, the test fails. - Changes: - all: https://git.openjdk.org/jdk/pull/15668/files - new: https://git.openjdk.org/jdk/pull/15668/files/27f39736..39ebc7c8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15668=01 - incr: https://webrevs.openjdk.org/?repo=jdk=15668=00-01 Stats: 79 lines in 1 file changed: 47 ins; 15 del; 17 mod Patch: https://git.openjdk.org/jdk/pull/15668.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15668/head:pull/15668 PR: https://git.openjdk.org/jdk/pull/15668
Re: RFR: 8294156: Allow PassFailJFrame.Builder to create test UI [v3]
On Mon, 23 Oct 2023 20:59:49 GMT, Alexey Ivanov wrote: >>> > Shouldn't we mention here a window added by `testUI` via builder? >>> >>> I don't think it's necessary: the builder uses `addTestWindow` under the >>> hood. It's the builder that requires documentation. >>> >>> In fact, the entire class requires a bit of TLC for documentation. I even >>> wrote some after Lawrence @lawrence-andrew created the first version of >>> `PassFailJFrame` but I never finished it. Even this part was implemented >>> nearly a year ago. >>> >>> I plan to create a wiki page on the [client-libs >>> wiki](https://wiki.openjdk.org/display/ClientLibs) to describe creating >>> manual tests with `PassFailJFrame`. >> >> Just saw this, and it mirrors what I wrote in another PR that we need docs, >> but I believe they are best placed inside the source itself. Not everyone >> who can update this source can update the wiki. >> That doesn't preclude you from putting more examples on the wiki and a >> reference to it from the source but I still think there should be docs in >> the source. >> >> Include how to use this feature and something like to the builder example >> you have here might be good too. > > @prrace I've just pushed documentation on how to use `PassFailJFrame`. It > provided two samples: the new one with the builder pattern and the classic > one. The samples can be copied and pasted into a file, they're ready to run, > except for imports and jtreg tags. I also submitted a bug to provide more comprehensive documentation: [JDK-8318688](https://bugs.openjdk.org/browse/JDK-8318688): _Write documentation for PassFailJFrame_ - PR Review Comment: https://git.openjdk.org/jdk/pull/15665#discussion_r1369276650
Re: RFR: 8294158: HTML formatting for PassFailJFrame instructions
On Wed, 20 Sep 2023 18:05:31 GMT, Phil Race wrote: >> I know HTML is *not case sensitive*. It's just HTML tags are always in lower >> case in recent years. >> >>> But I agree that this can be helpful for a certain amount of consistency >>> across tests(they can still use other tags in uppercase). >> >> Yep! In a way, this will enforce `` in lower case, and I hope all >> other tags in the instructions. >> >> >> >>> > Although neither nor are required, I prefer to always add them. These >>> > two elements don't add much overhead, do they? >>> >>> It is a matter of personal taste. >> >> Yes, it is… which could create inconsistencies. >> >>> Normally, I prefer to indent a content of such tags by extra spaces, and >>> This can create additional formatting difficulties under the 80(120) >>> character limit per line. >>> In those cases, I would prefer to omit those tags. >> >> I don't expect too many nesting levels in instructions. However, I agree it >> could be problematic… >> >> In the sample in #15661, I keep `` and the content on the same >> level. >> >> Now that I looked at it again, I realised that I didn't follow the >> indentation consistently: first-level `` aren't indented but third-level >> `` are. I should correct this. >> >> Because `` is used to format pieces of code, I decided not to indent >> the first-level `` elements to avoid indentation of the code inside. > > I don't have a problem with keeping it simple. If some one writing a test > doesn't see the HTML interpreted, they'll quickly be able to figure out why. > One thing about these and the other enhancements I see is that there's an > (increasing) need for a nice bit of documentation in PassFailJFrame itself > about all its features. > Easiest to do this as you are adding them, so a doc comment block can be > started with this one. > Probably as a class comment. > > Suggested text > /** > * PassFailJFrame documentation > * > * Instructions for the user can be either plain text or HTML as supported by > Swing. > * Text beginning with is presumed to be HTML. > */ @prrace I've added description on how to use `PassFailJFrame` to #15665, it handles both HTML formatting for instructions (this PR) as well as creating test UI by passing a method reference, or rather an implementation of an interface. - PR Review Comment: https://git.openjdk.org/jdk/pull/15660#discussion_r1369268704
Re: RFR: 8294156: Allow PassFailJFrame.Builder to create test UI [v3]
On Wed, 20 Sep 2023 18:12:25 GMT, Phil Race wrote: >>> Shouldn't we mention here a window added by `testUI` via builder? >> >> I don't think it's necessary: the builder uses `addTestWindow` under the >> hood. It's the builder that requires documentation. >> >> In fact, the entire class requires a bit of TLC for documentation. I even >> wrote some after Lawrence @lawrence-andrew created the first version of >> `PassFailJFrame` but I never finished it. Even this part was implemented >> nearly a year ago. >> >> I plan to create a wiki page on the [client-libs >> wiki](https://wiki.openjdk.org/display/ClientLibs) to describe creating >> manual tests with `PassFailJFrame`. > >> > Shouldn't we mention here a window added by `testUI` via builder? >> >> I don't think it's necessary: the builder uses `addTestWindow` under the >> hood. It's the builder that requires documentation. >> >> In fact, the entire class requires a bit of TLC for documentation. I even >> wrote some after Lawrence @lawrence-andrew created the first version of >> `PassFailJFrame` but I never finished it. Even this part was implemented >> nearly a year ago. >> >> I plan to create a wiki page on the [client-libs >> wiki](https://wiki.openjdk.org/display/ClientLibs) to describe creating >> manual tests with `PassFailJFrame`. > > Just saw this, and it mirrors what I wrote in another PR that we need docs, > but I believe they are best placed inside the source itself. Not everyone who > can update this source can update the wiki. > That doesn't preclude you from putting more examples on the wiki and a > reference to it from the source but I still think there should be docs in the > source. > > Include how to use this feature and something like to the builder example you > have here might be good too. @prrace I've just pushed documentation on how to use `PassFailJFrame`. It provided two samples: the new one with the builder pattern and the classic one. The samples can be copied and pasted into a file, they're ready to run, except for imports and jtreg tags. - PR Review Comment: https://git.openjdk.org/jdk/pull/15665#discussion_r1369259910
Re: RFR: 8294156: Allow PassFailJFrame.Builder to create test UI [v3]
> This enhances the `Builder` pattern added in
> [JDK-8294535](https://bugs.openjdk.org/browse/JDK-8294535) with a new method
> `testUI` which allows passing a lambda expression or a method reference to
> create *the test UI window*.
>
> The `PassFailJFrame` will automatically call the method on the EDT to create
> the UI, add it to the internal list of windows, install the window closing
> listener and finally position and show both the instructions and test UI.
>
> Alternatively, you can pass an already created window.
>
> The `main` method of a manual test could look as simple as a sequence of
> calls:
>
>
> public static void main(String[] args) throws Exception {
> PassFailJFrame.builder()
> .instructions(INSTRUCTIONS)
> .testUI(() -> createTestUI())
> .build()
> .awaitAndCheck();
> }
>
> where `createTestUI` returns a test UI window.
Alexey Ivanov has updated the pull request incrementally with one additional
commit since the last revision:
Add instructions on how to use PassFailJFrame
-
Changes:
- all: https://git.openjdk.org/jdk/pull/15665/files
- new: https://git.openjdk.org/jdk/pull/15665/files/62521e38..0be29d16
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=15665=02
- incr: https://webrevs.openjdk.org/?repo=jdk=15665=01-02
Stats: 73 lines in 1 file changed: 73 ins; 0 del; 0 mod
Patch: https://git.openjdk.org/jdk/pull/15665.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/15665/head:pull/15665
PR: https://git.openjdk.org/jdk/pull/15665
Re: RFR: 8169475 : WheelModifier.java fails by timeout [v2]
On Mon, 23 Oct 2023 03:28:00 GMT, Renjith Kannath Pariyangad
wrote:
>> Hi Reviewers,
>>
>> I have added additional CountDownLatch for making sure button is visible
>> before proceeding,(observe this was one of the key reason for failure) so
>> this helps to improve test case stability. Modified _await()_ function by
>> adding 1sec timeout with exception, this will help to reduce total execution
>> time in case of failure.
>> Please review and let me know your suggestions, if any.
>
> Renjith Kannath Pariyangad has updated the pull request incrementally with
> one additional commit since the last revision:
>
> Added null check before frame dispose
Changes requested by aivanov (Reviewer).
test/jdk/java/awt/event/MouseWheelEvent/WheelModifier/WheelModifier.java line
141:
> 139: r.mouseMove(sLoc.x + bSize.width / 2, sLoc.y + bSize.height * 2);
> 140: if (!exitSema.await(1, TimeUnit.SECONDS)) {
> 141: throw new RuntimeException("Mouse is not moved");
Suggestion:
throw new RuntimeException("Mouse did not exit");
test/jdk/java/awt/event/MouseWheelEvent/WheelModifier/WheelModifier.java line
148:
> 146: r.mouseWheel(1);
> 147: if (!wheelSema.await(1, TimeUnit.SECONDS)) {
> 148: throw new RuntimeException("Mouse is not wheeled");
Suggestion:
throw new RuntimeException("Mouse is not wheeled 1");
For consistency with the printed message.
test/jdk/java/awt/event/MouseWheelEvent/WheelModifier/WheelModifier.java line
174:
> 172: } finally {
> 173: if (test.f != null)
> 174: SwingUtilities.invokeAndWait(test.f::dispose);
This is not thread-safe.
Suggestion:
SwingUtilities.invokeAndWait(() -> {
if (test.f != null) {
test.f.dispose();
}
});
Please always use braces even when there's only one statement in the body.
-
PR Review: https://git.openjdk.org/jdk/pull/16281#pullrequestreview-1693154589
PR Review Comment: https://git.openjdk.org/jdk/pull/16281#discussion_r1369043388
PR Review Comment: https://git.openjdk.org/jdk/pull/16281#discussion_r1369043551
PR Review Comment: https://git.openjdk.org/jdk/pull/16281#discussion_r1369046750
Re: RFR: 8169475 : WheelModifier.java fails by timeout [v2]
On Mon, 23 Oct 2023 03:28:00 GMT, Renjith Kannath Pariyangad wrote: >> Hi Reviewers, >> >> I have added additional CountDownLatch for making sure button is visible >> before proceeding,(observe this was one of the key reason for failure) so >> this helps to improve test case stability. Modified _await()_ function by >> adding 1sec timeout with exception, this will help to reduce total execution >> time in case of failure. >> Please review and let me know your suggestions, if any. > > Renjith Kannath Pariyangad has updated the pull request incrementally with > one additional commit since the last revision: > > Added null check before frame dispose The updated test failed once on Linux when I ran the test on Friday, the exception was: Exception: java.lang.RuntimeException: Didn't receive focus in time Today I also got a few failures in 8u: Exception: java.lang.RuntimeException: Mouse is not pressed on Linux and on Windows. The good thing is that failure takes about 10 seconds instead of more than 5 minutes. - PR Comment: https://git.openjdk.org/jdk/pull/16281#issuecomment-1775706452
Re: RFR: 8305321: Remove unused exports in java.desktop [v5]
On Fri, 13 Oct 2023 15:19:53 GMT, Daniel Jeliński wrote: >> Please review this patch that removes a number of unused exports from >> java.desktop native libraries. >> >> In most cases I removed JNIEXPORT from methods and variables that are only >> used within a single shared library. Other than that: >> - removed `getSunFontIDs` that was reportedly used by rasterizer; as far as >> I could tell, rasterizer project is dead now, but if that's incorrect I can >> restore that export. >> - removed `colorValueID` in X11Color; that field was not used. >> - removed `J2dTraceInit` from header file. That method is only used >> internally by `J2dTraceImpl`. >> >> The methods `Transform_GetInfo` and `Transform_transform` are declared in >> GraphicsPrimitiveMgr, but are only used in TransformHelper. Let me know if I >> should move them to where they are used. >> >> The method `img_makePalette`, currently located in >> `share/native/libawt/awt/image/cvutils/img_colors.c`, is only used by >> `unix/native/common/awt/X11Color.c`; it could be moved to the same directory >> to avoid exporting the method from libawt. The files `img_colors.[ch]` do >> not have any references to other files in `cvutils`. >> >> Manually verified that the exports are no longer present after these >> changes. Tier1-3 and client libs tests still pass. > > Daniel Jeliński has updated the pull request incrementally with seven > additional commits since the last revision: > > - Revert "Remove Color_initIDs" > >This reverts commit 8a8c9a63de14773905a64a8067ddd68c8a6ab137. > - Revert "Remove KeyboardFocusManager_initIDs" > >This reverts commit 66bd9a136b4c34d7d600ad49ce67fcd060160fb8. > - Revert "Remove Rectangle_initIDs" > >This reverts commit 0d21e361cd92215bbc8bb971b5c3f26898b96a0b. > - Revert "Remove Button_initIDs" > >This reverts commit 423afbf3b61cad504ae10a99ceb02c318f3a83ae. > - Revert "Remove FileDialog_initIDs" > >This reverts commit e66a9ffdda6e451c4298f59786adbd434dd1adb5. > - Revert "Remove TextField_initIDs" > >This reverts commit 0c3b78d27b899d4301f8326f90f1cc36703cbd3d. > - Revert "Update copyright" > >This reverts commit 5808c65e5fbabc08b43da982736b40db87000cb5. Approved. Further clean-up will be handled separately. I agree. Yet if they're unused from outside, there's no need to export them. After all, you hid some functions from `GrPrim_*` family. A separate PR will do. > > `jvm` > > That's an odd beast; it is used to retrieve a pointer to JNIEnv. The pointer > to JNIEnv is available in all JNI functions, so we could do without a global > jvm variable. Yes, it's possible to do without it. I believe it's primarily used for up-calls. Anyway, it's not something to address here. - Marked as reviewed by aivanov (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13261#pullrequestreview-1693101493 PR Comment: https://git.openjdk.org/jdk/pull/13261#issuecomment-1775662294
Re: RFR: 8305321: Remove unused exports in java.desktop [v5]
On Fri, 13 Oct 2023 15:19:53 GMT, Daniel Jeliński wrote: >> Please review this patch that removes a number of unused exports from >> java.desktop native libraries. >> >> In most cases I removed JNIEXPORT from methods and variables that are only >> used within a single shared library. Other than that: >> - removed `getSunFontIDs` that was reportedly used by rasterizer; as far as >> I could tell, rasterizer project is dead now, but if that's incorrect I can >> restore that export. >> - removed `colorValueID` in X11Color; that field was not used. >> - removed `J2dTraceInit` from header file. That method is only used >> internally by `J2dTraceImpl`. >> >> The methods `Transform_GetInfo` and `Transform_transform` are declared in >> GraphicsPrimitiveMgr, but are only used in TransformHelper. Let me know if I >> should move them to where they are used. >> >> The method `img_makePalette`, currently located in >> `share/native/libawt/awt/image/cvutils/img_colors.c`, is only used by >> `unix/native/common/awt/X11Color.c`; it could be moved to the same directory >> to avoid exporting the method from libawt. The files `img_colors.[ch]` do >> not have any references to other files in `cvutils`. >> >> Manually verified that the exports are no longer present after these >> changes. Tier1-3 and client libs tests still pass. > > Daniel Jeliński has updated the pull request incrementally with seven > additional commits since the last revision: > > - Revert "Remove Color_initIDs" > >This reverts commit 8a8c9a63de14773905a64a8067ddd68c8a6ab137. > - Revert "Remove KeyboardFocusManager_initIDs" > >This reverts commit 66bd9a136b4c34d7d600ad49ce67fcd060160fb8. > - Revert "Remove Rectangle_initIDs" > >This reverts commit 0d21e361cd92215bbc8bb971b5c3f26898b96a0b. > - Revert "Remove Button_initIDs" > >This reverts commit 423afbf3b61cad504ae10a99ceb02c318f3a83ae. > - Revert "Remove FileDialog_initIDs" > >This reverts commit e66a9ffdda6e451c4298f59786adbd434dd1adb5. > - Revert "Remove TextField_initIDs" > >This reverts commit 0c3b78d27b899d4301f8326f90f1cc36703cbd3d. > - Revert "Update copyright" > >This reverts commit 5808c65e5fbabc08b43da982736b40db87000cb5. > * `AWTIsHeadless`: it's defined in `libawt/windows/awt_Toolkit.cpp` as well > as in `unix/native/libawt/awt/awt_LoadLibrary.c`. It is exported from > `awt.dll` and `libawt.so`. *It looks as if ~~its~~ this export can be removed > from `awt.dll`*. It is also exported from `libawt.so` and it's used from > `libawt_xawt.so`. I am not suggesting addressing it here, I'd rather take it to another issue. - PR Comment: https://git.openjdk.org/jdk/pull/13261#issuecomment-1775590943
Re: RFR: 8305321: Remove unused exports in java.desktop [v5]
On Mon, 23 Oct 2023 15:46:26 GMT, Alexey Ivanov wrote: > `jvm`: I think it was discussed already. I see it's exported from > `libawt.so`, it is imported by `libawt_xawt.so` and `libawt_headless.so`. Or > do I misunderstand the split? Is `libawt.so` the common code for both > `libawt_xawt.so` and `libawt_headless.so` where the former is headful and the > latter is headless? That's the way it is: `libawt.so` is the common code, so it's expected to contain and export the `jvm` variable. - PR Comment: https://git.openjdk.org/jdk/pull/13261#issuecomment-1775553821
Re: RFR: 8301846: Invalid TargetDataLine after screen lock when using JFileChooser or COM library [v3]
On Fri, 21 Jul 2023 03:35:57 GMT, Sergey Bylokhov wrote: >> Renjith Kannath Pariyangad has updated the pull request incrementally with >> one additional commit since the last revision: >> >> Disabled OLE1 from CoInit > > Marked as reviewed by serb (Reviewer). > @mrserb Could you take a look at the updated code, please? A gentle reminder. If we don't hear from you, @mrserb, in a week, we'll integrate. - PR Comment: https://git.openjdk.org/jdk/pull/14898#issuecomment-1775515452
Re: RFR: JDK-8314731 : Add support for the alt attribute in the image type input HTML tag [v2]
On Wed, 30 Aug 2023 14:20:41 GMT, ScientificWare wrote:
>> This is referenced in Java Bug Database as
>> - [JDK-8314731 : Adds support for the alt attribute in the image type input
>> HTML tag.](https://bugs.java.com/bugdatabase/view_bug?bug_id=8314731)
>>
>> This is tracked in JBS as
>> - [JDK-8314731 : Add support for the alt attribute in the image type input
>> HTML tag](https://bugs.openjdk.java.net/browse/JDK-8314731)
>>
>> According [HTML 3.2
>> specification](https://www.w3.org/TR/2018/SPSD-html32-20180315/#input)
>>
>> `alt` is not an attribute of the `input` element.
>>
>> According [HTML 4.01
>> specifications](https://www.w3.org/TR/html4/interact/forms.html#h-17.4) :
>>
>>> ... For accessibility reasons, authors should provide [alternate
>>> text](https://www.w3.org/TR/html4/struct/objects.html#alternate-text) for
>>> the image via the
>>> [alt](https://www.w3.org/TR/html4/struct/objects.html#adef-alt) attribute.
>>> ...
>>
>> This feature is not implemented in `FormView.java`.
>>
>> ⚠️ ~~This also affects the HTML 32 DTD~~
>>
>> 
>>
>> Left before the patch and right after the patch.
>>
>>
>> import java.awt.BorderLayout;
>> import java.awt.Dimension;
>> import javax.swing.JEditorPane;
>> import javax.swing.JFrame;
>> import javax.swing.JScrollPane;
>> import javax.swing.SwingUtilities;
>> import javax.swing.text.Document;
>> import javax.swing.text.html.HTMLEditorKit;
>> import javax.swing.text.html.StyleSheet;
>>
>> public class HTMLAddsSupportAltInputTag {
>> public static void main(String[] args) {
>> new HTMLAddsSupportAltInputTag();
>> }
>>
>> public HTMLAddsSupportAltInputTag() {
>> SwingUtilities.invokeLater(new Runnable(){
>> public void run(){
>> JEditorPane jEditorPane = new JEditorPane();
>> jEditorPane.setEditable(false);
>> JScrollPane scrollPane = new JScrollPane(jEditorPane);
>> HTMLEditorKit kit = new HTMLEditorKit();
>> jEditorPane.setEditorKit(kit);
>> StyleSheet styleSheet = kit.getStyleSheet();
>> styleSheet.addRule("""
>> body {
>> color: #000;
>> font-family:times;
>> margin: 4px;
>> }
>> """);
>> String htmlString = """
>>
>>
>> > src="file:oracle_logo_50x50.jpg" alt="Logo Oracle JPG">
>>
>> https://git.openjdk.org/jdk/pull/15319#issuecomment-1775513269
Re: RFR: 8305321: Remove unused exports in java.desktop [v5]
On Fri, 13 Oct 2023 15:19:53 GMT, Daniel Jeliński wrote: >> Please review this patch that removes a number of unused exports from >> java.desktop native libraries. >> >> In most cases I removed JNIEXPORT from methods and variables that are only >> used within a single shared library. Other than that: >> - removed `getSunFontIDs` that was reportedly used by rasterizer; as far as >> I could tell, rasterizer project is dead now, but if that's incorrect I can >> restore that export. >> - removed `colorValueID` in X11Color; that field was not used. >> - removed `J2dTraceInit` from header file. That method is only used >> internally by `J2dTraceImpl`. >> >> The methods `Transform_GetInfo` and `Transform_transform` are declared in >> GraphicsPrimitiveMgr, but are only used in TransformHelper. Let me know if I >> should move them to where they are used. >> >> The method `img_makePalette`, currently located in >> `share/native/libawt/awt/image/cvutils/img_colors.c`, is only used by >> `unix/native/common/awt/X11Color.c`; it could be moved to the same directory >> to avoid exporting the method from libawt. The files `img_colors.[ch]` do >> not have any references to other files in `cvutils`. >> >> Manually verified that the exports are no longer present after these >> changes. Tier1-3 and client libs tests still pass. > > Daniel Jeliński has updated the pull request incrementally with seven > additional commits since the last revision: > > - Revert "Remove Color_initIDs" > >This reverts commit 8a8c9a63de14773905a64a8067ddd68c8a6ab137. > - Revert "Remove KeyboardFocusManager_initIDs" > >This reverts commit 66bd9a136b4c34d7d600ad49ce67fcd060160fb8. > - Revert "Remove Rectangle_initIDs" > >This reverts commit 0d21e361cd92215bbc8bb971b5c3f26898b96a0b. > - Revert "Remove Button_initIDs" > >This reverts commit 423afbf3b61cad504ae10a99ceb02c318f3a83ae. > - Revert "Remove FileDialog_initIDs" > >This reverts commit e66a9ffdda6e451c4298f59786adbd434dd1adb5. > - Revert "Remove TextField_initIDs" > >This reverts commit 0c3b78d27b899d4301f8326f90f1cc36703cbd3d. > - Revert "Update copyright" > >This reverts commit 5808c65e5fbabc08b43da982736b40db87000cb5. In general, it looks good. The number of exports has reduced. However, I identified several more exports which don't seem to be used: * `Region_CountIterationRects` * `SurfaceData_IntersectBoundsXYWH` * `mul8table` Could you please verify? In addition to the above, I found a couple of weird cases… * `AWTIsHeadless`: it's defined in `libawt/windows/awt_Toolkit.cpp` as well as in `unix/native/libawt/awt/awt_LoadLibrary.c`. It is exported from `awt.dll` and `libawt.so`. It looks as if its export can be removed from `awt.dll`. It is also exported from `libawt.so` and it's used from `libawt_xawt.so`. * `jvm`: I think it was discussed already. I see it' exported from `libawt.so`, it is imported by `libawt_xawt.so` and `libawt_headless.so`. Or do I misunderstand the split? Is `libawt.so` the common code for both `libawt_xawt.so` and `libawt_headless.so` where the former is headful and the latter is headless? - PR Review: https://git.openjdk.org/jdk/pull/13261#pullrequestreview-1692916643
Re: RFR: JDK-8314731 : Add support for the alt attribute in the image type input HTML tag [v2]
On Wed, 30 Aug 2023 14:20:41 GMT, ScientificWare wrote:
>> This is referenced in Java Bug Database as
>> - [JDK-8314731 : Adds support for the alt attribute in the image type input
>> HTML tag.](https://bugs.java.com/bugdatabase/view_bug?bug_id=8314731)
>>
>> This is tracked in JBS as
>> - [JDK-8314731 : Add support for the alt attribute in the image type input
>> HTML tag](https://bugs.openjdk.java.net/browse/JDK-8314731)
>>
>> According [HTML 3.2
>> specification](https://www.w3.org/TR/2018/SPSD-html32-20180315/#input)
>>
>> `alt` is not an attribute of the `input` element.
>>
>> According [HTML 4.01
>> specifications](https://www.w3.org/TR/html4/interact/forms.html#h-17.4) :
>>
>>> ... For accessibility reasons, authors should provide [alternate
>>> text](https://www.w3.org/TR/html4/struct/objects.html#alternate-text) for
>>> the image via the
>>> [alt](https://www.w3.org/TR/html4/struct/objects.html#adef-alt) attribute.
>>> ...
>>
>> This feature is not implemented in `FormView.java`.
>>
>> ⚠️ ~~This also affects the HTML 32 DTD~~
>>
>> 
>>
>> Left before the patch and right after the patch.
>>
>>
>> import java.awt.BorderLayout;
>> import java.awt.Dimension;
>> import javax.swing.JEditorPane;
>> import javax.swing.JFrame;
>> import javax.swing.JScrollPane;
>> import javax.swing.SwingUtilities;
>> import javax.swing.text.Document;
>> import javax.swing.text.html.HTMLEditorKit;
>> import javax.swing.text.html.StyleSheet;
>>
>> public class HTMLAddsSupportAltInputTag {
>> public static void main(String[] args) {
>> new HTMLAddsSupportAltInputTag();
>> }
>>
>> public HTMLAddsSupportAltInputTag() {
>> SwingUtilities.invokeLater(new Runnable(){
>> public void run(){
>> JEditorPane jEditorPane = new JEditorPane();
>> jEditorPane.setEditable(false);
>> JScrollPane scrollPane = new JScrollPane(jEditorPane);
>> HTMLEditorKit kit = new HTMLEditorKit();
>> jEditorPane.setEditorKit(kit);
>> StyleSheet styleSheet = kit.getStyleSheet();
>> styleSheet.addRule("""
>> body {
>> color: #000;
>> font-family:times;
>> margin: 4px;
>> }
>> """);
>> String htmlString = """
>>
>>
>> > src="file:oracle_logo_50x50.jpg" alt="Logo Oracle JPG">
>>
>> https://git.openjdk.org/jdk/pull/15319#pullrequestreview-1692371686
PR Review Comment: https://git.openjdk.org/jdk/pull/15319#discussion_r1368560286
Re: RFR: JDK-8314731 : Add support for the alt attribute in the image type input HTML tag [v2]
On Fri, 29 Sep 2023 12:12:23 GMT, Alexey Ivanov wrote: > You should probably pass `altAtt` as the `description` parameter, yet I'm > unsure if it's ever used in this context. As I say, I'm unsure if it's used… The description could be exposed to Accessibility API. By default, `ImageIcon` uses the URL as the description if it isn't provided: https://github.com/openjdk/jdk/blob/99de9bb83ff70fe81c89751516a86a94c8f552be/src/java.desktop/share/classes/javax/swing/ImageIcon.java#L231-L233 Now that you handle the `alt` attribute, you can pass the description if it's provided. - PR Review Comment: https://git.openjdk.org/jdk/pull/15319#discussion_r1368559705
Re: RFR: JDK-8314731 : Add support for the alt attribute in the image type input HTML tag [v2]
On Fri, 29 Sep 2023 12:41:10 GMT, Alexey Ivanov wrote:
>> ScientificWare has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Update src/java.desktop/share/classes/javax/swing/text/html/FormView.java
>>
>> FormView.java : Remove a redundant space.
>>
>> Co-authored-by: Andrey Turbanov
>
> src/java.desktop/share/classes/javax/swing/text/html/FormView.java line 285:
>
>> 283: button = icon.getImageLoadStatus() ==
>> MediaTracker.COMPLETE ? new JButton(icon) : new JButton(altAtt);
>> 284: } catch (MalformedURLException e) {
>> 285: button = new JButton(srcAtt);
>
> Suggestion:
>
> button = new JButton(altAtt);
>
> If `altAtt` is provided, it should be used to handle invalid `srcAtt` too.
> @aivanov-jdk My apologies, I forgot to reply. I wanted to distinguish the two
> cases : (Missing resource or bad resource name) and malformed URL. Like in
> the previous example I gave.
What purpose does this distinction serve?
It could give an additional insight to the app developer. At the same time, the
app developer should ensure the image loads.
Does the user of the app care about it [the distinction]? Unlikely. The end
result is the image isn't loaded. As HTML specifies, the `alt` attribute
contains the image description, which is more useful to the user in _all_ the
cases where the image failed to load.
Therefore I think you should fallback to `altAtt` if it's provided.
This actually raises another question: what if `altAtt` isn't provided? Should
it fallback to `srcURL`?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15319#discussion_r1368554059
Re: RFR: 8169475 : WheelModifier.java fails by timeout
On Fri, 20 Oct 2023 10:13:40 GMT, Dmitry Markov wrote: > It would be good to ensure that f is not NULL before dispose. I know we're doing it all the time… But is it really relevant? This has been bothering me for a while now… Let's clear things up. If the frame is `null`, something has gone awry. If the test code itself didn't throw the exception, we'll get: Exception in thread "main" java.lang.NullPointerException at WheelModifier.main(WheelModifier.java:173) The bad thing is that any exception thrown from the test itself — in the `try` block — gets overridden by the exception thrown from the `finally` block. Thus, we get the same exception as above. The overall result is that the test fails, yet it may hide the real reason, which makes it harder to analyse the root cause of the failure. Taking into account the above, *`null`-check is good even though it makes the code longer.* - PR Review Comment: https://git.openjdk.org/jdk/pull/16281#discussion_r1366862441
Re: RFR: 8169475 : WheelModifier.java fails by timeout
On Fri, 20 Oct 2023 06:59:17 GMT, Renjith Kannath Pariyangad wrote: > Hi Reviewers, > > I have added additional CountDownLatch for making sure button is visible > before proceeding,(observe this was one of the key reason for failure) so > this helps to improve test case stability. Modified _await()_ function by > adding 1sec timeout with exception, this will help to reduce total execution > time in case of failure. > Please review and let me know your suggestions, if any. Please add the `null`-check in the dispose code. - Changes requested by aivanov (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16281#pullrequestreview-1689854730
Integrated: 8318448: Link PopupMenu/PopupMenuLocation.java failure to JDK-8259913
On Wed, 18 Oct 2023 13:21:17 GMT, Alexey Ivanov wrote: > Update ProblemList entry for java/awt/PopupMenu/PopupMenuLocation.java to > [JDK-8259913](https://bugs.openjdk.org/browse/JDK-8259913) which is a > specific bug for AWT menus in High DPI environment. This pull request has now been integrated. Changeset: 71c99a0e Author: Alexey Ivanov URL: https://git.openjdk.org/jdk/commit/71c99a0e59ff843d48f1c71fb045186e44f83943 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8318448: Link PopupMenu/PopupMenuLocation.java failure to JDK-8259913 Reviewed-by: prr - PR: https://git.openjdk.org/jdk/pull/16246
Integrated: 8318101: Additional test cases for CSSAttributeEqualityBug
On Fri, 13 Oct 2023 20:21:04 GMT, Alexey Ivanov wrote: > Adds additional test cases to > `javax/swing/text/html/CSS/CSSAttributeEqualityBug.java`. > > Currently, CSS parser in Java allows space between the number and the unit or > percent. This is what the additional test cases verify. > > There's also one additional case for `border-width: medium`. This pull request has now been integrated. Changeset: 2c23391d Author:Alexey Ivanov URL: https://git.openjdk.org/jdk/commit/2c23391de76be0994d8367fdfba63a98e9faa63d Stats: 25 lines in 1 file changed: 23 ins; 1 del; 1 mod 8318101: Additional test cases for CSSAttributeEqualityBug Reviewed-by: prr - PR: https://git.openjdk.org/jdk/pull/16193
RFR: 8318448: Link PopupMenu/PopupMenuLocation.java failure to JDK-8259913
Update ProblemList entry for java/awt/PopupMenu/PopupMenuLocation.java to [JDK-8259913](https://bugs.openjdk.org/browse/JDK-8259913) which is a specific bug for AWT menus in High DPI environment. - Commit messages: - 8318448: Link PopupMenu/PopupMenuLocation.java failure to JDK-8259913 Changes: https://git.openjdk.org/jdk/pull/16246/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16246=00 Issue: https://bugs.openjdk.org/browse/JDK-8318448 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16246.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16246/head:pull/16246 PR: https://git.openjdk.org/jdk/pull/16246
Re: RFR: JDK-8314246: javax/swing/JToolBar/4529206/bug4529206.java fails intermittently on Linux [v2]
On Wed, 16 Aug 2023 23:46:09 GMT, Harshitha Onkar wrote: > Oh, I didn't notice that `setAutoWaitForIdle()` was not used the first time. > I have changed it. I think adding a small amount of delay after > `makeToolbarFloat()` is good in addition to `setAutoWaitForIdle()` to > stabilize the test on slower systems. I think this was a bad decision: now the test **never** calls `waitForIdle()`. But it should do it… for stability… after it takes any action. This test does not use `Robot` to send any events, but `robot.setAutoWaitForIdle(true)` affects only *mouse and keyboard events*: [`Robot.autoWaitForIdle()`](https://github.com/openjdk/jdk/blob/6fc35142315f1616fa35e415005c9483939c6920/src/java.desktop/share/classes/java/awt/Robot.java#L688) is called from `afterEvent`, just like `autoDelay`: https://github.com/openjdk/jdk/blob/6fc35142315f1616fa35e415005c9483939c6920/src/java.desktop/share/classes/java/awt/Robot.java#L662-L665 I submitted [JDK-8318423](https://bugs.openjdk.org/browse/JDK-8318423): Add explicit robot.waitForIdle to JToolBar/4529206/bug4529206.java - PR Review Comment: https://git.openjdk.org/jdk/pull/15316#discussion_r1363726279
Re: RFR: 8318104: macOS 10.13 check in TabButtonAccessibility.m can be removed
On Tue, 17 Oct 2023 11:42:40 GMT, Abhishek Kumar wrote: > Since[ JDK-8317970](https://bugs.openjdk.org/browse/JDK-8317970) is > integrated to make macOS 11 the minimum version to build jdk, the 10.13 check > is not needed and can be removed from TabButtonAccessibility.m file. Marked as reviewed by aivanov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16218#pullrequestreview-1682160981
Re: RFR: 8318102: macos10.14 check in CSystemColors can be removed.
On Mon, 16 Oct 2023 07:43:06 GMT, Prasanta Sadhukhan wrote: > Since [ JDK-8317970](https://bugs.openjdk.org/browse/JDK-8317970) in > integrated to make macOS 11 the minimum version to build jdk, the 10.14 check > added in [JDK-8284166](https://bugs.openjdk.org/browse/JDK-8284166) is not > needed and can be removed.. Marked as reviewed by aivanov (Reviewer). src/java.desktop/macosx/native/libawt_lwawt/awt/CSystemColors.m line 1: > 1: /* Should the copyright year be updated to 2023? [JDK-8284166](https://bugs.openjdk.org/browse/JDK-8284166) updated it. - PR Review: https://git.openjdk.org/jdk/pull/16198#pullrequestreview-1680812899 PR Review Comment: https://git.openjdk.org/jdk/pull/16198#discussion_r1361175865
Re: RFR: 8317696: Fix compilation with clang-16 [v4]
On Wed, 11 Oct 2023 09:27:30 GMT, Jan Kratochvil wrote: >> `--with-toolchain-type=clang` fails the compilation for me with >> `clang-16.0.6-3.fc38.x86_64` >> >> While the warnings can be disabled I find better to just fix them. The GTK >> prototypes in JDK reported by clang are either missing or wrong. >> >> >> src/java.base/unix/native/libnet/DefaultProxySelector.c:378:41: error: >> passing arguments to a function without a prototype is deprecated in all >> versions of C and is not supported in C2x >> [-Werror,-Wdeprecated-non-prototype] >> proxies = (*g_proxy_resolver_lookup)(resolver, uri, NULL, ); >> ^ >> src/java.base/unix/native/libnet/DefaultProxySelector.c:397:63: error: >> passing arguments to a function without a prototype is deprecated in all >> versions of C and is not supported in C2x >> [-Werror,-Wdeprecated-non-prototype] >> (*g_network_address_parse_uri)(proxies[i], 0, >> ^ >> src/java.base/unix/native/libnet/DefaultProxySelector.c:402:70: error: >> passing arguments to a function without a prototype is deprecated in all >> versions of C and is not supported in C2x >> [-Werror,-Wdeprecated-non-prototype] >> phost = (*g_network_address_get_hostname)(conn); >> ^ >> src/java.base/unix/native/libnet/DefaultProxySelector.c:403:66: error: >> passing arguments to a function without a prototype is deprecated in all >> versions of C and is not supported in C2x >> [-Werror,-Wdeprecated-non-prototype] >> pport = (*g_network_address_get_port)(conn); >> ^ >> src/java.base/unix/native/libnet/DefaultProxySelector.c:445:22: error: >> passing arguments to a function without a prototype is deprecated in all >> versions of C and is not supported in C2x >> [-Werror,-Wdeprecated-non-prototype] >> (*g_strfreev)(proxies); >> ^ >> src/java.base/unix/native/libnet/DefaultProxySelector.c:448:25: error: >> passing arguments to a function without a prototype is deprecated in all >> versions of C and is not supported in C2x >> [-Werror,-Wdeprecated-non-prototype] >> (*g_clear_error)(); >> ^ >> Compiling X11Renderer.c (for libawt_headless.so) >> In file included from >> src/java.desktop/unix/native/common/java2d/x11/X11PMBlitLoops.c:29: >> In file included from >> src/java.desktop/unix/native/common/java2d/x11/X11Surf... > > Jan Kratochvil has updated the pull request incrementally with one additional > commit since the last revision: > > Remove the XShmQueryExtension declaration completely. Client tests are green. Looks good to me. - Marked as reviewed by aivanov (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16092#pullrequestreview-1679806119
Re: RFR: JDK-8317112 : Add screenshot for Frame/DefaultSizeTest.java
On Fri, 6 Oct 2023 03:24:36 GMT, Renjith Kannath Pariyangad wrote: > Hi Reviewers, > I have added screen shot capturing facility to the test case and that will > help for debugging in case of failure. Please review and let me know your > suggestions or comments if any. > > Regards, > Renjith. Marked as reviewed by aivanov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16067#pullrequestreview-1678845664
RFR: 8318101: Additional test cases for CSSAttributeEqualityBug
Adds additional test cases to `javax/swing/text/html/CSS/CSSAttributeEqualityBug.java`. Currently, CSS parser in Java allows space between the number and the unit or percent. This is what the additional test cases verify. There's also one additional case for `border-width: medium`. - Commit messages: - 8318101: Additional test cases for CSSAttributeEqualityBug Changes: https://git.openjdk.org/jdk/pull/16193/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16193=00 Issue: https://bugs.openjdk.org/browse/JDK-8318101 Stats: 25 lines in 1 file changed: 23 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16193.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16193/head:pull/16193 PR: https://git.openjdk.org/jdk/pull/16193
Re: RFR: 8169475 : WheelModifier.java fails by timeout
On Fri, 13 Oct 2023 15:07:30 GMT, Renjith Kannath Pariyangad wrote: > Hi Revivers, > > Part of stability improvement for the test case **WheelModifier.java** I have > moved _getLocationOnScreen()_ and _getSize()_ into EDT, part of warning > removal updated _BUTTON1_MASK_ with _BUTTON1_DOWN_MASK_ > > Please review and let me know your suggestions. Looks good to me. - Marked as reviewed by aivanov (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16185#pullrequestreview-1676789933
Re: RFR: 8305321: Remove unused exports in java.desktop [v3]
On Fri, 13 Oct 2023 15:02:10 GMT, Daniel Jeliński wrote: > …will file a follow-up PR once this one is merged You can submit it right away, so that we'll not forget about it. - PR Review Comment: https://git.openjdk.org/jdk/pull/13261#discussion_r1358385429
Re: RFR: 8315484: java/awt/dnd/RejectDragDropActionTest.java timed out [v8]
On Thu, 12 Oct 2023 22:01:22 GMT, Damon Nguyen wrote:
>> test/jdk/java/awt/dnd/RejectDragDropActionTest.java line 103:
>>
>>> 101: !p.equals(endPoint) && !incorrectActionDetected;
>>> 102: p.translate(sign(endPoint.x - p.x),
>>> 103: sign(endPoint.y - p.y))) {
>>
>> Suggestion:
>>
>> p.translate(sign(endPoint.x - p.x),
>> sign(endPoint.y - p.y))) {
>>
>> I'm probably the only one who prefers aligning wrapped lines… Doesn't it
>> look better?
>
> I agree, it does look neater. Unfortunately, my IDE doesn't default this
> (which makes sense) so I have to remember to do this when applicable. Thanks.
I configured IDE to do it by default. Yet it doesn't make sense in all the
cases.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16018#discussion_r1358382948
Re: RFR: 8305321: Remove unused exports in java.desktop [v3]
On Fri, 13 Oct 2023 14:38:59 GMT, Daniel Jeliński wrote: > Should I move the cleanup of Color as well? Removing `colorValueID` could be left here, I think: it removes an exported symbol which is no longer used. In addition to it, it makes `Java_java_awt_Color_initIDs` empty. - PR Review Comment: https://git.openjdk.org/jdk/pull/13261#discussion_r1358369477
Re: RFR: 8305321: Remove unused exports in java.desktop [v3]
On Fri, 13 Oct 2023 07:50:12 GMT, Daniel Jeliński wrote: >> ~Good point. I only checked the Linux exports/imports, will check the >> Windows-specific exports now.~ > > Wait a sec, these symbols are removed from the export list in this PR. Are > you saying that you still see them? I saw these symbols… unless I downloaded the wrong build. - PR Review Comment: https://git.openjdk.org/jdk/pull/13261#discussion_r1358365392
Re: RFR: 8305321: Remove unused exports in java.desktop [v3]
On Fri, 13 Oct 2023 11:17:31 GMT, Daniel Jeliński wrote: > Found and removed a few more initIDs methods that were empty on all platforms. It could be a better idea to move this clean up to a separate bugid with appropriate title. After all, removing empty initIDs affects Java sources as well. If anything goes wrong, it'll be easier to isolate the changes. What do you think? - PR Review Comment: https://git.openjdk.org/jdk/pull/13261#discussion_r1358342299
Integrated: 8318028: Remove unused class="centered" from FocusCycle.svg
On Thu, 12 Oct 2023 17:12:05 GMT, Alexey Ivanov wrote: > **Cleanup** > > A trivial modification which removes `class="centered"` from one of the > `` elements. It's a remnant from a previous version of the image. > > Classes aren't used, all `` elements are centered. This pull request has now been integrated. Changeset: 7d31146f Author:Alexey Ivanov URL: https://git.openjdk.org/jdk/commit/7d31146f4d4ec81728c591d839ee2bb942e5e5fa Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod 8318028: Remove unused class="centered" from FocusCycle.svg Reviewed-by: serb - PR: https://git.openjdk.org/jdk/pull/16168
Re: RFR: 8315484: java/awt/dnd/RejectDragDropActionTest.java timed out [v9]
On Thu, 12 Oct 2023 20:07:18 GMT, Damon Nguyen wrote: >> This test intermittently fails by timeout. Increasing the timeout alone >> doesn't solve the failure as it still fails in about 400 runs. Adding >> another delay and reducing the delay amount to 1000ms. Now, the test passes >> after 2 sets of 500 repeats on all OS's without a timeout. > > Damon Nguyen has updated the pull request incrementally with one additional > commit since the last revision: > > Alignment fix Marked as reviewed by aivanov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16018#pullrequestreview-1675090907
Re: RFR: 8266242: java/awt/GraphicsDevice/CheckDisplayModes.java failing on macOS 11 ARM
On Thu, 12 Oct 2023 17:54:07 GMT, Alexander Zuev wrote: > As a workaround changing the method that filters out valid resolutions to not > allow resolutions unsupported by Apple m1/m2 chips to be reported back to > Java side. > > Also removing test from problem list as it should pass again now. Marked as reviewed by aivanov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16169#pullrequestreview-1675104051
RFR: 8318028: Remove unused class="centered" from FocusCycle.svg
**Cleanup** A trivial modification which removes `class="centered"` from one of the `` elements. It's a remnant from a previous version of the image. Classes aren't used, all `` elements are centered. - Commit messages: - 8318028: Remove unused class="centered" from FocusCycle.svg Changes: https://git.openjdk.org/jdk/pull/16168/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16168=00 Issue: https://bugs.openjdk.org/browse/JDK-8318028 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16168.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16168/head:pull/16168 PR: https://git.openjdk.org/jdk/pull/16168
Re: RFR: 8315484: java/awt/dnd/RejectDragDropActionTest.java timed out [v3]
On Thu, 5 Oct 2023 17:43:21 GMT, Damon Nguyen wrote: > However, since the accumulation of the changes doesn't seem to cause any > failures anymore, should I just follow through with this PR? I can raise > another bug if needed for this intermittent failure. Can't identify what's > causing this though.. It could be that the increased timeout gives enough time for AWT to cleanup and it eventually exits. - PR Review Comment: https://git.openjdk.org/jdk/pull/16018#discussion_r1357077220
Re: RFR: 8315484: java/awt/dnd/RejectDragDropActionTest.java timed out [v8]
On Thu, 5 Oct 2023 16:20:08 GMT, Damon Nguyen wrote:
>> This test intermittently fails by timeout. Increasing the timeout alone
>> doesn't solve the failure as it still fails in about 400 runs. Adding
>> another delay and reducing the delay amount to 1000ms. Now, the test passes
>> after 2 sets of 500 repeats on all OS's without a timeout.
>
> Damon Nguyen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove excess
Marked as reviewed by aivanov (Reviewer).
test/jdk/java/awt/dnd/RejectDragDropActionTest.java line 103:
> 101: !p.equals(endPoint) && !incorrectActionDetected;
> 102: p.translate(sign(endPoint.x - p.x),
> 103: sign(endPoint.y - p.y))) {
Suggestion:
p.translate(sign(endPoint.x - p.x),
sign(endPoint.y - p.y))) {
I'm probably the only one who prefers aligning wrapped lines… Doesn't it look
better?
-
PR Review: https://git.openjdk.org/jdk/pull/16018#pullrequestreview-1674614574
PR Review Comment: https://git.openjdk.org/jdk/pull/16018#discussion_r1357075634
Re: RFR: 8305321: Remove unused exports in java.desktop [v3]
On Wed, 11 Oct 2023 17:48:28 GMT, Daniel Jeliński wrote: >> src/java.desktop/share/native/libawt/java2d/SurfaceData.h line 557: >> >>> 555: */ >>> 556: SurfaceDataOps * >>> 557: SurfaceData_GetOpsNoSetup(JNIEnv *env, jobject sData); >> >> ~~It seems to me none of the functions in `SurfaceData.h` should be >> exported. They all end up in `awt.dll` / `libawt.so`. I can't see any of >> these functions are accessed from other DLLs.~~ >> >> ~~Would you like to create a separate task for removing exports from >> `SurfaceData` functions?~~ >> >> Some are used from `fontmanager.dll`. But not as many as we export. >> >> Potentially all symbols exported from `awt.dll` except from `Java*` can be >> hidden… >> I found that `Disposer_AddRecord` and `J2dTraceImpl` are used from >> `lcms.dll`. > > Keep in mind that on Linux libawt is split into libawt, libawt_headless and > libawt_xawt, and the export/import dependencies are different. I totally understand this. However, Windows specific symbols: 98 000380A0 GDIWinSD_InitDC 109 000385F0 GDIWindowSurfaceData_GetComp 11A 000386C0 GDIWindowSurfaceData_GetOps 12B 000386D0 GDIWindowSurfaceData_GetOpsNoSetup 13C 000386E0 GDIWindowSurfaceData_GetWindow and others are still exported from `awt.dll`, I believe these can be made private. Yet I agree we need to be careful not to hide something that's used from other libraries. - PR Review Comment: https://git.openjdk.org/jdk/pull/13261#discussion_r1356588203
Re: RFR: 8305321: Remove unused exports in java.desktop [v3]
On Wed, 11 Oct 2023 17:54:31 GMT, Daniel Jeliński wrote: > `img_makePalette` is in libawt.so, `X11Color.c` is in libawt_xawt.so I see. I didn't check Linux builds, but I should've. - PR Comment: https://git.openjdk.org/jdk/pull/13261#issuecomment-1759299249
Re: RFR: 8301846: Invalid TargetDataLine after screen lock when using JFileChooser or COM library [v6]
On Tue, 1 Aug 2023 04:16:30 GMT, Renjith Kannath Pariyangad wrote: > Did further investigation on JDK-7116070 (name truncation issue) and found > its [MS > structure](https://learn.microsoft.com/en-us/windows-hardware/drivers/audio/extended-capabilities-from-a-wdm-audio-driver) > limitation, this structure can accommodate max 31 char only for _szPname_ . > > Workflow : **PLATFORM_API_WinOS_Ports.c** loading the description with the > help of _mixerGetDevCapsW_ function and result will be stored into the > [MIXERCAPSW](https://learn.microsoft.com/en-us/windows/win32/api/mmeapi/ns-mmeapi-mixercapsw) > structure and in this max size for _szPname_ is 31 char. In my analysis this > is a limitation and we can't do anything more, let me know if you are aware > any alternative solution for this. @Renjithkannath Please, add this evaluation to [JDK-7116070](https://bugs.openjdk.org/browse/JDK-7116070). The bug itself can be closed as *External*: we can do nothing about it, it's a limitation of Windows API. If Java migrates to a newer audio API, JDK-7116070 will be resolved. - PR Comment: https://git.openjdk.org/jdk/pull/14898#issuecomment-1759297005
Re: RFR: 8301846: Invalid TargetDataLine after screen lock when using JFileChooser or COM library [v3]
On Fri, 21 Jul 2023 03:35:57 GMT, Sergey Bylokhov wrote: >> Renjith Kannath Pariyangad has updated the pull request incrementally with >> one additional commit since the last revision: >> >> Disabled OLE1 from CoInit > > Marked as reviewed by serb (Reviewer). @mrserb Could you take a look at the updated code, please? - PR Comment: https://git.openjdk.org/jdk/pull/14898#issuecomment-1759280160
Re: RFR: 8301846: Invalid TargetDataLine after screen lock when using JFileChooser or COM library [v15]
On Thu, 12 Oct 2023 06:40:13 GMT, Renjith Kannath Pariyangad wrote: >> Hi Reviewers, >> >> Observations : >> 1. Without com initialize if we access Mixer for recording, library loaded >> invalid GUID and clipped description in windows(ID not found in registry). >> With com initialization library load proper GUID (same as registry). >> 2. For Play back device always loading proper device GUID irrespective of >> com Initialization. >> >> Test: >> Since screen lock and unlock workflow required for reproducing this issue, >> did coupe of iteration of manual testing post fix and confirmed its >> resolving the problem. >> To reconfirm nothing is broken, executed all audio related test cases on >> test bench post fix and all are green. >> >> Please review the changes and let me know your comments if any. >> >> Regards, >> Renjith. > > Renjith Kannath Pariyangad has updated the pull request incrementally with > one additional commit since the last revision: > > Added spaces around the assignment operator Marked as reviewed by aivanov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14898#pullrequestreview-1673740110
Re: RFR: 8305321: Remove unused exports in java.desktop [v3]
On Thu, 5 Oct 2023 16:29:26 GMT, Daniel Jeliński wrote: >> Please review this patch that removes a number of unused exports from >> java.desktop native libraries. >> >> In most cases I removed JNIEXPORT from methods and variables that are only >> used within a single shared library. Other than that: >> - removed `getSunFontIDs` that was reportedly used by rasterizer; as far as >> I could tell, rasterizer project is dead now, but if that's incorrect I can >> restore that export. >> - removed `colorValueID` in X11Color; that field was not used. >> - removed `J2dTraceInit` from header file. That method is only used >> internally by `J2dTraceImpl`. >> >> The methods `Transform_GetInfo` and `Transform_transform` are declared in >> GraphicsPrimitiveMgr, but are only used in TransformHelper. Let me know if I >> should move them to where they are used. >> >> The method `img_makePalette`, currently located in >> `share/native/libawt/awt/image/cvutils/img_colors.c`, is only used by >> `unix/native/common/awt/X11Color.c`; it could be moved to the same directory >> to avoid exporting the method from libawt. The files `img_colors.[ch]` do >> not have any references to other files in `cvutils`. >> >> Manually verified that the exports are no longer present after these >> changes. Tier1-3 and client libs tests still pass. > > Daniel Jeliński 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 four additional > commits since the last revision: > > - Merge remote-tracking branch 'origin' into desktop-exports > - Merge remote-tracking branch 'origin' into desktop-exports > - Make J2dTraceInit static > - Remove unnecessary exports > The method img_makePalette, currently located in > share/native/libawt/awt/image/cvutils/img_colors.c, is only used by > unix/native/common/awt/X11Color.c; it could be moved to the same directory to > avoid exporting the method from libawt. The files img_colors.[ch] do not have > any references to other files in cvutils. Isn't `X11Color.c` linked into `libawt`? I expect that it is. In this case, `img_makePalette` doesn't need to be exported, just declared in the header. > The methods Transform_GetInfo and Transform_transform are declared in > GraphicsPrimitiveMgr, but are only used in TransformHelper. Let me know if I > should move them to where they are used. I don't know the history… If they're not used anywhere else, we can move them to where they're used and declare them static there. But I don't have a strong opinion here. - PR Comment: https://git.openjdk.org/jdk/pull/13261#issuecomment-1758168753
Re: RFR: 8305321: Remove unused exports in java.desktop [v3]
On Thu, 5 Oct 2023 16:29:26 GMT, Daniel Jeliński wrote:
>> Please review this patch that removes a number of unused exports from
>> java.desktop native libraries.
>>
>> In most cases I removed JNIEXPORT from methods and variables that are only
>> used within a single shared library. Other than that:
>> - removed `getSunFontIDs` that was reportedly used by rasterizer; as far as
>> I could tell, rasterizer project is dead now, but if that's incorrect I can
>> restore that export.
>> - removed `colorValueID` in X11Color; that field was not used.
>> - removed `J2dTraceInit` from header file. That method is only used
>> internally by `J2dTraceImpl`.
>>
>> The methods `Transform_GetInfo` and `Transform_transform` are declared in
>> GraphicsPrimitiveMgr, but are only used in TransformHelper. Let me know if I
>> should move them to where they are used.
>>
>> The method `img_makePalette`, currently located in
>> `share/native/libawt/awt/image/cvutils/img_colors.c`, is only used by
>> `unix/native/common/awt/X11Color.c`; it could be moved to the same directory
>> to avoid exporting the method from libawt. The files `img_colors.[ch]` do
>> not have any references to other files in `cvutils`.
>>
>> Manually verified that the exports are no longer present after these
>> changes. Tier1-3 and client libs tests still pass.
>
> Daniel Jeliński 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 four additional
> commits since the last revision:
>
> - Merge remote-tracking branch 'origin' into desktop-exports
> - Merge remote-tracking branch 'origin' into desktop-exports
> - Make J2dTraceInit static
> - Remove unnecessary exports
src/java.desktop/share/native/libawt/java2d/SurfaceData.h line 557:
> 555: */
> 556: SurfaceDataOps *
> 557: SurfaceData_GetOpsNoSetup(JNIEnv *env, jobject sData);
~~It seems to me none of the functions in `SurfaceData.h` should be exported.
They all end up in `awt.dll` / `libawt.so`. I can't see any of these functions
are accessed from other DLLs.~~
~~Would you like to create a separate task for removing exports from
`SurfaceData` functions?~~
Some are used from `fontmanager.dll`. But not as many as we export.
Potentially all symbols exported from `awt.dll` except from `Java*` can be
hidden…
I found that `Disposer_AddRecord` and `J2dTraceImpl` are used from `lcms.dll`.
src/java.desktop/share/native/libawt/java2d/Trace.c line 39:
> 37: JNIEXPORT void JNICALL
> 38: J2dTraceImpl(int level, jboolean cr, const char *string, ...)
> 39: {
`J2dTraceImpl` is used from `lcms.dll`, so it should remain exported. I thought
it shouldn't be exported either.
src/java.desktop/unix/native/libawt/awt/initIDs.c line 47:
> 45: (JNIEnv *env, jclass clazz)
> 46: {
> 47: }
Shall we remove `Color.initIDs`? Its implementation is now empty for all
platforms.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13261#discussion_r1355356940
PR Review Comment: https://git.openjdk.org/jdk/pull/13261#discussion_r1355354455
PR Review Comment: https://git.openjdk.org/jdk/pull/13261#discussion_r1355437407
Re: RFR: 8317847: Typo in API documentation of class JPopupMenu
On Wed, 11 Oct 2023 04:54:54 GMT, ANUPAM DEV wrote: > Hello, > > I have fixed the typo in the comment for the method > JPopupMenu.setInvoker(Component invoker) > > before: Sets the invoker of this popup menu -- the component in which the > popup menu menu is to be displayed. > after: Sets the invoker of this popup menu -- the component in which the > popup menu is to be displayed. > > Please review the change. > > Regards, > Anupam Looks good to me. - Marked as reviewed by aivanov (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16135#pullrequestreview-1671413065
Re: RFR: JDK-8292276 : Add named colors from CSS Color Module Level 4 [v38]
On Tue, 10 Oct 2023 15:11:24 GMT, Andy Goryachev wrote: >>> would it make more sense to use a switch statement instead of a static map? >> >> I believe this was discussed somewhere… Looks like map provides better >> performance compared to `switch` statement. Fetching an element from a map >> gives a constant time, using a `switch` statement over strings would give a >> linear search time unless it's cleverly optimised by the compiler. >> >> @scientificware could point you to performance measurements, if there are >> ones. There were ones, if my memory serves me right. > > 1. I believe the switch uses hash value so it's not linear anymore > 2. most of the time these colors will not be needed > > just a suggestion, anyway I agree. Perhaps, we should reconsider it again. The map takes up heap memory even if look up is done once, after the map is initialised, it's kept in memory forever. From this point of view, `switch` statement could be more efficient. - PR Review Comment: https://git.openjdk.org/jdk/pull/9825#discussion_r1353278202
Re: RFR: JDK-8292276 : Add named colors from CSS Color Module Level 4 [v38]
On Tue, 10 Oct 2023 14:44:04 GMT, Andy Goryachev wrote: > would it make more sense to use a switch statement instead of a static map? I believe this was discussed somewhere… Looks like map provides better performance compared to `switch` statement. Fetching an element from a map gives a constant time, using a `switch` statement over strings would give a linear search time unless it's cleverly optimised by the compiler. @scientificware could point you to performance measurements, if there are ones. There were ones, if my memory serves me right. - PR Review Comment: https://git.openjdk.org/jdk/pull/9825#discussion_r1352765240
Re: RFR: JDK-8292276 : Add named colors from CSS Color Module Level 4 [v38]
On Mon, 9 Oct 2023 23:24:35 GMT, ScientificWare wrote:
>> This is referenced in Java Bug Database as
>> - [JDK-8292276 : Add named colors from CSS Color Module Level
>> 4](https://bugs.java.com/bugdatabase/view_bug?bug_id=8292276)
>>
>> This is tracked in JBS as
>> - [JDK-8292276 : Add named colors from CSS Color Module Level
>> 4](https://bugs.openjdk.java.net/browse/JDK-8292276)
>>
>> Adds missing color names, defined by CSS Level 4, in CSS.java :
>> CSS Color Module Level 4
>> W3C Candidate Recommendation Snapshot, 5 July 2022
>> [7.1 Named Colors](https://www.w3.org/TR/css-color-4/#named-color)
>>
>> Designed from : [ScientificWare JDK-8292276 : Add named colors from CSS
>> Color Module Level 4](https://github.com/scientificware/jdk/issues/12)
>
> ScientificWare has updated the pull request incrementally with one additional
> commit since the last revision:
>
> MissingColorNames.java :
> - Rename the color which doesn't belong to CSS-COLOR-4 specification.
src/java.desktop/share/classes/javax/swing/text/html/CSS.java line 1:
> 1: /*
Does it make sense to separate color processing into a helper class? The `CSS`
class is large enough, we can move parsing the colors to a package-private
class, let's say `CSSColors`. The `CSS` class will use the static methods of
that class.
What do you think?
src/java.desktop/share/classes/javax/swing/text/html/StyleSheet.java line 1637:
> 1635: }
> 1636: }
> 1637: else if (poundIndex < spaceIndex) {
You're not changing code here, it's better to preserve the formatting.
test/jdk/javax/swing/text/html/CSS/MissingColorNames.java line 323:
> 321: {"rgb(12 24 200)", "ff0c18c8"}
> 322: };
> 323: }
Please keep an empty line at the end of the files.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/9825#discussion_r1352429654
PR Review Comment: https://git.openjdk.org/jdk/pull/9825#discussion_r1352447626
PR Review Comment: https://git.openjdk.org/jdk/pull/9825#discussion_r1352757617
Re: RFR: 8316947: Write a test to check textArea triggers MouseEntered/MouseExited events properly [v4]
On Tue, 10 Oct 2023 11:32:41 GMT, Ravi Gupta wrote: >> Write a test to check textArea triggers MouseEntered/MouseExited events >> properly >> >> MouseEntered should be triggered only when the mouse enters the component >> and MouseExited should be triggered when the mouse goes out of the component. >> >> In TextArea, when we moved the mouse inside the component MouseMoved events >> are triggered properly. But when we slowly took the mouse outside the >> component that is towards the scrollbars, we could see a MouseEntered event >> being triggered followed by MouseExited.(before actually mouse enters the >> scrollbar) >> >> Testing: >> Tested using Mach5(20 times per platform) in macos,linux and windows and got >> all pass. > > Ravi Gupta has updated the pull request incrementally with one additional > commit since the last revision: > > 8316947: Review comments solved Marked as reviewed by aivanov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/15961#pullrequestreview-1667385865
Re: RFR: 8301846: Invalid TargetDataLine after screen lock when using JFileChooser or COM library [v14]
On Mon, 9 Oct 2023 03:20:17 GMT, Renjith Kannath Pariyangad wrote: >> Hi Reviewers, >> >> Observations : >> 1. Without com initialize if we access Mixer for recording, library loaded >> invalid GUID and clipped description in windows(ID not found in registry). >> With com initialization library load proper GUID (same as registry). >> 2. For Play back device always loading proper device GUID irrespective of >> com Initialization. >> >> Test: >> Since screen lock and unlock workflow required for reproducing this issue, >> did coupe of iteration of manual testing post fix and confirmed its >> resolving the problem. >> To reconfirm nothing is broken, executed all audio related test cases on >> test bench post fix and all are green. >> >> Please review the changes and let me know your comments if any. >> >> Regards, >> Renjith. > > Renjith Kannath Pariyangad has updated the pull request incrementally with > one additional commit since the last revision: > > Modified condition based or review Looks good except for the minor nit. src/java.desktop/windows/native/libjsound/PLATFORM_API_WinOS_DirectSound.cpp line 187: > 185: } > 186: > 187: HRESULT hr=::CoInitializeEx(NULL, COINIT_MULTITHREADED | > COINIT_DISABLE_OLE1DDE); Suggestion: HRESULT hr = ::CoInitializeEx(NULL, COINIT_MULTITHREADED | COINIT_DISABLE_OLE1DDE); Spaces around the assignment operator. - PR Review: https://git.openjdk.org/jdk/pull/14898#pullrequestreview-1665102713 PR Review Comment: https://git.openjdk.org/jdk/pull/14898#discussion_r1350694116
Re: RFR: 8317706: Exclude java/awt/Graphics2D/DrawString/RotTransText.java on linux
On Sun, 8 Oct 2023 22:15:59 GMT, Christoph Langer wrote: > This test is failing on several Linux configurations (SLES 15), so it should > be excluded. Looks good to me. - Marked as reviewed by aivanov (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16096#pullrequestreview-1665090306
Re: RFR: 8317696: Fix compilation with clang-16 [v3]
On Mon, 9 Oct 2023 15:01:41 GMT, Jan Kratochvil wrote: >> `--with-toolchain-type=clang` fails the compilation for me with >> `clang-16.0.6-3.fc38.x86_64` >> >> While the warnings can be disabled I find better to just fix them. The GTK >> prototypes in JDK reported by clang are either missing or wrong. >> >> >> src/java.base/unix/native/libnet/DefaultProxySelector.c:378:41: error: >> passing arguments to a function without a prototype is deprecated in all >> versions of C and is not supported in C2x >> [-Werror,-Wdeprecated-non-prototype] >> proxies = (*g_proxy_resolver_lookup)(resolver, uri, NULL, ); >> ^ >> src/java.base/unix/native/libnet/DefaultProxySelector.c:397:63: error: >> passing arguments to a function without a prototype is deprecated in all >> versions of C and is not supported in C2x >> [-Werror,-Wdeprecated-non-prototype] >> (*g_network_address_parse_uri)(proxies[i], 0, >> ^ >> src/java.base/unix/native/libnet/DefaultProxySelector.c:402:70: error: >> passing arguments to a function without a prototype is deprecated in all >> versions of C and is not supported in C2x >> [-Werror,-Wdeprecated-non-prototype] >> phost = (*g_network_address_get_hostname)(conn); >> ^ >> src/java.base/unix/native/libnet/DefaultProxySelector.c:403:66: error: >> passing arguments to a function without a prototype is deprecated in all >> versions of C and is not supported in C2x >> [-Werror,-Wdeprecated-non-prototype] >> pport = (*g_network_address_get_port)(conn); >> ^ >> src/java.base/unix/native/libnet/DefaultProxySelector.c:445:22: error: >> passing arguments to a function without a prototype is deprecated in all >> versions of C and is not supported in C2x >> [-Werror,-Wdeprecated-non-prototype] >> (*g_strfreev)(proxies); >> ^ >> src/java.base/unix/native/libnet/DefaultProxySelector.c:448:25: error: >> passing arguments to a function without a prototype is deprecated in all >> versions of C and is not supported in C2x >> [-Werror,-Wdeprecated-non-prototype] >> (*g_clear_error)(); >> ^ >> Compiling X11Renderer.c (for libawt_headless.so) >> In file included from >> src/java.desktop/unix/native/common/java2d/x11/X11PMBlitLoops.c:29: >> In file included from >> src/java.desktop/unix/native/common/java2d/x11/X11Surf... > > Jan Kratochvil has updated the pull request incrementally with one additional > commit since the last revision: > > Remove declaration changes which are not required >- suggested by djelinski The changes for `java.desktop` look good to me. src/java.desktop/unix/native/common/awt/awt_GraphicsEnv.h line 53: > 51: #define MITSHM_PERM_OWNER (0600) > 52: > 53: extern int XShmQueryExtension(Display* dpy); I wonder if it's even needed here: the declaration should come from `X11/extensions/XShm.h` which is included. - Marked as reviewed by aivanov (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16092#pullrequestreview-1665070667 PR Review Comment: https://git.openjdk.org/jdk/pull/16092#discussion_r1350667139
Re: RFR: 8316947: Write a test to check textArea triggers MouseEntered/MouseExited events properly [v3]
On Mon, 9 Oct 2023 05:07:13 GMT, Ravi Gupta wrote:
>> Write a test to check textArea triggers MouseEntered/MouseExited events
>> properly
>>
>> MouseEntered should be triggered only when the mouse enters the component
>> and MouseExited should be triggered when the mouse goes out of the component.
>>
>> In TextArea, when we moved the mouse inside the component MouseMoved events
>> are triggered properly. But when we slowly took the mouse outside the
>> component that is towards the scrollbars, we could see a MouseEntered event
>> being triggered followed by MouseExited.(before actually mouse enters the
>> scrollbar)
>>
>> Testing:
>> Tested using Mach5(20 times per platform) in macos,linux and windows and got
>> all pass.
>
> Ravi Gupta has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8316947: Review comments solved
Changes requested by aivanov (Reviewer).
test/jdk/java/awt/event/MouseEvent/MouseEnterExitTest.java line 50:
> 48: private volatile static boolean passed = true;
> 49: private volatile static Point compAt;
> 50: private volatile static Dimension compSize;
To my personal taste, I'd add blank line to visually separate groups of fields:
Suggestion:
private static Frame frame;
private volatile static boolean entered = false;
private volatile static boolean exited = false;
private volatile static boolean passed = true;
private volatile static Point compAt;
private volatile static Dimension compSize;
test/jdk/java/awt/event/MouseEvent/MouseEnterExitTest.java line 53:
> 51:
> 52: private static final MouseListener mouseListener = new MouseAdapter()
> {
> 53: public void mouseEntered(MouseEvent e) {
Suggestion:
private static final MouseListener mouseListener = new MouseAdapter() {
@Override
public void mouseEntered(MouseEvent e) {
Let's add `@Override` annotations, it's basically a new code, so it's better to
add them.
test/jdk/java/awt/event/MouseEvent/MouseEnterExitTest.java line 63:
> 61: }
> 62:
> 63: public void mouseExited(MouseEvent e) {
Suggestion:
@Override
public void mouseExited(MouseEvent e) {
test/jdk/java/awt/event/MouseEvent/MouseEnterExitTest.java line 101:
> 99: EventQueue.invokeAndWait(MouseEnterExitTest::initializeGUI);
> 100: robot.waitForIdle();
> 101: EventQueue.invokeAndWait(() -> {
Suggestion:
robot.waitForIdle();
EventQueue.invokeAndWait(() -> {
test/jdk/java/awt/event/MouseEvent/MouseEnterExitTest.java line 110:
> 108: robot.mouseMove(i, compAt.y);
> 109: }
> 110: if (!passed || entered || !exited) {
Suggestion:
if (!passed || entered || !exited) {
A couple of blank lines in the `main` method.
test/jdk/java/awt/event/MouseEvent/MouseEnterExitTest.java line 121:
> 119: }
> 120:
> 121: public static void disposeFrame() {
Suggestion:
private static void disposeFrame() {
It should be private.
-
PR Review: https://git.openjdk.org/jdk/pull/15961#pullrequestreview-1664717599
PR Review Comment: https://git.openjdk.org/jdk/pull/15961#discussion_r1350432826
PR Review Comment: https://git.openjdk.org/jdk/pull/15961#discussion_r1350434252
PR Review Comment: https://git.openjdk.org/jdk/pull/15961#discussion_r1350434537
PR Review Comment: https://git.openjdk.org/jdk/pull/15961#discussion_r1350435567
PR Review Comment: https://git.openjdk.org/jdk/pull/15961#discussion_r1350436186
PR Review Comment: https://git.openjdk.org/jdk/pull/15961#discussion_r1350436889
Re: RFR: 8316947: Write a test to check textArea triggers MouseEntered/MouseExited events properly [v2]
On Wed, 4 Oct 2023 17:23:51 GMT, Harshitha Onkar wrote: > Is there source code fix or PR associated with this test. I couldn't see the answer to this question. I guess the fix was done long time ago in JDK-4454304. Now, the test gets open-sourced. - PR Comment: https://git.openjdk.org/jdk/pull/15961#issuecomment-1751246111
Re: RFR: 8316947: Write a test to check textArea triggers MouseEntered/MouseExited events properly [v2]
On Thu, 5 Oct 2023 08:13:53 GMT, Ravi Gupta wrote:
>> Write a test to check textArea triggers MouseEntered/MouseExited events
>> properly
>>
>> MouseEntered should be triggered only when the mouse enters the component
>> and MouseExited should be triggered when the mouse goes out of the component.
>>
>> In TextArea, when we moved the mouse inside the component MouseMoved events
>> are triggered properly. But when we slowly took the mouse outside the
>> component that is towards the scrollbars, we could see a MouseEntered event
>> being triggered followed by MouseExited.(before actually mouse enters the
>> scrollbar)
>>
>> Testing:
>> Tested using Mach5(20 times per platform) in macos,linux and windows and got
>> all pass.
>
> Ravi Gupta has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8316947: Review comments solved
Changes requested by aivanov (Reviewer).
test/jdk/java/awt/event/MouseEvent/MouseEnterExitTest.java line 40:
> 38: * @key headful
> 39: * @bug 4454304
> 40: * @summary On Solaris,TextArea triggers MouseEntered when the mouse is
> inside the component
Suggestion:
* @summary On Solaris, TextArea triggers MouseEntered when the mouse is inside
the component
test/jdk/java/awt/event/MouseEvent/MouseEnterExitTest.java line 47:
> 45: private static Frame frame;
> 46: private static TextArea textArea;
> 47: private static List list;
Both `list` and `textArea` can be local variables in the `initializeGUI` method.
test/jdk/java/awt/event/MouseEvent/MouseEnterExitTest.java line 48:
> 46: private static TextArea textArea;
> 47: private static List list;
> 48: private static Robot robot;
Robot can be a local variable in the `main` method.
test/jdk/java/awt/event/MouseEvent/MouseEnterExitTest.java line 55:
> 53: private volatile static Dimension compSize;
> 54:
> 55: private static MouseListener mListener = new MouseAdapter() {
Suggestion:
private static final MouseListener mouseListener = new MouseAdapter() {
Why not make the field `mouseListener`?
test/jdk/java/awt/event/MouseEvent/MouseEnterExitTest.java line 101:
> 99: robot = new Robot();
> 100: robot.setAutoDelay(100);
> 101: robot.setAutoWaitForIdle(true);
Suggestion:
robot = new Robot();
robot.setAutoDelay(100);
robot.setAutoWaitForIdle(true);
EventQueue.invokeAndWait(MouseEnterExitTest::initializeGUI);
robot.waitForIdle();
Before continuing to get the coordinates of the frame, you want to ensure it's
completely shown.
-
PR Review: https://git.openjdk.org/jdk/pull/15961#pullrequestreview-1662583031
PR Review Comment: https://git.openjdk.org/jdk/pull/15961#discussion_r1349181886
PR Review Comment: https://git.openjdk.org/jdk/pull/15961#discussion_r1349181089
PR Review Comment: https://git.openjdk.org/jdk/pull/15961#discussion_r1349180170
PR Review Comment: https://git.openjdk.org/jdk/pull/15961#discussion_r1349178806
PR Review Comment: https://git.openjdk.org/jdk/pull/15961#discussion_r1349178102
Re: RFR: 8301846: Invalid TargetDataLine after screen lock when using JFileChooser or COM library [v13]
On Fri, 6 Oct 2023 11:17:01 GMT, Renjith Kannath Pariyangad
wrote:
>> Hi Reviewers,
>>
>> Observations :
>> 1. Without com initialize if we access Mixer for recording, library loaded
>> invalid GUID and clipped description in windows(ID not found in registry).
>> With com initialization library load proper GUID (same as registry).
>> 2. For Play back device always loading proper device GUID irrespective of
>> com Initialization.
>>
>> Test:
>> Since screen lock and unlock workflow required for reproducing this issue,
>> did coupe of iteration of manual testing post fix and confirmed its
>> resolving the problem.
>> To reconfirm nothing is broken, executed all audio related test cases on
>> test bench post fix and all are green.
>>
>> Please review the changes and let me know your comments if any.
>>
>> Regards,
>> Renjith.
>
> Renjith Kannath Pariyangad has updated the pull request incrementally with
> three additional commits since the last revision:
>
> - Whitespace error fixed
> - Whitespace error fixed
> - CoInitializeEx error check modified with better condition
Changes requested by aivanov (Reviewer).
src/java.desktop/windows/native/libjsound/PLATFORM_API_WinOS_DirectSound.cpp
line 188:
> 186:
> 187: HRESULT hr=::CoInitializeEx(NULL, COINIT_MULTITHREADED |
> COINIT_DISABLE_OLE1DDE);
> 188: if(hr != S_OK || hr != S_FALSE || hr != RPC_E_CHANGED_MODE) {
Suggestion:
HRESULT hr = ::CoInitializeEx(NULL, COINIT_MULTITHREADED |
COINIT_DISABLE_OLE1DDE);
if (FAILED(hr) && hr != RPC_E_CHANGED_MODE) {
You said Microsoft recommends using `FAILED` and `SUCCEEDED` macros, so we
should use them here too.
I think you got the condition wrong. The valid values to continue are
if (hr == S_OK || hr == S_FALSE || hr == RPC_E_CHANGED_MODE) {
// Good - do the stuff
}
Here we need to reverse the condition:
if (!(hr == S_OK || hr == S_FALSE || hr == RPC_E_CHANGED_MODE)) {
// Unlock and return
}
To negate the condition, you negate all the operators and `OR` becomes `AND`.
Thus, the condition becomes:
if (hr != S_OK && hr != S_FALSE && hr != RPC_E_CHANGED_MODE)) {
// Unlock and return
}
The first two conditions could be converted to `FAILED(hr)`, which gives us the
condition I suggested.
src/java.desktop/windows/native/libjsound/PLATFORM_API_WinOS_DirectSound.cpp
line 234:
> 232: }
> 233:
> 234: if(hr != RPC_E_CHANGED_MODE) {
Suggestion:
if (hr != RPC_E_CHANGED_MODE) {
Space between the `if` keyword and the opening parenthesis.
-
PR Review: https://git.openjdk.org/jdk/pull/14898#pullrequestreview-1662190325
PR Review Comment: https://git.openjdk.org/jdk/pull/14898#discussion_r1348907643
PR Review Comment: https://git.openjdk.org/jdk/pull/14898#discussion_r1348908246
Re: RFR: 8315484: java/awt/dnd/RejectDragDropActionTest.java timed out [v3]
On Thu, 5 Oct 2023 15:48:03 GMT, Damon Nguyen wrote: > I can't reproduce this on my machine Me neither. > Does removing the listeners work for you? I haven't run the test in the CI with listeners removed; I haven't tested it in any way at all. I'm just looking for explanation why the JVM doesn't exit. > And where are you checking the stack trace? I checked the CI logs. I'll explain privately. - PR Review Comment: https://git.openjdk.org/jdk/pull/16018#discussion_r1347661921
Re: RFR: 8315986: javax/swing/JMenuItem/4654927/bug4654927.java: component must be showing on the screen to determine its location [v4]
On Thu, 5 Oct 2023 15:29:15 GMT, Prasanta Sadhukhan wrote: > If you concede they are different, then what change you want me to do? If you > say center is safer and then still use menuLocation coordinate which is > obtained via getLocationScreen, it's confusing.. I see no big difference in dragging mouse from the centre to the centre compared to +10 offset. This is what I mean: https://github.com/aivanov-jdk/jdk/commit/da8213be220287ec4b9f7921fcd3ae4b4ece802d > I think it's good as it is... I think it can be simplified. - PR Review Comment: https://git.openjdk.org/jdk/pull/15677#discussion_r1347655722
Re: RFR: 8315986: javax/swing/JMenuItem/4654927/bug4654927.java: component must be showing on the screen to determine its location [v4]
On Thu, 5 Oct 2023 15:15:24 GMT, Prasanta Sadhukhan wrote: > But 1st case uses center coordinates via Util.getCenterPoint whereas 2nd case > uses getLocationOnScreen+10 so it's not exactly the same. Yes, they're somewhat different. Yet the centre is safer (what if `getLocationOnScreen+10` is out of menu bounds?), it doesn't change the test logic. > I guess we can do away with Util class and make it use > SwingUtilities.invokeAndWait but I guess we can do it at later point.. It's possible… yet I don't see much value in doing so, the `Util` class does it job… and avoids duplicating code. However, `glide` method doesn't provide a smooth glide: with its 10-pixel step, the mouse is moved from the "Menu" to the greyed out item in just two steps. It's still a drag yet I expected smoother movement. - PR Review Comment: https://git.openjdk.org/jdk/pull/15677#discussion_r1347606918
Integrated: 8313348: Fix typo in JFormattedTextField: 'it self'
On Mon, 31 Jul 2023 10:14:45 GMT, Alexey Ivanov wrote: > A trivial change: ‘it self’ → ‘itself’. > > In addition to it, I added a comma after ‘Similarly’ where it starts a > sentence. This pull request has now been integrated. Changeset: a1c9587c Author:Alexey Ivanov URL: https://git.openjdk.org/jdk/commit/a1c9587c27538bda3b0f6745d9c80ff4e1b9a77e Stats: 79 lines in 2 files changed: 61 ins; 2 del; 16 mod 8313348: Fix typo in JFormattedTextField: 'it self' Reviewed-by: honkar, dnguyen, psadhukhan - PR: https://git.openjdk.org/jdk/pull/15087
Re: RFR: 8315986: javax/swing/JMenuItem/4654927/bug4654927.java: component must be showing on the screen to determine its location [v2]
On Tue, 3 Oct 2023 00:17:32 GMT, Harshitha Onkar wrote: >> Prasanta Sadhukhan has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Add setAutoWaitForIdle > > LGTM Yesterday after talking to Harshitha @honkar-jdk, I also learned, there's an open bug [JDK-8316016](https://bugs.openjdk.org/browse/JDK-8316016): _getLocationOnScreen API returns incorrect values on macOS 14_. If we suspect `getLocationOnScreen` returns incorrect coordinates, the delays won't fix the problem. However, the delays may help to alleviate the `getLocationOnScreen` bug so that it returns the correct coordinates. But the root cause is misbehaving `getLocationOnScreen` rather than this test itself. - PR Comment: https://git.openjdk.org/jdk/pull/15677#issuecomment-1749076115
Re: RFR: 8315986: javax/swing/JMenuItem/4654927/bug4654927.java: component must be showing on the screen to determine its location [v4]
On Wed, 4 Oct 2023 13:58:59 GMT, Prasanta Sadhukhan wrote: > So, I think pt 7 "close menu" is happening before pt 8 "drag mouse"Yes, > "point" is being reused but I guess it's ok It is okay, yet I think it would be clearer and cleaner if the flow was like this: 1. Complete the first case, steps 1–5 above; 2. Click the menu to close it; 3. Drag the mouse. The comment in the code says, “Drag the mouse”… but before it does so, it once retrieves the coordinates once again, then closes the menu and only now it starts dragging the mouse. If you store the coordinates to `menuLocation` and `itemLocation` when performing the first test, the code becomes cleaner: one clearly sees the coordinates are re-used. (They're re-used either way but retrieved twice.) Then the sequence of the operations will become clearer too: first case (open menu, click greyed out item); close the menu; now start the second case (click the centre of the menu, which opens the menu again, drag the mouse to the greyed out item, release mouse). Both @prrace and I interpreted closing the menu as an unexpected thing but it is intentional, the test needs to close the menu before it can verify the second case. - PR Review Comment: https://git.openjdk.org/jdk/pull/15677#discussion_r1347554324
Re: RFR: 8301846: Invalid TargetDataLine after screen lock when using JFileChooser or COM library [v10]
On Wed, 4 Oct 2023 05:37:10 GMT, Renjith Kannath Pariyangad wrote: >> Hi Reviewers, >> >> Observations : >> 1. Without com initialize if we access Mixer for recording, library loaded >> invalid GUID and clipped description in windows(ID not found in registry). >> With com initialization library load proper GUID (same as registry). >> 2. For Play back device always loading proper device GUID irrespective of >> com Initialization. >> >> Test: >> Since screen lock and unlock workflow required for reproducing this issue, >> did coupe of iteration of manual testing post fix and confirmed its >> resolving the problem. >> To reconfirm nothing is broken, executed all audio related test cases on >> test bench post fix and all are green. >> >> Please review the changes and let me know your comments if any. >> >> Regards, >> Renjith. > > Renjith Kannath Pariyangad has updated the pull request incrementally with > one additional commit since the last revision: > > File permission updated To clarify the situation and to eliminate any misunderstanding, there are three possible cases: 1. COM is not initialised. `CoInitializeEx(COINIT_MULTITHREADED)` returns `S_OK`. This is the ideal situation. 2. The app has already initialised COM on the thread with `CoInitializeEx(COINIT_MULTITHREADED)`. Thus, the second call to `CoInitializeEx` will return `S_FALSE`. This case is also fine. 3. The app has already initialised COM on the thread with `CoInitializeEx(COINIT_APARTMENTTHREADED)`. Thus, the second call inside Java to `CoInitializeEx(COINIT_MULTITHREADED)` will return `RPC_E_CHANGED_MODE` because the concurrency model is changed. Yet this case is fine as well. In all the three cases, it is safe to proceed to call `DirectSoundEnumerate*` because the prerequisite—COM is initialised—is satisfied and because DirectSound APIs support both MTA and STA. In the first two cases, we *must call* `CoUninitialize` when finished; in the third case, we *must **not** call* `CoUninitialize`. If `CoInitializeEx` returns any other error, we must bail out. - PR Comment: https://git.openjdk.org/jdk/pull/14898#issuecomment-1748967705
Re: RFR: 8303904: Transparent Windows Paint Wrong (Opaque) w/o Volatile Buffering [v2]
On Wed, 7 Jun 2023 17:22:23 GMT, Jeremy wrote: > What is the fate of auto-closed PRs? @mickleness You can reopen the PR with the [`/open`](https://wiki.openjdk.org/display/SKARA/Pull+Request+Commands#PullRequestCommands-/open) command. > That is: as long as I don't delete this branch in my repo will this be > available for future reference if anyone dusts off this ticket in the future? If you keep the branch in your repo, you or anyone could use it as the base to continue working on this issue. - PR Comment: https://git.openjdk.org/jdk/pull/13196#issuecomment-1748756928
Re: RFR: 8301846: Invalid TargetDataLine after screen lock when using JFileChooser or COM library [v10]
On Wed, 4 Oct 2023 05:37:10 GMT, Renjith Kannath Pariyangad
wrote:
>> Hi Reviewers,
>>
>> Observations :
>> 1. Without com initialize if we access Mixer for recording, library loaded
>> invalid GUID and clipped description in windows(ID not found in registry).
>> With com initialization library load proper GUID (same as registry).
>> 2. For Play back device always loading proper device GUID irrespective of
>> com Initialization.
>>
>> Test:
>> Since screen lock and unlock workflow required for reproducing this issue,
>> did coupe of iteration of manual testing post fix and confirmed its
>> resolving the problem.
>> To reconfirm nothing is broken, executed all audio related test cases on
>> test bench post fix and all are green.
>>
>> Please review the changes and let me know your comments if any.
>>
>> Regards,
>> Renjith.
>
> Renjith Kannath Pariyangad has updated the pull request incrementally with
> one additional commit since the last revision:
>
> File permission updated
Changes requested by aivanov (Reviewer).
src/java.desktop/windows/native/libjsound/PLATFORM_API_WinOS_DirectSound.cpp
line 183:
> 181: INT32 cacheIndex;
> 182:
> 183: if (!DS_lockCache() || FAILED(::CoInitializeEx(NULL,
> COINIT_MULTITHREADED | COINIT_DISABLE_OLE1DDE))) {
This code doesn't handle the case where `CoInitializeEx` returns
`RPC_E_CHANGED_MODE`. As [discussed
previously](https://github.com/openjdk/jdk/pull/14898#discussion_r1327639556),
we can proceed in this case too, but we must not call `CoUninitialize`.
It also need to be handled in the similar code below.
-
PR Review: https://git.openjdk.org/jdk/pull/14898#pullrequestreview-1659463318
PR Review Comment: https://git.openjdk.org/jdk/pull/14898#discussion_r1347179257
Re: RFR: 8315484: java/awt/dnd/RejectDragDropActionTest.java timed out [v5]
On Wed, 4 Oct 2023 17:45:25 GMT, Damon Nguyen wrote: >> This test intermittently fails by timeout. Increasing the timeout alone >> doesn't solve the failure as it still fails in about 400 runs. Adding >> another delay and reducing the delay amount to 1000ms. Now, the test passes >> after 2 sets of 500 repeats on all OS's without a timeout. > > Damon Nguyen has updated the pull request incrementally with one additional > commit since the last revision: > > Move condition to loop parameters Changes requested by aivanov (Reviewer). test/jdk/java/awt/dnd/RejectDragDropActionTest.java line 101: > 99: robot.mousePress(InputEvent.BUTTON1_DOWN_MASK); > 100: for (Point p = new Point(startPoint); > 101: !p.equals(endPoint) || incorrectActionDetected; Suggestion: !p.equals(endPoint) && !incorrectActionDetected; You keep iterating while both conditions are true. As soon as either is false, exit the loop. - PR Review: https://git.openjdk.org/jdk/pull/16018#pullrequestreview-1659420937 PR Review Comment: https://git.openjdk.org/jdk/pull/16018#discussion_r1347152843
Re: RFR: 8315484: java/awt/dnd/RejectDragDropActionTest.java timed out [v3]
On Tue, 3 Oct 2023 22:36:15 GMT, Damon Nguyen wrote: >> test/jdk/java/awt/dnd/RejectDragDropActionTest.java line 91: >> >>> 89: robot.setAutoWaitForIdle(true); >>> 90: robot.waitForIdle(); >>> 91: robot.delay(DELAY_TIME); >> >> To reduce the initial delay, you may override `Frame.paint` and `countDown` >> a latch after calling `super.paint`. Once the latch is released, you create >> robot and start testing. >> >> `DELAY_TIME` could be set to `500` or even less. > > I reduced the `DELAY_TIME` to 500. Thanks for the suggestion. I got used to > setting the default delay times to 1000 for these types of tests, and am > trying to break the habit and reduce delays further to 500 where possible. Well, I meant that you could reduce the delay if you add the latch. However, 500ms should be enough now. Windows Vista and Windows 7 had long window animations when a window was shown on the screen, I believe this is where the delay of 1 second comes from. Windows 10 and 11 still have this animation but it's quicker. - PR Review Comment: https://git.openjdk.org/jdk/pull/16018#discussion_r1345658202
Re: RFR: 8315484: java/awt/dnd/RejectDragDropActionTest.java timed out [v3]
On Tue, 3 Oct 2023 22:49:17 GMT, Damon Nguyen wrote:
>> test/jdk/java/awt/dnd/RejectDragDropActionTest.java line 88:
>>
>>> 86:
>>> 87: try {
>>> 88: Robot robot = new Robot();
>>
>> To reduce the initial delay, you may override `Frame.paint` and `countDown`
>> a latch after calling `super.paint`. Once the latch is released, you create
>> robot and start testing.
>
> Sure can do. Is this to replace the initial `waitForIdle` after robot's
> creation? So, remove the `waitForIdle` and add `latch.await()` before robot's
> creation?
I'm not sure it's the right idea. If a paint occurred, it means the frame
should be displayed on the screen. Yet it does not account for window
animations. Perhaps, it does not make sense to introduce more complications
which could be not as reliable.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16018#discussion_r1345652111
Re: RFR: 8315484: java/awt/dnd/RejectDragDropActionTest.java timed out [v4]
On Tue, 3 Oct 2023 22:41:37 GMT, Damon Nguyen wrote:
>> This test intermittently fails by timeout. Increasing the timeout alone
>> doesn't solve the failure as it still fails in about 400 runs. Adding
>> another delay and reducing the delay amount to 1000ms. Now, the test passes
>> after 2 sets of 500 repeats on all OS's without a timeout.
>
> Damon Nguyen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove unnecessary delay. Modify delay amount. Add break.
test/jdk/java/awt/dnd/RejectDragDropActionTest.java line 107:
> 105: System.err.println("Incorrect action during loop");
> 106: break;
> 107: }
Actually, I meant adding it as part of the condition in the for-loop itself.
However, it works either way.
The initial bug report for
[JDK-4774532](https://bugs.openjdk.org/browse/JDK-4774532) says `dragOver`
shouldn't be called at all. Do we want to update the test for a stricter
assertion. For me, `dragOver` is never called.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16018#discussion_r1345647800
Re: RFR: 8316146: Open some swing tests 4 [v10]
On Tue, 3 Oct 2023 21:57:42 GMT, Alisen Chung wrote: >> Opening closed tests: >> 12 javax/swing/ToolTipManager/5078214/bug5078214.java >> 13 javax/swing/plaf/basic/BasicMenuItemUI/4239714/bug4239714.java >> 14 javax/swing/plaf/basic/BasicMenuUI/4244616/bug4244616.java >> 15 javax/swing/plaf/metal/4306431/bug4306431.java > > Alisen Chung has updated the pull request incrementally with one additional > commit since the last revision: > > fixed jtreg header Marked as reviewed by aivanov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/15875#pullrequestreview-1657303815
Re: RFR: 8315986: javax/swing/JMenuItem/4654927/bug4654927.java: component must be showing on the screen to determine its location [v4]
On Wed, 4 Oct 2023 09:30:55 GMT, Prasanta Sadhukhan wrote: >> Test was run without waiting for UI to be made visible leading to >> IllegalComponentStateException. >> Used robot.delay consistent with other headful tests to made the test wait >> after UI is created and shown. >> >> Test passed in several iterations in all platforms. Link in JBS > > Prasanta Sadhukhan has updated the pull request incrementally with one > additional commit since the last revision: > > jcheck Changes requested by aivanov (Reviewer). test/jdk/javax/swing/JMenuItem/4654927/bug4654927.java line 1: > 1: /* I looked at the test code more thoroughly. I guess it can be improved. Since GitHub doesn't allow adding comments to lines which weren't modified, I'll add my comment here. **The first case** is straightforward: 1. Click the menu on the menu bar to open it, 2. Click the greyed out menu item. The test saves the location of menu into `point`, then replaces it with location of the menu item. Yet it does request the same positions later in the test where it saves them into `menuLocation` and `itemLocation` correspondingly. Why not do it right away? The positions shouldn't change. Anyway the test relies on this fact because it saves the positions before the menu gets hidden. **The second case** tests mouse drag. It simulates clicking the menu to open its popup and then dragging the mouse to the menu item and then releasing the mouse button. Here it's more confusing… If you save the values in the first case above, you can clean up the code. Then closing the menu could be done *before* entering the block of code for dragging mouse. In my opinion, it will make the test code clearer. test/jdk/javax/swing/JMenuItem/4654927/bug4654927.java line 92: > 90: point = Util.getCenterPoint(menu); > 91: robot.mouseMove(point.x, point.y); > 92: robot.delay(250); This delay may help. I saw that moving mouse takes a bit of time on macOS, the following code heavily depends on the mouse being in the expected position. You may assert that mouse cursor is at the expected position. On the other hand, mouse press and further movements shouldn't precede the first mouse move event. If it happens, it could be a bug in macOS. Thus I can't see how this can lead to the test failure. Which case does the test fail by the way? Is it the first or the second? - PR Review: https://git.openjdk.org/jdk/pull/15677#pullrequestreview-1657258616 PR Review Comment: https://git.openjdk.org/jdk/pull/15677#discussion_r1345609705 PR Review Comment: https://git.openjdk.org/jdk/pull/15677#discussion_r1345616470
Re: RFR: 8315986: javax/swing/JMenuItem/4654927/bug4654927.java: component must be showing on the screen to determine its location [v2]
On Wed, 4 Oct 2023 08:32:31 GMT, Prasanta Sadhukhan wrote: >> Can you please tell which system it is not working? >> I guess harshita and vitaly tested on macos14 and it worked for them and I >> tested on other mac13,windows10 locally where it also worked, as well as in >> mach5 systems, whose link I have put in JBS. >> >> I also have seen the menu disappear while clicking but it does not cause the >> test to fail..I saw the video recording vitaly uploaded in JBS and it is >> same as in macos13 where it pass always (we also dont see any failure for >> this test in CI runs so far) > > Add a delay to make it work on mach5 mac14 system. Tested for 50 iterations, > link in JBS > I also have seen the menu disappear while clicking but it does not cause the > test to fail..I saw the video recording vitaly uploaded in JBS and it is same > as in macos13 where it pass always (we also dont see any failure for this > test in CI runs so far) But the menu should not disappear if a greyed out item is clicked. If you see it, you should address this problem. - PR Review Comment: https://git.openjdk.org/jdk/pull/15677#discussion_r1345572539
Re: RFR: 8315986: javax/swing/JMenuItem/4654927/bug4654927.java: component must be showing on the screen to determine its location [v2]
On Fri, 29 Sep 2023 05:05:07 GMT, Prasanta Sadhukhan
wrote:
>> Test was run without waiting for UI to be made visible leading to
>> IllegalComponentStateException.
>> Used robot.delay consistent with other headful tests to made the test wait
>> after UI is created and shown.
>>
>> Test passed in several iterations in all platforms. Link in JBS
>
> Prasanta Sadhukhan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Add setAutoWaitForIdle
Marked as reviewed by aivanov (Reviewer).
test/jdk/javax/swing/JMenuItem/4654927/bug4654927.java line 56:
> 54: String systemLAF = UIManager.getSystemLookAndFeelClassName();
> 55: // the test is not applicable to Motif L
> 56: if (systemLAF.endsWith("MotifLookAndFeel")){
Suggestion:
if (systemLAF.endsWith("MotifLookAndFeel")) {
-
PR Review: https://git.openjdk.org/jdk/pull/15677#pullrequestreview-1656078503
PR Review Comment: https://git.openjdk.org/jdk/pull/15677#discussion_r1344704658
Re: RFR: 8315484: java/awt/dnd/RejectDragDropActionTest.java timed out [v2]
On Tue, 3 Oct 2023 18:08:16 GMT, Harshitha Onkar wrote: >> Sure, I just added it. Previously when I initially worked on this test, >> setAutoWaitForIdle had a weird interaction with how the test was setup. >> Since the test used a delay of 50ms between each of the movements when >> dragging the selection, it caused long delays. Looks like this is resolved >> now, and the method doesn't add any unreasonable delays anymore. > > Sounds good! > Since the test used a delay of 50ms between each of the movements when > dragging the selection, it caused long delays. As far as I can see, the test still uses it. - PR Review Comment: https://git.openjdk.org/jdk/pull/16018#discussion_r1344701813
Re: RFR: 8315484: java/awt/dnd/RejectDragDropActionTest.java timed out [v3]
On Tue, 3 Oct 2023 18:09:03 GMT, Damon Nguyen wrote:
>> This test intermittently fails by timeout. Increasing the timeout alone
>> doesn't solve the failure as it still fails in about 400 runs. Adding
>> another delay and reducing the delay amount to 1000ms. Now, the test passes
>> after 2 sets of 500 repeats on all OS's without a timeout.
>
> Damon Nguyen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add setAutoWaitForIdle
test/jdk/java/awt/dnd/RejectDragDropActionTest.java line 1:
> 1: /*
I've got questions whether all the AWT resources are released.
When the test fails with timeout, the screen shot does not have the frame, it
displays an empty desktop.
The stack trace for the main thread:
java.base/java.lang.Object.wait0(Native Method)
java.base/java.lang.Object.wait(Object.java:375)
java.base/java.lang.Thread.join(Thread.java:2045)
java.base/java.lang.Thread.join(Thread.java:2121)
java.base/java.lang.ApplicationShutdownHooks.runHooks(ApplicationShutdownHooks.java:114)
java.base/java.lang.ApplicationShutdownHooks$1.run(ApplicationShutdownHooks.java:47)
java.base/java.lang.Shutdown.runHooks(Shutdown.java:130)
java.base/java.lang.Shutdown.exit(Shutdown.java:167)
java.base/java.lang.Runtime.exit(Runtime.java:188)
java.base/java.lang.System.exit(System.java:1920)
com.sun.javatest.regtest.agent.AStatus.exit(AStatus.java:198)
com.sun.javatest.regtest.agent.MainWrapper.main(MainWrapper.java:95)"
That is the main method of the test class terminated. Yet JVM does not exit for
whatever reason.
At the same time, the thread dump contains both `AWT-Windows` and
`AWT-EventQueue-0` threads, which means AWT prevents the JVM from exiting. Why?
I have no clue.
Will removing the mouse listeners from the frame let it be disposed of?
if (frame != null) {
frame.removeMouseListener((MouseListener) dgr);
frame.removeMouseMotionListener((MouseMotionListener) dgr);
frame.dispose();
}
Although the changes that you've made make the test faster, it may still time
out unless we find the root cause.
test/jdk/java/awt/dnd/RejectDragDropActionTest.java line 88:
> 86:
> 87: try {
> 88: Robot robot = new Robot();
To reduce the initial delay, you may override `Frame.paint` and `countDown` a
latch after calling `super.paint`. Once the latch is released, you create robot
and start testing.
test/jdk/java/awt/dnd/RejectDragDropActionTest.java line 89:
> 87: try {
> 88: Robot robot = new Robot();
> 89: robot.setAutoWaitForIdle(true);
Now that you set `autoWaitForIdle`, the delay inside the loop is redundant —
waiting for idle after sending each event delays the execution even more.
test/jdk/java/awt/dnd/RejectDragDropActionTest.java line 91:
> 89: robot.setAutoWaitForIdle(true);
> 90: robot.waitForIdle();
> 91: robot.delay(DELAY_TIME);
To reduce the initial delay, you may override `Frame.paint` and `countDown` a
latch after calling `super.paint`. Once the latch is released, you create robot
and start testing.
`DELAY_TIME` could be set to `500` or even less.
test/jdk/java/awt/dnd/RejectDragDropActionTest.java line 99:
> 97:
> 98: robot.delay(DELAY_TIME);
> 99:
The delay here is unnecessary, nothing has changed after the initial delay.
test/jdk/java/awt/dnd/RejectDragDropActionTest.java line 102:
> 100: robot.mouseMove(startPoint.x, startPoint.y);
> 101: robot.mousePress(InputEvent.BUTTON1_DOWN_MASK);
> 102: for (Point p = new Point(startPoint); !p.equals(endPoint);
The `for` loop may also verify the value of `incorrectActionDetected` and break
if it becomes `true`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16018#discussion_r1344694558
PR Review Comment: https://git.openjdk.org/jdk/pull/16018#discussion_r1344643970
PR Review Comment: https://git.openjdk.org/jdk/pull/16018#discussion_r1344648294
PR Review Comment: https://git.openjdk.org/jdk/pull/16018#discussion_r1344645185
PR Review Comment: https://git.openjdk.org/jdk/pull/16018#discussion_r1344646235
PR Review Comment: https://git.openjdk.org/jdk/pull/16018#discussion_r1344649760
Re: RFR: 8316146: Open some swing tests 4 [v7]
On Mon, 2 Oct 2023 22:14:50 GMT, Damon Nguyen wrote: > @alisenchung Looks like `Util.blockTillDisplayed(mainFrame)` has been removed > which was causing compilation issues earlier. Now the updated test works > correctly. Ah, the test didn't compile earlier… - PR Comment: https://git.openjdk.org/jdk/pull/15875#issuecomment-1745073650
Re: RFR: 8316146: Open some swing tests 4 [v9]
On Tue, 3 Oct 2023 02:44:59 GMT, Alisen Chung wrote: >> Opening closed tests: >> 12 javax/swing/ToolTipManager/5078214/bug5078214.java >> 13 javax/swing/plaf/basic/BasicMenuItemUI/4239714/bug4239714.java >> 14 javax/swing/plaf/basic/BasicMenuUI/4244616/bug4244616.java >> 15 javax/swing/plaf/metal/4306431/bug4306431.java > > Alisen Chung has updated the pull request incrementally with one additional > commit since the last revision: > > fixed jtreg header test/jdk/javax/swing/ToolTipManager/bug5078214.java line 1: > 1: /* Interestingly, the test now shows the UI on secondary screen for me. It's not a problem, the test passes. - PR Review Comment: https://git.openjdk.org/jdk/pull/15875#discussion_r1344170795
Re: RFR: 8316146: Open some swing tests 4 [v6]
On Fri, 29 Sep 2023 19:27:33 GMT, Alexey Ivanov wrote: >> Alisen Chung has updated the pull request incrementally with one additional >> commit since the last revision: >> >> fixed skippedexception, fixed comment > > test/jdk/javax/swing/ToolTipManager/bug5078214.java line 66: > >> 64: "with the taskbar at the bottom position."); >> 65: } >> 66: bounds = getGraphicsConfig().getBounds(); > > It's better to use the pattern that was used before: assign the result of > `getGraphicsConfig()` to `testConfig` because `getGraphicsConfig()` is not a > simple getter which returns the value of a field. On the other hand, it's > more like premature optimisation; it's unlikely that enumerating screens > takes a lot of time, especially now when you use the default config only. It's marked resolved but I can't see any related changes. - PR Review Comment: https://git.openjdk.org/jdk/pull/15875#discussion_r1344164215
Re: RFR: 8316146: Open some swing tests 4 [v9]
On Tue, 3 Oct 2023 02:44:59 GMT, Alisen Chung wrote: >> Opening closed tests: >> 12 javax/swing/ToolTipManager/5078214/bug5078214.java >> 13 javax/swing/plaf/basic/BasicMenuItemUI/4239714/bug4239714.java >> 14 javax/swing/plaf/basic/BasicMenuUI/4244616/bug4244616.java >> 15 javax/swing/plaf/metal/4306431/bug4306431.java > > Alisen Chung has updated the pull request incrementally with one additional > commit since the last revision: > > fixed jtreg header Changes requested by aivanov (Reviewer). test/jdk/javax/swing/ToolTipManager/bug5078214.java line 31: > 29: * @build jtreg.SkippedException > 30: * @run main bug5078214 > 31: */ The test does not compile: runner starting test: javax/swing/ToolTipManager/bug5078214.java runner finished test: javax/swing/ToolTipManager/bug5078214.java Error. can't find jtreg.SkippedException in test directory or libraries Test results: error: 1 You still use **`/test/lib`** for `jtreg.SkippedException`. - PR Review: https://git.openjdk.org/jdk/pull/15875#pullrequestreview-1655261619 PR Review Comment: https://git.openjdk.org/jdk/pull/15875#discussion_r1344137620
Re: RFR: 8314755: Resource leak: SwingWorker listener keeps strong reference to executor [v4]
On Tue, 12 Sep 2023 12:34:12 GMT, Christopher Sahnwaldt wrote: >> In >> https://github.com/openjdk/jdk/commit/b8af3d50192f8bc98d83f8102f0fd1989f302e32 >> the weak reference was accidentally changed from a field to a local >> variable, which means that the PropertyChangeListener keeps a strong >> reference to executorService, which is a resource leak > > Christopher Sahnwaldt has updated the pull request incrementally with one > additional commit since the last revision: > > SwingWorkerExecutorLeakTest.java: use AppContext.getAppContext() instead of > SunToolkit.createNewAppContext() to create AppContext, set necessary system > properties > Maybe this issue is a non-issue. Someone who knows how `AppContext` is > currently used should check. > > I just found [this > comment](https://bugs.openjdk.org/browse/JDK-8289616?focusedCommentId=14516108#comment-14516108) > by @prrace: "AppContext.dispose() is no longer called from anywhere in JDK > code." So as I explained in my previous comment, this is a non-issue. I'll > close it. Likely, it's not an issue any more. `AppContext` was used to isolate different applets which run in the same JVM as well as to isolate the deployment stack (Java Console as an example) from applets and Java Web Start apps. As the comment you linked to explains, there should never be more than one `AppContext` and its `dispose()` method is never called. > I guess such a situation only occurs if an application starts a GUI, later > shuts down the GUI, but keeps the JVM running. Should be pretty rare. I think so. GUI application usually has its GUI throughout its entire lifespan. - PR Comment: https://git.openjdk.org/jdk/pull/15081#issuecomment-1743576856
Re: RFR: 8316146: Open some swing tests 4 [v6]
On Fri, 29 Sep 2023 19:31:08 GMT, Alisen Chung wrote: >> Opening closed tests: >> 12 javax/swing/ToolTipManager/5078214/bug5078214.java >> 13 javax/swing/plaf/basic/BasicMenuItemUI/4239714/bug4239714.java >> 14 javax/swing/plaf/basic/BasicMenuUI/4244616/bug4244616.java >> 15 javax/swing/plaf/metal/4306431/bug4306431.java > > Alisen Chung has updated the pull request incrementally with one additional > commit since the last revision: > > fixed skippedexception, fixed comment Marked as reviewed by aivanov (Reviewer). test/jdk/javax/swing/ToolTipManager/bug5078214.java line 66: > 64: "with the taskbar at the bottom position."); > 65: } > 66: bounds = getGraphicsConfig().getBounds(); It's better to use the pattern that was used before: assign the result of `getGraphicsConfig()` to `testConfig` because `getGraphicsConfig()` is not a simple getter which returns the value of a field. On the other hand, it's more like premature optimisation; it's unlikely that enumerating screens takes a lot of time, especially now when you use the default config only. - PR Review: https://git.openjdk.org/jdk/pull/15875#pullrequestreview-1651371634 PR Review Comment: https://git.openjdk.org/jdk/pull/15875#discussion_r1341727276
Re: RFR: JDK-8314731 : Add support for the alt attribute in the image type input HTML tag [v2]
On Wed, 30 Aug 2023 14:20:41 GMT, ScientificWare wrote:
>> This is referenced in Java Bug Database as
>> - [JDK-8314731 : Adds support for the alt attribute in the image type input
>> HTML tag.](https://bugs.java.com/bugdatabase/view_bug?bug_id=8314731)
>>
>> This is tracked in JBS as
>> - [JDK-8314731 : Add support for the alt attribute in the image type input
>> HTML tag](https://bugs.openjdk.java.net/browse/JDK-8314731)
>>
>> According [HTML 3.2
>> specification](https://www.w3.org/TR/2018/SPSD-html32-20180315/#input)
>>
>> `alt` is not an attribute of the `input` element.
>>
>> According [HTML 4.01
>> specifications](https://www.w3.org/TR/html4/interact/forms.html#h-17.4) :
>>
>>> ... For accessibility reasons, authors should provide [alternate
>>> text](https://www.w3.org/TR/html4/struct/objects.html#alternate-text) for
>>> the image via the
>>> [alt](https://www.w3.org/TR/html4/struct/objects.html#adef-alt) attribute.
>>> ...
>>
>> This feature is not implemented in `FormView.java`.
>>
>> ⚠️ ~~This also affects the HTML 32 DTD~~
>>
>> 
>>
>> Left before the patch and right after the patch.
>>
>>
>> import java.awt.BorderLayout;
>> import java.awt.Dimension;
>> import javax.swing.JEditorPane;
>> import javax.swing.JFrame;
>> import javax.swing.JScrollPane;
>> import javax.swing.SwingUtilities;
>> import javax.swing.text.Document;
>> import javax.swing.text.html.HTMLEditorKit;
>> import javax.swing.text.html.StyleSheet;
>>
>> public class HTMLAddsSupportAltInputTag {
>> public static void main(String[] args) {
>> new HTMLAddsSupportAltInputTag();
>> }
>>
>> public HTMLAddsSupportAltInputTag() {
>> SwingUtilities.invokeLater(new Runnable(){
>> public void run(){
>> JEditorPane jEditorPane = new JEditorPane();
>> jEditorPane.setEditable(false);
>> JScrollPane scrollPane = new JScrollPane(jEditorPane);
>> HTMLEditorKit kit = new HTMLEditorKit();
>> jEditorPane.setEditorKit(kit);
>> StyleSheet styleSheet = kit.getStyleSheet();
>> styleSheet.addRule("""
>> body {
>> color: #000;
>> font-family:times;
>> margin: 4px;
>> }
>> """);
>> String htmlString = """
>>
>>
>> > src="file:oracle_logo_50x50.jpg" alt="Logo Oracle JPG">
>>
>> 281: URL srcURL = new URL(base, srcAtt);
> 282: ImageIcon icon = new ImageIcon(srcURL);
You should probably pass `altAtt` as the `description` parameter, yet I'm
unsure if it's ever used in this context.
src/java.desktop/share/classes/javax/swing/text/html/FormView.java line 283:
> 281: URL srcURL = new URL(base, srcAtt);
> 282: ImageIcon icon = new ImageIcon(srcURL);
> 283: button = icon.getImageLoadStatus() ==
> MediaTracker.COMPLETE ? new JButton(icon) : new JButton(altAtt);
Initially I thought the status could change, it can't.
Looks good to me.
src/java.desktop/share/classes/javax/swing/text/html/FormView.java line 285:
> 283: button = icon.getImageLoadStatus() ==
> MediaTracker.COMPLETE ? new JButton(icon) : new JButton(altAtt);
> 284: } catch (MalformedURLException e) {
> 285: button = new JButton(srcAtt);
Suggestion:
button = new JButton(altAtt);
If `altAtt` is provided, it should be used to handle invalid `srcAtt` too.
-
Changes requested by aivanov (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/15319#pullrequestreview-1650659110
PR Review Comment: https://git.openjdk.org/jdk/pull/15319#discussion_r1341292119
PR Review Comment: https://git.openjdk.org/jdk/pull/15319#discussion_r1341310689
PR Review Comment: https://git.openjdk.org/jdk/pull/15319#discussion_r1341317037
Re: RFR: 8316146: Open some swing tests 4 [v5]
On Thu, 28 Sep 2023 20:39:44 GMT, Alisen Chung wrote:
>> Opening closed tests:
>> 12 javax/swing/ToolTipManager/5078214/bug5078214.java
>> 13 javax/swing/plaf/basic/BasicMenuItemUI/4239714/bug4239714.java
>> 14 javax/swing/plaf/basic/BasicMenuUI/4244616/bug4244616.java
>> 15 javax/swing/plaf/metal/4306431/bug4306431.java
>
> Alisen Chung has updated the pull request incrementally with one additional
> commit since the last revision:
>
> removed nested loop and used default graphics configuration
Changes requested by aivanov (Reviewer).
test/jdk/javax/swing/BasicMenuUI/bug4244616.java line 72:
> 70: }
> 71: } finally {
> 72: // Restore streams, check results
Suggestion:
// Restore streams
You don't check the results here — you've already checked the results above.
test/jdk/javax/swing/ToolTipManager/bug5078214.java line 66:
> 64: throw new SkippedException("We need at least one screen "
> +
> 65: "with the taskbar at the bottom position.");
> 66: }
This code will never throw `SkippedException`.
First you have to test whether `getGraphicsConfig()` returned `null` and throw
the exception. Otherwise, `NullPointerException` will be thrown, which fails
the test instead of skipping it.
Alternatively, you may throw `SkippedException` from `getGraphicsConfig()`
instead of returning `null`.
-
PR Review: https://git.openjdk.org/jdk/pull/15875#pullrequestreview-1650635609
PR Review Comment: https://git.openjdk.org/jdk/pull/15875#discussion_r1341278831
PR Review Comment: https://git.openjdk.org/jdk/pull/15875#discussion_r1341274950
Re: RFR: 8313348: Fix typo in JFormattedTextField: 'it self' [v2]
On Thu, 7 Sep 2023 10:24:05 GMT, Alexey Ivanov wrote: > Anyway, I have updated `BasicTreeUI`; in addition to fixing the typo there, I > expanded the wildcard imports. > > Do you have any more comments? @mrserb @mrserb Have I addressed your concerns? Do you have any additional comments? - PR Review Comment: https://git.openjdk.org/jdk/pull/15087#discussion_r1340606749
Integrated: 8313810: BoxLayout uses instead of list for layout options
On Mon, 7 Aug 2023 19:43:38 GMT, Alexey Ivanov wrote:
> A clean-up of documentation for
> [`BoxLayout`](https://docs.oracle.com/en/java/javase/17/docs/api/java.desktop/javax/swing/BoxLayout.html#class-description):
>
> 1. Use the `` element instead of `` to list layout options or
> axes.
> 2. Add links to `GridBagLayout` and `Box` classes.
> 3. Markup references to classes, mostly `BoxLayout` itself, with `{@code}`.
> 4. Separate the introduction to layout description from the description of
> horizontal layout. They're now in different paragraphs for easier scanning.
> 5. Add the missing ‘a’ articles.
> 6. Organise imports, including expanding the wildcard import.
>
> The updated version:
> [**`BoxLayout.html`**](https://cr.openjdk.org/~aivanov/8313810/api/java.desktop/javax/swing/BoxLayout.html#class-description)
> (without the CSS).
This pull request has now been integrated.
Changeset: 09dad0e9
Author:Alexey Ivanov
URL:
https://git.openjdk.org/jdk/commit/09dad0e96b37e3fcd1a13040e0de85ebc04b07c2
Stats: 34 lines in 1 file changed: 14 ins; 1 del; 19 mod
8313810: BoxLayout uses instead of list for layout options
8313811: Improve description of how BoxLayout lays out components
Reviewed-by: prr
-
PR: https://git.openjdk.org/jdk/pull/15182
Integrated: 4622866: javax.swing.text.Document.remove(int, int) has a misleading picture
On Wed, 13 Sep 2023 07:46:50 GMT, Alexey Ivanov wrote:
> The image in the [documentation for
> `Document.remove`](https://docs.oracle.com/en/java/javase/17/docs/api/java.desktop/javax/swing/text/Document.html#remove(int,int))
> looks as if the portion between the offsets 6 and 10 is removed. However,
> the entire region for _‘quick ’_ is highlighted.
>
> I updated the image to mark the start offset 4 and the end offset 10. The new
> image is in SVG format which looks crisp on modern High DPI displays.
>
> I added a sentence that describes the image, which makes the documentation
> accessible and clearer.
>
> Also, I marked up references to classes and members with `{@code}`.
>
> Look at the updated version: [`Document.remove` in JDK
> 22](https://cr.openjdk.org/~aivanov/4622866/api/java.desktop/javax/swing/text/Document.html#remove(int,int)).
This pull request has now been integrated.
Changeset: 73a47f0c
Author:Alexey Ivanov
URL:
https://git.openjdk.org/jdk/commit/73a47f0c4a4f01f62ef55d64120e58535df12623
Stats: 170 lines in 3 files changed: 159 ins; 0 del; 11 mod
4622866: javax.swing.text.Document.remove(int, int) has a misleading picture
Reviewed-by: prr
-
PR: https://git.openjdk.org/jdk/pull/15701
Re: RFR: 8316146: Open some swing tests 4 [v2]
On Thu, 28 Sep 2023 17:10:39 GMT, Phil Race wrote: >>> Since this isn't a test for applet I think we should keep it for now >> >> What do you mean? >> >> All the test does is it verifies the `installComponents` method is >> accessible in a subclass of `BasicMenuItemUI`. The >> [`installComponents`](https://docs.oracle.com/en/java/javase/21/docs/api/java.desktop/javax/swing/plaf/basic/BasicMenuItemUI.html#installComponents(javax.swing.JMenuItem)) >> method is protected since JDK 1.3, it's now a public API. Removing it >> requires a CSR. >> >> Can it be accidentally removed? Can it be accidentally made package-private >> or private? *Very unlikely.* Therefore, we can save time by not running this >> test. > > the comment about applet is a bit out of context. Its from a comment I made > off-line that if you were taking > a test that was written as an Applet and it explicitly a test for applets, > that would be a good example of > a test to drop since JDK 22 doesn't support Applets, even thought the API is > still there. > > In this case, I agree I would not have bothered porting this test that > basically corresponds to the signature tests, > but its also harmless, notwithstanding the very tiny cost of running this as > a (now) headless test. > But since it was already done, I think we can keep it. It is a thousand times > better than it was before in its old form. Thank you for clarification. Let's keep it then. - PR Review Comment: https://git.openjdk.org/jdk/pull/15875#discussion_r1340468384
Re: RFR: 8316146: Open some swing tests 4 [v3]
On Thu, 28 Sep 2023 16:05:41 GMT, Alexey Ivanov wrote:
>> test/jdk/javax/swing/ToolTipManager/bug5078214.java line 147:
>>
>>> 145: });
>>> 146:
>>> 147: }
>>
>> As I said, there's absolutely no reason to call `GraphicsEnvironment`
>> methods on EDT — these are not Swing components.
>>
>> There's no reason to store any of these as static fields, they are local
>> variables.
>>
>> Suggestion:
>>
>> private static GraphicsConfiguration getGraphicsConfig() {
>> GraphicsDevice[] devices =
>> GraphicsEnvironment.getLocalGraphicsEnvironment()
>> .getScreenDevices();
>> for (GraphicsDevice device : devices) {
>> GraphicsConfiguration[] gc = device.getConfigurations();
>> for (int i = 0; i < gc.length; i++) {
>> insets = Toolkit.getDefaultToolkit().getScreenInsets(gc[i]);
>> if (insets.bottom != 0) {
>> return gc[i];
>> }
>> }
>> }
>> return null;
>> }
>>
>>
>> You call this function from the `main` method.
>
> The nested loop could also be enhanced for:
>
> for (GraphicsConfiguration config : device.getConfigurations()) {
> insets = Toolkit.getDefaultToolkit().getScreenInsets(config);
> if (insets.bottom != 0) {
> return config;
> }
> }
And [I still have the
concern](https://github.com/openjdk/jdk/pull/15875#discussion_r1335795417)
about iterating over all the configurations. It hasn't been addressed.
Since the configuration is not set, I think the inner loop is not needed at
all, it should be replaced with
```java
GraphicsConfiguration config = device.getDefaultConfiguration();
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15875#discussion_r1340386748
