Re: RFR: 8303689: javac -Xlint could/should report on "dangling" doc comments [v7]
> Please review the updates to support a proposed new
> `-Xlint:dangling-doc-comments` option.
>
> The work can be thought of as in 3 parts:
>
> 1. An update to the `javac` internal class `DeferredLintHandler` so that it
> is possible to specify the appropriately configured `Lint` object when it is
> time to consider whether to generate the diagnostic or not.
>
> 2. Updates to the `javac` front end to record "dangling docs comments" found
> near the beginning of a declaration, and to report them using an instance of
> `DeferredLintHandler`. This allows the warnings to be enabled or disabled
> using the standard mechanisms for `-Xlint` and `@SuppressWarnings`. The
> procedure for handling dangling doc comments is described in this comment in
> `JavacParser`.
>
> * Dangling documentation comments are handled as follows.
> * 1. {@code Scanner} adds all doc comments to a queue of
> * recent doc comments. The queue is flushed whenever
> * it is known that the recent doc comments should be
> * ignored and should not cause any warnings.
> * 2. The primary documentation comment is the one obtained
> * from the first token of any declaration.
> * (using {@code token.getDocComment()}.
> * 3. At the end of the "signature" of the declaration
> * (that is, before any initialization or body for the
> * declaration) any other "recent" comments are saved
> * in a map using the primary comment as a key,
> * using this method, {@code saveDanglingComments}.
> * 4. When the tree node for the declaration is finally
> * available, and the primary comment, if any,
> * is "attached", (in {@link #attach}) any related
> * dangling comments are also attached to the tree node
> * by registering them using the {@link #deferredLintHandler}.
> * 5. (Later) Warnings may be genereated for the dangling
> * comments, subject to the {@code -Xlint} and
> * {@code @SuppressWarnings}.
>
>
> 3. Updates to the make files to disable the warnings in modules for which
> the
> warning is generated. This is often because of the confusing use of `/**` to
> create box or other standout comments.
Jonathan Gibbons has updated the pull request with a new target base due to a
merge or a rebase. The pull request now contains 10 commits:
- Merge with upstream/master
- Merge remote-tracking branch 'upstream/master' into 8303689.dangling-comments
- Merge remote-tracking branch 'upstream/master' into 8303689.dangling-comments
- Merge with upstream/master
- update test
- improve handling of ignorable doc comments
suppress warning when building test code
- Merge remote-tracking branch 'upstream/master' into 8303689.dangling-comments
- call `saveDanglingDocComments` for local variable declarations
- adjust call for `saveDanglingDocComments` for enum members
- JDK-8303689: javac -Xlint could/should report on "dangling" doc comments
-
Changes: https://git.openjdk.org/jdk/pull/18527/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18527&range=06
Stats: 485 lines in 59 files changed: 387 ins; 3 del; 95 mod
Patch: https://git.openjdk.org/jdk/pull/18527.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18527/head:pull/18527
PR: https://git.openjdk.org/jdk/pull/18527
Integrated: 8330178: Clean up non-standard use of /** comments in `java.base`
On Thu, 18 Apr 2024 20:44:00 GMT, Jonathan Gibbons wrote: > Please review a set of updates to clean up use of `/**` comments in the > vicinity of declarations. > > There are various categories of update: > > * "Box comments" beginning with `/**` > * Misplaced doc comments before package or import statements > * Misplaced doc comments after the annotations for a declaration > * Dangling `/**` comments relating to a group of declarations, separate from > the doc comments for each of those declarations > * Use of `/**` for the legal header at or near the top of the file > > In one case, two doc comments before a declaration were merged, which fixes a > bug/omission in the documented serialized form. This pull request has now been integrated. Changeset: 9cc163a9 Author:Jonathan Gibbons URL: https://git.openjdk.org/jdk/commit/9cc163a999eb8e9597d45b095b642c25071043bd Stats: 134 lines in 23 files changed: 50 ins; 56 del; 28 mod 8330178: Clean up non-standard use of /** comments in `java.base` Reviewed-by: darcy, iris, dfuchs, aivanov, naoto - PR: https://git.openjdk.org/jdk/pull/18846
Re: RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis
On Tue, 23 Apr 2024 14:25:22 GMT, Magnus Ihse Bursie wrote: > There's a huge amount of changes for just hsdis... You might have to separate > out the infrastructure changes that seem to amount to most of the changes > here. > > This is going to take me a while to get through. Sorry, it's still a WIP, and I believe something broke and scattered changes from one of my other local branches into this current changeset - PR Comment: https://git.openjdk.org/jdk/pull/18915#issuecomment-2072483937
Re: RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis
On Tue, 23 Apr 2024 13:56:32 GMT, Julian Waters wrote: > WIP > > This changeset contains hsdis for Windows/gcc Port. It supports both the > binutils and capstone backends, though the LLVM backend is left out due to > compatibility issues encountered during the build. Currently, which gcc > distributions are supported is still to be clarified, as several, ranging > from Cygwin, to MinGW64, to the gcc subsystems from MSYS2. For this reason, > the build system hack in place at the moment to compile the binutils backend > on Windows is still left in place, for now. There's a huge amount of changes for just hsdis... You might have to separate out the infrastructure changes that seem to amount to most of the changes here. This is going to take me a while to get through. - PR Comment: https://git.openjdk.org/jdk/pull/18915#issuecomment-2072458273
RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis
WIP This changeset contains hsdis for Windows/gcc Port. It supports both the binutils and capstone backends, though the LLVM backend is left out due to compatibility issues encountered during the build. Currently, which gcc distributions are supported is still to be clarified, as several, ranging from Cygwin, to MinGW64, to the gcc subsystems from MSYS2. For this reason, the build system hack in place at the moment to compile the binutils backend on Windows is still left in place, for now. - Commit messages: - Implementation of hsdis for 8288293: Windows/gcc Port Changes: https://git.openjdk.org/jdk/pull/18915/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18915&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8330988 Stats: 347 lines in 28 files changed: 161 ins; 22 del; 164 mod Patch: https://git.openjdk.org/jdk/pull/18915.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18915/head:pull/18915 PR: https://git.openjdk.org/jdk/pull/18915
Integrated: 8289770: Remove Windows version macro from ShellFolder2.cpp
On Thu, 11 Apr 2024 09:33:09 GMT, Alexey Ivanov wrote: > This clean-up PR removes unused Windows version macro from `ShellFolder2.cpp`. > > `IS_WINVISTA` was not used at all. > > `IS_WINXP` guarded support for icons with alpha channel. It is now safe to > assume Java runs on a Windows version later than Windows XP. Java launchers > specify 6.0 as the minimum OS version which corresponds to Windows Vista. This pull request has now been integrated. Changeset: 3d5eeac3 Author:Alexey Ivanov URL: https://git.openjdk.org/jdk/commit/3d5eeac3a38ece4a23ea6da2dfe5939d64e81cea Stats: 18 lines in 1 file changed: 0 ins; 13 del; 5 mod 8289770: Remove Windows version macro from ShellFolder2.cpp Reviewed-by: jwaters, tr, serb - PR: https://git.openjdk.org/jdk/pull/18736
Re: RFR: 8322135: javax/swing/JTable/JTableScrollPrintTest.java & javax/swing/JTable/PrintAllPagesTest.java throws java.lang.InternalError: HTHEME is null [v2]
On Tue, 23 Apr 2024 05:02:53 GMT, Tejesh R wrote: > > Do you mind if I shorten the subject of the bug? _Printing JTable throws > > InternalError: HTHEME is null_? > > Sure, it's better to shorten it. And since its only for windows L&F, it can > be mentioned in subject I guess. I've updated the subject to: _Printing JTable in Windows L&F throws InternalError: HTHEME is null_. - PR Comment: https://git.openjdk.org/jdk/pull/18706#issuecomment-2072138953
Re: RFR: 8322135: javax/swing/JTable/JTableScrollPrintTest.java & javax/swing/JTable/PrintAllPagesTest.java throws java.lang.InternalError: HTHEME is null [v6]
On Tue, 23 Apr 2024 11:02:48 GMT, Tejesh R wrote: >> Getting a theme for particular dpi failed in windows L&F during print test. >> Before [JDK-8294427](https://bugs.openjdk.org/browse/JDK-8294427) fix, theme >> was independent of DPI. After the fix >> (https://github.com/openjdk/jdk/commit/a63afa4aa62863d1a199a0fb7d2f56ff8fcd04fd) >> getting a theme in Windows L&F became dependent on DPI. Now it works fine >> in paint to a frame or window but for printing the DPI is computed with >> scaling factor which might not be w.r.t set of connected monitors and hence >> error is returned according to [this from windows >> document](https://learn.microsoft.com/en-us/windows/win32/api/uxtheme/nf-uxtheme-openthemedatafordpi). >> >> The suggested fix is to handle printer cases with default dpi since in error >> condition 0 is returned with `openThemeImpl(widget, dpi)`. The fix seems to >> be working fine without causing any regression (Tested in CI systems and >> also the affected tests). > > Tejesh R has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains seven commits: > > - Fixing merge conflicts > - Review updates > - Review updates > - Review updates > - Moved failure handling inside openThemeImpl method > - Updated with null check > - Fix Marked as reviewed by aivanov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18706#pullrequestreview-2016981437
Re: RFR: 8322135: javax/swing/JTable/JTableScrollPrintTest.java & javax/swing/JTable/PrintAllPagesTest.java throws java.lang.InternalError: HTHEME is null [v6]
> Getting a theme for particular dpi failed in windows L&F during print test. > Before [JDK-8294427](https://bugs.openjdk.org/browse/JDK-8294427) fix, theme > was independent of DPI. After the fix > (https://github.com/openjdk/jdk/commit/a63afa4aa62863d1a199a0fb7d2f56ff8fcd04fd) > getting a theme in Windows L&F became dependent on DPI. Now it works fine in > paint to a frame or window but for printing the DPI is computed with scaling > factor which might not be w.r.t set of connected monitors and hence error is > returned according to [this from windows > document](https://learn.microsoft.com/en-us/windows/win32/api/uxtheme/nf-uxtheme-openthemedatafordpi). > > The suggested fix is to handle printer cases with default dpi since in error > condition 0 is returned with `openThemeImpl(widget, dpi)`. The fix seems to > be working fine without causing any regression (Tested in CI systems and also > the affected tests). Tejesh R has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains seven commits: - Fixing merge conflicts - Review updates - Review updates - Review updates - Moved failure handling inside openThemeImpl method - Updated with null check - Fix - Changes: https://git.openjdk.org/jdk/pull/18706/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18706&range=05 Stats: 17 lines in 3 files changed: 9 ins; 0 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/18706.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18706/head:pull/18706 PR: https://git.openjdk.org/jdk/pull/18706
Re: RFR: 8322135: javax/swing/JTable/JTableScrollPrintTest.java & javax/swing/JTable/PrintAllPagesTest.java throws java.lang.InternalError: HTHEME is null [v5]
> Getting a theme for particular dpi failed in windows L&F during print test. > Before [JDK-8294427](https://bugs.openjdk.org/browse/JDK-8294427) fix, theme > was independent of DPI. After the fix > (https://github.com/openjdk/jdk/commit/a63afa4aa62863d1a199a0fb7d2f56ff8fcd04fd) > getting a theme in Windows L&F became dependent on DPI. Now it works fine in > paint to a frame or window but for printing the DPI is computed with scaling > factor which might not be w.r.t set of connected monitors and hence error is > returned according to [this from windows > document](https://learn.microsoft.com/en-us/windows/win32/api/uxtheme/nf-uxtheme-openthemedatafordpi). > > The suggested fix is to handle printer cases with default dpi since in error > condition 0 is returned with `openThemeImpl(widget, dpi)`. The fix seems to > be working fine without causing any regression (Tested in CI systems and also > the affected tests). Tejesh R has updated the pull request incrementally with one additional commit since the last revision: Review updates - Changes: - all: https://git.openjdk.org/jdk/pull/18706/files - new: https://git.openjdk.org/jdk/pull/18706/files/9c96cf68..0896dab8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18706&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18706&range=03-04 Stats: 9 lines in 3 files changed: 0 ins; 0 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/18706.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18706/head:pull/18706 PR: https://git.openjdk.org/jdk/pull/18706
Re: RFR: 8322135: javax/swing/JTable/JTableScrollPrintTest.java & javax/swing/JTable/PrintAllPagesTest.java throws java.lang.InternalError: HTHEME is null [v4]
On Tue, 23 Apr 2024 10:34:32 GMT, Alexey Ivanov wrote: >> Means only till openThemeImpl return value and not further up the hierarchy ? > > Yes, only the low-level methods until it's put into a map. Yeah, sounds good. Will do that. - PR Review Comment: https://git.openjdk.org/jdk/pull/18706#discussion_r1576036004
Re: RFR: 8322135: javax/swing/JTable/JTableScrollPrintTest.java & javax/swing/JTable/PrintAllPagesTest.java throws java.lang.InternalError: HTHEME is null [v4]
On Tue, 23 Apr 2024 10:30:49 GMT, Tejesh R wrote: >> src/java.desktop/windows/classes/sun/awt/windows/ThemeReader.java line 110: >> >>> 108:// See documentation for SetWindowTheme on MSDN. >>> 109:setWindowTheme(widget.substring(0, i)); >>> 110:theme = getOpenThemeValue(widget.substring(i + 2), dpi); >> >> I suggest changing the type of `theme` variable from `Long` to `long` as >> well as the return value of `openThemeImpl`. > > Means only till openThemeImpl return value and not further up the hierarchy ? Yes, only the low-level methods until it's put into a map. - PR Review Comment: https://git.openjdk.org/jdk/pull/18706#discussion_r1576031141
Re: RFR: 8322135: javax/swing/JTable/JTableScrollPrintTest.java & javax/swing/JTable/PrintAllPagesTest.java throws java.lang.InternalError: HTHEME is null [v2]
On Tue, 23 Apr 2024 10:23:33 GMT, Tejesh R wrote: >> I didn't get the need for helper method? > > Ok, I got it. And I have updated. But if we want to change `theme` to `long`, > then I guess we have to do it every calling function I guess. So is it better > to leave it as `Long` ? I still think we should change `Long` to `long`. Yes, you'll need to update it in the two methods. In `getThemeImpl`, it will be boxed into `Long` automatically before being put into the map. - PR Review Comment: https://git.openjdk.org/jdk/pull/18706#discussion_r1576027262
Re: RFR: 8322135: javax/swing/JTable/JTableScrollPrintTest.java & javax/swing/JTable/PrintAllPagesTest.java throws java.lang.InternalError: HTHEME is null [v2]
On Tue, 23 Apr 2024 10:31:02 GMT, Alexey Ivanov wrote: >> Ok, I got it. And I have updated. But if we want to change `theme` to >> `long`, then I guess we have to do it every calling function I guess. So is >> it better to leave it as `Long` ? > > I still think we should change `Long` to `long`. Yes, you'll need to update > it in the two methods. In `getThemeImpl`, it will be boxed into `Long` > automatically before being put into the map. When the primitive type `long` is used, there's no confusion whether the value can be `null` or not. - PR Review Comment: https://git.openjdk.org/jdk/pull/18706#discussion_r1576028406
Re: RFR: 8322135: javax/swing/JTable/JTableScrollPrintTest.java & javax/swing/JTable/PrintAllPagesTest.java throws java.lang.InternalError: HTHEME is null [v4]
On Tue, 23 Apr 2024 10:24:22 GMT, Alexey Ivanov wrote: >> Tejesh R has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Review updates > > src/java.desktop/windows/classes/sun/awt/windows/ThemeReader.java line 110: > >> 108:// See documentation for SetWindowTheme on MSDN. >> 109:setWindowTheme(widget.substring(0, i)); >> 110:theme = getOpenThemeValue(widget.substring(i + 2), dpi); > > I suggest changing the type of `theme` variable from `Long` to `long` as well > as the return value of `openThemeImpl`. Means only till openThemeImpl return value and not further up the hierarchy ? - PR Review Comment: https://git.openjdk.org/jdk/pull/18706#discussion_r1576026977
Re: RFR: 8322135: javax/swing/JTable/JTableScrollPrintTest.java & javax/swing/JTable/PrintAllPagesTest.java throws java.lang.InternalError: HTHEME is null [v4]
On Tue, 23 Apr 2024 10:19:40 GMT, Tejesh R wrote:
>> Getting a theme for particular dpi failed in windows L&F during print test.
>> Before [JDK-8294427](https://bugs.openjdk.org/browse/JDK-8294427) fix, theme
>> was independent of DPI. After the fix
>> (https://github.com/openjdk/jdk/commit/a63afa4aa62863d1a199a0fb7d2f56ff8fcd04fd)
>> getting a theme in Windows L&F became dependent on DPI. Now it works fine
>> in paint to a frame or window but for printing the DPI is computed with
>> scaling factor which might not be w.r.t set of connected monitors and hence
>> error is returned according to [this from windows
>> document](https://learn.microsoft.com/en-us/windows/win32/api/uxtheme/nf-uxtheme-openthemedatafordpi).
>>
>> The suggested fix is to handle printer cases with default dpi since in error
>> condition 0 is returned with `openThemeImpl(widget, dpi)`. The fix seems to
>> be working fine without causing any regression (Tested in CI systems and
>> also the affected tests).
>
> Tejesh R has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Review updates
Looks good to me now.
You should add `8322135` to the list of bugs in the tests which failed because
of this bug.
Update the copyright year in modified files.
src/java.desktop/windows/classes/sun/awt/windows/ThemeReader.java line 110:
> 108:// See documentation for SetWindowTheme on MSDN.
> 109:setWindowTheme(widget.substring(0, i));
> 110:theme = getOpenThemeValue(widget.substring(i + 2), dpi);
I suggest changing the type of `theme` variable from `Long` to `long` as well
as the return value of `openThemeImpl`.
src/java.desktop/windows/classes/sun/awt/windows/ThemeReader.java line 125:
> 123: }
> 124: return theme;
> 125: }
Suggestion:
private static long getOpenThemeValue(String widget, int dpi) {
long theme = openTheme(widget, dpi);
if (theme == 0) {
theme = openTheme(widget, defaultDPI);
}
return theme;
}
Yes, that's what I meant. Let's use the primitive `long`.
-
Changes requested by aivanov (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18706#pullrequestreview-2016847876
PR Review Comment: https://git.openjdk.org/jdk/pull/18706#discussion_r1576019642
PR Review Comment: https://git.openjdk.org/jdk/pull/18706#discussion_r1576017834
Re: RFR: 8322135: javax/swing/JTable/JTableScrollPrintTest.java & javax/swing/JTable/PrintAllPagesTest.java throws java.lang.InternalError: HTHEME is null [v2]
On Tue, 23 Apr 2024 07:11:30 GMT, Tejesh R wrote:
>> src/java.desktop/windows/classes/sun/awt/windows/ThemeReader.java line 117:
>>
>>> 115:if (theme == null || theme == 0) {
>>> 116:theme = openTheme(widget, defaultDPI);
>>> 117:}
>>
>> `theme` can't be `null` because `openTheme` returns `long`. Perhaps, the
>> declaration should be changed to
>>
>> long theme;
>>
>>
>> This is still incorrect. If `i > 0`, there's a prerequisite to calling
>> `openTheme`. Likely, you need another helper method.
>
> I didn't get the need for helper method?
Ok, I got it. And I have updated. But if we want to change `theme` to `long`,
then I guess we have to do it every calling function I guess. So is it better
to leave it as `Long` ?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18706#discussion_r1576018740
Re: RFR: 8322135: javax/swing/JTable/JTableScrollPrintTest.java & javax/swing/JTable/PrintAllPagesTest.java throws java.lang.InternalError: HTHEME is null [v4]
> Getting a theme for particular dpi failed in windows L&F during print test. > Before [JDK-8294427](https://bugs.openjdk.org/browse/JDK-8294427) fix, theme > was independent of DPI. After the fix > (https://github.com/openjdk/jdk/commit/a63afa4aa62863d1a199a0fb7d2f56ff8fcd04fd) > getting a theme in Windows L&F became dependent on DPI. Now it works fine in > paint to a frame or window but for printing the DPI is computed with scaling > factor which might not be w.r.t set of connected monitors and hence error is > returned according to [this from windows > document](https://learn.microsoft.com/en-us/windows/win32/api/uxtheme/nf-uxtheme-openthemedatafordpi). > > The suggested fix is to handle printer cases with default dpi since in error > condition 0 is returned with `openThemeImpl(widget, dpi)`. The fix seems to > be working fine without causing any regression (Tested in CI systems and also > the affected tests). Tejesh R has updated the pull request incrementally with one additional commit since the last revision: Review updates - Changes: - all: https://git.openjdk.org/jdk/pull/18706/files - new: https://git.openjdk.org/jdk/pull/18706/files/f78ed3e0..9c96cf68 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18706&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18706&range=02-03 Stats: 14 lines in 1 file changed: 9 ins; 3 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/18706.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18706/head:pull/18706 PR: https://git.openjdk.org/jdk/pull/18706
Re: RFR: 8327696: [TESTBUG] "javax/swing/JTable/KeyBoardNavigation/KeyBoardNavigation.java" test instruction needs to be corrected [v2]
> Instructions set has been updated as per OS specific. JTable keyboard > navigation is tested in each OS and according it's current implementation the > instructions has been updated (Few has been removed and few has been > updated). > PassFailJFrame.builder is used. Tejesh R has updated the pull request incrementally with one additional commit since the last revision: Review updates - Changes: - all: https://git.openjdk.org/jdk/pull/18855/files - new: https://git.openjdk.org/jdk/pull/18855/files/5a999e62..17a87679 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18855&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18855&range=00-01 Stats: 40 lines in 1 file changed: 0 ins; 2 del; 38 mod Patch: https://git.openjdk.org/jdk/pull/18855.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18855/head:pull/18855 PR: https://git.openjdk.org/jdk/pull/18855
Re: RFR: 8327696: [TESTBUG] "javax/swing/JTable/KeyBoardNavigation/KeyBoardNavigation.java" test instruction needs to be corrected
On Tue, 23 Apr 2024 06:44:32 GMT, Abhishek Kumar wrote:
>> Instructions set has been updated as per OS specific. JTable keyboard
>> navigation is tested in each OS and according it's current implementation
>> the instructions has been updated (Few has been removed and few has been
>> updated).
>> PassFailJFrame.builder is used.
>
> test/jdk/javax/swing/JTable/KeyBoardNavigation.java line 105:
>
>> 103: TableCellRenderer headerRenderer =
>> colorColumn.getHeaderRenderer();
>> 104: if (headerRenderer instanceof DefaultTableCellRenderer)
>> 105: ((DefaultTableCellRenderer)
>> headerRenderer).setToolTipText("Hi Mom!");
>
> Suggestion:
> Usage of enhanced `instanceOf` avoids the casting to
> `DefaultTableCellRenderer` below. Use `{ }` for single if statement too.
>
> Suggestion:
>
> if (colorColumn.getHeaderRenderer() instanceof
> DefaultTableCellRenderer headerRenderer ) {
>headerRenderer.setToolTipText("Hi Mom!");
>}
>
>
> Same can be used at L115 as well.
> `int cellValue = (value instanceof Number number) ? number.intValue() : 0;`
Yeah, updated.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18855#discussion_r1575791723
Re: RFR: 8327696: [TESTBUG] "javax/swing/JTable/KeyBoardNavigation/KeyBoardNavigation.java" test instruction needs to be corrected
On Mon, 22 Apr 2024 21:49:54 GMT, Damon Nguyen wrote: >> Instructions set has been updated as per OS specific. JTable keyboard >> navigation is tested in each OS and according it's current implementation >> the instructions has been updated (Few has been removed and few has been >> updated). >> PassFailJFrame.builder is used. > > test/jdk/javax/swing/JTable/KeyBoardNavigation.java line 235: > >> 233: Control-shift-PageUp/PageDown - extend selection >> left/right >> 234: end of row >> 235: """; > > I see that for "Navigate In", you mix capitalization for `Tab` and > `shift-tab`. Looks a bit off. Maybe better to standardize capitalization for > keys. Maybe title-case or all-caps the key names. Sure, I have now Capitalized the Keys. - PR Review Comment: https://git.openjdk.org/jdk/pull/18855#discussion_r1575786947
Re: RFR: 8319598: SMFParser misinterprets interrupted running status [v2]
On Sat, 11 Nov 2023 12:32:15 GMT, Jan Trukenmüller wrote: >> The MIDI file parser misinterprets events without status byte when they >> appear directly after a Meta of SysEx event. >> >> For my bugfix I had to decide between two possible solutions: >> - Strict solution: Throw an InvalidMidiDataException >> - Tolerant solution: Use the status of the last channel event as running >> status >> >> I used the tolerant solution. >> I will send an email to the list client-libs-dev with my reasons. > > Jan Trukenmüller has updated the pull request incrementally with one > additional commit since the last revision: > > reduced line lengths Marked as reviewed by serb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16546#pullrequestreview-2016467422
Re: RFR: 8327696: [TESTBUG] "javax/swing/JTable/KeyBoardNavigation/KeyBoardNavigation.java" test instruction needs to be corrected
On Mon, 22 Apr 2024 21:41:12 GMT, Damon Nguyen wrote: >> Instructions set has been updated as per OS specific. JTable keyboard >> navigation is tested in each OS and according it's current implementation >> the instructions has been updated (Few has been removed and few has been >> updated). >> PassFailJFrame.builder is used. > > test/jdk/javax/swing/JTable/KeyBoardNavigation.java line 190: > >> 188: PassFailJFrame.builder() >> 189: .instructions(INSTRUCTIONS) >> 190: .rows(11) > > Suggestion: > > .rows((int) INSTRUCTIONS.lines().count() + 2) > > > This is what we used for our tests. I default to using this now myself. This doesn't work here since the Instruction rows are quite more. - PR Review Comment: https://git.openjdk.org/jdk/pull/18855#discussion_r1575777024
Re: RFR: 8327696: [TESTBUG] "javax/swing/JTable/KeyBoardNavigation/KeyBoardNavigation.java" test instruction needs to be corrected
On Mon, 22 Apr 2024 16:03:12 GMT, Alisen Chung wrote: >> Instructions set has been updated as per OS specific. JTable keyboard >> navigation is tested in each OS and according it's current implementation >> the instructions has been updated (Few has been removed and few has been >> updated). >> PassFailJFrame.builder is used. > > test/jdk/javax/swing/JTable/KeyBoardNavigation.java line 130: > >> 128: frame.add(scrollPane); >> 129: frame.pack(); >> 130: frame.setVisible(true); > > no need for frame.setVisible since PassFailJFrame will do that Done. - PR Review Comment: https://git.openjdk.org/jdk/pull/18855#discussion_r1575773246
Re: RFR: 8322135: javax/swing/JTable/JTableScrollPrintTest.java & javax/swing/JTable/PrintAllPagesTest.java throws java.lang.InternalError: HTHEME is null [v2]
On Mon, 22 Apr 2024 15:09:37 GMT, Alexey Ivanov wrote:
>> Tejesh R has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Moved failure handling inside openThemeImpl method
>
> src/java.desktop/windows/classes/sun/awt/windows/ThemeReader.java line 117:
>
>> 115:if (theme == null || theme == 0) {
>> 116:theme = openTheme(widget, defaultDPI);
>> 117:}
>
> `theme` can't be `null` because `openTheme` returns `long`. Perhaps, the
> declaration should be changed to
>
> long theme;
>
>
> This is still incorrect. If `i > 0`, there's a prerequisite to calling
> `openTheme`. Likely, you need another helper method.
I didn't get the need for helper method?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18706#discussion_r1575758721
Re: RFR: 8323965: modify fix for 8317771 to remove reflection instantiation of the inner class
On Tue, 23 Apr 2024 02:25:57 GMT, Alexander Zuev wrote: >> I replaced reflection with using an accessor >> @azuev-java please review > > The problem with this fix is that on the test example attached to the bug any > attempt of navigation trough the items of JTree whole voice over is enabled > causes java to stop responding. I see in the logs that it does call this > exact place thousands of time constantly. So it seems like it makes the > problem with java stalling on large size trees to re-appear. @azuev-java I'm restoring the context: There was a cycle that recursively collected children, and on new MacOS it worked for a very long time.. JDK-8317771 . I added a solution for JTree, which works much faster, but there was a reflection, you asked to remove it. Unlike the old algorithm, it now works in seconds, not minutes... The essence of JDK-8329667 is that the custom tree did not work due to reflection. I removed the reflection, I did not add any additional acceleration. As for speeding up the tree, I suggest adding caching, similar to how it is implemented in tables. - PR Comment: https://git.openjdk.org/jdk/pull/18867#issuecomment-2071587990
