Re: RFR: 8092272: [D3D 3D] Need a robust 3D states management for texture [v2]
> Moves the filter setting of the samplers from the device parameters > configuration to the use-site, allowing for dynamic changes in the sampler. > This PR does internal plumbing work only to bring it close to the ES2 > pipeline. A followup PR will create the public API. > > Summary of the changes: > * Created a new (internal for now) `TextureData` object that is intended to > contain all the data of texture (map) of `PhongMaterial`, such as filters, > addressing, wrapping mode, mipmaps etc. **This PR deals only with filters** > as a starting point, more settings can be added later. > * Creates an update mechanism from the Java side material to the native D3D > layer. The public API `PhoneMaterial` is *not* changed yet. The peer > `NGPhongMaterial` is configured to receive update from the public > `PhongMaterial` when the public API is created via new > `ObjectProperty` properties. > * Small refactoring in the D3D layer with a new map types enum to control the > texture settings more easily. > > The JBS issue lists some regressions in a comment, but I couldn't reproduce > them. It looks like the sampler settings needed to be added anywhere, and > that was the easiest to do at the time. Now they were just moved. Nir Lisker has updated the pull request incrementally with one additional commit since the last revision: Addressed review comments - Changes: - all: https://git.openjdk.org/jfx/pull/1281/files - new: https://git.openjdk.org/jfx/pull/1281/files/34e1d1ab..bd3c9204 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1281=01 - incr: https://webrevs.openjdk.org/?repo=jfx=1281=00-01 Stats: 37 lines in 4 files changed: 3 ins; 5 del; 29 mod Patch: https://git.openjdk.org/jfx/pull/1281.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1281/head:pull/1281 PR: https://git.openjdk.org/jfx/pull/1281
Re: RFR: 8092272: [D3D 3D] Need a robust 3D states management for texture
On Thu, 9 Nov 2023 02:44:44 GMT, Michael Strauß wrote: >> Moves the filter setting of the samplers from the device parameters >> configuration to the use-site, allowing for dynamic changes in the sampler. >> This PR does internal plumbing work only to bring it close to the ES2 >> pipeline. A followup PR will create the public API. >> >> Summary of the changes: >> * Created a new (internal for now) `TextureData` object that is intended to >> contain all the data of texture (map) of `PhongMaterial`, such as filters, >> addressing, wrapping mode, mipmaps etc. **This PR deals only with filters** >> as a starting point, more settings can be added later. >> * Creates an update mechanism from the Java side material to the native D3D >> layer. The public API `PhoneMaterial` is *not* changed yet. The peer >> `NGPhongMaterial` is configured to receive update from the public >> `PhongMaterial` when the public API is created via new >> `ObjectProperty` properties. >> * Small refactoring in the D3D layer with a new map types enum to control >> the texture settings more easily. >> >> The JBS issue lists some regressions in a comment, but I couldn't reproduce >> them. It looks like the sampler settings needed to be added anywhere, and >> that was the easiest to do at the time. Now they were just moved. > > modules/javafx.graphics/src/main/native-prism-d3d/D3DPhongMaterial.h line 41: > >> 39: self_illumination, >> 40: num_map_types >> 41: }; > > 1. Types generally seem to use PascalCase in this library, and constants > UPPER_CASE or CamelCase. > 2. You can use `enum class` to prevent the constants from polluting the > global namespace. > 3. The underlying type doesn't need to be specified, it's `int` by default. > You're casting it to `int` in `D3DMeshView.cc` anyway. About 2, I tried that initially and got into problems iterating over the enum in `D3DMeshView.cc` line 293. How would you perform the iteration? - PR Review Comment: https://git.openjdk.org/jfx/pull/1281#discussion_r1390020946
Re: RFR: 8301302: Platform preferences API [v23]
> Please read [this > document](https://gist.github.com/mstr2/9f46f92c98d3c86aa6a0b4224a9a6548) for > an introduction to the Platform Preferences API, and how it interacts with > the proposed style theme and stage appearance features. Michael Strauß has updated the pull request incrementally with one additional commit since the last revision: Rename Appearance to ColorScheme - Changes: - all: https://git.openjdk.org/jfx/pull/1014/files - new: https://git.openjdk.org/jfx/pull/1014/files/d012dec9..9b607c6a Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1014=22 - incr: https://webrevs.openjdk.org/?repo=jfx=1014=21-22 Stats: 121 lines in 6 files changed: 45 ins; 44 del; 32 mod Patch: https://git.openjdk.org/jfx/pull/1014.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1014/head:pull/1014 PR: https://git.openjdk.org/jfx/pull/1014
Re: RFR: 8303478: DatePicker throws uncatchable exception on tab out from garbled text [v3]
> The fix prevents the DatePicker from losing focus if the date is not parsable. brunesto has updated the pull request incrementally with one additional commit since the last revision: minor - Changes: - all: https://git.openjdk.org/jfx/pull/1274/files - new: https://git.openjdk.org/jfx/pull/1274/files/7e2361a5..decc7226 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1274=02 - incr: https://webrevs.openjdk.org/?repo=jfx=1274=01-02 Stats: 4 lines in 1 file changed: 0 ins; 3 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/1274.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1274/head:pull/1274 PR: https://git.openjdk.org/jfx/pull/1274
Re: RFR: 8303478: DatePicker throws uncatchable exception on tab out from garbled text [v2]
On Thu, 2 Nov 2023 11:55:46 GMT, Kevin Rushforth wrote:
>> tests/system/src/test/java/test/robot/javafx/scene/DatePickerOnFocusLostTest.java
>> line 98:
>>
>>> 96: // 3. Click on button to grab the focus and hence attempt to
>>> datePicker.commitValue()
>>> 97: // 4. Verify that in case of typo, the value was reverted, as well
>>> as the editor's text
>>> 98: public void testDatePickerCommit(boolean typo) throws Exception {
>>
>> you actually can also write a normal JUnit test as far as I get this. Then
>> there is no need for `Util.runAndWait`, `sleep` and so on.
>>
>> See: `test.javafx.scene.control.DatePickerTest`
>
> A headless test is the preferred way to do it, if feasible. If it is, move
> the test to the `javafx.controls` module.
I have removed the robot test, and replaced it by a simple test case in the
existing class `DatePickerTest` in `javafx.controls`
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1274#discussion_r1389638701
Re: RFR: 8303478: DatePicker throws uncatchable exception on tab out from garbled text [v2]
> The fix prevents the DatePicker from losing focus if the date is not parsable. brunesto has updated the pull request incrementally with one additional commit since the last revision: replaced robot test by plain junit - Changes: - all: https://git.openjdk.org/jfx/pull/1274/files - new: https://git.openjdk.org/jfx/pull/1274/files/43c0f940..7e2361a5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1274=01 - incr: https://webrevs.openjdk.org/?repo=jfx=1274=00-01 Stats: 244 lines in 2 files changed: 30 ins; 214 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/1274.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1274/head:pull/1274 PR: https://git.openjdk.org/jfx/pull/1274
Re: RFR: 8092272: [D3D 3D] Need a robust 3D states management for texture
On Thu, 9 Nov 2023 03:15:27 GMT, Michael Strauß wrote:
>> Moves the filter setting of the samplers from the device parameters
>> configuration to the use-site, allowing for dynamic changes in the sampler.
>> This PR does internal plumbing work only to bring it close to the ES2
>> pipeline. A followup PR will create the public API.
>>
>> Summary of the changes:
>> * Created a new (internal for now) `TextureData` object that is intended to
>> contain all the data of texture (map) of `PhongMaterial`, such as filters,
>> addressing, wrapping mode, mipmaps etc. **This PR deals only with filters**
>> as a starting point, more settings can be added later.
>> * Creates an update mechanism from the Java side material to the native D3D
>> layer. The public API `PhoneMaterial` is *not* changed yet. The peer
>> `NGPhongMaterial` is configured to receive update from the public
>> `PhongMaterial` when the public API is created via new
>> `ObjectProperty` properties.
>> * Small refactoring in the D3D layer with a new map types enum to control
>> the texture settings more easily.
>>
>> The JBS issue lists some regressions in a comment, but I couldn't reproduce
>> them. It looks like the sampler settings needed to be added anywhere, and
>> that was the easiest to do at the time. Now they were just moved.
>
> modules/javafx.graphics/src/main/native-prism-d3d/D3DMeshView.cc line 295:
>
>> 293: for (int i = 0; i < map_type::num_map_types; i++) {
>> 294: map_type type = static_cast(i);
>> 295: SUCCEEDED(device->SetTexture(type, material->getMap(type)));
>
> Wrapping the calls with the `SUCCEEDED` macro doesn't do anything useful.
Yes, but it's done everywhere else in the class, and was done for these calls
before I changed this code. Maybe a better error handling design is needed.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1281#discussion_r1389570725
Re: RFR: 8092272: [D3D 3D] Need a robust 3D states management for texture
On Thu, 9 Nov 2023 03:16:51 GMT, Michael Strauß wrote:
>> Moves the filter setting of the samplers from the device parameters
>> configuration to the use-site, allowing for dynamic changes in the sampler.
>> This PR does internal plumbing work only to bring it close to the ES2
>> pipeline. A followup PR will create the public API.
>>
>> Summary of the changes:
>> * Created a new (internal for now) `TextureData` object that is intended to
>> contain all the data of texture (map) of `PhongMaterial`, such as filters,
>> addressing, wrapping mode, mipmaps etc. **This PR deals only with filters**
>> as a starting point, more settings can be added later.
>> * Creates an update mechanism from the Java side material to the native D3D
>> layer. The public API `PhoneMaterial` is *not* changed yet. The peer
>> `NGPhongMaterial` is configured to receive update from the public
>> `PhongMaterial` when the public API is created via new
>> `ObjectProperty` properties.
>> * Small refactoring in the D3D layer with a new map types enum to control
>> the texture settings more easily.
>>
>> The JBS issue lists some regressions in a comment, but I couldn't reproduce
>> them. It looks like the sampler settings needed to be added anywhere, and
>> that was the easiest to do at the time. Now they were just moved.
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/scene/paint/TextureData.java
> line 12:
>
>> 10: */
>> 11: // Here we can support mipmaps, wrapping modes (which exists internally
>> and can be pulled out), addressing modes etc.
>> 12: public class TextureData {
>
> 1. This type could be a record.
> 2. There's no actual texture data (i.e. pixels) here, maybe a better name
> would be `SamplerParameters` or something like that.
1. Not when it will be promoted to public API. Adding record components breaks
backwards compatibility, so making this a record will not allow adding more
configuration later on. What might be possible is making it an interface and
using a record as an implementation.
2. I think that this class can go beyond sampler configurations. For example,
wrapping is a render state, not a sampler state. Maybe something more general
like "TextureParameters" can work.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1281#discussion_r1389539073
Integrated: 8318841: macOS: Memory leak with MenuItem when Menu.useSystemMenuBar(true) is used
On Thu, 2 Nov 2023 11:39:47 GMT, Johan Vos wrote: > When the Java layer removes a systemmenu, release the native resources > related to this systemmenu. > This removes the strong JNI Global ref, which prevents its references from > being gc'ed. > > The current implementation for the mac-specific system menu creates a menu, > but never releases its resources. In the `dealloc` of this menu, the strong > jni refs are deleted. > With this PR, we now release the native resources associated with a menuItem > when that one is removed from a menu. A consequence is that this menuItem > should never be used after being removed from its current menu (e.g. it > should not be re-added, or its text/shortcut should not be altered). > The current implementation will create a new MacMenuDelegate every time a > menuItem is inserted into a menu, so there should be no references to the > native resources lingering. This pull request has now been integrated. Changeset: 8dd3c37c Author:Johan Vos URL: https://git.openjdk.org/jfx/commit/8dd3c37cae3dd52dc3a258033d0e8f168ceaf307 Stats: 80 lines in 4 files changed: 80 ins; 0 del; 0 mod 8318841: macOS: Memory leak with MenuItem when Menu.useSystemMenuBar(true) is used Reviewed-by: kcr, jpereda - PR: https://git.openjdk.org/jfx/pull/1277
Re: RFR: 8303826: Add FX test for JDK-8252255
On Thu, 9 Nov 2023 03:43:48 GMT, Prasanta Sadhukhan wrote: > Manual regression test for 8252255: Blurry rendering of SwingNode with HiDPI > scaling in JavaFX is added I think this test only needs a single reviewer. - PR Comment: https://git.openjdk.org/jfx/pull/1282#issuecomment-1805779453
Re: RFR: 8318841: macOS: Memory leak with MenuItem when Menu.useSystemMenuBar(true) is used [v6]
On Fri, 10 Nov 2023 09:58:21 GMT, Johan Vos wrote:
>> tests/system/src/test/java/test/javafx/stage/SystemMenuBarTest.java line 164:
>>
>>> 162: });
>>> 163: }
>>> 164: Platform.runLater( () -> {
>>
>> Why is `Platform.runLater` needed here? To wait until the last
>> `Platform.runLater` in the for loop ends? Is it guaranteed that it will be
>> called only after the previous one ends?
>>
>> Minor: remove unneeded white space: `Platform.runLater(() -> {`
>
> No, it is needed because we want to give the JavaFX platform the opportunity
> to do "whatever is needed" after the last runLater() is done. Hence, we put
> what we want to do on the Runnable queue of the FX thread, so that we don't
> depend on internals.
This makes sense. To answer one of Jose's questions:
> Is it guaranteed that it will be called only after the previous one ends?
Yes, this is specified behavior of `Platform::runLater`.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1277#discussion_r1389427353
Re: RFR: 8318841: macOS: Memory leak with MenuItem when Menu.useSystemMenuBar(true) is used [v7]
On Fri, 10 Nov 2023 10:20:23 GMT, Johan Vos wrote: >> When the Java layer removes a systemmenu, release the native resources >> related to this systemmenu. >> This removes the strong JNI Global ref, which prevents its references from >> being gc'ed. >> >> The current implementation for the mac-specific system menu creates a menu, >> but never releases its resources. In the `dealloc` of this menu, the strong >> jni refs are deleted. >> With this PR, we now release the native resources associated with a menuItem >> when that one is removed from a menu. A consequence is that this menuItem >> should never be used after being removed from its current menu (e.g. it >> should not be re-added, or its text/shortcut should not be altered). >> The current implementation will create a new MacMenuDelegate every time a >> menuItem is inserted into a menu, so there should be no references to the >> native resources lingering. > > Johan Vos has updated the pull request incrementally with one additional > commit since the last revision: > > formatting Marked as reviewed by kcr (Lead). - PR Review: https://git.openjdk.org/jfx/pull/1277#pullrequestreview-1724829012
Re: Wayland
Hi Johan, Robot will be a challenge. For mouse and keyboard simulation this could work: https://www.kernel.org/doc/html/v4.12/input/uinput.html of maybe: https://docs.flatpak.org/pt-br/latest/portal-api-reference.html#gdbus-org.freedesktop.portal.RemoteDesktop For screenshot: https://docs.flatpak.org/pt-br/latest/portal-api-reference.html#gdbus-org.freedesktop.portal.Screenshot Haven't tested anything yet. I did some experiments on the jfx-sandbox (it displays the window on wayland with software rendering), but the conclusion so far is that it's better to ditch gtk and use wayland-client directly, except for system dialogs such as file open. -- Thiago. Em sex., 10 de nov. de 2023 às 06:39, Johan Vos escreveu: > Hi Thiago, > > Thanks for the work on Wayland. I spent some time on it in the past as > well, and I'll hope to find some time to look at your work soon. > > The main worry I had in the past was how to deal with the robot, where we > need to get pixels from the screen -- did you tackle that? > > - Johan > > On Fri, Nov 3, 2023 at 1:14 AM Thiago Milczarek Sayão < > thiago.sa...@gmail.com> wrote: > >> Hi, >> >> About Wayland: >> >> Porting es2 to use EGL instead of GLX is pretty straightforward, so >> converting a X11GL* to WaylandGL* is easy (GLX is X11 only, so this is why >> EGL is needed). >> The problem is Gtk4 and/or Gtk3 with Wayland won't allow you to paint >> directly to the window as es2 do. >> I've tried to use Gtk4 and composite it with a GL texture - it would >> work, but does't fit well with es2 as it is designed to swapBuffers. >> >> I'm looking for the shortest path to get it working. >> >> I'm thinking now to use wayland-client directly for most things, so it >> will be possible to create the WaylandGL* infrastructure and render >> directly to a surface. >> Compositing directly with wayland would be better anyways. Using Gtk (3 >> or 4) would require rendering somewhere else (other than onscreen wayland >> surface) and then composite the result with Gtk. >> >> So, the conclusion for now is that Gtk won't work with wayland for the >> use of another toolkit (outside of the gtk rendering scope). >> >> wayland-client, here I go. >> >> -- Thiago. >> >> >> >> >> >> >> >> >> >> Em sáb., 21 de out. de 2023 às 18:12, Thiago Milczarek Sayão < >> thiago.sa...@gmail.com> escreveu: >> >>> EGL is the way to go for Linux on Wayland and X11. >>> >>> I don't know how to do it yet, but it seems the solution is to use >>> DMABUF with EGL and share it with GTK (I suspect on a GtkGLArea widget, but >>> not sure yet). >>> >>> Firefox and WebKit GTK uses the same technic. >>> >>> Gtk4 does not allow application painting, so direct surface context is >>> not possible. >>> >>> We are using gtk3 and it is still possible, but I want to use >>> GtkHeaderbar and clean up glass code. Since it means sharing the >>> window/surface with GTK, direct surface rendering also doesn't work. >>> >>> >>> https://mozillagfx.wordpress.com/2021/10/30/switching-the-linux-graphics-stack-from-glx-to-egl/ >>> >>> >>> https://blogs.igalia.com/carlosgc/2023/04/03/webkitgtk-accelerated-compositing-rendering/ >>> >>> https://blog.gtk.org/2021/05/10/adventures-in-graphics-apis/ >>> >>> It's been nice to learn. >>> >>> Em seg., 16 de out. de 2023 20:51, Thiago Milczarek Sayão < >>> thiago.sa...@gmail.com> escreveu: >>> Hi, I am investigating about prism on Wayland. It looks like we could just replace the GLX calls on X11 backend to EGL and it will work on both (maybe rename it). It will need some work to pass the native display around - it assumes X11 Display everywhere and it could be a Display or a wl_display. -- Thiago. Em dom., 15 de out. de 2023 às 16:06, Thiago Milczarek Sayão < thiago.sa...@gmail.com> escreveu: > Hi, > > Update: It now works on wayland with -Dprism.order=sw > > > > Em dom., 15 de out. de 2023 às 10:49, Thiago Milczarek Sayão < > thiago.sa...@gmail.com> escreveu: > >> Hi, >> >> https://github.com/openjdk/jfx-sandbox/tree/tsayao_wayland >> >> I did some experiments here. So far, so good. >> >> 1) Replaced GDK events for Gtk Signals - It's the way to go for newer >> gtk. >> 2) Replaced drawing directly in the window and added a GtkDrawingArea >> with a GtkHeaderBar which allows control over the whole window >> size and allows to get rid of extents* calls - this cleans up a lot. >> 3) Unified the WindowContext to clean it up. >> >> I also integrated the IME replacement proposed here: >> https://github.com/openjdk/jfx/pull/1080 >> >> It almost runs with software rendering on Wayland, but something >> still touches X11. >> >> To finally make it work on Wayland it requires to implement it on >> prism es2. I see that there's a EGL part of Monocle. I still don't >> completely understand it, but wouldn't it work as a
Re: RFR: 8319779: SystemMenu: memory leak due to listener never being removed
On Fri, 10 Nov 2023 10:34:08 GMT, Johan Vos wrote:
> A listener was added but never removed.
> This patch removes the listener when the menu it links to is cleared. Fix for
> https://bugs.openjdk.org/browse/JDK-8319779
Should there be an updated test for this?
I see a lot of raw type use (`ListChangeListener`, `ObservableList`), any
chance those can be avoided?
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/GlassSystemMenu.java
line 63:
> 61: private MenuBar glassSystemMenuBar = null;
> 62: private final Map menuListeners = new
> HashMap<>();
> 63: private final Map listenerItems
> = new HashMap<>();
minor:
Suggestion:
private final Map menuListeners = new HashMap<>();
private final Map listenerItems = new
HashMap<>();
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/GlassSystemMenu.java
line 171:
> 169: if (item instanceof MenuBase) {
> 170: // submenu
> 171: addMenu(glassMenu, (MenuBase)item);
Suggestion:
if (item instanceof MenuBase baseItem) {
// submenu
addMenu(glassMenu, baseItem);
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/GlassSystemMenu.java
line 189:
> 187:
> 188: private ListChangeListener createListener(final Menu glassMenu) {
> 189: ListChangeListener answer =
> ((ListChangeListener.Change change) -> {
minor: I see extra parenthesis around the whole, and I think you can just
`return` this immediately (no need for `answer` local)
-
PR Review: https://git.openjdk.org/jfx/pull/1283#pullrequestreview-1724683737
PR Review Comment: https://git.openjdk.org/jfx/pull/1283#discussion_r1389343964
PR Review Comment: https://git.openjdk.org/jfx/pull/1283#discussion_r1389338458
PR Review Comment: https://git.openjdk.org/jfx/pull/1283#discussion_r1389340165
RFR: 8319779: SystemMenu: memory leak due to listener never being removed
A listener was added but never removed. This patch removes the listener when the menu it links to is cleared. Fix for https://bugs.openjdk.org/browse/JDK-8319779 - Commit messages: - A listener was added but never removed. Changes: https://git.openjdk.org/jfx/pull/1283/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1283=00 Issue: https://bugs.openjdk.org/browse/JDK-8319779 Stats: 61 lines in 1 file changed: 40 ins; 19 del; 2 mod Patch: https://git.openjdk.org/jfx/pull/1283.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1283/head:pull/1283 PR: https://git.openjdk.org/jfx/pull/1283
Re: RFR: 8318841: macOS: Memory leak with MenuItem when Menu.useSystemMenuBar(true) is used [v7]
> When the Java layer removes a systemmenu, release the native resources > related to this systemmenu. > This removes the strong JNI Global ref, which prevents its references from > being gc'ed. > > The current implementation for the mac-specific system menu creates a menu, > but never releases its resources. In the `dealloc` of this menu, the strong > jni refs are deleted. > With this PR, we now release the native resources associated with a menuItem > when that one is removed from a menu. A consequence is that this menuItem > should never be used after being removed from its current menu (e.g. it > should not be re-added, or its text/shortcut should not be altered). > The current implementation will create a new MacMenuDelegate every time a > menuItem is inserted into a menu, so there should be no references to the > native resources lingering. Johan Vos has updated the pull request incrementally with one additional commit since the last revision: formatting - Changes: - all: https://git.openjdk.org/jfx/pull/1277/files - new: https://git.openjdk.org/jfx/pull/1277/files/cbe5ea46..2d4ac874 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1277=06 - incr: https://webrevs.openjdk.org/?repo=jfx=1277=05-06 Stats: 9 lines in 1 file changed: 1 ins; 4 del; 4 mod Patch: https://git.openjdk.org/jfx/pull/1277.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1277/head:pull/1277 PR: https://git.openjdk.org/jfx/pull/1277
Re: RFR: 8318841: macOS: Memory leak with MenuItem when Menu.useSystemMenuBar(true) is used [v7]
On Fri, 10 Nov 2023 10:17:18 GMT, Johan Vos wrote: >> When the Java layer removes a systemmenu, release the native resources >> related to this systemmenu. >> This removes the strong JNI Global ref, which prevents its references from >> being gc'ed. >> >> The current implementation for the mac-specific system menu creates a menu, >> but never releases its resources. In the `dealloc` of this menu, the strong >> jni refs are deleted. >> With this PR, we now release the native resources associated with a menuItem >> when that one is removed from a menu. A consequence is that this menuItem >> should never be used after being removed from its current menu (e.g. it >> should not be re-added, or its text/shortcut should not be altered). >> The current implementation will create a new MacMenuDelegate every time a >> menuItem is inserted into a menu, so there should be no references to the >> native resources lingering. > > Johan Vos has updated the pull request incrementally with one additional > commit since the last revision: > > formatting Marked as reviewed by jpereda (Reviewer). - PR Review: https://git.openjdk.org/jfx/pull/1277#pullrequestreview-1724472898
Re: RFR: 8318841: macOS: Memory leak with MenuItem when Menu.useSystemMenuBar(true) is used [v6]
On Thu, 9 Nov 2023 22:07:34 GMT, Jose Pereda wrote:
>> Johan Vos has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Use only 10 cycles instead of 50 (preventing this test to take 50 seconds
>> in case it fails).
>
> tests/system/src/test/java/test/javafx/stage/SystemMenuBarTest.java line 148:
>
>> 146: stage.show();
>> 147: stage.requestFocus();
>> 148: Thread t = new Thread(){
>
> minor, use `new Thread(() -> {...});`(or else add a whitespace before the
> curly brace: `new Thread() {`)
I'll go with the whitespace then, as I find it much more clear to explicitly
construct, assign, and start a thread.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1277#discussion_r1389194261
Re: RFR: 8318841: macOS: Memory leak with MenuItem when Menu.useSystemMenuBar(true) is used [v6]
On Thu, 9 Nov 2023 22:09:30 GMT, Jose Pereda wrote:
>> Johan Vos has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Use only 10 cycles instead of 50 (preventing this test to take 50 seconds
>> in case it fails).
>
> tests/system/src/test/java/test/javafx/stage/SystemMenuBarTest.java line 164:
>
>> 162: });
>> 163: }
>> 164: Platform.runLater( () -> {
>
> Why is `Platform.runLater` needed here? To wait until the last
> `Platform.runLater` in the for loop ends? Is it guaranteed that it will be
> called only after the previous one ends?
>
> Minor: remove unneeded white space: `Platform.runLater(() -> {`
No, it is needed because we want to give the JavaFX platform the opportunity to
do "whatever is needed" after the last runLater() is done. Hence, we put what
we want to do on the Runnable queue of the FX thread, so that we don't depend
on internals.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1277#discussion_r1389184538
Re: Wayland
Hi Thiago, Thanks for the work on Wayland. I spent some time on it in the past as well, and I'll hope to find some time to look at your work soon. The main worry I had in the past was how to deal with the robot, where we need to get pixels from the screen -- did you tackle that? - Johan On Fri, Nov 3, 2023 at 1:14 AM Thiago Milczarek Sayão < thiago.sa...@gmail.com> wrote: > Hi, > > About Wayland: > > Porting es2 to use EGL instead of GLX is pretty straightforward, so > converting a X11GL* to WaylandGL* is easy (GLX is X11 only, so this is why > EGL is needed). > The problem is Gtk4 and/or Gtk3 with Wayland won't allow you to paint > directly to the window as es2 do. > I've tried to use Gtk4 and composite it with a GL texture - it would work, > but does't fit well with es2 as it is designed to swapBuffers. > > I'm looking for the shortest path to get it working. > > I'm thinking now to use wayland-client directly for most things, so it > will be possible to create the WaylandGL* infrastructure and render > directly to a surface. > Compositing directly with wayland would be better anyways. Using Gtk (3 or > 4) would require rendering somewhere else (other than onscreen wayland > surface) and then composite the result with Gtk. > > So, the conclusion for now is that Gtk won't work with wayland for the use > of another toolkit (outside of the gtk rendering scope). > > wayland-client, here I go. > > -- Thiago. > > > > > > > > > > Em sáb., 21 de out. de 2023 às 18:12, Thiago Milczarek Sayão < > thiago.sa...@gmail.com> escreveu: > >> EGL is the way to go for Linux on Wayland and X11. >> >> I don't know how to do it yet, but it seems the solution is to use DMABUF >> with EGL and share it with GTK (I suspect on a GtkGLArea widget, but not >> sure yet). >> >> Firefox and WebKit GTK uses the same technic. >> >> Gtk4 does not allow application painting, so direct surface context is >> not possible. >> >> We are using gtk3 and it is still possible, but I want to use >> GtkHeaderbar and clean up glass code. Since it means sharing the >> window/surface with GTK, direct surface rendering also doesn't work. >> >> >> https://mozillagfx.wordpress.com/2021/10/30/switching-the-linux-graphics-stack-from-glx-to-egl/ >> >> >> https://blogs.igalia.com/carlosgc/2023/04/03/webkitgtk-accelerated-compositing-rendering/ >> >> https://blog.gtk.org/2021/05/10/adventures-in-graphics-apis/ >> >> It's been nice to learn. >> >> Em seg., 16 de out. de 2023 20:51, Thiago Milczarek Sayão < >> thiago.sa...@gmail.com> escreveu: >> >>> Hi, >>> >>> I am investigating about prism on Wayland. >>> >>> It looks like we could just replace the GLX calls on X11 backend to EGL >>> and it will work on both (maybe rename it). >>> >>> It will need some work to pass the native display around - it assumes >>> X11 Display everywhere and it could be a Display or a wl_display. >>> >>> -- Thiago. >>> >>> Em dom., 15 de out. de 2023 às 16:06, Thiago Milczarek Sayão < >>> thiago.sa...@gmail.com> escreveu: >>> Hi, Update: It now works on wayland with -Dprism.order=sw Em dom., 15 de out. de 2023 às 10:49, Thiago Milczarek Sayão < thiago.sa...@gmail.com> escreveu: > Hi, > > https://github.com/openjdk/jfx-sandbox/tree/tsayao_wayland > > I did some experiments here. So far, so good. > > 1) Replaced GDK events for Gtk Signals - It's the way to go for newer > gtk. > 2) Replaced drawing directly in the window and added a GtkDrawingArea > with a GtkHeaderBar which allows control over the whole window > size and allows to get rid of extents* calls - this cleans up a lot. > 3) Unified the WindowContext to clean it up. > > I also integrated the IME replacement proposed here: > https://github.com/openjdk/jfx/pull/1080 > > It almost runs with software rendering on Wayland, but something still > touches X11. > > To finally make it work on Wayland it requires to implement it on > prism es2. I see that there's a EGL part of Monocle. I still don't > completely understand it, but wouldn't it work as a drop-in replacement? > > -- Thiago. > > >
