Re: RFR: 8333403: Write a test to check various components events are triggered properly [v3]
On Tue, 25 Jun 2024 01:02:29 GMT, Damon Nguyen wrote: >> Ravi Gupta has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8333403: review comments fixed > > Tested on MacOS. The test passes and looks functional. Minor comments. > > Have you tried on Windows with different scaling? Looks like the only > component location you access is for the panel position, which should be > safe. I tried different scalings on MacOS with no issue. Curious because this > was an issue with JButton a while before. @DamonGuy I have tried on Windows with different scaling , works fine for me . - PR Comment: https://git.openjdk.org/jdk/pull/19521#issuecomment-2196120650
Re: RFR: 8334599: Improve code from JDK-8302671
On Mon, 24 Jun 2024 20:40:54 GMT, Alexey Ivanov wrote: > Will this resolve Phil's concern? Both arrays will use the same size. @prrace Does this address your concerns? If it does I will commit it - PR Review Comment: https://git.openjdk.org/jdk/pull/19798#discussion_r1657972007
Re: RFR: 8334495: Use FFM instead of jdk.internal.misc.Unsafe in java.desktop font implementation [v2]
On Wed, 26 Jun 2024 20:51:18 GMT, Alisen Chung wrote: >> Phil Race has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8334495 > > src/java.desktop/share/classes/sun/font/GlyphList.java line 364: > >> 362: * a native call which does a getPrimitiveArrayCritical and a >> 363: * memcpy for the typical amount of image data (30-150 bytes) >> 364: * Consider a native method if there is a performance problem >> (which > > should this comment be removed now that we aren't using the unsafe loop > anymore? Yes, in fact I thought I HAD removed it. Thanks for spotting. > src/java.desktop/unix/classes/sun/font/XRGlyphCacheEntry.java line 86: > >> 84: // 'void*' (see field 'cellInfo' of struct 'GlyphInfo' >> 85: // in src/share/native/sun/font/fontscalerdefs.h). >> 86: // On 64-bit Big-endian architectures it would be wrong to >> access this > > "wrong to write this field as an int"? "access" because it is true for read and write. - PR Review Comment: https://git.openjdk.org/jdk/pull/19777#discussion_r1657620353 PR Review Comment: https://git.openjdk.org/jdk/pull/19777#discussion_r1657619571
Re: RFR: 8334495: Use FFM instead of jdk.internal.misc.Unsafe in java.desktop font implementation [v3]
> Migrate font code from jdk.internal.misc.Unsafe to using FFM. > This reduces the coupling between the java.desktop module and the internals > of the java.base module. > > The code being changed here is not particularly performance sensitive, and it > is not executed in the most common cases. > The main impact performance-wise is a total of around 37ms in initialisation > costs on my x64 macbook. > A minimal program that just draws a string to an image - does not even put up > a window - runs at around 690-700ms. > There's variability in that number and the overall time for a JDK without the > change is around (660-670ms) > In the small test, this is the first and only use of FFM, so the one-off part > cost should move elsewhere when FFM starts > to be used earlier in the JDK itself. Phil Race has updated the pull request incrementally with one additional commit since the last revision: 8334495 - Changes: - all: https://git.openjdk.org/jdk/pull/19777/files - new: https://git.openjdk.org/jdk/pull/19777/files/99acf847..d3ca7f4b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19777=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19777=01-02 Stats: 6 lines in 1 file changed: 0 ins; 6 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19777.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19777/head:pull/19777 PR: https://git.openjdk.org/jdk/pull/19777
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v9]
On Thu, 27 Jun 2024 10:06:49 GMT, Abhishek Kumar wrote:
>> In GTK LAF, the menu mnemonics are always displayed which is different from
>> the native behavior. In native application **(tested with gedit for normal
>> buttons and tested with libreoffice for menu**), the menu mnemonics toggle
>> on press of `ALT` key. Menu mnemonics are hidden initially and then toggles
>> between show/hide on `ALT` press.
>> Proposed fix is to handle the `ALT` key press for GTK LAF and mimic the
>> native behavior. Fix is similar to the `ALT` key processing in Windows LAF.
>> Automated test case is added to verify the fix and tested in Ubuntu and
>> Oracle linux.
>>
>> CI testing is green and link attached in JBS.
>
> Abhishek Kumar has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Mnemonic handler class
> - Mnemonic handler added and previous implementation revert back
I think we're moving in the right direction.
My idea was to extract the common functionality as the methods
`setMnemonicHidden`, `isMnemonicHidden` and `repaintMnemonicsInWindow`,
`repaintMnemonicsInContainer` into a helper class.
`AltProcessor`… Your version for GTK and the version in Aqua look the same to
me. I think it makes sense to create a common `AltProcessor` class which would
be used by GTK and Aqua.
The Windows version is different, so it'll stay this way… It doesn't look worth
trying to fit that version into a shared class.
src/java.desktop/share/classes/com/sun/java/swing/plaf/gtk/GTKLookAndFeel.java
line 1461:
> 1459:
> 1460: KeyboardFocusManager.getCurrentKeyboardFocusManager().
> 1461: addKeyEventPostProcessor(MnemonicHandler.altProcessor);
Suggestion:
KeyboardFocusManager.getCurrentKeyboardFocusManager()
.addKeyEventPostProcessor(MnemonicHandler.altProcessor);
Better wrap the line before the dot operator — this conveys that it's a
continuation of a call sequence rather than a wrapped parameter to a method.
src/java.desktop/share/classes/com/sun/java/swing/plaf/gtk/GTKLookAndFeel.java
line 1469:
> 1467: /**
> 1468: * Called by UIManager when this look and feel is uninstalled.
> 1469: */
Does it repeat the javadoc of the super class? Likely it does, use
`{@inheritDoc}` instead.
src/java.desktop/share/classes/com/sun/java/swing/plaf/gtk/GTKLookAndFeel.java
line 1470:
> 1468: * Called by UIManager when this look and feel is uninstalled.
> 1469: */
> 1470: @Override
You may want to add the `@Override` annotation to `initialize` method too… for
consistency.
src/java.desktop/share/classes/javax/swing/plaf/synth/SynthGraphicsUtils.java
line 672:
> 670: int mnemIndex =
> lh.getMenuItem().getDisplayedMnemonicIndex();
> 671: // Check to see if the Mnemonic should be rendered in
> GTK.
> 672: if (mnemIndex >= 0 &&
> MnemonicHandler.isMnemonicHidden()) {
Is 0 a valid mnemonic? Probably not.
src/java.desktop/share/classes/sun/swing/MnemonicHandler.java line 40:
> 38: import javax.swing.UIManager;
> 39:
> 40: public class MnemonicHandler {
Suggestion:
public final class MnemonicHandler {
It's an utility class that should not be extended. In addition to that, it
should have a private constructor to prevent instantiating the class.
src/java.desktop/share/classes/sun/swing/MnemonicHandler.java line 41:
> 39:
> 40: public class MnemonicHandler {
> 41: public static final AltProcessor altProcessor = new AltProcessor();
I'd like to see `AltProcessor` here but it's different for Windows. Thus,
`AltProcessor` should be local to `*RootUI` as it is now.
If `AltProcessor` in GTK and Aqua are similar or the same, it makes sense to
extract them into a common helper class.
src/java.desktop/share/classes/sun/swing/MnemonicHandler.java line 43:
> 41: public static final AltProcessor altProcessor = new AltProcessor();
> 42:
> 43: protected static boolean isMnemonicHidden;
Suggestion:
private static boolean isMnemonicHidden;
It's accessed via `isMnemonicHidden` and `setMnemonicHidden`; since the class
cannot be extended, it should not have protected members.
src/java.desktop/share/classes/sun/swing/MnemonicHandler.java line 103:
> 101: * Repaints all the components with the mnemonics in the given
> window and all its owned windows.
> 102: */
> 103: static void repaintMnemonicsInWindow(final Window w) {
Suggestion:
public static void repaintMnemonicsInWindow(final Window w) {
This method would be called from `AltProcessor` class in the three different
L, it should be `public`.
src/java.desktop/share/classes/sun/swing/MnemonicHandler.java line 120:
> 118: * Recursively searches for all the subcomponents.
> 119: */
> 120: static void repaintMnemonicsInContainer(final Container cont) {
If `repaintMnemonicsInContainer` is never called directly but only from
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-1774464453
Re: RFR: 8334868: Ensure CheckUninstallModalHook is called in WPageDialogPeer._show
On Thu, 27 Jun 2024 13:15:19 GMT, Julian Waters wrote: > It would be helpful if JNI had a jobject unique_ptr type for C++ But `std::unique_ptr` wasn't available when JNI had been conceived… It could be added… The declaration in your sample still looks cumbersome… or _unwieldy_ as you said. Writing a custom deleter for each `jobject` doesn't look good either. Most local references don't require explicit `DeleteLocalRef`, which also helps, I guess. > Oops, you sent your correction about the custom deleter just as I sent my post C++ has got many new features… I'm learning these feature on the fly since I don't use C++ as often. - PR Comment: https://git.openjdk.org/jdk/pull/19867#issuecomment-2194699149
Re: RFR: 8334868: Ensure CheckUninstallModalHook is called in WPageDialogPeer._show
On Mon, 24 Jun 2024 20:16:30 GMT, Alexey Ivanov wrote:
> This is somewhat a continuation for
> [JDK-8307160](https://bugs.openjdk.org/browse/JDK-8307160) and
> [JDK-8334509](https://bugs.openjdk.org/browse/JDK-8334509).
>
> The former removed the `doIt` flag in #18584, but it introduced a regression.
>
> The regression is resolved by the latter in #19786, and it added the 'doIt'
> flag again.
>
> I think using a flag makes the code less clear. When reading the code, one
> has to keep track of the current value of the `doIt` flag.
>
> I raised my concern in [comments in PR
> 19786](https://github.com/openjdk/jdk/pull/19786#discussion_r1649191308):
>
>> I'd rather keep `JNI_FALSE` here — it's more explicit and therefore clearer.
>> You don't have to keep track of the value of `doIt` flag while reading the
>> code.
>>
>> In fact, I'd prefer no `doIt` flag at all… yet it makes handling the code
>> below `if (ret)` slightly harder.
>
> I came up with a solution which simplifies handling the clean-up. The
> solution relies on C++ destructors which are called when the declared objects
> go out of scope.
>
> The C++ object wraps a lambda function to clean up and invokes this function
> in its destructor. Such C++ class has to be a templated class because there's
> no defined type to represent a lambda. The class has to be declared at the
> top of the file because it needs C++ linkage.
>
> There are two `Cleaner` objects declared in the code of
> `Java_sun_awt_windows_WPageDialogPeer__1show`:
>
> **`refCleaner`** is declared right after all the references to Java objects
> are initialised. The corresponding `refCleanup` lambda deletes all the
> references and replaces the `CLEANUP_SHOW` macro. Before JDK-8307160, the
> code jumped to a go-to label to perform the clean-up.
>
> **`postCleaner`** is declared after `AwtDialog::CheckInstallModalHook` is
> called. Its lambda `postCleanup` uninstalls the modal hook and handles focus
> transfer as well as saves the updated printer parameters.
>
> It is `postCleaner` that resolves the bug JDK-8334868 by ensuring
> `CheckUninstallModalHook` and `ModalActivateNextWindow` are called if
> `::PageSetupDlg` is called.
>
> As the result of using `refCleaner`, all the `return` statements in the
> function use an explicit value, which makes the code cleaner. There's no need
> to use a `go to` statement or insert a macro to delete references to Java
> objects, about which one had to remember — the destructor of the `refCleaner`
> handles deleting references when `refCleaner` goes out of scope, that is when
> the function returns.
std::unique_ptr has the option for a custom deleter, meaning it can be used to
clean up Java objects and other cases where a simple delete call does not
suffice, for instance
```c++
std::unique_ptr self(env->GetObjectField(target,
AwtPrintDialog::controlID), [] (jobject* object) -> void {
env->DeleteLocalRef(*object); });
Admittedly for jobjects this is a lot more cumbersome, since instead of
replacing the pointer with a smart pointer, you wrap the pointer inside a smart
pointer... Which is unwieldy. It would be helpful if JNI had a jobject
unique_ptr type for C++
Oops, you sent your correction about the custom deleter just as I sent my post
-
PR Comment: https://git.openjdk.org/jdk/pull/19867#issuecomment-2194667659
PR Comment: https://git.openjdk.org/jdk/pull/19867#issuecomment-2194669210
Re: RFR: 8334868: Ensure CheckUninstallModalHook is called in WPageDialogPeer._show
On Thu, 27 Jun 2024 12:54:55 GMT, Alexey Ivanov wrote: > > …I believe I was referring to the use of C++'s std::unique_ptr, which has > > the functionality for cleanup that we need. > > Yes, `std::unique_ptr` could be useful for handling automatic deallocation of > objects created with the `new` operator. > > ~~It won't help with releasing references to Java objects though.~~ Since `std::unique_ptr` accepts a custom deleter, it may be used for deleting any object. - PR Comment: https://git.openjdk.org/jdk/pull/19867#issuecomment-2194662542
Re: RFR: 8334868: Ensure CheckUninstallModalHook is called in WPageDialogPeer._show
On Thu, 27 Jun 2024 11:26:24 GMT, Julian Waters wrote: > …I believe I was referring to the use of C++'s std::unique_ptr, which has the > functionality for cleanup that we need. Yes, `std::unique_ptr` could be useful for handling automatic deallocation of objects created with the `new` operator. It won't help with releasing references to Java objects though. > That is currently blocked by awt.dll not being allowed to use the C++ > Standard Library, but if one looks at the current awt.dll with dumpbin, > awt.dll is actually already is linked to msvcp.dll, meaning it already uses > C++ Standard Library somehow (I don't know what exactly makes it dependent on > msvcp.dll) I remember STL has been banned from AWT code, yet I don't know the reasons for it. It could be to keep the size smaller. I can see that `awt.dll` depends on `msvcp140.dll` and imports one function from it: `?_Xlength_error@std@@YAXPEBD@Z`. It doesn't look like the C++ Standard Library is used. I can't find where it comes from though… It could be worth getting rid of this dependency. I built a small sample app which uses `std::unique_ptr` and `std::make_unique` and it does not depend on `msvcp140.dll` at all. This makes sense, template libraries are header-only and will become part of the executable (or the dynamic library). The only references to `std` namespace that I found in client are in `ShellFolder.cpp` which uses `std::bad_alloc`, it uses this type in `catch` blocks and it may throw an object of this class. So, it looks `awt.dll` doesn't use the C++ Standard Library. However, using C++ objects with [RAII](https://en.wikipedia.org/wiki/Resource_acquisition_is_initialization) makes the code shorter and frees the programmer from repeating clean-up blocks¹. Indeed, it's already used by C++ classes in AWT: [`JNILocalFrame`](https://github.com/openjdk/jdk/blob/79a23017fc7154738c375fbb12a997525c3bf9e7/src/java.desktop/windows/native/libawt/windows/awt_Toolkit.h#L64-L77) and [`CriticalSection`](https://github.com/openjdk/jdk/blob/79a23017fc7154738c375fbb12a997525c3bf9e7/src/java.desktop/windows/native/libawt/windows/awt_Toolkit.h#L85-L119) along with its `Lock` class inside. ¹ Repeated clean-up blocks can be seen in #18584 in nearly all the files. - PR Comment: https://git.openjdk.org/jdk/pull/19867#issuecomment-2194603793
Re: RFR: 8334868: Ensure CheckUninstallModalHook is called in WPageDialogPeer._show
On Mon, 24 Jun 2024 20:16:30 GMT, Alexey Ivanov wrote: > This is somewhat a continuation for > [JDK-8307160](https://bugs.openjdk.org/browse/JDK-8307160) and > [JDK-8334509](https://bugs.openjdk.org/browse/JDK-8334509). > > The former removed the `doIt` flag in #18584, but it introduced a regression. > > The regression is resolved by the latter in #19786, and it added the 'doIt' > flag again. > > I think using a flag makes the code less clear. When reading the code, one > has to keep track of the current value of the `doIt` flag. > > I raised my concern in [comments in PR > 19786](https://github.com/openjdk/jdk/pull/19786#discussion_r1649191308): > >> I'd rather keep `JNI_FALSE` here — it's more explicit and therefore clearer. >> You don't have to keep track of the value of `doIt` flag while reading the >> code. >> >> In fact, I'd prefer no `doIt` flag at all… yet it makes handling the code >> below `if (ret)` slightly harder. > > I came up with a solution which simplifies handling the clean-up. The > solution relies on C++ destructors which are called when the declared objects > go out of scope. > > The C++ object wraps a lambda function to clean up and invokes this function > in its destructor. Such C++ class has to be a templated class because there's > no defined type to represent a lambda. The class has to be declared at the > top of the file because it needs C++ linkage. > > There are two `Cleaner` objects declared in the code of > `Java_sun_awt_windows_WPageDialogPeer__1show`: > > **`refCleaner`** is declared right after all the references to Java objects > are initialised. The corresponding `refCleanup` lambda deletes all the > references and replaces the `CLEANUP_SHOW` macro. Before JDK-8307160, the > code jumped to a go-to label to perform the clean-up. > > **`postCleaner`** is declared after `AwtDialog::CheckInstallModalHook` is > called. Its lambda `postCleanup` uninstalls the modal hook and handles focus > transfer as well as saves the updated printer parameters. > > It is `postCleaner` that resolves the bug JDK-8334868 by ensuring > `CheckUninstallModalHook` and `ModalActivateNextWindow` are called if > `::PageSetupDlg` is called. > > As the result of using `refCleaner`, all the `return` statements in the > function use an explicit value, which makes the code cleaner. There's no need > to use a `go to` statement or insert a macro to delete references to Java > objects, about which one had to remember — the destructor of the `refCleaner` > handles deleting references when `refCleaner` goes out of scope, that is when > the function returns. It's been quite a while since I said that, so I don't fully remember what I was talking about, but I believe I was referring to the use of C++'s std::unique_ptr, which has the functionality for cleanup that we need. That is currently blocked by awt.dll not being allowed to use the C++ Standard Library, but if one looks at the current awt.dll with dumpbin, awt.dll is actually already is linked to msvcp.dll, meaning it already uses C++ Standard Library somehow (I don't know what exactly makes it dependent on msvcp.dll) - PR Comment: https://git.openjdk.org/jdk/pull/19867#issuecomment-2194433117
Re: RFR: 8297191 : [macos] printing page range "page 2 to 2" or "page 2 to 4" on macOS leads to not print
On Wed, 26 Jun 2024 20:30:55 GMT, Alisen Chung wrote: > > Yes, there is some disconnect. For all OS (MAC with above changes) print > > range is working properly only with following values firstPage =0 and > > lastPage =-1 at **RasterPrinterJob.java** and highly depend on > > **pageRangesAttr** . I don't think there is any issue with native code, > > with this change I have brought MAC same as other OS. > > Shouldn't the solution then be to fix the issue with RasterPrinterJob.java > not working properly with correct firstPage and lastPage values? The first question to answer here is whether the page range is set and preserved in the print job attributes. I haven't looked into it, but I presume this is the case. The page range could be handled on the Java level, it may not be passed to the native level at all. What I mean is that JDK uses the page range to print the specified pages. The implementation on macOS could be adjusted in a similar way, as I said in [my previous comment](https://github.com/openjdk/jdk/pull/19740#issuecomment-2183359828). It should be possible to print a range of pages (which can consist of several intervals) without displaying the Print dialog, thus the `PageRanges` should be handled. As far as I understand, the `SunPageSelection.RANGE` attribute is somewhat informational, and specifies that `PageRanges` attribute is set. > It also looks like the indices are different between macOS and java. Perhaps > there's code somewhere that should have but hasn't converted the indices over? It's true. Different classes use different indices. The intervals in `PageRanges` count pages from 1 whereas `PrinterJob` uses 0-based indexing. According to Apple documentation, [`knowsPageRange`](https://developer.apple.com/documentation/appkit/nsview/1483774-knowspagerange?language=objc) returns `YES` if the range is known, in which case the [NSRange](https://developer.apple.com/documentation/foundation/nsrange?language=objc#4292523) structure contains the range of pages, and the pages start from 1. - PR Comment: https://git.openjdk.org/jdk/pull/19740#issuecomment-2194389772
Re: RFR: 8334868: Ensure CheckUninstallModalHook is called in WPageDialogPeer._show
On Mon, 24 Jun 2024 20:16:30 GMT, Alexey Ivanov wrote: > This is somewhat a continuation for > [JDK-8307160](https://bugs.openjdk.org/browse/JDK-8307160) and > [JDK-8334509](https://bugs.openjdk.org/browse/JDK-8334509). > > The former removed the `doIt` flag in #18584, but it introduced a regression. > > The regression is resolved by the latter in #19786, and it added the 'doIt' > flag again. > > I think using a flag makes the code less clear. When reading the code, one > has to keep track of the current value of the `doIt` flag. > > I raised my concern in [comments in PR > 19786](https://github.com/openjdk/jdk/pull/19786#discussion_r1649191308): > >> I'd rather keep `JNI_FALSE` here — it's more explicit and therefore clearer. >> You don't have to keep track of the value of `doIt` flag while reading the >> code. >> >> In fact, I'd prefer no `doIt` flag at all… yet it makes handling the code >> below `if (ret)` slightly harder. > > I came up with a solution which simplifies handling the clean-up. The > solution relies on C++ destructors which are called when the declared objects > go out of scope. > > The C++ object wraps a lambda function to clean up and invokes this function > in its destructor. Such C++ class has to be a templated class because there's > no defined type to represent a lambda. The class has to be declared at the > top of the file because it needs C++ linkage. > > There are two `Cleaner` objects declared in the code of > `Java_sun_awt_windows_WPageDialogPeer__1show`: > > **`refCleaner`** is declared right after all the references to Java objects > are initialised. The corresponding `refCleanup` lambda deletes all the > references and replaces the `CLEANUP_SHOW` macro. Before JDK-8307160, the > code jumped to a go-to label to perform the clean-up. > > **`postCleaner`** is declared after `AwtDialog::CheckInstallModalHook` is > called. Its lambda `postCleanup` uninstalls the modal hook and handles focus > transfer as well as saves the updated printer parameters. > > It is `postCleaner` that resolves the bug JDK-8334868 by ensuring > `CheckUninstallModalHook` and `ModalActivateNextWindow` are called if > `::PageSetupDlg` is called. > > As the result of using `refCleaner`, all the `return` statements in the > function use an explicit value, which makes the code cleaner. There's no need > to use a `go to` statement or insert a macro to delete references to Java > objects, about which one had to remember — the destructor of the `refCleaner` > handles deleting references when `refCleaner` goes out of scope, that is when > the function returns. src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp line 537: > 535: env->DeleteLocalRef(page); > 536: env->DeleteLocalRef(self); > 537: }; In the long term, it is better to create a special class that deletes global and local references. It could be these constructs to which @TheShermanTanker referred in [this comment](https://github.com/openjdk/jdk/pull/18584#pullrequestreview-1975384014): https://github.com/openjdk/jdk/pull/18584#pullrequestreview-1975384014;>…I have a change in mind relying on _RAII_ that would look much cleaner and neater. - PR Review Comment: https://git.openjdk.org/jdk/pull/19867#discussion_r1656909846
Re: RFR: 8334868: Ensure CheckUninstallModalHook is called in WPageDialogPeer._show
On Wed, 26 Jun 2024 23:42:16 GMT, Alisen Chung wrote:
>> src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp line 640:
>>
>>> 638: HGLOBAL oldG = AwtPrintControl::getPrintHDMode(env, self);
>>> 639: if (setup.hDevMode != oldG) {
>>> 640:AwtPrintControl::setPrintHDMode(env, self, setup.hDevMode);
>>
>> what does setPrintHDMode and setPrintHDName do? do they need to be called as
>> part of cleanup even if there is an exception and we return JNI_FALSE?
>
> i think this code preserves the printer settings after the dialog closes (and
> only on success before this fix), so is it possible in the case the printer
> is removed and causes an exception, we shouldn't preserve these settings?
It's a good question…
Yes, the code saves (or updates) the printer name and mode.
These lines of code were called in cases where OK or Cancel is clicked in the
print dialog. However, it was skipped in case of an error, just like
`CheckUninstallModalHook`.
I also feel that these lines belong in the successful case, right before
`return JNI_TRUE`, but I could quickly find any confirmation whether
`setup.hDevMode` or `setup.hDevNames` can be modified. For this reason, I
preserved the previous code flow.
I presume that neither `setup.hDevMode` or `setup.hDevNames` changes if the
user clicks Cancel in `::PageSetupDlg`. It still leaves the question open what
happens when the user clicks OK but we catch an error inside `if (ret)`. The
old handles stored inside `self` could have become invalid, so it's safer to
update the handles in `self` even if an error occurs.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19867#discussion_r1656892141
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v8]
On Tue, 25 Jun 2024 15:47:49 GMT, Alexey Ivanov wrote: >> Abhishek Kumar has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Remove access modifier from method declaration > > Changes requested by aivanov (Reviewer). @aivanov-jdk Updated the implementation for mnemonic handler separately inside `sun.swing` package and installed the listener specific to GTK LookAndFeel only. Mnemonic handler class is responsible for tracking the mnemonics show/hide status and repainting the components. Changes are reverted back for `SynthLookAndFeel` and `SynthRootPaneUI` files. - PR Comment: https://git.openjdk.org/jdk/pull/18992#issuecomment-2194302062
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v9]
> In GTK LAF, the menu mnemonics are always displayed which is different from > the native behavior. In native application **(tested with gedit for normal > buttons and tested with libreoffice for menu**), the menu mnemonics toggle on > press of `ALT` key. Menu mnemonics are hidden initially and then toggles > between show/hide on `ALT` press. > Proposed fix is to handle the `ALT` key press for GTK LAF and mimic the > native behavior. Fix is similar to the `ALT` key processing in Windows LAF. > Automated test case is added to verify the fix and tested in Ubuntu and > Oracle linux. > > CI testing is green and link attached in JBS. Abhishek Kumar has updated the pull request incrementally with two additional commits since the last revision: - Mnemonic handler class - Mnemonic handler added and previous implementation revert back - Changes: - all: https://git.openjdk.org/jdk/pull/18992/files - new: https://git.openjdk.org/jdk/pull/18992/files/b6cfd95c..e1aae36e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18992=08 - incr: https://webrevs.openjdk.org/?repo=jdk=18992=07-08 Stats: 302 lines in 6 files changed: 166 ins; 126 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/18992.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18992/head:pull/18992 PR: https://git.openjdk.org/jdk/pull/18992
Re: RFR: 8155030: The Menu Mnemonics are always displayed for GTK LAF [v8]
On Tue, 25 Jun 2024 15:35:47 GMT, Alexey Ivanov wrote: >> Abhishek Kumar has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Remove access modifier from method declaration > > src/java.desktop/share/classes/com/sun/java/swing/plaf/gtk/GTKLookAndFeel.java > line 868: > >> 866: "ctrl released ENTER", "release" >> 867: }, >> 868: "RootPane.altPress", true, > > `RootPane.hideMnemonics` or `RootPane.showMnemonicsOnAltPress` could be a > more descriptive property name. Removed this property now. > You may add another condition to the if statement, mnemIndex < 0 — if there's > no mnemonic defined, there's no need to query the properties from UIManager > at all. Updated. > src/java.desktop/share/classes/javax/swing/plaf/synth/SynthLookAndFeel.java > line 1046: > >> 1044: >> 1045: // Toggle flag for drawing the mnemonic state >> 1046: private static boolean isMnemonicHidden = true; > > I wonder why it defaults to `true` where only one L derived from Synth sets > it to true. That's correct. Modified it to be `false` by default and based on L it can be set to `true`. For GTK initialization, it is set to `true` to hide mnemonics by default. > test/jdk/com/sun/java/swing/plaf/gtk/TestMenuMnemonicOnAltPress.java line 72: > >> 70: pt = fileMenu.getLocationOnScreen(); >> 71: fileMenuWidth = fileMenu.getWidth(); >> 72: fileMenuHeight = fileMenu.getHeight(); > > If you save width and height in a `Dimension` object, `fileMenuSize`, you'll > be able to use `new Rectangle(pt, fileMenuSize)` for capturing the screen. > > Since you need that rectangle only, you can create the rectangle here: > > Suggestion: > > fileMenuRect = new Rectangle(fileMenu.getLocationOnScreen(), > fileMenu.getSize()); Updated. - PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1656861152 PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1656861413 PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1656860692 PR Review Comment: https://git.openjdk.org/jdk/pull/18992#discussion_r1656858840
Re: RFR: 5021949: JSplitPane setEnabled(false) shouldn't be partially functional [v7]
On Thu, 27 Jun 2024 07:40:43 GMT, Prasanta Sadhukhan wrote: >> Issue is seen in that if we call setEnabled(false) over JSplitPane than it >> can't be dragged via its divider, But if SplitPane have one touch expandable >> true than user can click those buttons and change the divider position. >> So, if splitpane is disabled, then both dragging in divider and >> one-touch-expandable click should be disabled. >> Fix is made to override setEnabled and disable one-touch-expandable buttons >> actions.. > > Prasanta Sadhukhan has updated the pull request incrementally with one > additional commit since the last revision: > > Remove headful Marked as reviewed by aivanov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19695#pullrequestreview-2144903031
Re: RFR: 8333403: Write a test to check various components events are triggered properly [v4]
> This testcase checks for the following assertions for Component events: > > 1. When components are resized, moved, hidden and shown the respective events > are triggered. > 2. When the components are hidden/disabled also,the component events like > resized/moved are triggered. > 3. When a hidden component is hidden again, or a visible component is shown > again, the events should not be fired. > 4. When a window is minimized/restored then hidden and shown component events > should be triggered. > > 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: 8333403: Review comments fixed - Changes: - all: https://git.openjdk.org/jdk/pull/19521/files - new: https://git.openjdk.org/jdk/pull/19521/files/191a9771..9a3e95f1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19521=03 - incr: https://webrevs.openjdk.org/?repo=jdk=19521=02-03 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/19521.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19521/head:pull/19521 PR: https://git.openjdk.org/jdk/pull/19521
Re: RFR: 8333403: Write a test to check various components events are triggered properly [v2]
On Mon, 10 Jun 2024 20:07:34 GMT, Alisen Chung wrote: >> Ravi Gupta has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8333403: Copyright Header added > > test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 237: > >> 235: } >> 236: >> 237: // Disable the components and do the same set of >> operations > > if you're doing the same operations, you should write a helper function and > call that function instead created a helper function resetFrame() - PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1656748100
Re: RFR: 5021949: JSplitPane setEnabled(false) shouldn't be partially functional [v7]
> Issue is seen in that if we call setEnabled(false) over JSplitPane than it > can't be dragged via its divider, But if SplitPane have one touch expandable > true than user can click those buttons and change the divider position. > So, if splitpane is disabled, then both dragging in divider and > one-touch-expandable click should be disabled. > Fix is made to override setEnabled and disable one-touch-expandable buttons > actions.. Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision: Remove headful - Changes: - all: https://git.openjdk.org/jdk/pull/19695/files - new: https://git.openjdk.org/jdk/pull/19695/files/26895e7a..7cd3e1fb Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19695=06 - incr: https://webrevs.openjdk.org/?repo=jdk=19695=05-06 Stats: 5 lines in 1 file changed: 1 ins; 4 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19695.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19695/head:pull/19695 PR: https://git.openjdk.org/jdk/pull/19695
