On Wed, 26 Jun 2024 23:37:48 GMT, Alisen Chung <ach...@openjdk.org> 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 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? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/19867#discussion_r1655642352
