Re: qemu-img convert: Compression can not be disabled when converting from .qcow2 to .raw

[Nir Soffer]

On Fri, Jun 21, 2024 at 5:48 PM Sven Ott  wrote:

> Hi, I want to mount a VM image to a loop device and give it some excess
> space.
>
> To do so, I download a .qcow2 file, add some 0 bytes with truncate, and
> then convert the image from QCOW2 to RAW format with qemu-img convert,
> like so:
>
> ```
>
> GUEST_IMG=focal-server-cloudimg-amd64
>
> wget https://cloud-images.ubuntu.com/focal/current/$GUEST_IMG
>
> truncate -s 5G $GUEST_IMG.img
>

This is not needed, and ineffective...


>
> qemu-img convert -f qcow2 -O raw $GUEST_IMG.img $GUEST_IMG.raw
>

Since the first thing done in this command is truncating the target image
to 0 bytes.

You can use -n to avoid creation of the target image and use your image,
but this is also not
needed.

You can convert the image:

qemu-img convert -f qccow2 -O raw src.qcow2 dst.raw

and then resize the raw image:

qmeu-img resize dst.raw newsize

You can also resize before converting, it does not matter if you resize
before or after.

Note that you will have to grow the pv/lv/filessystem inside the guest to
use the additional space.

Nir

Re: [PATCH v2] Consider discard option when writing zeros

[Nir Soffer]

On Wed, Jun 19, 2024 at 8:40 PM Nir Soffer  wrote:

> - Need to run all block tests
>

Stale note, make check pass

Re: [PATCH v2] Consider discard option when writing zeros

[Nir Soffer]

On Wed, Jun 19, 2024 at 8:40 PM Nir Soffer  wrote:

> - Need to run all block tests
>

Stale note, make check pass

Re: [PATCH v2] Consider discard option when writing zeros

[Nir Soffer]

Tested using:

$ cat test-unmap.sh
#!/bin/sh

qemu=${1:?Usage: $0 qemu-executable}
img=/tmp/test.raw

echo
echo "defaults - write zeroes"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -z 0 1m"\nquit' | $qemu -monitor stdio \
-drive if=none,file=$img,format=raw >/dev/null
du -sh $img

echo
echo "defaults - write zeroes unmap"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -zu 0 1m"\nquit' | $qemu -monitor stdio \
-drive if=none,file=$img,format=raw >/dev/null
du -sh $img

echo
echo "defaults - write actual zeros"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -P 0 0 1m"\nquit' | $qemu -monitor stdio \
-drive if=none,file=$img,format=raw >/dev/null
du -sh $img

echo
echo "discard=off - write zeroes unmap"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -zu 0 1m"\nquit' | $qemu -monitor stdio \
-drive if=none,file=$img,format=raw,discard=off >/dev/null
du -sh $img

echo
echo "detect-zeros=on - write actual zeros"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -P 0 0 1m"\nquit' | $qemu -monitor stdio \
-drive if=none,file=$img,format=raw,detect-zeroes=on >/dev/null
du -sh $img

echo
echo "detect-zeros=unmap,discard=unmap - write actual zeros"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -P 0 0 1m"\nquit' |  $qemu -monitor stdio \
-drive if=none,file=$img,format=raw,detect-zeroes=unmap,discard=unmap
>/dev/null
du -sh $img

echo
echo "discard=unmap - write zeroes"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -z 0 1m"\nquit' | $qemu -monitor stdio \
-drive if=none,file=$img,format=raw,discard=unmap >/dev/null
du -sh $img

echo
echo "discard=unmap - write zeroes unmap"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -zu 0 1m"\nquit' | $qemu -monitor stdio \
-drive if=none,file=$img,format=raw,discard=unmap >/dev/null
du -sh $img

rm $img


Before this change:

$ cat before.out

defaults - write zeroes
1.0M /tmp/test.raw

defaults - write zeroes unmap
0 /tmp/test.raw

defaults - write actual zeros
1.0M /tmp/test.raw

discard=off - write zeroes unmap
0 /tmp/test.raw

detect-zeros=on - write actual zeros
1.0M /tmp/test.raw

detect-zeros=unmap,discard=unmap - write actual zeros
0 /tmp/test.raw

discard=unmap - write zeroes
1.0M /tmp/test.raw

discard=unmap - write zeroes unmap
0 /tmp/test.raw
[nsoffer build (consider-discard-option)]$


After this change:

$ cat after.out

defaults - write zeroes
1.0M /tmp/test.raw

defaults - write zeroes unmap
1.0M /tmp/test.raw

defaults - write actual zeros
1.0M /tmp/test.raw

discard=off - write zeroes unmap
1.0M /tmp/test.raw

detect-zeros=on - write actual zeros
1.0M /tmp/test.raw

detect-zeros=unmap,discard=unmap - write actual zeros
0 /tmp/test.raw

discard=unmap - write zeroes
1.0M /tmp/test.raw

discard=unmap - write zeroes unmap
0 /tmp/test.raw


Differences:

$ diff -u before.out after.out
--- before.out 2024-06-19 20:24:09.234083713 +0300
+++ after.out 2024-06-19 20:24:20.526165573 +0300
@@ -3,13 +3,13 @@
 1.0M /tmp/test.raw

 defaults - write zeroes unmap
-0 /tmp/test.raw
+1.0M /tmp/test.raw

 defaults - write actual zeros
 1.0M /tmp/test.raw

 discard=off - write zeroes unmap
-0 /tmp/test.raw
+1.0M /tmp/test.raw

On Wed, Jun 19, 2024 at 8:40 PM Nir Soffer  wrote:

> When opening an image with discard=off, we punch hole in the image when
> writing zeroes, making the image sparse. This breaks users that want to
> ensure that writes cannot fail with ENOSPACE by using fully allocated
> images.
>
> bdrv_co_pwrite_zeroes() correctly disable BDRV_REQ_MAY_UNMAP if we
> opened the child without discard=unmap or discard=on. But we don't go
> through this function when accessing the top node. Move the check down
> to bdrv_co_do_pwrite_zeroes() which seems to be used in all code paths.
>
> Issues:
> - We don't punch hole by default, so images are kept allocated. Before
>   this change we punched holes by default. I'm not sure this is a good
>   change in behavior.
> - Need to run all block tests
> - Not sure that we have tests covering unmapping, we may need new tests
> - We may need new tests to cover this change
>
> Signed-off-by: Nir Soffer 
> ---
>
> Changes since v1:
> - Replace the incorrect has_discard change with the right fix
>
> v1 was here:
> https://lists.nongnu.org/archive/html/qemu-block/2024-06/msg00198.html
>
>  block/io.c | 9 +
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/block/io.c b/block/io.c
> index 7217cf811b..301514c880 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1860,10 +1860,15 @@ bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
> int64_t offset, int64_t bytes,
>  /* By definition there is no user buffer so this flag doesn't make
> sense */
> 

Re: [PATCH v2] Consider discard option when writing zeros

[Nir Soffer]

Tested using:

$ cat test-unmap.sh
#!/bin/sh

qemu=${1:?Usage: $0 qemu-executable}
img=/tmp/test.raw

echo
echo "defaults - write zeroes"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -z 0 1m"\nquit' | $qemu -monitor stdio \
-drive if=none,file=$img,format=raw >/dev/null
du -sh $img

echo
echo "defaults - write zeroes unmap"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -zu 0 1m"\nquit' | $qemu -monitor stdio \
-drive if=none,file=$img,format=raw >/dev/null
du -sh $img

echo
echo "defaults - write actual zeros"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -P 0 0 1m"\nquit' | $qemu -monitor stdio \
-drive if=none,file=$img,format=raw >/dev/null
du -sh $img

echo
echo "discard=off - write zeroes unmap"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -zu 0 1m"\nquit' | $qemu -monitor stdio \
-drive if=none,file=$img,format=raw,discard=off >/dev/null
du -sh $img

echo
echo "detect-zeros=on - write actual zeros"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -P 0 0 1m"\nquit' | $qemu -monitor stdio \
-drive if=none,file=$img,format=raw,detect-zeroes=on >/dev/null
du -sh $img

echo
echo "detect-zeros=unmap,discard=unmap - write actual zeros"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -P 0 0 1m"\nquit' |  $qemu -monitor stdio \
-drive if=none,file=$img,format=raw,detect-zeroes=unmap,discard=unmap
>/dev/null
du -sh $img

echo
echo "discard=unmap - write zeroes"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -z 0 1m"\nquit' | $qemu -monitor stdio \
-drive if=none,file=$img,format=raw,discard=unmap >/dev/null
du -sh $img

echo
echo "discard=unmap - write zeroes unmap"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -zu 0 1m"\nquit' | $qemu -monitor stdio \
-drive if=none,file=$img,format=raw,discard=unmap >/dev/null
du -sh $img

rm $img


Before this change:

$ cat before.out

defaults - write zeroes
1.0M /tmp/test.raw

defaults - write zeroes unmap
0 /tmp/test.raw

defaults - write actual zeros
1.0M /tmp/test.raw

discard=off - write zeroes unmap
0 /tmp/test.raw

detect-zeros=on - write actual zeros
1.0M /tmp/test.raw

detect-zeros=unmap,discard=unmap - write actual zeros
0 /tmp/test.raw

discard=unmap - write zeroes
1.0M /tmp/test.raw

discard=unmap - write zeroes unmap
0 /tmp/test.raw
[nsoffer build (consider-discard-option)]$


After this change:

$ cat after.out

defaults - write zeroes
1.0M /tmp/test.raw

defaults - write zeroes unmap
1.0M /tmp/test.raw

defaults - write actual zeros
1.0M /tmp/test.raw

discard=off - write zeroes unmap
1.0M /tmp/test.raw

detect-zeros=on - write actual zeros
1.0M /tmp/test.raw

detect-zeros=unmap,discard=unmap - write actual zeros
0 /tmp/test.raw

discard=unmap - write zeroes
1.0M /tmp/test.raw

discard=unmap - write zeroes unmap
0 /tmp/test.raw


Differences:

$ diff -u before.out after.out
--- before.out 2024-06-19 20:24:09.234083713 +0300
+++ after.out 2024-06-19 20:24:20.526165573 +0300
@@ -3,13 +3,13 @@
 1.0M /tmp/test.raw

 defaults - write zeroes unmap
-0 /tmp/test.raw
+1.0M /tmp/test.raw

 defaults - write actual zeros
 1.0M /tmp/test.raw

 discard=off - write zeroes unmap
-0 /tmp/test.raw
+1.0M /tmp/test.raw

On Wed, Jun 19, 2024 at 8:40 PM Nir Soffer  wrote:

> When opening an image with discard=off, we punch hole in the image when
> writing zeroes, making the image sparse. This breaks users that want to
> ensure that writes cannot fail with ENOSPACE by using fully allocated
> images.
>
> bdrv_co_pwrite_zeroes() correctly disable BDRV_REQ_MAY_UNMAP if we
> opened the child without discard=unmap or discard=on. But we don't go
> through this function when accessing the top node. Move the check down
> to bdrv_co_do_pwrite_zeroes() which seems to be used in all code paths.
>
> Issues:
> - We don't punch hole by default, so images are kept allocated. Before
>   this change we punched holes by default. I'm not sure this is a good
>   change in behavior.
> - Need to run all block tests
> - Not sure that we have tests covering unmapping, we may need new tests
> - We may need new tests to cover this change
>
> Signed-off-by: Nir Soffer 
> ---
>
> Changes since v1:
> - Replace the incorrect has_discard change with the right fix
>
> v1 was here:
> https://lists.nongnu.org/archive/html/qemu-block/2024-06/msg00198.html
>
>  block/io.c | 9 +
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/block/io.c b/block/io.c
> index 7217cf811b..301514c880 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1860,10 +1860,15 @@ bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
> int64_t offset, int64_t bytes,
>  /* By definition there is no user buffer so this flag doesn't make
> sense */
> 

[PATCH v2] Consider discard option when writing zeros

[Nir Soffer]

When opening an image with discard=off, we punch hole in the image when
writing zeroes, making the image sparse. This breaks users that want to
ensure that writes cannot fail with ENOSPACE by using fully allocated
images.

bdrv_co_pwrite_zeroes() correctly disable BDRV_REQ_MAY_UNMAP if we
opened the child without discard=unmap or discard=on. But we don't go
through this function when accessing the top node. Move the check down
to bdrv_co_do_pwrite_zeroes() which seems to be used in all code paths.

Issues:
- We don't punch hole by default, so images are kept allocated. Before
  this change we punched holes by default. I'm not sure this is a good
  change in behavior.
- Need to run all block tests
- Not sure that we have tests covering unmapping, we may need new tests
- We may need new tests to cover this change

Signed-off-by: Nir Soffer 
---

Changes since v1:
- Replace the incorrect has_discard change with the right fix

v1 was here:
https://lists.nongnu.org/archive/html/qemu-block/2024-06/msg00198.html

 block/io.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/block/io.c b/block/io.c
index 7217cf811b..301514c880 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1860,10 +1860,15 @@ bdrv_co_do_pwrite_zeroes(BlockDriverState *bs, int64_t 
offset, int64_t bytes,
 /* By definition there is no user buffer so this flag doesn't make sense */
 if (flags & BDRV_REQ_REGISTERED_BUF) {
 return -EINVAL;
 }
 
+/* If opened with discard=off we should never unmap. */
+if (!(bs->open_flags & BDRV_O_UNMAP)) {
+flags &= ~BDRV_REQ_MAY_UNMAP;
+}
+
 /* Invalidate the cached block-status data range if this write overlaps */
 bdrv_bsc_invalidate_range(bs, offset, bytes);
 
 assert(alignment % bs->bl.request_alignment == 0);
 head = offset % alignment;
@@ -2313,14 +2318,10 @@ int coroutine_fn bdrv_co_pwrite_zeroes(BdrvChild 
*child, int64_t offset,
 {
 IO_CODE();
 trace_bdrv_co_pwrite_zeroes(child->bs, offset, bytes, flags);
 assert_bdrv_graph_readable();
 
-if (!(child->bs->open_flags & BDRV_O_UNMAP)) {
-flags &= ~BDRV_REQ_MAY_UNMAP;
-}
-
 return bdrv_co_pwritev(child, offset, bytes, NULL,
BDRV_REQ_ZERO_WRITE | flags);
 }
 
 /*
-- 
2.45.1

[PATCH v2] Consider discard option when writing zeros

[Nir Soffer]

When opening an image with discard=off, we punch hole in the image when
writing zeroes, making the image sparse. This breaks users that want to
ensure that writes cannot fail with ENOSPACE by using fully allocated
images.

bdrv_co_pwrite_zeroes() correctly disable BDRV_REQ_MAY_UNMAP if we
opened the child without discard=unmap or discard=on. But we don't go
through this function when accessing the top node. Move the check down
to bdrv_co_do_pwrite_zeroes() which seems to be used in all code paths.

Issues:
- We don't punch hole by default, so images are kept allocated. Before
  this change we punched holes by default. I'm not sure this is a good
  change in behavior.
- Need to run all block tests
- Not sure that we have tests covering unmapping, we may need new tests
- We may need new tests to cover this change

Signed-off-by: Nir Soffer 
---

Changes since v1:
- Replace the incorrect has_discard change with the right fix

v1 was here:
https://lists.nongnu.org/archive/html/qemu-block/2024-06/msg00198.html

 block/io.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/block/io.c b/block/io.c
index 7217cf811b..301514c880 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1860,10 +1860,15 @@ bdrv_co_do_pwrite_zeroes(BlockDriverState *bs, int64_t 
offset, int64_t bytes,
 /* By definition there is no user buffer so this flag doesn't make sense */
 if (flags & BDRV_REQ_REGISTERED_BUF) {
 return -EINVAL;
 }
 
+/* If opened with discard=off we should never unmap. */
+if (!(bs->open_flags & BDRV_O_UNMAP)) {
+flags &= ~BDRV_REQ_MAY_UNMAP;
+}
+
 /* Invalidate the cached block-status data range if this write overlaps */
 bdrv_bsc_invalidate_range(bs, offset, bytes);
 
 assert(alignment % bs->bl.request_alignment == 0);
 head = offset % alignment;
@@ -2313,14 +2318,10 @@ int coroutine_fn bdrv_co_pwrite_zeroes(BdrvChild 
*child, int64_t offset,
 {
 IO_CODE();
 trace_bdrv_co_pwrite_zeroes(child->bs, offset, bytes, flags);
 assert_bdrv_graph_readable();
 
-if (!(child->bs->open_flags & BDRV_O_UNMAP)) {
-flags &= ~BDRV_REQ_MAY_UNMAP;
-}
-
 return bdrv_co_pwritev(child, offset, bytes, NULL,
BDRV_REQ_ZERO_WRITE | flags);
 }
 
 /*
-- 
2.45.1

Re: How to stop QEMU punching holes in backing store

[Nir Soffer]

> On 19 Jun 2024, at 8:54, Justin  wrote:
> 
>>I've run strace and I see calls to fallocate with these flags:
>>FALLOC_FL_KEEP_SIZE|FALLOC_FL_PUNCH_HOLE
>> 
>>I've tried passing these options: discard=off,detect-zeroes=off but
>>this does not help. This is the full set of relevant options I'm
>>using:
>> 
>>-drive file=/vms/vm0/drive,format=raw,if=virtio,discard=off,detect-zeroes=
>>off
>> 
>> You don't need to disable detect-zeros - in my tests it makes dd if=/dev/zero
>> 5 times faster (770 MiB/s -> 3 GiB/s) since zero writes are converted to
>> fallocate(FALLOC_FL_KEEP_SIZE|FALLOC_FL_ZERO_RANGE).
>> 
>> The issue seems to be ignoring the discard option when opening the image,
>> and is fixed by this:
>> https://lists.nongnu.org/archive/html/qemu-block/2024-06/msg00198.html
> 
> Thanks. When might this patch (or something similar) be merged?

The patch need more work, when the work is done and qemu maintainers are happy 
it is a good estimate :-)

> 
>> I think the change needs more work to keep the default behavior
>> since most users want sparse images, but it seems to do what you
>> want - keeping images thick.
> 
> It seems that this patch is making the code align more closely with
> the documentation? To, me, it appeared fairly clear that discard=unmap
> would punch holes, and thus the inverse setting would stop hole
> punching.

Punching holes is used both for discard (e.g. fstrim in the guest) and for 
writing zeros.

I think that discard should work only when you set discard=unmap or 
disacard=on, but writing zeros should always punch holes unless you set 
discard=off. I don’t think this behavior is documented now but it should be, at 
least the intent to keep images sparse when possible.

Nir

Re: [PATCH] block/file-posix: Consider discard flag when opening

[Nir Soffer]

> On 19 Jun 2024, at 11:16, Kevin Wolf  wrote:
> 
> Am 18.06.2024 um 23:24 hat Nir Soffer geschrieben:
>> Set has_discard only when BDRV_O_UNMAP is not set. With this users that
>> want to keep their images fully allocated can disable hole punching
>> when writing zeros or discarding using:
>> 
>>   -drive file=thick.img,discard=off
>> 
>> This change is not entirely correct since it changes the default discard
>> behavior.  Previously we always allowed punching holes, but now you have
>> must use discard=unmap|on to enable it. We probably need to add the
>> BDDR_O_UNMAP flag by default.
>> 
>> make check still works, so maybe we don't have tests for sparsifying
>> images, or maybe you need to run special tests that do not run by
>> default. We needs tests for keeping images non-sparse.
>> 
>> Signed-off-by: Nir Soffer 
> 
> So first of all, I agree with you that this patch is wrong. ;-)
> 
> At first, I failed to understand the problem this is trying to solve. I
> put a debug message in handle_aiocb_discard() and tried with which
> options it triggers. [1] To me, this looked exactly like it should be.
> We only try to discard blocks when discard=unmap is given as an option.
> 
> That leaves the case of write_zeroes. And while at the first sight, the
> code looked good, we do seem to have a problem there and it tried to
> unmap even with discard=off.
> 
>> block/file-posix.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/block/file-posix.c b/block/file-posix.c
>> index be25e35ff6..acac2abadc 100644
>> --- a/block/file-posix.c
>> +++ b/block/file-posix.c
>> @@ -738,11 +738,11 @@ static int raw_open_common(BlockDriverState *bs, QDict 
>> *options,
>> ret = -EINVAL;
>> goto fail;
>> }
>> #endif /* !defined(CONFIG_LINUX_IO_URING) */
>> 
>> -s->has_discard = true;
>> +s->has_discard = !!(bdrv_flags & BDRV_O_UNMAP);
>> s->has_write_zeroes = true;
>> 
>> if (fstat(s->fd, ) < 0) {
>> ret = -errno;
>> error_setg_errno(errp, errno, "Could not stat file");
> 
> s->has_discard is about what the host supports, not about the semantics
> of the QEMU block node. So this doesn't feel right to me.
> 
> So for the buggy case, write_zeroes, bdrv_co_pwrite_zeroes() has code
> that considers the case and clears the ~BDRV_REQ_MAY_UNMAP flags:
> 
>if (!(child->bs->open_flags & BDRV_O_UNMAP)) {
>flags &= ~BDRV_REQ_MAY_UNMAP;
>}
> 
> But it turns out that we don't necessarily even go through this function
> for the top node which has discard=off, so it can't take effect:
> 
> (gdb) bt
> #0  0x74f2f144 in __pthread_kill_implementation () at /lib64/libc.so 
> <http://libc.so/>.6
> #1  0x74ed765e in raise () at /lib64/libc.so <http://libc.so/>.6
> #2  0x74ebf902 in abort () at /lib64/libc.so <http://libc.so/>.6
> #3  0x5615aff0 in raw_do_pwrite_zeroes (bs=0x57f4bcf0, offset=0, 
> bytes=1048576, flags=BDRV_REQ_MAY_UNMAP, blkdev=false) at 
> ../block/file-posix.c:3643
> #4  0x5615557e in raw_co_pwrite_zeroes (bs=0x57f4bcf0, offset=0, 
> bytes=1048576, flags=BDRV_REQ_MAY_UNMAP) at ../block/file-posix.c:3655
> #5  0x560cde2a in bdrv_co_do_pwrite_zeroes (bs=0x57f4bcf0, 
> offset=0, bytes=1048576, flags=6) at ../block/io.c:1901
> #6  0x560c72f9 in bdrv_aligned_pwritev (child=0x57f51460, 
> req=0x7fffed5ff800, offset=0, bytes=1048576, align=1, qiov=0x0, 
> qiov_offset=0, flags=6) at ../block/io.c:2100
> #7  0x560c6b41 in bdrv_co_do_zero_pwritev (child=0x57f51460, 
> offset=0, bytes=1048576, flags=6, req=0x7fffed5ff800) at ../block/io.c:2183
> #8  0x560c6647 in bdrv_co_pwritev_part (child=0x57f51460, 
> offset=0, bytes=1048576, qiov=0x0, qiov_offset=0, flags=6) at 
> ../block/io.c:2283
> #9  0x560c634f in bdrv_co_pwritev (child=0x57f51460, offset=0, 
> bytes=1048576, qiov=0x0, flags=6) at ../block/io.c:2216
> #10 0x560c75b5 in bdrv_co_pwrite_zeroes (child=0x57f51460, 
> offset=0, bytes=1048576, flags=BDRV_REQ_MAY_UNMAP) at ../block/io.c:2322
> #11 0x56117d24 in raw_co_pwrite_zeroes (bs=0x57f44980, offset=0, 
> bytes=1048576, flags=BDRV_REQ_MAY_UNMAP) at ../block/raw-format.c:307
> #12 0x560cde2a in bdrv_co_do_pwrite_zeroes (bs=0x57f44980, 
> offset=0, bytes=1048576, flags=6) at ../block/io.c:1901
> #13 0x560c72f9 in bdrv_aligned_pwritev (child=0x57f513f0, 
> req=0x7fffed5ffd90, offset=0, bytes=1048576, align=1, qiov=0x0, 
> q

Re: [PATCH] block/file-posix: Consider discard flag when opening

[Nir Soffer]

> On 19 Jun 2024, at 11:16, Kevin Wolf  wrote:
> 
> Am 18.06.2024 um 23:24 hat Nir Soffer geschrieben:
>> Set has_discard only when BDRV_O_UNMAP is not set. With this users that
>> want to keep their images fully allocated can disable hole punching
>> when writing zeros or discarding using:
>> 
>>   -drive file=thick.img,discard=off
>> 
>> This change is not entirely correct since it changes the default discard
>> behavior.  Previously we always allowed punching holes, but now you have
>> must use discard=unmap|on to enable it. We probably need to add the
>> BDDR_O_UNMAP flag by default.
>> 
>> make check still works, so maybe we don't have tests for sparsifying
>> images, or maybe you need to run special tests that do not run by
>> default. We needs tests for keeping images non-sparse.
>> 
>> Signed-off-by: Nir Soffer 
> 
> So first of all, I agree with you that this patch is wrong. ;-)
> 
> At first, I failed to understand the problem this is trying to solve. I
> put a debug message in handle_aiocb_discard() and tried with which
> options it triggers. [1] To me, this looked exactly like it should be.
> We only try to discard blocks when discard=unmap is given as an option.
> 
> That leaves the case of write_zeroes. And while at the first sight, the
> code looked good, we do seem to have a problem there and it tried to
> unmap even with discard=off.
> 
>> block/file-posix.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/block/file-posix.c b/block/file-posix.c
>> index be25e35ff6..acac2abadc 100644
>> --- a/block/file-posix.c
>> +++ b/block/file-posix.c
>> @@ -738,11 +738,11 @@ static int raw_open_common(BlockDriverState *bs, QDict 
>> *options,
>> ret = -EINVAL;
>> goto fail;
>> }
>> #endif /* !defined(CONFIG_LINUX_IO_URING) */
>> 
>> -s->has_discard = true;
>> +s->has_discard = !!(bdrv_flags & BDRV_O_UNMAP);
>> s->has_write_zeroes = true;
>> 
>> if (fstat(s->fd, ) < 0) {
>> ret = -errno;
>> error_setg_errno(errp, errno, "Could not stat file");
> 
> s->has_discard is about what the host supports, not about the semantics
> of the QEMU block node. So this doesn't feel right to me.
> 
> So for the buggy case, write_zeroes, bdrv_co_pwrite_zeroes() has code
> that considers the case and clears the ~BDRV_REQ_MAY_UNMAP flags:
> 
>if (!(child->bs->open_flags & BDRV_O_UNMAP)) {
>flags &= ~BDRV_REQ_MAY_UNMAP;
>}
> 
> But it turns out that we don't necessarily even go through this function
> for the top node which has discard=off, so it can't take effect:
> 
> (gdb) bt
> #0  0x74f2f144 in __pthread_kill_implementation () at /lib64/libc.so 
> <http://libc.so/>.6
> #1  0x74ed765e in raise () at /lib64/libc.so <http://libc.so/>.6
> #2  0x74ebf902 in abort () at /lib64/libc.so <http://libc.so/>.6
> #3  0x5615aff0 in raw_do_pwrite_zeroes (bs=0x57f4bcf0, offset=0, 
> bytes=1048576, flags=BDRV_REQ_MAY_UNMAP, blkdev=false) at 
> ../block/file-posix.c:3643
> #4  0x5615557e in raw_co_pwrite_zeroes (bs=0x57f4bcf0, offset=0, 
> bytes=1048576, flags=BDRV_REQ_MAY_UNMAP) at ../block/file-posix.c:3655
> #5  0x560cde2a in bdrv_co_do_pwrite_zeroes (bs=0x57f4bcf0, 
> offset=0, bytes=1048576, flags=6) at ../block/io.c:1901
> #6  0x560c72f9 in bdrv_aligned_pwritev (child=0x57f51460, 
> req=0x7fffed5ff800, offset=0, bytes=1048576, align=1, qiov=0x0, 
> qiov_offset=0, flags=6) at ../block/io.c:2100
> #7  0x560c6b41 in bdrv_co_do_zero_pwritev (child=0x57f51460, 
> offset=0, bytes=1048576, flags=6, req=0x7fffed5ff800) at ../block/io.c:2183
> #8  0x560c6647 in bdrv_co_pwritev_part (child=0x57f51460, 
> offset=0, bytes=1048576, qiov=0x0, qiov_offset=0, flags=6) at 
> ../block/io.c:2283
> #9  0x560c634f in bdrv_co_pwritev (child=0x57f51460, offset=0, 
> bytes=1048576, qiov=0x0, flags=6) at ../block/io.c:2216
> #10 0x560c75b5 in bdrv_co_pwrite_zeroes (child=0x57f51460, 
> offset=0, bytes=1048576, flags=BDRV_REQ_MAY_UNMAP) at ../block/io.c:2322
> #11 0x56117d24 in raw_co_pwrite_zeroes (bs=0x57f44980, offset=0, 
> bytes=1048576, flags=BDRV_REQ_MAY_UNMAP) at ../block/raw-format.c:307
> #12 0x560cde2a in bdrv_co_do_pwrite_zeroes (bs=0x57f44980, 
> offset=0, bytes=1048576, flags=6) at ../block/io.c:1901
> #13 0x560c72f9 in bdrv_aligned_pwritev (child=0x57f513f0, 
> req=0x7fffed5ffd90, offset=0, bytes=1048576, align=1, qiov=0x0, 
> q

Re: [External] : Re: Eclipse and Gradle in OpenJFX

[Nir Lisker]

https://github.com/eclipse/buildship/issues/658 is not that relevant
because it describes a problem in Gradle's integration with Eclipse, not
with Buildship, but it's reported on Buildship, so I don't think there's
much to resolve there except for alignment issues. It has since been solved
on Gradle's side. If you go to Preferences > Grade > Experimental features
> Enable module support, the described issue will be resolved (and maybe
even without it because I'm not sure these features are experimental
anymore - Gradle 7 enabled module support by default
https://docs.gradle.org/7.0/release-notes.html#promoted-features).

On Wed, Jun 19, 2024 at 3:28 AM Andy Goryachev 
wrote:

> Thank you Kevin, for this bit of insight.  I see the Buildship issue
> mentioned in one of the comments is still open with no one assigned
>
>
>
> https://github.com/eclipse/buildship/issues/658
>
>
>
> Chances that it will be fixed in a reasonable time frame are slim to
> none.  I would recommend to make the changes (remove the gradle nature from
> eclipse config files) in the meantime, as it allows for clean import of the
> whole thing into Eclipse.
>
>
>
> Once the situation with Buildship changes, we can remove the Eclipse files
> altogether and rely on gradle import per JDK-8223374.
>
>
>
> I am also very interested in Nir's view on the subject.
>
>
>
> -andy
>
>
>
>
>
>
>
> *From: *Kevin Rushforth 
> *Date: *Tuesday, June 18, 2024 at 16:09
> *To: *Andy Goryachev , Thiago Milczarek Sayão <
> thiago.sa...@gmail.com>
> *Cc: *openjfx-dev@openjdk.org 
> *Subject: *Re: [External] : Re: Eclipse and Gradle in OpenJFX
>
> We also did that for NetBeans, meaning we removed the NetBeans
> IDE-specific files and use their gradle plug-in (with somewhat mixed
> results). See https://bugs.openjdk.org/browse/JDK-8223375
>
> FWIW, there is an open issue to do the same for Eclipse, but I think it
> hasn't been looked at in a while. Nir is the assignee so he will likely
> have some thoughts on this. https://bugs.openjdk.org/browse/JDK-8223374
>
> -- Kevin
>
> On 6/18/2024 1:14 PM, Andy Goryachev wrote:
>
> Interesting, thank you, Thiago.
>
>
>
> Maybe it's just the quality of gradle support in IntelliJ, or complexity
> of the Buildship plug-in in Eclipse.  It never worked for me, and removing
> the gradle nature from Eclipse project files has an added benefit of
> removing an extra dependency.
>
>
>
> -andy
>
>
>
>
>
>
>
> *From: *Thiago Milczarek Sayão 
> 
> *Date: *Tuesday, June 18, 2024 at 12:51
> *To: *Andy Goryachev 
> 
> *Cc: *openjfx-dev@openjdk.org 
> 
> *Subject: *[External] : Re: Eclipse and Gradle in OpenJFX
>
> Andy,
>
>
>
> We kind of did the opposite for Intellij (got rid of the .iml files and
> went for gradle import):
>
>
>
> https://github.com/openjdk/jfx/pull/1009
> <https://urldefense.com/v3/__https:/github.com/openjdk/jfx/pull/1009__;!!ACWV5N9M2RV99hQ!KGyKiKF64SpFYCZE7vMq0nzFAyNGv-gTQJoxy9lGH2dg15mO5dl-ZuQha_yGdjFGH_l740roARzs-f231vB1WSlkRk4$>
>
>
>
> I couldn't get it to detect the manual tests tho. Changing some gradle
> files worked, but would require a deeper review, so we went without it.
>
>
>
> -- Thiago.
>
>
>
> Em ter., 18 de jun. de 2024 às 16:05, Andy Goryachev <
> andy.goryac...@oracle.com> escreveu:
>
> Dear developers:
>
>
>
> Does anyone use gradle in Eclipse (Buildship plug-in) with the OpenJFX
> repo?
>
>
>
> The reason I am asking is that in my experience, the gradle nature in
> OpenJFX is either misconfigured, or obsolete, or both.  There is a rather
> old wiki page [0] which describes the Eclipse setup, though I don't think
> it is correct anymore.  The initial import of the repository in Eclipse
> triggers an internal gradle run which creates/modifies a bunch of
> .classpath and .project files which must be undone before the workspace
> becomes usable.  In any case, only a proper command line gradle build is
> supported anyway.
>
>
>
> I would like to propose removing the gradle nature from Eclipse's .project
> files in OpenJFX.  Once done, the projects can be trivially imported into a
> new workspace with no extra steps required.  This change has no impact on
> command line build whatsoever.
>
>
>
> What do you think?
>
>
>
> Thank you
>
> -andy
>
>
>
>
>
>
>
> *References*
>
>
>
> [1]
> https://wiki.openjdk.org/display/OpenJFX/Using+an+IDE#UsinganIDE-ConfigureEclipsetousethelatestJDK
>
>
>

Re: Eclipse and Gradle in OpenJFX

[Nir Lisker]

It has been a while since I tried a clean import of jfx, but the
instructions in
https://wiki.openjdk.org/display/OpenJFX/Using+an+IDE#UsinganIDE-UsingEclipse
seem correct, unless something changed. What you describe is written there.
Only the initial import requires a gradle build, and after reverting the
project and classpath changes, you can work with ECJ (unless you need tasks
to build native code etc.)

What is worth looking at is the 'eclipse' gradle task that tries to
generate these files according to the gradle files. However, the way the
project is built in gradle is both complicated and very old (compliant with
gradle 2 I think), so I don't know how well that will work. In my modern
projects, the 'eclipse' task works really well and allows me to not check
in the eclipse files: a gradle refresh/synch is run once (does all the
configuration and dependency management), and then running 'eclipse' once
sets up the eclipse side.

On Tue, Jun 18, 2024 at 10:52 PM Thiago Milczarek Sayão <
thiago.sa...@gmail.com> wrote:

> Andy,
>
> We kind of did the opposite for Intellij (got rid of the .iml files and
> went for gradle import):
>
> https://github.com/openjdk/jfx/pull/1009
>
> I couldn't get it to detect the manual tests tho. Changing some gradle
> files worked, but would require a deeper review, so we went without it.
>
> -- Thiago.
>
> Em ter., 18 de jun. de 2024 às 16:05, Andy Goryachev <
> andy.goryac...@oracle.com> escreveu:
>
>> Dear developers:
>>
>>
>>
>> Does anyone use gradle in Eclipse (Buildship plug-in) with the OpenJFX
>> repo?
>>
>>
>>
>> The reason I am asking is that in my experience, the gradle nature in
>> OpenJFX is either misconfigured, or obsolete, or both.  There is a rather
>> old wiki page [0] which describes the Eclipse setup, though I don't think
>> it is correct anymore.  The initial import of the repository in Eclipse
>> triggers an internal gradle run which creates/modifies a bunch of
>> .classpath and .project files which must be undone before the workspace
>> becomes usable.  In any case, only a proper command line gradle build is
>> supported anyway.
>>
>>
>>
>> I would like to propose removing the gradle nature from Eclipse's
>> .project files in OpenJFX.  Once done, the projects can be trivially
>> imported into a new workspace with no extra steps required.  This change
>> has no impact on command line build whatsoever.
>>
>>
>>
>> What do you think?
>>
>>
>>
>> Thank you
>>
>> -andy
>>
>>
>>
>>
>>
>>
>>
>> *References*
>>
>>
>>
>> [1]
>> https://wiki.openjdk.org/display/OpenJFX/Using+an+IDE#UsinganIDE-ConfigureEclipsetousethelatestJDK
>>
>

Re: How to stop QEMU punching holes in backing store

[Nir Soffer]

On Wed, Jun 5, 2024 at 5:27 PM Justin  wrote:

> Hi. I'm using QEMU emulator version 5.2.0 on Linux. I am using
> thick-provisioned RAW files as the VM backing store. I've found that
> QEMU is punching holes in my RAW files (it's replacing some zero
> blocks with holes), which means that the number of blocks allocated to
> the VM volumes decreases. It keeps doing this; I've manually used
> fallocate(1) to reallocate the full number of blocks to the VM backing
> store files, and sometime later QEMU punches some more holes.
>
> How do I completely disable all hole punching?
>
> The problem with this behavious is that this confuses capacity
> management software into thinking that there is enough free space to
> create more VMs. The file-system for the VM backing stores becomes
> over-committed. Later, when a VM starts writing non-zero data to the
> holes, the VM hangs because QEMU cannot write to the backing store
> because there are no free blocks available. There is no recovery other
> than deleting files, so it basically means one or more VMs have to be
> sacrificed for the greater good.
>

On the other hand, using a thin disk means that storage operations like
copying a disk, backup or writing zeros are much more efficient.

I would check if it is possible to fix the capacity management system to
consider the disk virtual instead of available space.


> I've run strace and I see calls to fallocate with these flags:
> FALLOC_FL_KEEP_SIZE|FALLOC_FL_PUNCH_HOLE
>
> I've tried passing these options: discard=off,detect-zeroes=off but
> this does not help. This is the full set of relevant options I'm
> using:
>
> -drive
> file=/vms/vm0/drive,format=raw,if=virtio,discard=off,detect-zeroes=off
>

You don't need to disable detect-zeros - in my tests it makes dd
if=/dev/zero
5 times faster (770 MiB/s -> 3 GiB/s) since zero writes are converted to
fallocate(FALLOC_FL_KEEP_SIZE|FALLOC_FL_ZERO_RANGE).

The issue seems to be ignoring the discard option when opening the image,
and is fixed by this:
https://lists.nongnu.org/archive/html/qemu-block/2024-06/msg00198.html

I think the change needs more work to keep the default behavior since most
users
want sparse images, but it seems to do what you want - keeping images thick.

Nir

[PATCH] block/file-posix: Consider discard flag when opening

[Nir Soffer]

Set has_discard only when BDRV_O_UNMAP is not set. With this users that
want to keep their images fully allocated can disable hole punching
when writing zeros or discarding using:

   -drive file=thick.img,discard=off

This change is not entirely correct since it changes the default discard
behavior.  Previously we always allowed punching holes, but now you have
must use discard=unmap|on to enable it. We probably need to add the
BDDR_O_UNMAP flag by default.

make check still works, so maybe we don't have tests for sparsifying
images, or maybe you need to run special tests that do not run by
default. We needs tests for keeping images non-sparse.

Signed-off-by: Nir Soffer 
---
 block/file-posix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index be25e35ff6..acac2abadc 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -738,11 +738,11 @@ static int raw_open_common(BlockDriverState *bs, QDict 
*options,
 ret = -EINVAL;
 goto fail;
 }
 #endif /* !defined(CONFIG_LINUX_IO_URING) */
 
-s->has_discard = true;
+s->has_discard = !!(bdrv_flags & BDRV_O_UNMAP);
 s->has_write_zeroes = true;
 
 if (fstat(s->fd, ) < 0) {
 ret = -errno;
 error_setg_errno(errp, errno, "Could not stat file");
-- 
2.45.1

[PATCH] block/file-posix: Consider discard flag when opening

[Nir Soffer]

Set has_discard only when BDRV_O_UNMAP is not set. With this users that
want to keep their images fully allocated can disable hole punching
when writing zeros or discarding using:

   -drive file=thick.img,discard=off

This change is not entirely correct since it changes the default discard
behavior.  Previously we always allowed punching holes, but now you have
must use discard=unmap|on to enable it. We probably need to add the
BDDR_O_UNMAP flag by default.

make check still works, so maybe we don't have tests for sparsifying
images, or maybe you need to run special tests that do not run by
default. We needs tests for keeping images non-sparse.

Signed-off-by: Nir Soffer 
---
 block/file-posix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index be25e35ff6..acac2abadc 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -738,11 +738,11 @@ static int raw_open_common(BlockDriverState *bs, QDict 
*options,
 ret = -EINVAL;
 goto fail;
 }
 #endif /* !defined(CONFIG_LINUX_IO_URING) */
 
-s->has_discard = true;
+s->has_discard = !!(bdrv_flags & BDRV_O_UNMAP);
 s->has_write_zeroes = true;
 
 if (fstat(s->fd, ) < 0) {
 ret = -errno;
 error_setg_errno(errp, errno, "Could not stat file");
-- 
2.45.1

[Bug 2063827] Re: Gnome apps segfault in Nvidia Wayland sessions on Noble

[Nir Nachmani Major]

I have Intel CPU and have never installed Steam. I switched to Xorg for
now because between this bug and bug 1876632, Wayland is not usable.

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/2063827

Title:
  Gnome apps segfault in Nvidia Wayland sessions on Noble

To manage notifications about this bug go to:
https://bugs.launchpad.net/ubuntu/+source/egl-wayland/+bug/2063827/+subscriptions


-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

Re: RFR: 8311895: CSS Transitions [v23]

[Nir Lisker]

On Wed, 5 Jun 2024 21:59:13 GMT, Michael Strauß  wrote:

>> Implementation of [CSS 
>> Transitions](https://www.w3.org/TR/css-transitions-1/).
>> 
>> ### Future enhancements
>> CSS transition support for backgrounds and borders: #1471 
>> 
>> ### Limitations
>> This implementation supports both shorthand and longhand notations for the 
>> `transition` property. However, due to limitations of JavaFX CSS, mixing 
>> both notations doesn't work:
>> 
>> .button {
>> transition: -fx-background-color 1s;
>> transition-easing-function: linear;
>> }
>> 
>> This issue should be addressed in a follow-up enhancement.
>
> Michael Strauß has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   link to easing function images in scene

Marked as reviewed by nlisker (Reviewer).

-

PR Review: https://git.openjdk.org/jfx/pull/870#pullrequestreview-2111578408

[IPsec] The ESP Echo Protocol document for IPsecME

[Yoav Nir]

Hi, folks

At IETF 119, Jen Likova presented [1] the ESP Echo Protocol draft [2].

The conversation in the room was lively, but did not look like the kind of 
consensus that we just confirm on the list.

So rather than starting an adoption call now, we’d like to encourage people to 
discuss it on the list, to see if we are approaching such a consensus.

Regards,

Yoav on behalf of the chairs

[1] https://youtu.be/n-yH3jtQmdQ?t=1205  (presentation starts at the 20:10 mark)
[2] https://datatracker.ietf.org/doc/draft-colitti-ipsecme-esp-ping/___
IPsec mailing list -- ipsec@ietf.org
To unsubscribe send an email to ipsec-le...@ietf.org

Re: JavaFX 3D Feature request: Ability to set texture wrap mode for PhongMaterial

[Nir Lisker]

Hi Risto,

There is some work on this already here:
https://github.com/openjdk/jfx/pull/1281. I didn't have time to continue
with it, so it was auto-closed. To answer your points specifically:

Hi, sometimes it is useful to clamp texture coordinates that exceed the
> normal 0-1 range, however currently the default behaviour is to repeat them
> and there is no way to configure it.
> It would be useful to have a TextureWrap property in the PhongMaterial
> class that would have options like REPEAT, CLAMP, MIRRORED_REPEAT, but the
> first 2 would be enough.


I think you're talking about two options that are different, yet similar.
There's texture addressing [1] and texture wrapping [2] (showing links for
D3D). Wrapping mode is a render state and is performed before texture
addressing, which is a sampler state.

I am currently trying to replicate the rendering of a custom engine that
> clamps texture coordinates using the JavaFX 3D api however currently it
> doesn't seem like it's possible.
>

You might have a way of doing that with the mesh's texture coordinates.
Also have a look at the FXyz library [3] and this SO question [4].

Alternative would be to have some sort of a Texture class that is part of
> the PhongMaterial which would have that property instead, it could also
> have additional properties like texture filtering mode (as right now there
> is no way to configure that either).


Texture filtering is the first thing that is implemented in the above PR.
Bilinear and nearest-point are available for mag and min filters; mipmap
isn't really working for reasons I couldn't determine. You can try it out;
see my results from August last year [5].
The next step in that PR is figuring out what configuration options go into
this class (more can be added later, and it should probably be an interface
instead of a class anyway). We don't want to cram too many unrelated things
into it, making it a kitchen sink class, so I suggested starting
conservatively. We also need a unified API for the different current and
(possible) future piplines (D3D9, D3D11, OpenGL, Metal, Vulkan), but I did
some of that research already and the piplines agree rather nicely
[6], so I'm more confident on that front. Specifically, finding out if
render states and sampler states both belong there together because this
configuration class is applied *per map* (diffuse, specular, emissive,
normal), not *per material*. By the way, filtering is also a sampler
state, like addressing, so at the very least addressing can be added there.

- Nir

[1]
https://learn.microsoft.com/en-us/windows/win32/direct3d9/texture-addressing-modes
[2]
https://learn.microsoft.com/en-us/windows/win32/direct3d9/texture-wrapping
[3] https://github.com/FXyz/FXyz
[4]
https://stackoverflow.com/questions/49130485/javafx-shape3d-texturing-dont-strectch-the-image
[5] https://mail.openjdk.org/pipermail/openjfx-dev/2023-August/042181.html
[6] https://mail.openjdk.org/pipermail/openjfx-dev/2023-August/042136.html

On Sat, Jun 8, 2024 at 10:52 PM Risto Vaher 
wrote:

> Hi, sometimes it is useful to clamp texture coordinates that exceed the
> normal 0-1 range, however currently the default behaviour is to repeat them
> and there is no way to configure it.
> It would be useful to have a TextureWrap property in the PhongMaterial
> class that would have options like REPEAT, CLAMP, MIRRORED_REPEAT, but the
> first 2 would be enough.
> I am currently trying to replicate the rendering of a custom engine that
> clamps texture coordinates using the JavaFX 3D api however currently it
> doesn't seem like it's possible.
>
> Alternative would be to have some sort of a Texture class that is part of
> the PhongMaterial which would have that property instead, it could also
> have additional properties like texture filtering mode (as right now there
> is no way to configure that either).
>

[Bug 2063827] Re: Gnome Control Center fails to open on Wayland

[Nir Nachmani Major]

I have the same issue with NVIDA GeForce 4080 and driver 535.171.04 on
Ubuntu 24.04 with Wayland. Problem is that if I update to a newer driver
(which fixes the issue) I get another bug with letters and icons
disappearing after return from suspend (described here:
https://askubuntu.com/questions/1512797/most-characters-disappearing-in-
gnome-after-suspend/1516115#1516115)

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/2063827

Title:
  Gnome Control Center fails to open on Wayland

To manage notifications about this bug go to:
https://bugs.launchpad.net/ubuntu/+source/nvidia-graphics-drivers-535/+bug/2063827/+subscriptions


-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

Re: RFR: 8332110: [macos] jpackage tries to sign added files without the --mac-sign option [v2]

[Nir Lisker]

On Thu, 30 May 2024 22:54:12 GMT, Alexander Matveev  
wrote:

>> This issue is reproducible with and without `--mac-sign`. jpackage will 
>> "_ad-hoc_" sign application bundle when `--mac-sign` is not specified by 
>> using pseudo-identity "_-_". This is why jpackage tries to sign added files 
>> and this is expected behavior by jpackage. "codesign" fails since added 
>> content made application bundle structure invalid. There is nothing we can 
>> do on jpackage side to sign such invalid bundles. As proposed solution we 
>> will output possible reason for "codesign" failure if it fails and 
>> `--app-content` was specified and possible solution. Proposed message: "One 
>> of the possible reason for "codesign" failure is additional content provided 
>> via "--app-content", which made application bundle structure invalid. Make 
>> sure to provide additional content in a way it will not break application 
>> bundle structure, otherwise add additional content as post-processing step."
>> 
>> Example:
>> Lets assume we have "ReadMe" folder with "ReadMe.txt" file in it.
>> 1) jpackage --type app-image -n Test --app-content ReadMe/ReadMe.txt ...
>> "codesign" will fail with "In subcomponent: Test.app/Contents/ReadMe.txt". 
>> This is expected and "ReadMe.txt" placed in "Test.app/Contents" which is 
>> also expected.
>> 2) jpackage --type app-image -n Test --app-content ReadMe ...
>> Works and "ReadMe.txt" will be placed under "Test.app/Contents/ReadMe".
>> 
>> Sample output before fix:
>> 
>> Error: "codesign" failed with following output:
>> Test.app: replacing existing signature
>> Test.app: code object is not signed at all
>> In subcomponent: Test.app/Contents/ReadMe.txt
>> 
>> 
>> Sample output after fix:
>> 
>> "codesign" failed and additional application content was supplied via the 
>> "--app-content" parameter. Probably the additional content broke the 
>> integrity of the application bundle and caused the failure. Ensure content 
>> supplied via the "--app-content" parameter does not break the integrity of 
>> the application bundle, or add it in the post-processing step.
>> Error: "codesign" failed with following output:
>> Test.app: replacing existing signature
>> Test.app: code object is not signed at all
>> In subcomponent: Test.app/Contents/ReadMe.txt
>
> Alexander Matveev has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8332110: jpackage tries to sign added files without the --mac-sign option 
> [v2]

I see, but it doesn't say where to put license files, which are usually in the 
root. Do you know where these belong?

-

PR Comment: https://git.openjdk.org/jdk/pull/19377#issuecomment-2143012428

Re: RFR: 8332110: [macos] jpackage tries to sign added files without the --mac-sign option

[Nir Lisker]

On Sat, 25 May 2024 01:12:56 GMT, Alexander Matveev 
 wrote:

> > For your example. This almost seems like an Apple bug if you can add a 
> > directory to the Contents directory but not a file?
> 
> Not sure if it is an Apple bug.

Can this be reported to Apple somehow?

-

PR Comment: https://git.openjdk.org/jdk/pull/19377#issuecomment-2142953200

Re: RFR: 8332748: Grammatical errors in animation API docs [v2]

[Nir Lisker]

On Wed, 29 May 2024 15:52:36 GMT, Lukasz Kostyra  wrote:

>> Checked code and fixed the gramatical error in Transition files: 
>> s/interval/intervals
>> 
>> Fill and Stroke Transition had correct grammatical form already, so those 
>> were untouched. I couldn't find any other instances of this error in our 
>> javadoc documentation.
>
> Lukasz Kostyra has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Add missing fix for Scale Transition

I think that 1 Reviewer is enough, but give it some time for others to have a 
chance to review.

-

Marked as reviewed by nlisker (Reviewer).

PR Review: https://git.openjdk.org/jfx/pull/1466#pullrequestreview-2085810041

Re: RFR: 8332748: Grammatical errors in animation API docs

[Nir Lisker]

On Wed, 29 May 2024 06:53:40 GMT, Lukasz Kostyra  wrote:

> Checked code and fixed the gramatical error in Transition files: 
> s/interval/intervals
> 
> Fill and Stroke Transition had correct grammatical form already, so those 
> were untouched. I couldn't find any other instances of this error in our 
> javadoc documentation.

`ScaleTransition` is missing a fix.

-

PR Comment: https://git.openjdk.org/jfx/pull/1466#issuecomment-2137545249

Re: CFV: New OpenJFX Reviewer: John Hendrikx

[Nir Lisker]

Resending the vote on the mailing list because I sent it only to Kevin.

Vote: YES

On Thu, May 23, 2024 at 2:38 AM Nir Lisker  wrote:

> Vote: YES
>
> On Thu, May 23, 2024 at 2:24 AM Kevin Rushforth <
> kevin.rushfo...@oracle.com> wrote:
>
>> I hereby nominate John Hendrikx [1] to OpenJFX Reviewer.
>>
>> John is an OpenJFX community member, who has contributed 39 commits [2]
>> to OpenJFX. John regularly participates in code reviews, especially in
>> the areas of JavaFX properties, scene graph and UI controls, offering
>> valuable feedback.
>>
>> Votes are due by June 5, 2024 at 23:59 UTC.
>>
>> Only current OpenJFX Reviewers [3] are eligible to vote on this
>> nomination. Votes must be cast in the open by replying to this mailing
>> list.
>>
>> For Three-Vote Consensus voting instructions, see [4]. Nomination to a
>> project Reviewer is described in [5].
>>
>> Thanks.
>>
>> -- Kevin
>>
>>
>> [1] https://openjdk.org/census#jhendrikx
>>
>> [2]
>>
>> https://github.com/search?q=repo%3Aopenjdk%2Fjfx+author-name%3A%22John+Hendrikx%22=commits
>>
>> [3] https://openjdk.java.net/census#openjfx
>>
>> [4] https://openjdk.java.net/bylaws#three-vote-consensus
>>
>> [5] https://openjdk.java.net/projects#project-reviewer
>>
>>

Re: RFR: 8311895: CSS Transitions [v20]

[Nir Lisker]

On Sun, 26 May 2024 08:08:26 GMT, Michael Strauß  wrote:

>> Implementation of [CSS 
>> Transitions](https://gist.github.com/mstr2/c72f8c9faa87de14926978f517a6018a).
>> 
>> ### Future enhancements
>> CSS transitions requires all participating objects to implement the 
>> `Interpolatable` interface. For example, targeting `-fx-background-color` 
>> only works if all background-related objects are interpolatable: `Color`, 
>> `BackgroundFill`, and `Background`.
>> 
>> In a follow-up PR, the following types will implement the `Interpolatable` 
>> interface:
>> `LinearGradient`, `RadialGradient`, `Stop`, `Background`, `BackgroundFill`, 
>> `BackgroundImage`, `BackgroundPosition`, `BackgroundSize`, 
>> `BackgroundStroke`, `BorderWidths`, `CornerRadii`, `Insets`.
>> 
>> ### Limitations
>> This implementation supports both shorthand and longhand notations for the 
>> `transition` property. However, due to limitations of JavaFX CSS, mixing 
>> both notations doesn't work:
>> 
>> .button {
>> transition: -fx-background-color 1s;
>> transition-easing-function: linear;
>> }
>> 
>> This issue should be addressed in a follow-up enhancement.
>
> Michael Strauß has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   documentation change

I reviewed only the API and some of the internal code. Didn't test it. Looks 
good.

I think the CSR needs updating.

-

Marked as reviewed by nlisker (Reviewer).

PR Review: https://git.openjdk.org/jfx/pull/870#pullrequestreview-2079580350
PR Comment: https://git.openjdk.org/jfx/pull/870#issuecomment-2132184809

Re: RFR: 8311895: CSS Transitions [v19]

[Nir Lisker]

On Sat, 25 May 2024 21:39:24 GMT, Michael Strauß  wrote:

>> Implementation of [CSS 
>> Transitions](https://gist.github.com/mstr2/c72f8c9faa87de14926978f517a6018a).
>> 
>> ### Future enhancements
>> CSS transitions requires all participating objects to implement the 
>> `Interpolatable` interface. For example, targeting `-fx-background-color` 
>> only works if all background-related objects are interpolatable: `Color`, 
>> `BackgroundFill`, and `Background`.
>> 
>> In a follow-up PR, the following types will implement the `Interpolatable` 
>> interface:
>> `LinearGradient`, `RadialGradient`, `Stop`, `Background`, `BackgroundFill`, 
>> `BackgroundImage`, `BackgroundPosition`, `BackgroundSize`, 
>> `BackgroundStroke`, `BorderWidths`, `CornerRadii`, `Insets`.
>> 
>> ### Limitations
>> This implementation supports both shorthand and longhand notations for the 
>> `transition` property. However, due to limitations of JavaFX CSS, mixing 
>> both notations doesn't work:
>> 
>> .button {
>> transition: -fx-background-color 1s;
>> transition-easing-function: linear;
>> }
>> 
>> This issue should be addressed in a follow-up enhancement.
>
> Michael Strauß has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   added documentation

modules/javafx.graphics/src/main/docs/javafx/scene/doc-files/cssref.html line 
749:

> 747: The property value is set programmatically
> 748: The property is bound
> 749: The node becomes invisible

I would mention that this relates to the `visible` property and not to the 
`opacity` one (the node is invisible if opacity is 0).

Other than that, looks good.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1614945779

Re: RFR: 8311895: CSS Transitions [v18]

[Nir Lisker]

On Sat, 25 May 2024 20:40:39 GMT, Michael Strauß  wrote:

>> Implementation of [CSS 
>> Transitions](https://gist.github.com/mstr2/c72f8c9faa87de14926978f517a6018a).
>> 
>> ### Future enhancements
>> CSS transitions requires all participating objects to implement the 
>> `Interpolatable` interface. For example, targeting `-fx-background-color` 
>> only works if all background-related objects are interpolatable: `Color`, 
>> `BackgroundFill`, and `Background`.
>> 
>> In a follow-up PR, the following types will implement the `Interpolatable` 
>> interface:
>> `LinearGradient`, `RadialGradient`, `Stop`, `Background`, `BackgroundFill`, 
>> `BackgroundImage`, `BackgroundPosition`, `BackgroundSize`, 
>> `BackgroundStroke`, `BorderWidths`, `CornerRadii`, `Insets`.
>> 
>> ### Limitations
>> This implementation supports both shorthand and longhand notations for the 
>> `transition` property. However, due to limitations of JavaFX CSS, mixing 
>> both notations doesn't work:
>> 
>> .button {
>> transition: -fx-background-color 1s;
>> transition-easing-function: linear;
>> }
>> 
>> This issue should be addressed in a follow-up enhancement.
>
> Michael Strauß has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   address review comments

This is what I would expect, so looks good. Where is this mentioned to the user?

-

PR Comment: https://git.openjdk.org/jfx/pull/870#issuecomment-2131460092

Re: RFR: 8311895: CSS Transitions [v17]

[Nir Lisker]

On Sat, 25 May 2024 21:04:24 GMT, Michael Strauß  wrote:

>> modules/javafx.graphics/src/main/docs/javafx/scene/doc-files/cssref.html 
>> line 690:
>> 
>>> 688: changed, it smoothly transitions to the new value over a 
>>> period of time. Implicit transitions are supported
>>> 689: for all primitive types, as well as for types that implement 
>>> javafx.animation.Interpolatable.
>>> 690: Transitions can be defined for any node in the JavaFX scene 
>>> graph with the following properties:
>> 
>> The way this is phrased makes it sound like the node has "the following 
>> properties", not the transition. Maybe move that part:
>> "Transitions with the following properties can be defined for any node in 
>> the JavaFX scene graph", or just add a comma.
>
> I understand that you're saying that `property`, `duration`, 
> `timing-function`, and `delay` are all sub-properties of `transition`.
> 
> However, from a CSS perspective, `transition-property`, 
> `transition-duration`, `transition-timing-function` and `transition-delay` 
> are properties of `Node`, in the same way as `-fx-background-color`, 
> `-fx-background-insets`, etc. are properties of `Node`.
> 
> `transition` is a short-hand property that combines all four properties into 
> one (we don't have a short-hand property for backgrounds yet). I think that 
> both mental models are basically correct (four properties of node, vs. four 
> sub-properties of transition).

I understand. I find it a bit confusing, but OK to leave as is.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1614892590

Re: RFR: 8311895: CSS Transitions [v17]

[Nir Lisker]

On Sat, 25 May 2024 20:37:44 GMT, Michael Strauß  wrote:

>> modules/javafx.graphics/src/main/java/javafx/animation/Interpolator.java 
>> line 277:
>> 
>>> 275:  * @since 23
>>> 276:  */
>>> 277: public enum StepPosition {
>> 
>> I think it would be helpful to include (or link to) images that show what 
>> the steps for each option looks like. The verbal description is a bit 
>> technical.
>
> I've included the images that are also used in the CSS reference 
> documentation. Now there are two copies of these images in two different 
> `doc-files` folders, but I guess that's okay.

I think it's fine. Another option is to link to the part of the reference where 
they are.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1614872766

Re: RFR: 8311895: CSS Transitions [v2]

[Nir Lisker]

On Mon, 31 Jul 2023 18:10:46 GMT, Michael Strauß  wrote:

>> modules/javafx.graphics/src/main/java/javafx/animation/Interpolator.java 
>> line 319:
>> 
>>> 317:  * The output time value is determined by the {@link StepPosition}.
>>> 318:  *
>>> 319:  * @param intervals the number of intervals in the step 
>>> interpolator
>> 
>> minor: When I see a plural like `intervals` (or `employees`) I think of a 
>> list of objects. Perhaps `intervalCount` would be better?
>
> Doesn't sound better to me, but I'll defer to what most people feel is best.

I somewhat agree with John. I would probably just use `numberOfIntervals`, but 
`intervalCount` of `numIntervals` is also fine.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1614560326

Re: RFR: 8311895: CSS Transitions [v17]

[Nir Lisker]

On Fri, 24 May 2024 11:18:35 GMT, Michael Strauß  wrote:

>> Implementation of [CSS 
>> Transitions](https://gist.github.com/mstr2/c72f8c9faa87de14926978f517a6018a).
>> 
>> ### Future enhancements
>> CSS transitions requires all participating objects to implement the 
>> `Interpolatable` interface. For example, targeting `-fx-background-color` 
>> only works if all background-related objects are interpolatable: `Color`, 
>> `BackgroundFill`, and `Background`.
>> 
>> In a follow-up PR, the following types will implement the `Interpolatable` 
>> interface:
>> `LinearGradient`, `RadialGradient`, `Stop`, `Background`, `BackgroundFill`, 
>> `BackgroundImage`, `BackgroundPosition`, `BackgroundSize`, 
>> `BackgroundStroke`, `BorderWidths`, `CornerRadii`, `Insets`.
>> 
>> ### Limitations
>> This implementation supports both shorthand and longhand notations for the 
>> `transition` property. However, due to limitations of JavaFX CSS, mixing 
>> both notations doesn't work:
>> 
>> .button {
>> transition: -fx-background-color 1s;
>> transition-easing-function: linear;
>> }
>> 
>> This issue should be addressed in a follow-up enhancement.
>
> Michael Strauß has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 57 commits:
> 
>  - Merge branch 'refs/heads/master' into feature/css-transitions
>  - extract magic string to named constant
>  - use existing property in test
>  - fixed documentation
>  - Merge branch 'master' into feature/css-transitions
>  - update 'since' tags
>  - Fix javadoc error
>  - Change javadoc comment
>  - Merge branch 'master' into feature/css-transitions
>  - Discard redundant transitions in StyleableProperty impls
>  - ... and 47 more: https://git.openjdk.org/jfx/compare/94aa2b68...a43dee30

I did a quick review of some of the code, mostly the API. I still don't know 
what happens when a value is programmatically set while a css transition is in 
progress. What I understood is that binding the property will not allow the 
transition to start/continue, but didn't see where setting a value was 
mentioned.

Otherwise looks good. I don't intend to review this beyond my current comments, 
so you can integrate once resolved.

modules/javafx.graphics/src/main/docs/javafx/scene/doc-files/cssref.html line 
688:

> 686: Transitions
> 687: JavaFX supports implicit transitions for properties that 
> are styled by CSS. When a property value is
> 688: changed, it smoothly transitions to the new value over a period 
> of time. Implicit transitions are supported

Maybe not so smoothly when using a step interpolator?

modules/javafx.graphics/src/main/docs/javafx/scene/doc-files/cssref.html line 
690:

> 688: changed, it smoothly transitions to the new value over a period 
> of time. Implicit transitions are supported
> 689: for all primitive types, as well as for types that implement 
> javafx.animation.Interpolatable.
> 690: Transitions can be defined for any node in the JavaFX scene graph 
> with the following properties:

The way this is phrased makes it sound like the node has "the following 
properties", not the transition. Maybe move that part:
"Transitions with the following properties can be defined for any node in the 
JavaFX scene graph", or just add a comma.

modules/javafx.graphics/src/main/java/com/sun/javafx/css/TransitionDefinition.java
 line 40:

> 38:  * @param duration duration of the transition
> 39:  * @param delay delay after which the transition is started; if negative, 
> the transition starts
> 40:  *  immediately, but will appear to have begun at an earlier 
> point in time

Why accept a negative delay? An 
[`Animation`](https://openjfx.io/javadoc/22/javafx.graphics/javafx/animation/Animation.html#getDelay())
 doesn't accept it.

modules/javafx.graphics/src/main/java/javafx/animation/Interpolator.java line 
277:

> 275:  * @since 23
> 276:  */
> 277: public enum StepPosition {

I think it would be helpful to include (or link to) images that show what the 
steps for each option looks like. The verbal description is a bit technical.

modules/javafx.graphics/src/main/java/javafx/animation/Interpolator.java line 
328:

> 326:  * @since 23
> 327:  */
> 328: public static Interpolator STEPS(int intervals, StepPosition 
> position) {

Static method names shouldn't be named like constants, although `Interpolator` 
does this for other methods already. Not sure if this trend should continue.

-

PR Review: https://git.openjdk.org/jfx/pull/870#pullrequestreview-2078912077
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1614833351
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1614836286
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1614815486
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1614554508
PR Review Comment: 

Re: RFR: 8322964: Optimize performance of CSS selector matching [v9]

[Nir Lisker]

On Mon, 11 Mar 2024 16:54:25 GMT, John Hendrikx  wrote:

>> Improves performance of selector matching in the CSS subsystem. This is done 
>> by using custom set implementation which are highly optimized for the most 
>> common cases where the number of selectors is small (most commonly 1 or 2). 
>> It also should be more memory efficient for medium sized and large 
>> applications which have many style names defined in various CSS files.
>> 
>> Due to the optimization, the concept of `StyleClass`, which was only 
>> introduced to assign a fixed bit index for each unique style class name 
>> encountered, is no longer needed. This is because style classes are no 
>> longer stored in a `BitSet` which required a fixed index per encountered 
>> style class.
>> 
>> The performance improvements are the result of several factors:
>> - Less memory use when only very few style class names are used in selectors 
>> and styles from a large pool of potential styles (a `BitSet` for potentially 
>> 1000 different style names needed 1000 bits (worst case)  as it was not 
>> sparse).
>> - Specialized sets for small number of elements (0, 1, 2, 3-9 and 10+)
>> - Specialized sets are append only (reduces code paths) and can be made read 
>> only without requiring a wrapper
>> - Iterator creation is avoided when doing `containsAll` check thanks to the 
>> inverse function `isSuperSetOf`
>> - Avoids making a copy of the list of style class names to compare (to 
>> convert them to `StyleClass` and put them into a `Set`) -- this copy could 
>> not be cached and was always discarded immediately after...
>> 
>> The overall performance was tested using the JFXCentral application which 
>> displays about 800 nodes on its start page with about 1000 styles in various 
>> style sheets (and which allows to refresh this page easily).  
>> 
>> On JavaFX 20, the fastest refresh speed was 121 ms on my machine.  With the 
>> improvements in this PR, the fastest refresh had become 89 ms.  The speed 
>> improvement is the result of a 30% faster `Scene#doCSSPass`, which takes up 
>> the bulk of the time to refresh the JFXCentral main page (about 100 ms 
>> before vs 70 ms after the change).
>
> John Hendrikx has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Move getStyleClassNames to location it was introduced to reduce diff

The code looks good. I didn't test it, but I'm fine with integrating.

-

PR Comment: https://git.openjdk.org/jfx/pull/1316#issuecomment-2129765316

Re: RFR: 8332313: Update code review guidelines [v4]

[Nir Lisker]

On Tue, 21 May 2024 22:32:21 GMT, Kevin Rushforth  wrote:

>> Update the code review guidelines for JavaFX.
>> 
>> The JavaFX 
>> [CONTRIBUTING](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/CONTRIBUTING.md)
>>  guidelines includes guidance for creating, reviewing, and integrating 
>> changes to JavaFX, along with a pointer to a [Code Review 
>> Policies](https://wiki.openjdk.org/display/OpenJFX/Code+Reviews) Wiki page.
>> 
>> This PR updates these guidelines to improve the quality of reviews, with a 
>> goal of improving JavaFX and decreasing the chance of introducing a serious 
>> regression or other critical bug.
>> 
>> The source branch has three commits:
>> 1. Converts the Code Review Policies Wiki page to a 
>> [README-code-reviews.md](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/README-code-reviews.md)
>>  file in the repo and updates hyperlinks to the new location.
>> 2. Update `README-code-reviews.md` with new guidelines
>> 3. Update `CONTRIBUTING.md` to highlight important requirements  (and minor 
>> changes to `README-code-reviews.md`)
>> 
>> Commit 1 is content neutral, so it might be helpful for reviewers to look at 
>> the changes starting from the second commit.
>> 
>> The updates are:
>> 
>> * In the Overview section, add a list of items for Reviewers, PR authors, 
>> and sponsoring Committers to verify prior to integration
>> * Create a "Guidelines for reviewing a PR" subsection of the Code review 
>> policies section
>> * Create a "Before you integrate or sponsor a PR" subsection of the Code 
>> review policies section
>> * Update the `CONTRIBUTING.md` page to highlight important requirements
>
> Kevin Rushforth has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Clarify need for CSR when API is modified or removed as well as added

Marked as reviewed by nlisker (Reviewer).

-

PR Review: https://git.openjdk.org/jfx/pull/1455#pullrequestreview-2069892902

Re: RFR: 8332313: Update code review guidelines [v2]

[Nir Lisker]

On Sat, 18 May 2024 14:24:49 GMT, Kevin Rushforth  wrote:

> Maybe it's worth changing "adds any new" to "adds, removes, or modifies any"?

This looks like a good idea.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1605798488

Re: RFR: 8332313: Update code review guidelines [v2]

[Nir Lisker]

On Fri, 17 May 2024 14:58:41 GMT, Nir Lisker  wrote:

>> Kevin Rushforth has updated the pull request with a new target base due to a 
>> merge or a rebase. The incremental webrev excludes the unrelated changes 
>> brought in by the merge/rebase. The pull request contains 20 additional 
>> commits since the last revision:
>> 
>>  - Wait for re-review if you've pushed a change addressing a substantive 
>> comment
>>  - Typo + remove sentence that invites reformatting PRs
>>  - Wording changes, added example of API addition
>>  - Formatting
>>  - Add item about checking the target branch
>>  - Remove trailing period from previous
>>  - Minor changes regarding syncing from upstream master
>>  - Clarify that GHA tests might fail for a reason unrelated to the PR
>>  - Fix typo
>>
>>"but It" --> "but it"
>>  - Remove bullet about checking the commit message (Skara already checks)
>>  - ... and 10 more: https://git.openjdk.org/jfx/compare/1b088e5b...9cf7f920
>
> README-code-reviews.md line 72:
> 
>> 70: * Focus first on substantive comments rather than stylistic comments
>> 71: * Check whether there is an automated test; if not, ask for one, if it 
>> is feasible
>> 72: * Make sure that the PR has executed the GitHub Actions (GHA) tests; if 
>> they aren't being run, ask the PR author to enable GHA workflows; if the 
>> test fails on some platforms, check whether it is a real bug (sometimes a 
>> job fails becau se of GHA infrastucture changes or we see a spurious GHA 
>> failure)
> 
> becau se -> because

Never mind, got simul-fixed.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1605160705

Re: RFR: 8332313: Update code review guidelines [v2]

[Nir Lisker]

On Fri, 17 May 2024 14:10:43 GMT, Kevin Rushforth  wrote:

>> Update the code review guidelines for JavaFX.
>> 
>> The JavaFX 
>> [CONTRIBUTING](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/CONTRIBUTING.md)
>>  guidelines includes guidance for creating, reviewing, and integrating 
>> changes to JavaFX, along with a pointer to a [Code Review 
>> Policies](https://wiki.openjdk.org/display/OpenJFX/Code+Reviews) Wiki page.
>> 
>> This PR updates these guidelines to improve the quality of reviews, with a 
>> goal of improving JavaFX and decreasing the chance of introducing a serious 
>> regression or other critical bug.
>> 
>> The source branch has three commits:
>> 1. Converts the Code Review Policies Wiki page to a 
>> [README-code-reviews.md](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/README-code-reviews.md)
>>  file in the repo and updates hyperlinks to the new location.
>> 2. Update `README-code-reviews.md` with new guidelines
>> 3. Update `CONTRIBUTING.md` to highlight important requirements  (and minor 
>> changes to `README-code-reviews.md`)
>> 
>> Commit 1 is content neutral, so it might be helpful for reviewers to look at 
>> the changes starting from the second commit.
>> 
>> The updates are:
>> 
>> * In the Overview section, add a list of items for Reviewers, PR authors, 
>> and sponsoring Committers to verify prior to integration
>> * Create a "Guidelines for reviewing a PR" subsection of the Code review 
>> policies section
>> * Create a "Before you integrate or sponsor a PR" subsection of the Code 
>> review policies section
>> * Update the `CONTRIBUTING.md` page to highlight important requirements
>
> Kevin Rushforth has updated the pull request with a new target base due to a 
> merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains 20 additional 
> commits since the last revision:
> 
>  - Wait for re-review if you've pushed a change addressing a substantive 
> comment
>  - Typo + remove sentence that invites reformatting PRs
>  - Wording changes, added example of API addition
>  - Formatting
>  - Add item about checking the target branch
>  - Remove trailing period from previous
>  - Minor changes regarding syncing from upstream master
>  - Clarify that GHA tests might fail for a reason unrelated to the PR
>  - Fix typo
>
>"but It" --> "but it"
>  - Remove bullet about checking the commit message (Skara already checks)
>  - ... and 10 more: https://git.openjdk.org/jfx/compare/1b088e5b...9cf7f920

README-code-reviews.md line 69:

> 67: * Carefully consider the risk of regression
> 68: * Carefully consider any compatibility concerns
> 69: * Check whether it adds any new public or protected API, even implicitly 
> (such as a public method that overrides a protected method, or a class that 
> is moved from a non-exported to an exported package); if it does, indicate 
> that it needs a CSR

I think that if it looks like an oversight (the author forgot about the default 
constructor), it's better to indicate that than to indicate that the PR needs a 
CSR. Maybe something like:
"if it does and it doesn't look like an oversight, indicate that it needs a CSR"

README-code-reviews.md line 72:

> 70: * Focus first on substantive comments rather than stylistic comments
> 71: * Check whether there is an automated test; if not, ask for one, if it is 
> feasible
> 72: * Make sure that the PR has executed the GitHub Actions (GHA) tests; if 
> they aren't being run, ask the PR author to enable GHA workflows; if the test 
> fails on some platforms, check whether it is a real bug (sometimes a job 
> fails becau se of GHA infrastucture changes or we see a spurious GHA failure)

becau se -> because

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1605152176
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1605147892

Re: RFR: 8332328: [GHA] GitHub Actions build fails on Linux: unable to find gcc-13

[Nir Lisker]

On Thu, 16 May 2024 15:42:05 GMT, Kevin Rushforth  wrote:

> It looks like the list of packages available in the Ubuntu 22.04 GitHub 
> Actions test runner no longer includes `gcc-13` as of yesterday. I didn't 
> find any documentation to indicate that this was intentional, but now that 
> Ubuntu 24.04 is available we can fix the problem by updating to that version. 
> We will need to do this at some point anyway, so we might as well do it now.
> 
> With this fix, the GHA test build now passes on all platforms. See the test 
> results to verify this.
> 
> Given that GHA builds are currently broken on Linux, and that this fix 
> doesn't affect the product at all (just our GHA tests), I will integrate this 
> as soon as it is approved by 1 Reviewer, as an exception to the usual 
> requirement to wait for 24 hours.

Note that the Ubuntu 24 runner is in beta: 
https://github.com/actions/runner-images#available-images.

-

PR Comment: https://git.openjdk.org/jfx/pull/1457#issuecomment-2115686652

Re: RFR: 8332313: Update code review guidelines

[Nir Lisker]

On Wed, 15 May 2024 20:08:10 GMT, John Hendrikx  wrote:

>> or is it?  :-)
>
> Isn't this automatic? Seems weird you could integrate without these passing.

> or is it? :-)

![image](https://github.com/openjdk/jfx/assets/37422899/8daab7cf-f050-4964-b8a6-731666422293)

Looks to me like it is...

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602201785

Re: RFR: 8332313: Update code review guidelines

[Nir Lisker]

On Wed, 15 May 2024 17:45:56 GMT, Kevin Rushforth  wrote:

> Update the code review guidelines for JavaFX.
> 
> The JavaFX 
> [CONTRIBUTING](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/CONTRIBUTING.md)
>  guidelines includes guidance for creating, reviewing, and integrating 
> changes to JavaFX, along with a pointer to a [Code Review 
> Policies](https://wiki.openjdk.org/display/OpenJFX/Code+Reviews) Wiki page.
> 
> This PR updates these guidelines to improve the quality of reviews, with a 
> goal of improving JavaFX and decreasing the chance of introducing a serious 
> regression or other critical bug.
> 
> The source branch has three commits:
> 1. Converts the Code Review Policies Wiki page to a 
> [README-code-reviews.md](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/README-code-reviews.md)
>  file in the repo and updates hyperlinks to the new location.
> 2. Update `README-code-reviews.md` with new guidelines
> 3. Update `CONTRIBUTING.md` to highlight important requirements  (and minor 
> changes to `README-code-reviews.md`)
> 
> Commit 1 is content neutral, so it might be helpful for reviewers to look at 
> the changes starting from the second commit.
> 
> The updates are:
> 
> * In the Overview section, add a list of items for Reviewers, PR authors, and 
> sponsoring Committers to verify prior to integration
> * Create a "Guidelines for reviewing a PR" subsection of the Code review 
> policies section
> * Create a "Before you integrate or sponsor a PR" subsection of the Code 
> review policies section
> * Update the `CONTRIBUTING.md` page to highlight important requirements

README-code-reviews.md line 48:

> 46: All code reviews must be done via a pull request submitted against this 
> GitHub repo, [openjdk/jfx](https://github.com/openjdk/jfx). A JBS bug ID must 
> exist before the pull request will be reviewed. See 
> [CONTRIBUTING.md](CONTRIBUTING.md) for information on how to submit a pull 
> request.
> 47: 
> 48: All fixes must be reviewed by at least one reviewer with the "Reviewer" 
> role (aka a "R"eviewer). We have a different code review threshold for 
> different types of changes. If there is disagreement as to whether a fix is 
> low-impact or high-impact, then it is considered high-impact. In other words 
> we will always err on the side of quality by "rounding up" to the next higher 
> category. The contributor can say whether they think something is low-impact 
> or high-impact, but It is up to a Reviewer to confirm this. A Reviewer either 
> adds a comment indicating that they think a single review is sufficient, or 
> else issues the Skara `/reviewers 2` command requesting a second reviewer (a 
> Reviewer can request more than 2 reviewers in some cases where a fix might be 
> especially risky or cut across multiple functional areas).

"but **It** is" -> it

README-code-reviews.md line 48:

> 46: All code reviews must be done via a pull request submitted against this 
> GitHub repo, [openjdk/jfx](https://github.com/openjdk/jfx). A JBS bug ID must 
> exist before the pull request will be reviewed. See 
> [CONTRIBUTING.md](CONTRIBUTING.md) for information on how to submit a pull 
> request.
> 47: 
> 48: All fixes must be reviewed by at least one reviewer with the "Reviewer" 
> role (aka a "R"eviewer). We have a different code review threshold for 
> different types of changes. If there is disagreement as to whether a fix is 
> low-impact or high-impact, then it is considered high-impact. In other words 
> we will always err on the side of quality by "rounding up" to the next higher 
> category. The contributor can say whether they think something is low-impact 
> or high-impact, but It is up to a Reviewer to confirm this. A Reviewer either 
> adds a comment indicating that they think a single review is sufficient, or 
> else issues the Skara `/reviewers 2` command requesting a second reviewer (a 
> Reviewer can request more than 2 reviewers in some cases where a fix might be 
> especially risky or cut across multiple functional areas).

About requesting reviews. I think that only some people can request reviews 
through GitHub, I never managed to do it on this repo, probably a matter of 
permissions. Might worth clarifying how this works.

README-code-reviews.md line 58:

> 56: * Determine whether this needs 2 reviewers and whether it needs a CSR; 
> issue the `/reviewers 2` or `/csr` command as needed
> 57: * If you want to indicate your approval, but still feel additional 
> reviewers are needed, you may increase the number of reviewers (e.g., from 2 
> to 3)
> 58: * If you want an area expert to review a PR, indicate this in a comment 
> of the form: `Reviewers: @PERSON1 @PERSON2`; the requested reviewers can 
> indicate whether or not they plan to review it

Should a list of experts per area be available somewhere? The Wiki has an old 
"code ownership" table that is out of date. Usually you get to know the experts 
only after they have reviewed 

Gradle support

[Nir Lisker]

Hello,

I see that dependency scanning is supported for Maven pom files, however,
many projects use Gradle as their dependency management tool. Has Gradle
support been considered?

Best,
Nir

Re: RFR: 8320563: Remove D3D9 code paths in favor of D3D9Ex [v3]

[Nir Lisker]

On Fri, 26 Apr 2024 07:53:59 GMT, Lukasz Kostyra  wrote:

>> JFX minimum requirements guarantee 9Ex availability, so old non-Ex paths are 
>> no longer needed.
>> 
>> In multiple parts (ex. Mesh, Graphics, etc.) where the Device is acquired I 
>> changed the type to explicitly use `IDirect3DDevice9Ex`. Technically it 
>> doesn't matter much (`IDirect3DDevice9Ex` inherits `IDirect3DDevice` - it 
>> was leveraged to transparently use the Ex device in the backend) but now we 
>> don't have the non-Ex device, so that keeps it a bit more consistent and 
>> clear IMO.
>> 
>> Verified by running tests on Windows 11, did not notice any regressions. 
>> Unfortunately I have no way to test this on older systems.
>
> Lukasz Kostyra has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Change pd3dEx to pd3d9

Marked as reviewed by nlisker (Reviewer).

-

PR Review: https://git.openjdk.org/jfx/pull/1445#pullrequestreview-2024451797

Re: RFR: 8320563: Remove D3D9 code paths in favor of D3D9Ex

[Nir Lisker]

On Tue, 23 Apr 2024 10:33:58 GMT, Lukasz Kostyra  wrote:

> JFX minimum requirements guarantee 9Ex availability, so old non-Ex paths are 
> no longer needed.
> 
> In multiple parts (ex. Mesh, Graphics, etc.) where the Device is acquired I 
> changed the type to explicitly use `IDirect3DDevice9Ex`. Technically it 
> doesn't matter much (`IDirect3DDevice9Ex` inherits `IDirect3DDevice` - it was 
> leveraged to transparently use the Ex device in the backend) but now we don't 
> have the non-Ex device, so that keeps it a bit more consistent and clear IMO.
> 
> Verified by running tests on Windows 11, did not notice any regressions. 
> Unfortunately I have no way to test this on older systems.

Tested the functionality with the 3DLighting app, things look the same as 
before the patch. Left a minor comment.

modules/javafx.graphics/src/main/native-prism-d3d/D3DPipeline.cc line 237:

> 235: }
> 236: 
> 237: int getMaxSampleSupport(IDirect3D9Ex *d3d9, UINT adapter) {

Minor: In some cases you also change the name of the variable to add the "Ex" 
suffix., like in

D3DContext::D3DContext(IDirect3D9Ex *pd3dEx, UINT adapter)
 ^

Here and In `PiplineManager.h` it's left as `IDirect3D9Ex * pd3d9;` without 
"Ex".
I don't mind it either way (I would probably not bother changing them myself), 
but perhaps we should remain consistent.

-

Marked as reviewed by nlisker (Reviewer).

PR Review: https://git.openjdk.org/jfx/pull/1445#pullrequestreview-2022879147
PR Review Comment: https://git.openjdk.org/jfx/pull/1445#discussion_r1579699815

Re: RFR: 8320563: Remove D3D9 code paths in favor of D3D9Ex

[Nir Lisker]

On Tue, 23 Apr 2024 10:33:58 GMT, Lukasz Kostyra  wrote:

> JFX minimum requirements guarantee 9Ex availability, so old non-Ex paths are 
> no longer needed.
> 
> In multiple parts (ex. Mesh, Graphics, etc.) where the Device is acquired I 
> changed the type to explicitly use `IDirect3DDevice9Ex`. Technically it 
> doesn't matter much (`IDirect3DDevice9Ex` inherits `IDirect3DDevice` - it was 
> leveraged to transparently use the Ex device in the backend) but now we don't 
> have the non-Ex device, so that keeps it a bit more consistent and clear IMO.
> 
> Verified by running tests on Windows 11, did not notice any regressions. 
> Unfortunately I have no way to test this on older systems.

Found the problem. That merge bumped the min JDK to 21, which I was using 
anyway, but it required recompiling the native D3D resources with the new JDK. 
The application works now.

-

PR Comment: https://git.openjdk.org/jfx/pull/1445#issuecomment-2077287504

Re: RFR: 8320563: Remove D3D9 code paths in favor of D3D9Ex

[Nir Lisker]

On Tue, 23 Apr 2024 10:33:58 GMT, Lukasz Kostyra  wrote:

> JFX minimum requirements guarantee 9Ex availability, so old non-Ex paths are 
> no longer needed.
> 
> In multiple parts (ex. Mesh, Graphics, etc.) where the Device is acquired I 
> changed the type to explicitly use `IDirect3DDevice9Ex`. Technically it 
> doesn't matter much (`IDirect3DDevice9Ex` inherits `IDirect3DDevice` - it was 
> leveraged to transparently use the Ex device in the backend) but now we don't 
> have the non-Ex device, so that keeps it a bit more consistent and clear IMO.
> 
> Verified by running tests on Windows 11, did not notice any regressions. 
> Unfortunately I have no way to test this on older systems.

I traced the issue to commit id 3914db26f3abb573ed0e320a361477e1d3e7a9ac, which 
is a merge Kevin did ~6 weeks ago. Placing the head on this commit or after 
causes the above error, but moving 1 commit back to "Create release notes for 
JavaFX 22" lets the application start normally.

I understand that this PR has nothing to do with this, I just can't test it 
considering this problem.

-

PR Comment: https://git.openjdk.org/jfx/pull/1445#issuecomment-2076940263

Re: RFR: 8320563: Remove D3D9 code paths in favor of D3D9Ex

[Nir Lisker]

On Tue, 23 Apr 2024 10:33:58 GMT, Lukasz Kostyra  wrote:

> JFX minimum requirements guarantee 9Ex availability, so old non-Ex paths are 
> no longer needed.
> 
> In multiple parts (ex. Mesh, Graphics, etc.) where the Device is acquired I 
> changed the type to explicitly use `IDirect3DDevice9Ex`. Technically it 
> doesn't matter much (`IDirect3DDevice9Ex` inherits `IDirect3DDevice` - it was 
> leveraged to transparently use the Ex device in the backend) but now we don't 
> have the non-Ex device, so that keeps it a bit more consistent and clear IMO.
> 
> Verified by running tests on Windows 11, did not notice any regressions. 
> Unfortunately I have no way to test this on older systems.

I tried to run the 3DLighting application and I'm getting an error. However, it 
looks like it's not this patch that is causing it since going back a few 
commits also gives this error. The jfx22 branch doesn't have this issue so it 
will take some investigation to find out what's going on. I'm on Win 10.


#
# A fatal error has been detected by the Java Runtime Environment:
#
#  EXCEPTION_ACCESS_VIOLATION (0xc005) at pc=0x01b44b9ad260, pid=6332, 
tid=18096
#
# JRE version: OpenJDK Runtime Environment (21.0.1+12) (build 21.0.1+12-29)
# Java VM: OpenJDK 64-Bit Server VM (21.0.1+12-29, mixed mode, sharing, tiered, 
compressed oops, compressed class ptrs, g1 gc, windows-amd64)
# Problematic frame:
# J 170 c2 java.lang.String.length()I java.base@21.0.1 (11 bytes) @ 
0x01b44b9ad260 [0x01b44b9ad220+0x0040]
#
# No core dump will be written. Minidumps are not enabled by default on client 
versions of Windows
#
# An error report file with more information is saved as:
# C:\Users\Nir\git\jfx\tests\performance\3DLighting\hs_err_pid6332.log
[5.830s][warning][os] Loading hsdis library failed
#
# If you would like to submit a bug report, please visit:
#   https://bugreport.java.com/bugreport/crash.jsp
#

-

PR Comment: https://git.openjdk.org/jfx/pull/1445#issuecomment-2076903265

Re: Wayland

[Nir Lisker]

Not sure it helps with warmup, but marking a foreign function as critical
can improve performance:
https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/foreign/Linker.Option.html#critical(boolean)
.

On Mon, Apr 22, 2024 at 10:02 PM Philip Race  wrote:

> No, it wasn't. I didn't even use jextracted code.
> The startup cost is around initialisation of FFM - around 70 ms (IIRC)
> overhead on my MacBook
> Then creation of VarHandles and MethodHandles - 2-5 ms each is what I
> measured, so do these lazily if you can.
> And warmup cost is that it takes about 1 iterations to get code fully
> compiled.
>
> java -XX:+PrintFlagsFinal -version 2>&1 | grep CompileThreshold
>  intx CompileThreshold =
> 1  {pd product} {default}
> double CompileThresholdScaling  =
> 1.00  {product} {default}
> uintx IncreaseFirstTierCompileThresholdAt  =
> 50{product} {default}
>  intx Tier2CompileThreshold=
> 0 {product} {default}
>  intx Tier3CompileThreshold=
> 2000  {product} {default}
>  intx Tier4CompileThreshold=
> 15000 {product} {default}
>
> -phil.
>
>
> On 4/22/24 11:45 AM, Thiago Milczarek Sayão wrote:
>
> I think the startup time might be related to all static symbol lookups.
> So I'm manually including just what is needed:
>
> jextract --output src -t com.sun.glass.wayland.extracted \
>   --header-class-name GlassWayland \
>   `pkg-config --libs glib-2.0 gio-2.0 libportal wayland-client` \
>   `pkg-config --cflags-only-I glib-2.0 gio-2.0 libportal wayland-client` \
>glass-wayland.h \
>--include-function xdp_portal_initable_new \
>--include-function xdp_session_close \
>--include-function xdp_portal_open_file \
>--include-function xdp_portal_open_file_finish \
>--include-function g_object_unref \
>--include-function g_timeout_add \
>--include-function g_add_idle \
>--include-function g_main_loop_run \
>--include-function g_main_loop_new \
>--include-function g_main_loop_ref \
>--include-function g_main_loop_unref \
>--include-function g_main_loop_quit \
>--include-function g_settings_new \
>--include-function g_settings_get_int \
>--include-function wl_display_connect \
>--include-function wl_display_disconnect \
>--include-function wl_display_roundtrip \
>--include-function wl_display_dispatch_pending \
>--include-typedef GAsyncReadyCallback \
>--include-typedef GSourceFunc \
>--include-typedef GError
>
>
> Em seg., 22 de abr. de 2024 às 13:24, Philip Race 
> escreveu:
>
>> As a reminder, using FFM will require all FX *applications* to specify
>> --enable-native-access on the command line
>> Although this is likely coming to JNI soon too.
>>
>> https://docs.oracle.com/en/java/javase/21/core/restricted-methods.html
>>
>> But one thing to watch out for with FFM is startup + warm up time.
>> I struggled a lot with that in using FFM for just one library in the
>> java.desktop module.
>>
>> -phil
>>
>> On 4/22/24 9:12 AM, Nir Lisker wrote:
>>
>> Sorry, we bumped to Java 21 in JavaFX 22 I think since we preserve the
>> N-1 rule.
>>
>> On Mon, Apr 22, 2024 at 6:03 PM Nir Lisker  wrote:
>>
>>> I think that we'll be able to bump to Java 25 in JavaFX 25, like we did
>>> with 21. I suggested initially to bump to Java 22 exactly for FFM as it's
>>> very useful for JavaFX, but was told we shouldn't since it's not an LTS
>>> version.
>>>
>>> I have no idea how long the work on Wayland will take including the code
>>> review (a rather long process), but you should be able to request code
>>> reviews with FFM and have it ready for integration by Java 25.
>>>
>>> On Mon, Apr 22, 2024 at 5:49 PM Thiago Milczarek Sayão <
>>> thiago.sa...@gmail.com> wrote:
>>>
>>>> I was just experimenting, but it seems to be less work than going with
>>>> JNI.
>>>> If I am correct, the next Java LTS will be 25, which will be required
>>>> on JavaFX 29 to be released on September/29.
>>>>
>>>> It's 7 years - that's really too much.
>>>>
>>>> Maybe it's still worthwhile to prototype using FFM and then port
>>>> everything to JNI.
>>>>
>>>> -- Thiago.
>&g

Re: Wayland

[Nir Lisker]

Sorry, we bumped to Java 21 in JavaFX 22 I think since we preserve the N-1
rule.

On Mon, Apr 22, 2024 at 6:03 PM Nir Lisker  wrote:

> I think that we'll be able to bump to Java 25 in JavaFX 25, like we did
> with 21. I suggested initially to bump to Java 22 exactly for FFM as it's
> very useful for JavaFX, but was told we shouldn't since it's not an LTS
> version.
>
> I have no idea how long the work on Wayland will take including the code
> review (a rather long process), but you should be able to request code
> reviews with FFM and have it ready for integration by Java 25.
>
> On Mon, Apr 22, 2024 at 5:49 PM Thiago Milczarek Sayão <
> thiago.sa...@gmail.com> wrote:
>
>> I was just experimenting, but it seems to be less work than going with
>> JNI.
>> If I am correct, the next Java LTS will be 25, which will be required on
>> JavaFX 29 to be released on September/29.
>>
>> It's 7 years - that's really too much.
>>
>> Maybe it's still worthwhile to prototype using FFM and then port
>> everything to JNI.
>>
>> -- Thiago.
>>
>>
>> Em seg., 22 de abr. de 2024 às 11:21, Kevin Rushforth <
>> kevin.rushfo...@oracle.com> escreveu:
>>
>>> Note also that we cannot use Panama in the JavaFX internals yet, since
>>> the minimum version of the JDK is 21.
>>>
>>> -- Kevin
>>>
>>>
>>> On 4/21/2024 10:51 AM, Thiago Milczarek Sayão wrote:
>>> > Hi,
>>> >
>>> > I did a small test app to explore Wayland client and portals (for
>>> > Robot and dialogs such as file open/save).
>>> >
>>> > https://github.com/tsayao/wayland-test/blob/main/wayland-test.c
>>> >
>>> > It seems it will work as a glass backend, but some walls will be hit
>>> > on the way :)
>>> >
>>> > I have tried to use jextract (from project Panama) to work directly
>>> > with java, but it seems it does not support wl_ types.
>>> >
>>> > -- Thiago.
>>>
>>>

Re: Wayland

[Nir Lisker]

I think that we'll be able to bump to Java 25 in JavaFX 25, like we did
with 21. I suggested initially to bump to Java 22 exactly for FFM as it's
very useful for JavaFX, but was told we shouldn't since it's not an LTS
version.

I have no idea how long the work on Wayland will take including the code
review (a rather long process), but you should be able to request code
reviews with FFM and have it ready for integration by Java 25.

On Mon, Apr 22, 2024 at 5:49 PM Thiago Milczarek Sayão <
thiago.sa...@gmail.com> wrote:

> I was just experimenting, but it seems to be less work than going with JNI.
> If I am correct, the next Java LTS will be 25, which will be required on
> JavaFX 29 to be released on September/29.
>
> It's 7 years - that's really too much.
>
> Maybe it's still worthwhile to prototype using FFM and then port
> everything to JNI.
>
> -- Thiago.
>
>
> Em seg., 22 de abr. de 2024 às 11:21, Kevin Rushforth <
> kevin.rushfo...@oracle.com> escreveu:
>
>> Note also that we cannot use Panama in the JavaFX internals yet, since
>> the minimum version of the JDK is 21.
>>
>> -- Kevin
>>
>>
>> On 4/21/2024 10:51 AM, Thiago Milczarek Sayão wrote:
>> > Hi,
>> >
>> > I did a small test app to explore Wayland client and portals (for
>> > Robot and dialogs such as file open/save).
>> >
>> > https://github.com/tsayao/wayland-test/blob/main/wayland-test.c
>> >
>> > It seems it will work as a glass backend, but some walls will be hit
>> > on the way :)
>> >
>> > I have tried to use jextract (from project Panama) to work directly
>> > with java, but it seems it does not support wl_ types.
>> >
>> > -- Thiago.
>>
>>

Re: Wayland

[Nir Lisker]

Ah, yes, opaque types are indeed unsupported:
https://github.com/openjdk/jextract/blob/master/doc/GUIDE.md#unsupported-features.
As Jorn said there, if the API exposes methods that use opaque types, then
you wouldn't have a problem. Also, if you have the .c file where they are
defined, jextract can handle them. It could be a bit of a hack though.

I wrote a jextract GUI wrapper with JavaFX, which I tested only on Windows
for now. I will try to get the Linux and Mac versions up soon as well. I
find it very helpful compared to the command line and I think it could help
you with the complex headers there.

Note that jextract generates Java 22 compatible code, which is unusable in
JavaFX for now (we comply with Java 21).

On Mon, Apr 22, 2024 at 1:36 AM Thiago Milczarek Sayão <
thiago.sa...@gmail.com> wrote:

> I mailed the jextract list and Jorn Vernee explained that wayland use
> opaque types, which are just defined as such:
>
> struct wl_registry;
>
> I guess you just pass it around and it's defined in the internal .c file.
> Those are not supported by jextract.
>
> I'll find a way to get around it.
>
> But I've been playing with it all day, it seems very good. I was able to
> generate bindings for:
>
> GMain - for the main loop;
> GSettings - for reading settings;
> XDG Portal - for screen capture, screenshot, file dialogs
>
> It looks like this:
>
> 1) To get a setting
>
> try(var a = Arena.ofConfined()) {
> var mouseSettings = 
> g_settings_new(a.allocateUtf8String("org.gnome.desktop.interface"));
> int size = g_settings_get_int(mouseSettings, 
> a.allocateUtf8String("cursor-size"));
> g_object_unref(mouseSettings);
> return new Size(size, size);
> }
>
>
> 2) Callbacks
>
> @Override
> protected void _invokeLater(Runnable runnable) {
> MemorySegment call = GSourceFunc.allocate(p -> {
> runnable.run();
> return 0;
> }, Arena.ofAuto());
>
> g_idle_add(call, MemorySegment.NULL);
> }
>
>
> It looks correct to me, but untested.
>
> -- Thiago.
>
> Em dom., 21 de abr. de 2024 às 18:15, Nir Lisker 
> escreveu:
>
>> Can you link to where all the headers are? I found some in
>> https://gitlab.freedesktop.org/wayland/wayland/-/tree/main/src, but I
>> couldn't see where wl_registry is defined. From what I see, wl_XYZ types
>> are structs, which are supported.
>>
>> By the way, there's a new guide for jextract at
>> https://github.com/openjdk/jextract/blob/master/doc/GUIDE.md. When
>> working with complex headers (includes upon includes), it's important to
>> specify the correct target header.
>>
>> On Sun, Apr 21, 2024 at 11:35 PM Thiago Milczarek Sayão <
>> thiago.sa...@gmail.com> wrote:
>>
>>> jextract --output src -t org.freedesktop.wayland.client
>>> --header-class-name WlClient `pkg-config --cflags-only-I wayland-client`
>>> `pkg-config --libs wayland-client`  /usr/include/wayland-client.h
>>>
>>> WARNING: Skipping wl_registry (type Declared(wl_registry) is not
>>> supported)
>>>
>>> I would need this to hook the events with wl_registry_add_listener, but
>>> currently the code generation for this is not working.
>>>
>>>
>>>
>>>
>>>
>>> Em dom., 21 de abr. de 2024 às 16:39, Nir Lisker 
>>> escreveu:
>>>
>>>> What are wl_ types? jextract only supports c headers.
>>>>
>>>> On Sun, Apr 21, 2024 at 10:31 PM Thiago Milczarek Sayão <
>>>> thiago.sa...@gmail.com> wrote:
>>>>
>>>>> Hi,
>>>>>
>>>>> I did a small test app to explore Wayland client and portals (for
>>>>> Robot and dialogs such as file open/save).
>>>>>
>>>>> https://github.com/tsayao/wayland-test/blob/main/wayland-test.c
>>>>>
>>>>> It seems it will work as a glass backend, but some walls will be hit
>>>>> on the way :)
>>>>>
>>>>> I have tried to use jextract (from project Panama) to work directly
>>>>> with java, but it seems it does not support wl_ types.
>>>>>
>>>>> -- Thiago.
>>>>>
>>>>

Re: Wayland

[Nir Lisker]

Can you link to where all the headers are? I found some in
https://gitlab.freedesktop.org/wayland/wayland/-/tree/main/src, but I
couldn't see where wl_registry is defined. From what I see, wl_XYZ types
are structs, which are supported.

By the way, there's a new guide for jextract at
https://github.com/openjdk/jextract/blob/master/doc/GUIDE.md. When working
with complex headers (includes upon includes), it's important to specify
the correct target header.

On Sun, Apr 21, 2024 at 11:35 PM Thiago Milczarek Sayão <
thiago.sa...@gmail.com> wrote:

> jextract --output src -t org.freedesktop.wayland.client
> --header-class-name WlClient `pkg-config --cflags-only-I wayland-client`
> `pkg-config --libs wayland-client`  /usr/include/wayland-client.h
>
> WARNING: Skipping wl_registry (type Declared(wl_registry) is not supported)
>
> I would need this to hook the events with wl_registry_add_listener, but
> currently the code generation for this is not working.
>
>
>
>
>
> Em dom., 21 de abr. de 2024 às 16:39, Nir Lisker 
> escreveu:
>
>> What are wl_ types? jextract only supports c headers.
>>
>> On Sun, Apr 21, 2024 at 10:31 PM Thiago Milczarek Sayão <
>> thiago.sa...@gmail.com> wrote:
>>
>>> Hi,
>>>
>>> I did a small test app to explore Wayland client and portals (for Robot
>>> and dialogs such as file open/save).
>>>
>>> https://github.com/tsayao/wayland-test/blob/main/wayland-test.c
>>>
>>> It seems it will work as a glass backend, but some walls will be hit on
>>> the way :)
>>>
>>> I have tried to use jextract (from project Panama) to work directly with
>>> java, but it seems it does not support wl_ types.
>>>
>>> -- Thiago.
>>>
>>

Re: Wayland

[Nir Lisker]

What are wl_ types? jextract only supports c headers.

On Sun, Apr 21, 2024 at 10:31 PM Thiago Milczarek Sayão <
thiago.sa...@gmail.com> wrote:

> Hi,
>
> I did a small test app to explore Wayland client and portals (for Robot
> and dialogs such as file open/save).
>
> https://github.com/tsayao/wayland-test/blob/main/wayland-test.c
>
> It seems it will work as a glass backend, but some walls will be hit on
> the way :)
>
> I have tried to use jextract (from project Panama) to work directly with
> java, but it seems it does not support wl_ types.
>
> -- Thiago.
>

Re: Possible leak on setOnAction

[Nir Lisker]

I didn't find such memory leaks in my application, though I don't do stage
handling. What I would look at is where the `stage` reference in the lambda
is coming from. You say you have a list of open stages. When you close a
stage, do you remove the references to that stage from all places? What
about the object that is holding the reference `stage` in the lambda?

On Thu, Apr 18, 2024 at 1:58 PM Thiago Milczarek Sayão <
thiago.sa...@gmail.com> wrote:

> Hi,
>
> I'm pretty sure setOnAction is holding references.
>
> I have a "Open Windows" menu on my application where it lists the Stages
> opened and if you click, it calls stage.toFront():
>
> menuItem.seOnAction(e -> stage.toFront())
>
> I had many crash reports, all OOM. I got the hprof files and analyzed them
> - turns out this was holding references to all closed stages.
>
> To fix it, I call setOnAction(null) when the stage is closed.
>
> I will investigate further and provide an example.
>
> -- Thiago.
>

Re: How to configure the default connection for virsh

[Nir Soffer]

On Thu, Apr 18, 2024 at 5:20 PM Daniel P. Berrangé 
wrote:

> On Thu, Apr 18, 2024 at 05:07:24PM +0300, Nir Soffer wrote:
> > We are using minikube vms, which require adding the user to to libvirt
> > group[1].
> > We use `virsh -c qemu:///system` for debugging these vms or related
> libvirt
> > networks.
> >
> > Using virsh without specifying the '-c' argument is a common mistake that
> > leads to
> > trouble, for example creating the libvirt default network incorrectly.
> >
> > I wonder if it is possible to configure virsh so it uses -c
> qemu:///system
> > by default?
>
> The $HOME/.config/libvirt/libvirt.conf client config file can contain
>
>uri_default = "qemu:///system"
>
> Or you can set per shell with
>
>export LIBVIRT_DEFAULT_URI=qemu:///system
>
> Or you can set it just for virsh with
>
>export VIRSH_DEFAULT_CONNECT_URI=qemu:///system
>
> See:
>
>   https://libvirt.org/uri.html#default-uri-choice
>   https://libvirt.org/manpages/virsh.html#environment
>
>
Awesome, thanks!
___
Users mailing list -- users@lists.libvirt.org
To unsubscribe send an email to users-le...@lists.libvirt.org

How to configure the default connection for virsh

[Nir Soffer]

We are using minikube vms, which require adding the user to to libvirt
group[1].
We use `virsh -c qemu:///system` for debugging these vms or related libvirt
networks.

Using virsh without specifying the '-c' argument is a common mistake that
leads to
trouble, for example creating the libvirt default network incorrectly.

I wonder if it is possible to configure virsh so it uses -c qemu:///system
by default?

We know that this is not great, but we don't know a better way to use
minikube
with the kvm2 driver. The minikube vms are started for creating a
development
environment and during CI, and they cannot require a password or any
complicated
setup.

[1] https://github.com/kubernetes/minikube/issues/3467

Nir
___
Users mailing list -- users@lists.libvirt.org
To unsubscribe send an email to users-le...@lists.libvirt.org

Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v3]

[Nir Lisker]

On Thu, 28 Mar 2024 22:21:32 GMT, drmarmac  wrote:

>> modules/javafx.controls/src/main/java/javafx/scene/control/ControlUtils.java 
>> line 166:
>> 
>>> 164: sm.startAtomic();
>>> 165: 
>>> 166: final List removed = new 
>>> ArrayList<>(c.getRemovedSize());
>> 
>> I wonder if we should add a check for 0 size here to bypass all this code 
>> and unnecessary object allocations if nothing is removed (same for added)
>
> We certainly could, or maybe use wasRemoved(), but I doubt there will be much 
> impact. I guess it's preferred to keep changes unrelated to the issue 
> minimal, so I'd leave it as it is if everyone's ok with that.

These short circuiting operations tend to be worth it only if it's a critical 
path. The GC will collect the allocations very efficiently these days. I didn't 
check what this code segment is used for, but unless it's looped somewhere, I 
doubt there will by any change.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1430#discussion_r1543793491

Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v2]

[Nir Lisker]

On Wed, 27 Mar 2024 23:18:43 GMT, Marius Hanl  wrote:

>> modules/javafx.controls/src/main/java/javafx/scene/control/MultipleSelectionModelBase.java
>>  line 773:
>> 
>>> 771: .collect(Collectors.toList());
>>> 772: 
>>> 773: sortedNewIndices.forEach(this::set);
>> 
>> Why do the double-iteration pattern here and not do the `peek` operation in 
>> a `forEach` like in the other 2 places?
>
> `forEach` is void, so we can not return a list afterwards.

You don't need to return a list, you create it ahead of time like was done in 
line 167

List indices = new ArrayList<>();

and the add the elements in `forEach`.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1430#discussion_r1542145635

Re: RFR: 8327179: Update the 3D lighting application [v5]

[Nir Lisker]

On Wed, 27 Mar 2024 20:38:58 GMT, Nir Lisker  wrote:

>> Update for the 3D lighting test tool as described in the JBS issue.
>
> Nir Lisker has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Added gradle script

Looks like the line specified by jcheck was off.

-

PR Comment: https://git.openjdk.org/jfx/pull/1387#issuecomment-2024008625

Re: RFR: 8327179: Update the 3D lighting application [v6]

[Nir Lisker]

> Update for the 3D lighting test tool as described in the JBS issue.

Nir Lisker has updated the pull request incrementally with one additional 
commit since the last revision:

  Whitespace?

-

Changes:
  - all: https://git.openjdk.org/jfx/pull/1387/files
  - new: https://git.openjdk.org/jfx/pull/1387/files/f063d088..ae471f2e

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jfx=1387=05
 - incr: https://webrevs.openjdk.org/?repo=jfx=1387=04-05

  Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod
  Patch: https://git.openjdk.org/jfx/pull/1387.diff
  Fetch: git fetch https://git.openjdk.org/jfx.git pull/1387/head:pull/1387

PR: https://git.openjdk.org/jfx/pull/1387

Re: RFR: 8327179: Update the 3D lighting application [v5]

[Nir Lisker]

On Wed, 27 Mar 2024 20:38:58 GMT, Nir Lisker  wrote:

>> Update for the 3D lighting test tool as described in the JBS issue.
>
> Nir Lisker has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Added gradle script

I added the application to the gradle build file so it can be hooked up with 
its module dependencies. You can now produce a jar that bundles the resource 
with the classes. You still need to point to the modules at runtime.
Let me know if this is a good enough solution.

The way the project is configured in the main build file is not the best, but 
it will require large changes to do it better.

Not sure what jcheck wants. It's complaining about empty spaces that have a `}` 
appear after them. This is the error: 
https://github.com/openjdk/jfx/pull/1387/files#annotation_19798318258.

-

PR Comment: https://git.openjdk.org/jfx/pull/1387#issuecomment-2023946848
PR Comment: https://git.openjdk.org/jfx/pull/1387#issuecomment-2023948839

Re: RFR: 8327179: Update the 3D lighting application [v5]

[Nir Lisker]

> Update for the 3D lighting test tool as described in the JBS issue.

Nir Lisker has updated the pull request incrementally with one additional 
commit since the last revision:

  Added gradle script

-

Changes:
  - all: https://git.openjdk.org/jfx/pull/1387/files
  - new: https://git.openjdk.org/jfx/pull/1387/files/9b3f0c5e..f063d088

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jfx=1387=04
 - incr: https://webrevs.openjdk.org/?repo=jfx=1387=03-04

  Stats: 26 lines in 1 file changed: 26 ins; 0 del; 0 mod
  Patch: https://git.openjdk.org/jfx/pull/1387.diff
  Fetch: git fetch https://git.openjdk.org/jfx.git pull/1387/head:pull/1387

PR: https://git.openjdk.org/jfx/pull/1387

Re: RFR: 8328746: Remove unused imports in demo apps

[Nir Lisker]

On Thu, 21 Mar 2024 21:50:37 GMT, Andy Goryachev  wrote:

> Using Eclipse IDE to remove unused imports in **demo apps** (3D, Ensemble, 
> etc.) and update the copyright year to 2024. Using wildcard for more than 10 
> static imports.
> 
> 
> --
> 
> This is a trivial change (though fairly large), 1 reviewer is probably enough.

I find it helpful to separate the imports by their high-level domain name, 
"java.", "javafx.", "com." etc. It makes it easier to see the "span" or 
requirements of the class.

-

PR Comment: https://git.openjdk.org/jfx/pull/1420#issuecomment-2022856424

Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v2]

[Nir Lisker]

On Wed, 27 Mar 2024 09:11:56 GMT, drmarmac  wrote:

>> This PR removes potentially incorrect usages of Stream.peek().
>> The changed code should be covered by the tests that are already present.
>
> drmarmac has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Remove outdated comment

modules/javafx.controls/src/main/java/javafx/scene/control/MultipleSelectionModelBase.java
 line 773:

> 771: .collect(Collectors.toList());
> 772: 
> 773: sortedNewIndices.forEach(this::set);

Why do the double-iteration pattern here and not do the `peek` operation in a 
`forEach` like in the other 2 places?

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1430#discussion_r1541157874

Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v2]

[Nir Lisker]

On Wed, 27 Mar 2024 12:23:47 GMT, Marius Hanl  wrote:

>> I'd say .forEach() is used correctly here, according to docs, it guarantees 
>> execution of the side-effects (add to removed list & clear index), just not 
>> in any particular order. This way we avoid multiple iteration.
>
> Another idea is to use `toList()`, which is a very efficient operation and 
> then iterate over it.

> In the java.util.stream package 
> [docs](https://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/util/stream/package-summary.html#SideEffects)
>  it is mentioned that `forEach()` method operates only via side-effects. So 
> do you think we should avoid using `forEach()` here and iterate the generated 
> list separately to clear selected index?

`forEach` is used correctly here. From the docs:
> With the exception of terminal operations 
> [forEach](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/util/stream/Stream.html#forEach(java.util.function.Consumer))
>  and 
> [forEachOrdered](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/util/stream/Stream.html#forEachOrdered(java.util.function.Consumer)),
>  side-effects of behavioral parameters may not always be executed when the 
> stream implementation can optimize away the execution of behavioral 
> parameters without affecting the result of the computation.

> Another idea is to use `toList()`, which is a very efficient operation and 
> then iterate over it.

That's still 2 iterations. If the code is not performance-critical it doesn't 
matter.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1430#discussion_r1541140317

Re: RFR: 8327179: Update the 3D lighting application [v4]

[Nir Lisker]

On Wed, 13 Mar 2024 22:32:59 GMT, Nir Lisker  wrote:

>> Update for the 3D lighting test tool as described in the JBS issue.
>
> Nir Lisker has updated the pull request incrementally with five additional 
> commits since the last revision:
> 
>  - Added spacing
>  - Renamed constant
>  - Updated benchmark reset message
>  - Simplified models creation
>  - Revert resource package

I can't reproduce the exception. The path to the image is correct. Both the 
class and the image are under the `attenuation` package. Here is my launch 
command:


C:\Program Files\Java\jdk-21\bin\javaw.exe 
"-Djava.library.path=C:\Users\Nir\git\jfx\modules\javafx.graphics\build\module-lib"
--add-modules=javafx.controls,javafx.swing
-Dfile.encoding=UTF-8
-Dstdout.encoding=UTF-8
-Dstderr.encoding=UTF-8
-p 
"C:\Users\Nir\git\jfx\modules\javafx.controls\bin;C:\Users\Nir\git\jfx\modules\javafx.graphics\bin;C:\Users\Nir\git\jfx\modules\javafx.base\bin;C:\Users\Nir\git\jfx\modules\javafx.swing\bin"
-classpath "C:\Users\Nir\git\jfx\tests\performance\3DLighting\bin"
-XX:+ShowCodeDetailsInExceptionMessages
--add-exports javafx.graphics/test.com.sun.javafx.pgstub=javafx.controls
--add-exports javafx.base/test.com.sun.javafx.binding=javafx.controls
--add-exports javafx.base/test.util.memory=javafx.controls
--add-exports javafx.base/test.javafx.collections=javafx.controls
--add-exports javafx.base/com.sun.javafx.property=javafx.graphics
--add-exports javafx.base/test.javafx.collections=javafx.graphics
--add-exports javafx.base/test.util.memory=javafx.graphics
--add-exports java.base/sun.security.util=javafx.graphics
--add-reads javafx.base=java.management
--add-reads javafx.base=jdk.management attenuation.LightingApplication


Perhaps it's a platform-dependent path issue?

-

PR Comment: https://git.openjdk.org/jfx/pull/1387#issuecomment-2018183992

Re: RFR: 8328718: Remove unused imports in javafx.controls

[Nir Lisker]

On Thu, 21 Mar 2024 18:47:24 GMT, Andy Goryachev  wrote:

> Using Eclipse IDE to remove unused imports in javafx.controls and update the 
> copyright year to 2024.  Using wildcard for more than 10 static imports.
> 
> This is a trivial change, 1 reviewer is probably enough.

I can, but it will take me some time to get to it.

-

PR Comment: https://git.openjdk.org/jfx/pull/1416#issuecomment-2015727802

[ovirt-users] Re: virt-v2v paused by system after one hour or a bit more

[Nir Soffer]

On Thu, Mar 21, 2024 at 7:03 PM Cyril VINH-TUNG  wrote:

> Hello
>
> Here's the technique we use :
> - create manually the vm on ovirt with same disks (same size that original
> but you can choose target type, thin provision or preallocated)
> - on any node, force activating the disks to make them writable at the os
> level (lvm, vgchange...)
> - if the disk type is the same on target and destination, you can use dd
> over netcat to copy the disks
> - if the type is not the same, you might use qemu-img convert over netcat
>

This is very fragile and dangerous, you must really know what you are doing
:-)

If you already imported the disks from the other system, uploading them to
any storage domain
in any wanted (and supported) image format and allocation policy is much
easier and faster
using ovirt-img.

See https://www.ovirt.org/media/ovirt-img-v8.pdf


>
> If you have physical access to the node, you might use a flat backup
>
> Another workaround is to backup/restore the vm with a backup tool that
> works both with vmware and ovirt... I would say vprotect or vinchin
>

This should also work, but when importing vms from vmware, it is not enough
to copy the disk data a is,
you want to modify it to remove vmware bits and add the ovirt bits -
virt-v2v does all this for you.

Nir
___
Users mailing list -- users@ovirt.org
To unsubscribe send an email to users-le...@ovirt.org
Privacy Statement: https://www.ovirt.org/privacy-policy.html
oVirt Code of Conduct: 
https://www.ovirt.org/community/about/community-guidelines/
List Archives: 
https://lists.ovirt.org/archives/list/users@ovirt.org/message/6MASYRHUZPJCRX45G3TO73OURT5LFWCB/

[ovirt-users] Re: virt-v2v paused by system after one hour or a bit more

[Nir Soffer]

On Thu, Mar 21, 2024 at 12:44 PM Claus Serbe via Users 
wrote:

> Hi,
>
> I am migrating some vmware VM's from an NFS Storage via rhv-upload in
> virt-v2v, what is working good.
>
> But now I try to move some bigger VM's with several disks and sadly after
> a while (I would guess around an hour) the Ovirt-engine shows me "Paused by
> system" instead of transfering, so when the next disk should be imported,
> it will fail
>
> In the ovirt-engine.log I see the following lines for the remaining 4
> disks.
>
> 2024-03-21 06:14:06,815-04 ERROR
> [org.ovirt.engine.core.dal.dbbroker.auditloghandling.AuditLogDirector]
> (EE-ManagedScheduledExecutorService-engineScheduledThreadPool-Thread-35)
> [f61b3906-804d-470f-8524-6507081fbdec] EVENT_ID:
> UPLOAD_IMAGE_PAUSED_BY_SYSTEM_TIMEOUT(1,071), Upload was paused by system.
> Reason: timeout due to transfer inactivity.
> 2024-03-21 06:14:17,915-04 ERROR
> [org.ovirt.engine.core.dal.dbbroker.auditloghandling.AuditLogDirector]
> (EE-ManagedScheduledExecutorService-engineScheduledThreadPool-Thread-14)
> [aef8e312-d811-4a39-b5fb-342157209bce] EVENT_ID:
> UPLOAD_IMAGE_PAUSED_BY_SYSTEM_TIMEOUT(1,071), Upload was paused by system.
> Reason: timeout due to transfer inactivity.
> 2024-03-21 06:14:24,959-04 ERROR
> [org.ovirt.engine.core.dal.dbbroker.auditloghandling.AuditLogDirector]
> (EE-ManagedScheduledExecutorService-engineScheduledThreadPool-Thread-85)
> [860b012d-78a4-49f8-a875-52f4299c8298] EVENT_ID:
> UPLOAD_IMAGE_PAUSED_BY_SYSTEM_TIMEOUT(1,071), Upload was paused by system.
> Reason: timeout due to transfer inactivity.
> 2024-03-21 06:14:46,099-04 ERROR
> [org.ovirt.engine.core.dal.dbbroker.auditloghandling.AuditLogDirector]
> (EE-ManagedScheduledExecutorService-engineScheduledThreadPool-Thread-65)
> [f93869ee-2ecb-4f54-b3e9-b12259637b0b] EVENT_ID:
> UPLOAD_IMAGE_PAUSED_BY_SYSTEM_TIMEOUT(1,071), Upload was paused by system.
> Reason: timeout due to transfer inactivity.
>
>
> There are 2 strange things.
>
> 1. When I start virt-v2v it will create all 6 disks and set them to
> transferring, but virt-v2v will import one after the other, what leads to
> kind of unused/timing out transferring tickets.
>

This is a incorrect usage of ovirt-imageio API in virt-v2v, please report
it here:
https://github.com/libguestfs/virt-v2v/issues

The right way to use the API is:
1. Start transfer
2. Upload the data
3. End the transfer

It does not matter if you create all the disk at the start of the
operation, but starting a transfer must be done
right before you upload the data.


> 2. When I copy the disk images to a local disk before, it works. Maybe
> just because of faster transfer speeds.
>
> Is there a possibility to transfer parallel or maybe extend the timeout?
>

Sure you can upload in parallel, but I'm not sure virt-v2v will probably
have issues importing in parallel from other
systems (e.g. vmware would not allow this).

We tested uploading 10 100g images in parallel using the ovirt-img tool,
each upload using 4 connections
(total of 40 upload connections).

You may be able to extend the timeout, but this is not recommended since
your system will not clean up quickly
after a bad client disconnects uncleanly without ending the transfer.
Unfortunately I don't remember the which
timeout should be modified on the engine side, maybe Arik or Albert can
help with this.

Nir
___
Users mailing list -- users@ovirt.org
To unsubscribe send an email to users-le...@ovirt.org
Privacy Statement: https://www.ovirt.org/privacy-policy.html
oVirt Code of Conduct: 
https://www.ovirt.org/community/about/community-guidelines/
List Archives: 
https://lists.ovirt.org/archives/list/users@ovirt.org/message/2TUOTBUYA6GEZYJK3W4EC3PSDJLYXHK3/

[ovirt-users] Re: Create Vm without Storage Domain

[Nir Soffer]

On Wed, Mar 20, 2024 at 12:06 PM Shafi Mohammed 
wrote:

> Hi Guys ,
>
> Thanks for the Info .
>
> I am trying to migrate my data from local to Storage domain . But it
> requires twice the effort in terms of space and time to copy the data to
> the local file system and then upload the disk to it .
>
> I created a storage domain and tried to expose it to the NBD server to
> write the data from the source Vm disk . But I'm facing an nbd
> permission issue  . Even a copy or write is restricted .
>
> Actual Command
> qemu-nbd -f qcow2 /rhev/data-center/mnt/192.168.108.27:
> _storage/d62b04f8-973f-4168-9e69-1f334a4968b6/images/cd6b9fc4-ef8a-4f40-ac8d-f18d355223d0/d2a57e6f-029e-46cc-85f8-ce151d027dcb
>
>
>
> *qemu-nbd: Failed to blk_new_open
> '/rhev/data-center/mnt/192.168.108.27:_storage/d62b04f8-973f-4168-9e69-1f334a4968b6/images/cd6b9fc4-ef8a-4f40-ac8d-f18d355223d0/d2a57e6f-029e-46cc-85f8-ce151d027dcb':
> Could not open
> '/rhev/data-center/mnt/192.168.108.27:_storage/d62b04f8-973f-4168-9e69-1f334a4968b6/images/cd6b9fc4-ef8a-4f40-ac8d-f18d355223d0/d2a57e6f-029e-46cc-85f8-ce151d027dcb':
> Permission denied*
> Please suggest me if Ovirt has any API  to open a disk from a
> storage domain and write data to it
>

You are trying to reinvent ovirt-img. Try it:
https://www.ovirt.org/media/ovirt-img-v8.pdf
___
Users mailing list -- users@ovirt.org
To unsubscribe send an email to users-le...@ovirt.org
Privacy Statement: https://www.ovirt.org/privacy-policy.html
oVirt Code of Conduct: 
https://www.ovirt.org/community/about/community-guidelines/
List Archives: 
https://lists.ovirt.org/archives/list/users@ovirt.org/message/BK2RAMNASXIYOBKXSXG7JLN7ZPL2SI4R/

Re: RFR: 8325566: [TestBug] Util.shutdown() to hide ALL Windows [v2]

[Nir Lisker]

On Tue, 19 Mar 2024 22:28:54 GMT, Andy Goryachev  wrote:

>> tests/system/src/test/java/test/util/Util.java line 383:
>> 
>>> 381: runAndWait(() -> {
>>> 382: for (Window w : new ArrayList<>(Window.getWindows())) {
>>> 383: w.hide();
>> 
>> I think you can use `List.copyOf` instead of an `ArrayList` since it doesn't 
>> get modified (it's a one-time copy).
>> The iteration can also be
>> 
>> List.copyOf(Window.getWindows()).forEach(Window::hide);
>> 
>> if you want.
>
> The only issue with this style is - it's hard to set breakpoints.
> In this case it's not that important.
> Thank you!

It's a matter of style mostly, so I don't really mind it. The debug issue can 
be remedied by adding breakpoints inside Lambdas, which all IDEs support I 
think (Eclipse and IntelliJ for sure).

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1407#discussion_r1531207452

Re: RFR: 8325566: [TestBug] Util.shutdown() to hide ALL Windows

[Nir Lisker]

On Mon, 18 Mar 2024 22:53:03 GMT, Andy Goryachev  wrote:

> Changing `Util.shutdown()` to hide **all** showing Windows and then call 
> `Platform.exit()`.
> This simplifies the tests and ensures a clean shutdown.

tests/system/src/test/java/test/util/Util.java line 383:

> 381: runAndWait(() -> {
> 382: for (Window w : new ArrayList<>(Window.getWindows())) {
> 383: w.hide();

I think you can use `List.copyOf` instead of an `ArrayList` since it doesn't 
get modified (it's a one-time copy).
The iteration can also be

List.copyOf(Window.getWindows()).forEach(Window::hide);

if you want.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1407#discussion_r1531195323

Re: RFR: 8327179: Update the 3D lighting application

[Nir Lisker]

On Tue, 5 Mar 2024 16:44:00 GMT, Ambarish Rapte  wrote:

> Executing the LightingSample manually fails with an exception

I think the package names of the main and resources weren't aligned.

> The copyright year new files should be 2024 only. Some new files have more 
> than one copyright year. If you are moving any existing files then please use 
> git mv so that the files are shown as moved.

For now, for the ease of reviewing, I reverted the package name change.

> The effect of lighting on mesh vs a box is quite different.

I think this is the same issue you submitted in the past: 
https://bugs.openjdk.org/browse/JDK-8269133

-

PR Comment: https://git.openjdk.org/jfx/pull/1387#issuecomment-1996005413

Re: RFR: 8327179: Update the 3D lighting application [v4]

[Nir Lisker]

> Update for the 3D lighting test tool as described in the JBS issue.

Nir Lisker has updated the pull request incrementally with five additional 
commits since the last revision:

 - Added spacing
 - Renamed constant
 - Updated benchmark reset message
 - Simplified models creation
 - Revert resource package

-

Changes:
  - all: https://git.openjdk.org/jfx/pull/1387/files
  - new: https://git.openjdk.org/jfx/pull/1387/files/3f2994ff..9b3f0c5e

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jfx=1387=03
 - incr: https://webrevs.openjdk.org/?repo=jfx=1387=02-03

  Stats: 48 lines in 6 files changed: 9 ins; 5 del; 34 mod
  Patch: https://git.openjdk.org/jfx/pull/1387.diff
  Fetch: git fetch https://git.openjdk.org/jfx.git pull/1387/head:pull/1387

PR: https://git.openjdk.org/jfx/pull/1387

Re: RFR: 8327179: Update the 3D lighting application [v3]

[Nir Lisker]

On Tue, 5 Mar 2024 16:49:28 GMT, Ambarish Rapte  wrote:

>> Nir Lisker has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   copyright headers
>
> tests/performance/3DLighting/src/main/java/lighting3D/Benchmark.java line 49:
> 
>> 47: stopGraphic.setBoundsType(TextBoundsType.VISUAL);
>> 48: stopGraphic.setFill(Color.RED);
>> 49: stopGraphic.setFont(Font.font(20));
> 
> Play and Stop button could be same size. It would be good to look at. 
> Currently the Stop button seems lesser than half size of play button.

As you will see, the font sizes and visual sizes don't render as you would 
expect. Not sure why, but this way they look better. Perhaps small adjustments 
are in order, but if you match the sizes it comes out off.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1387#discussion_r1523953194

Re: RFR: 8327179: Update the 3D lighting application [v3]

[Nir Lisker]

> Update for the 3D lighting test tool as described in the JBS issue.

Nir Lisker has updated the pull request incrementally with one additional 
commit since the last revision:

  copyright headers

-

Changes:
  - all: https://git.openjdk.org/jfx/pull/1387/files
  - new: https://git.openjdk.org/jfx/pull/1387/files/2646489d..3f2994ff

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jfx=1387=02
 - incr: https://webrevs.openjdk.org/?repo=jfx=1387=01-02

  Stats: 80 lines in 8 files changed: 75 ins; 0 del; 5 mod
  Patch: https://git.openjdk.org/jfx/pull/1387.diff
  Fetch: git fetch https://git.openjdk.org/jfx.git pull/1387/head:pull/1387

PR: https://git.openjdk.org/jfx/pull/1387

Re: RFR: 8327179: Update the 3D lighting application [v2]

[Nir Lisker]

> Update for the 3D lighting test tool as described in the JBS issue.

Nir Lisker has updated the pull request incrementally with one additional 
commit since the last revision:

  Undo files move

-

Changes:
  - all: https://git.openjdk.org/jfx/pull/1387/files
  - new: https://git.openjdk.org/jfx/pull/1387/files/7f0e562f..2646489d

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jfx=1387=01
 - incr: https://webrevs.openjdk.org/?repo=jfx=1387=00-01

  Stats: 12 lines in 8 files changed: 2 ins; 2 del; 8 mod
  Patch: https://git.openjdk.org/jfx/pull/1387.diff
  Fetch: git fetch https://git.openjdk.org/jfx.git pull/1387/head:pull/1387

PR: https://git.openjdk.org/jfx/pull/1387

Re: [Acme] Happy Birthday ACME!

[Yoav Nir]

Hi, Rob

The first question whenever someone proposes a bis document is, of course, “are 
you volunteering to edit?”

Jokes aside, it’s always a question of whether or not it is worth the effort. 
Not just for whoever is editing, but the usual effort associated with any 
document, such as WG participants, shepherd, AD, IESG, various directorates, 
RFC editor.

So before embarking on something like this, it’s not enough to just count the 
number of errata. You need to put yourself in the shoes of a naive implementer 
who doesn’t know about errata. Are the errors big enough that they might lead 
them to make a mistake in the implementation?  Text that uses “example.com 
” instead of “example.org ” probably 
isn’t. Text that says that a challenge object with an error cannot have the 
“processing” status when it can likely is.

As for adding other RFCs, that’s again a judgement call. We did merge some 
add-ons to IKEv2 in one of its revisions. It make sense to merge them if the 
add-on is so obvious and so necessary, that pretty much every implementation of 
8555 would also implement the other document. Is RFC8738 like that? Or are IP 
identifiers so rare and curious that many implementations exclude them?

As always, it’s up to the group whether making a significantly bigger document 
with some of the add-ons makes sense. In general, groups and ADs tend to prefer 
smaller documents, but that is decided on a case-by-case basis.

Yoav

> On 11 Mar 2024, at 23:08, Rob Stradling  
> wrote:
> 
> RFC8555 was published [1] 5 years ago today!
> 
> Just thinking aloud, 'cos I'm curious what folks here think...
> At what point might it make sense to start work on an 8555-bis?
> 
> There's a fairly long list of Errata [2]: 10 Verified, 5 Reported, and 4 Held 
> for Document Update.
> 
> Would it make sense for an 8555-bis document to incorporate and obsolete any 
> of the "add-on" RFCs / I-Ds, such as RFC8738, that have been published since 
> RFC8555?  Or, conversely, would it be preferable to not do that?
> 
> With 5 years of deployment experience behind us, have any "missing" features 
> in RFC8555 been identified that would be best addressed by updating the core 
> specification (i.e., in an 8555-bis document) rather than by writing new 
> "add-on" I-Ds?  Or, conversely, are "add-on" I-Ds always the preferred 
> approach?  (The "missing" feature that immediately springs to my mind is 
> "profiles" [3]).
> 
> 
> [1] 
> https://mailarchive.ietf.org/arch/msg/rfc-dist/25pD6Za_dVkXMbJwyPhBJR6nIlo/
> [2] https://www.rfc-editor.org/errata_search.php?rfc=8555
> [3] https://mailarchive.ietf.org/arch/msg/acme/BLVAayrTrUCegT4s2twci3Q2BY8/
> 
> --
> Rob Stradling
> Senior Research & Development Scientist
> Sectigo Limited
> 
> ___
> Acme mailing list
> Acme@ietf.org 
> https://www.ietf.org/mailman/listinfo/acme

___
Acme mailing list
Acme@ietf.org
https://www.ietf.org/mailman/listinfo/acme

Re: RFR: JDK-8322964 Optimize performance of CSS selector matching [v6]

[Nir Lisker]

On Sat, 9 Mar 2024 06:48:25 GMT, John Hendrikx  wrote:

>> Improves performance of selector matching in the CSS subsystem. This is done 
>> by using custom set implementation which are highly optimized for the most 
>> common cases where the number of selectors is small (most commonly 1 or 2). 
>> It also should be more memory efficient for medium sized and large 
>> applications which have many style names defined in various CSS files.
>> 
>> Due to the optimization, the concept of `StyleClass`, which was only 
>> introduced to assign a fixed bit index for each unique style class name 
>> encountered, is no longer needed. This is because style classes are no 
>> longer stored in a `BitSet` which required a fixed index per encountered 
>> style class.
>> 
>> The performance improvements are the result of several factors:
>> - Less memory use when only very few style class names are used in selectors 
>> and styles from a large pool of potential styles (a `BitSet` for potentially 
>> 1000 different style names needed 1000 bits (worst case)  as it was not 
>> sparse).
>> - Specialized sets for small number of elements (0, 1, 2, 3-9 and 10+)
>> - Specialized sets are append only (reduces code paths) and can be made read 
>> only without requiring a wrapper
>> - Iterator creation is avoided when doing `containsAll` check thanks to the 
>> inverse function `isSuperSetOf`
>> - Avoids making a copy of the list of style class names to compare (to 
>> convert them to `StyleClass` and put them into a `Set`) -- this copy could 
>> not be cached and was always discarded immediately after...
>> 
>> The overall performance was tested using the JFXCentral application which 
>> displays about 800 nodes on its start page with about 1000 styles in various 
>> style sheets (and which allows to refresh this page easily).  
>> 
>> On JavaFX 20, the fastest refresh speed was 121 ms on my machine.  With the 
>> improvements in this PR, the fastest refresh had become 89 ms.  The speed 
>> improvement is the result of a 30% faster `Scene#doCSSPass`, which takes up 
>> the bulk of the time to refresh the JFXCentral main page (about 100 ms 
>> before vs 70 ms after the change).
>
> John Hendrikx has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Remove commented code in BitSetTest
>  - Remove unnecessary addAll overrides

modules/javafx.graphics/src/main/java/javafx/css/StyleClass.java line 32:

> 30:  * @deprecated for removal
> 31:  */
> 32: public final class StyleClass {

Needs the `@Deprecated(forRemoval = true)` annotation on the class, and the doc 
tag needs an explanation of why it's being removed ("erroneously exposed", "no 
longer needed", "flawed from the start"...) and what should be used instead, if 
at all.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1316#discussion_r1518566872

Re: [Acme] ACME leadership changes

[Yoav Nir]

Hi, Tomofumi.

Welcome to the working group.

Looking forward to working with you.

Yoav

> On 7 Mar 2024, at 23:48, Roman Danyliw  wrote:
> 
> Hi WG!
> 
> As Deb (Cooley) will be the new, incoming SEC AD at the Brisbane meeting, 
> there is a vacancy left in the co-chair team of ACME.  It is my pleasure to 
> announced that Tomofumi Okubo will be stepping in as the new-chair.  Tomofumi 
> brings a wealth of experience in PKI and as a contributor in PKI-related IETF 
> WGs.
> 
> Tomofumi: Thank you for your willingness to serve.
> 
> Deb: Congratulations on your new AD role!  Thank you for your leadership in 
> the WG.
> 
> Yoav (Nir): Despite these transitions, thank you for your continued service 
> as co-chair in the WG!
> 
> Deb isn't going too far from ACME.  After the AD transition in Brisbane, the 
> responsible AD for ACME will transition from me to her.
> 
> Regards,
> Roman
> 
> 
> ___
> Acme mailing list
> Acme@ietf.org
> https://www.ietf.org/mailman/listinfo/acme

___
Acme mailing list
Acme@ietf.org
https://www.ietf.org/mailman/listinfo/acme

Re: RFR: 8092102: Labeled: truncated property

[Nir Lisker]

On Mon, 4 Mar 2024 21:04:28 GMT, Andy Goryachev  wrote:

> Adds Labeled.truncated property which indicates when the text is visually 
> truncated (and the ellipsis string is inserted) in order to fit the available 
> width.
> 
> The new property reacts to changes in the following properties:
> - ellipsisString
> - font
> - text
> - width
> - wrapText
> 
> For some reason, line 859 generates a javadoc "co comment" warning, despite 
> the javadoc comment present at the property declaration in line 832.
> 
> I don't think it's worth creating a headful test (headless won't work) due to 
> relative simplicity of the code.
> 
> **Alternative**
> 
> The desired functionality can be just as easily achieved on an application 
> level, by adding a similar property to a subclass.  What is the benefit of 
> adding this functionality to the core?

This enhancement should be discussed in the mailing list. I've never needed 
this myself, but it already had an old JBS ticket, so I can see that others did 
and for a legitimate reason (adding tooltips). I don't mind if it's added or 
not.

modules/javafx.controls/src/main/java/javafx/scene/control/Labeled.java line 
823:

> 821: /**
> 822:  * Truncated read-only property indicates whether the text has been 
> truncated
> 823:  * when it cannot fit into the available width.

No need to repeat the name and type in the first sentence as it's used in the 
summary table. Most phrasings opt (inconsistently) for: "Indicates whether..." 
or just "Whether..." (I prefer the former).

I'm also not sure about the sole role of the width. Can the text not be 
truncated for a reason other than not fitting in the width, like not fitting in 
the height?

modules/javafx.controls/src/main/java/javafx/scene/control/Labeled.java line 
830:

> 828:  *
> 829:  * @since 23
> 830:  * @defaultValue false

I don't think that a default value is applicable here since it's completely 
dependent on other properties and can't be set. It's possible to call the `get` 
of this property for the first time and get `true`. The value here is the value 
used in the get method:

return property == null ? DEFAULT_VALUE : property.get();

at least in all the cases I've seen.

modules/javafx.controls/src/main/java/javafx/scene/control/Labeled.java line 
832:

> 830:  * @defaultValue false
> 831:  */
> 832: private ObservableBooleanValue truncated;

The property name should probably be `textTruncated` to be in line with 
`textOverrun`, `textFill`, `wrapText`...

Otherwise it could look like the labeled is truncated.

modules/javafx.controls/src/main/java/javafx/scene/control/Labeled.java line 
834:

> 832: private ObservableBooleanValue truncated;
> 833: 
> 834: public final ObservableBooleanValue truncatedProperty() {

`ObservableBooleanValue` is not a property, so it's odd to use it as the return 
type of one. Usually `ReadOnlyBooleanProperty` is used. This implementation 
through binding is also unique for read-only properties. A look in `Node` and 
`Labeled` show a different way of implementing read-only properties.

modules/javafx.controls/src/main/java/javafx/scene/control/Labeled.java line 
850:

> 848: protected boolean computeValue() {
> 849: if (isWrapText()) {
> 850: return false;

Are you sure that allowing text to wrap necessarily means it won't be 
truncated? What if the max height doesn't allow another line?

-

PR Review: https://git.openjdk.org/jfx/pull/1389#pullrequestreview-1917119989
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1512963900
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1512912954
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1512968169
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1512956759
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1512898960

RFR: 8327179: Update the 3D lighting application

[Nir Lisker]

Update for the 3D lighting test tool as described in the JBS issue.

-

Commit messages:
 - Whitespaces
 - Initial commit

Changes: https://git.openjdk.org/jfx/pull/1387/files
 Webrev: https://webrevs.openjdk.org/?repo=jfx=1387=00
  Issue: https://bugs.openjdk.org/browse/JDK-8327179
  Stats: 1733 lines in 15 files changed: 1021 ins; 708 del; 4 mod
  Patch: https://git.openjdk.org/jfx/pull/1387.diff
  Fetch: git fetch https://git.openjdk.org/jfx.git pull/1387/head:pull/1387

PR: https://git.openjdk.org/jfx/pull/1387

Re: Validation Support

[Nir Lisker]

Hi Dirk,

JavaFX has some input validation support in terms of focus control and
rejecting invalid characters. Can you  explain what your proposal adds? Are
there JBS issues asking for this?

Thanks,
Nir

On Fri, Mar 1, 2024, 13:50 Dirk Lemmermann  wrote:

> Hi everyone,
>
> I updated the validation framework ValidatorFX today in our project to the
> latest release and I really like it a lot. It is a small compact API and
> works with any observable as opposed to the validation support provided by
> ControlsFX.
>
> Using it made me wonder whether it would make sense to bundle it or
> something like it directly with JavaFX. Developers often mention missing
> validation support as a drawback of using JavaFX. Adding this would take
> one item off from the list of arguments against using JavaFX.
>
> Many UI frameworks do have built-in validation support, e.g. Vaadin [0],
> Angular, [1], or QT [2]
>
> What do you think?
>
> —Dirk
>
> [0]
> https://vaadin.com/docs/latest/binding-data/components-binder-validation
> [1] https://angular.io/guide/form-validation
> [2] https://doc.qt.io/qt-6/qtquick-input-textinput.html
>
>

Re: RFR: 8314147: Updated the PhongMaterial documentation [v10]

[Nir Lisker]

On Wed, 28 Feb 2024 18:48:21 GMT, Nir Lisker  wrote:

>> Overhaul to the `PhongMaterial` documentation (and a bit to its superclass 
>> `Material`). Except for the introduction, I divided the documentation into 3 
>> sections: qualitative explanation, mathematical model (I wouldn't think it 
>> necessary, but the current doc explains it), and examples.
>> 
>> The reason for the verbosity of the doc is that I envisioned 2 target 
>> audiences for this class. One is a Java developer who wants to understand 
>> the terminology and workings of computer graphics or of the artists who are 
>> already familiar with this domain. (How many Java developers know what 
>> diffuse, specular and normal maps are?) The other is an artist who is 
>> already familiar with the domain, but wants to see how this class compares 
>> with other renderers. For this reason, I looked at the terminology used by 
>> engines like Blender, Maya, UE4 and Unity and tried to mention the 
>> comparisons (like bump vs. height vs. normal maps, or specular vs. 
>> roughness/smoothness).
>> 
>> The examples I chose and some of the schematics are not the best, looking at 
>> it retroactively, but I want to give enough time for reviewers and get this 
>> into 22.
>
> Nir Lisker has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Resize specular color images

Thanks everyone for the quick and detailed review!

-

PR Comment: https://git.openjdk.org/jfx/pull/1378#issuecomment-1971049164

[jfx22] Integrated: 8314147: Updated the PhongMaterial documentation

[Nir Lisker]

On Thu, 29 Feb 2024 11:46:11 GMT, Nir Lisker  wrote:

> This pull request contains a backport of commit 
> [d9645730](https://github.com/openjdk/jfx/commit/d9645730f1e76e95e0bb93ceaeb5550390bf95c1)
>  from the [openjdk/jfx](https://git.openjdk.org/jfx) repository.

This pull request has now been integrated.

Changeset: 3b925780
Author:Nir Lisker 
URL:   
https://git.openjdk.org/jfx/commit/3b925780ab87c4987bd35a0d5dbcb66e8a2a5b31
Stats: 521 lines in 39 files changed: 461 ins; 13 del; 47 mod

8314147: Updated the PhongMaterial documentation

Reviewed-by: kcr
Backport-of: d9645730f1e76e95e0bb93ceaeb5550390bf95c1

-

PR: https://git.openjdk.org/jfx/pull/1385

[jfx22] RFR: 8314147: Updated the PhongMaterial documentation

[Nir Lisker]

This pull request contains a backport of commit 
[d9645730](https://github.com/openjdk/jfx/commit/d9645730f1e76e95e0bb93ceaeb5550390bf95c1)
 from the [openjdk/jfx](https://git.openjdk.org/jfx) repository.

-

Commit messages:
 - Backport d9645730f1e76e95e0bb93ceaeb5550390bf95c1

Changes: https://git.openjdk.org/jfx/pull/1385/files
 Webrev: https://webrevs.openjdk.org/?repo=jfx=1385=00
  Issue: https://bugs.openjdk.org/browse/JDK-8314147
  Stats: 521 lines in 39 files changed: 461 ins; 13 del; 47 mod
  Patch: https://git.openjdk.org/jfx/pull/1385.diff
  Fetch: git fetch https://git.openjdk.org/jfx.git pull/1385/head:pull/1385

PR: https://git.openjdk.org/jfx/pull/1385

Integrated: 8314147: Updated the PhongMaterial documentation

[Nir Lisker]

On Thu, 22 Feb 2024 20:38:00 GMT, Nir Lisker  wrote:

> Overhaul to the `PhongMaterial` documentation (and a bit to its superclass 
> `Material`). Except for the introduction, I divided the documentation into 3 
> sections: qualitative explanation, mathematical model (I wouldn't think it 
> necessary, but the current doc explains it), and examples.
> 
> The reason for the verbosity of the doc is that I envisioned 2 target 
> audiences for this class. One is a Java developer who wants to understand the 
> terminology and workings of computer graphics or of the artists who are 
> already familiar with this domain. (How many Java developers know what 
> diffuse, specular and normal maps are?) The other is an artist who is already 
> familiar with the domain, but wants to see how this class compares with other 
> renderers. For this reason, I looked at the terminology used by engines like 
> Blender, Maya, UE4 and Unity and tried to mention the comparisons (like bump 
> vs. height vs. normal maps, or specular vs. roughness/smoothness).
> 
> The examples I chose and some of the schematics are not the best, looking at 
> it retroactively, but I want to give enough time for reviewers and get this 
> into 22.

This pull request has now been integrated.

Changeset: d9645730
Author:Nir Lisker 
URL:   
https://git.openjdk.org/jfx/commit/d9645730f1e76e95e0bb93ceaeb5550390bf95c1
Stats: 521 lines in 39 files changed: 461 ins; 13 del; 47 mod

8314147: Updated the PhongMaterial documentation

Reviewed-by: arapte, kcr

-

PR: https://git.openjdk.org/jfx/pull/1378

Re: RFR: 8314147: Updated the PhongMaterial documentation [v10]

[Nir Lisker]

> Overhaul to the `PhongMaterial` documentation (and a bit to its superclass 
> `Material`). Except for the introduction, I divided the documentation into 3 
> sections: qualitative explanation, mathematical model (I wouldn't think it 
> necessary, but the current doc explains it), and examples.
> 
> The reason for the verbosity of the doc is that I envisioned 2 target 
> audiences for this class. One is a Java developer who wants to understand the 
> terminology and workings of computer graphics or of the artists who are 
> already familiar with this domain. (How many Java developers know what 
> diffuse, specular and normal maps are?) The other is an artist who is already 
> familiar with the domain, but wants to see how this class compares with other 
> renderers. For this reason, I looked at the terminology used by engines like 
> Blender, Maya, UE4 and Unity and tried to mention the comparisons (like bump 
> vs. height vs. normal maps, or specular vs. roughness/smoothness).
> 
> The examples I chose and some of the schematics are not the best, looking at 
> it retroactively, but I want to give enough time for reviewers and get this 
> into 22.

Nir Lisker has updated the pull request incrementally with one additional 
commit since the last revision:

  Resize specular color images

-

Changes:
  - all: https://git.openjdk.org/jfx/pull/1378/files
  - new: https://git.openjdk.org/jfx/pull/1378/files/e5466476..60b41ca7

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jfx=1378=09
 - incr: https://webrevs.openjdk.org/?repo=jfx=1378=08-09

  Stats: 0 lines in 5 files changed: 0 ins; 0 del; 0 mod
  Patch: https://git.openjdk.org/jfx/pull/1378.diff
  Fetch: git fetch https://git.openjdk.org/jfx.git pull/1378/head:pull/1378

PR: https://git.openjdk.org/jfx/pull/1378

Re: RFR: 8314147: Updated the PhongMaterial documentation [v9]

[Nir Lisker]

On Wed, 28 Feb 2024 18:24:30 GMT, Nir Lisker  wrote:

>> Overhaul to the `PhongMaterial` documentation (and a bit to its superclass 
>> `Material`). Except for the introduction, I divided the documentation into 3 
>> sections: qualitative explanation, mathematical model (I wouldn't think it 
>> necessary, but the current doc explains it), and examples.
>> 
>> The reason for the verbosity of the doc is that I envisioned 2 target 
>> audiences for this class. One is a Java developer who wants to understand 
>> the terminology and workings of computer graphics or of the artists who are 
>> already familiar with this domain. (How many Java developers know what 
>> diffuse, specular and normal maps are?) The other is an artist who is 
>> already familiar with the domain, but wants to see how this class compares 
>> with other renderers. For this reason, I looked at the terminology used by 
>> engines like Blender, Maya, UE4 and Unity and tried to mention the 
>> comparisons (like bump vs. height vs. normal maps, or specular vs. 
>> roughness/smoothness).
>> 
>> The examples I chose and some of the schematics are not the best, looking at 
>> it retroactively, but I want to give enough time for reviewers and get this 
>> into 22.
>
> Nir Lisker has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Rename and resize images

I think I got all the images' sizes reduced.

Except for a few small points in discussion with Ambarish, I think that this is 
ready to go in in time for 22.

-

PR Comment: https://git.openjdk.org/jfx/pull/1378#issuecomment-1969586936

Re: RFR: 8314147: Updated the PhongMaterial documentation [v9]

[Nir Lisker]

> Overhaul to the `PhongMaterial` documentation (and a bit to its superclass 
> `Material`). Except for the introduction, I divided the documentation into 3 
> sections: qualitative explanation, mathematical model (I wouldn't think it 
> necessary, but the current doc explains it), and examples.
> 
> The reason for the verbosity of the doc is that I envisioned 2 target 
> audiences for this class. One is a Java developer who wants to understand the 
> terminology and workings of computer graphics or of the artists who are 
> already familiar with this domain. (How many Java developers know what 
> diffuse, specular and normal maps are?) The other is an artist who is already 
> familiar with the domain, but wants to see how this class compares with other 
> renderers. For this reason, I looked at the terminology used by engines like 
> Blender, Maya, UE4 and Unity and tried to mention the comparisons (like bump 
> vs. height vs. normal maps, or specular vs. roughness/smoothness).
> 
> The examples I chose and some of the schematics are not the best, looking at 
> it retroactively, but I want to give enough time for reviewers and get this 
> into 22.

Nir Lisker has updated the pull request incrementally with one additional 
commit since the last revision:

  Rename and resize images

-

Changes:
  - all: https://git.openjdk.org/jfx/pull/1378/files
  - new: https://git.openjdk.org/jfx/pull/1378/files/d586d748..e5466476

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jfx=1378=08
 - incr: https://webrevs.openjdk.org/?repo=jfx=1378=07-08

  Stats: 32 lines in 50 files changed: 0 ins; 0 del; 32 mod
  Patch: https://git.openjdk.org/jfx/pull/1378.diff
  Fetch: git fetch https://git.openjdk.org/jfx.git pull/1378/head:pull/1378

PR: https://git.openjdk.org/jfx/pull/1378

Re: RFR: 8314147: Updated the PhongMaterial documentation [v7]

[Nir Lisker]

On Wed, 28 Feb 2024 14:21:55 GMT, Kevin Rushforth  wrote:

> One other thing I noticed is that the file names have spaces in them, which 
> is not a best practice and causes problems for scripts. Can you change all 
> spaces to _ in the names of the newly-added files?

Yes. What about folders? Also, do you prefer camelCase or the `_` naming?

-

PR Comment: https://git.openjdk.org/jfx/pull/1378#issuecomment-1969301404

Re: RFR: 8314147: Updated the PhongMaterial documentation [v8]

[Nir Lisker]

> Overhaul to the `PhongMaterial` documentation (and a bit to its superclass 
> `Material`). Except for the introduction, I divided the documentation into 3 
> sections: qualitative explanation, mathematical model (I wouldn't think it 
> necessary, but the current doc explains it), and examples.
> 
> The reason for the verbosity of the doc is that I envisioned 2 target 
> audiences for this class. One is a Java developer who wants to understand the 
> terminology and workings of computer graphics or of the artists who are 
> already familiar with this domain. (How many Java developers know what 
> diffuse, specular and normal maps are?) The other is an artist who is already 
> familiar with the domain, but wants to see how this class compares with other 
> renderers. For this reason, I looked at the terminology used by engines like 
> Blender, Maya, UE4 and Unity and tried to mention the comparisons (like bump 
> vs. height vs. normal maps, or specular vs. roughness/smoothness).
> 
> The examples I chose and some of the schematics are not the best, looking at 
> it retroactively, but I want to give enough time for reviewers and get this 
> into 22.

Nir Lisker has updated the pull request incrementally with one additional 
commit since the last revision:

  Fixed typos from review comments

-

Changes:
  - all: https://git.openjdk.org/jfx/pull/1378/files
  - new: https://git.openjdk.org/jfx/pull/1378/files/2989624d..d586d748

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jfx=1378=07
 - incr: https://webrevs.openjdk.org/?repo=jfx=1378=06-07

  Stats: 15 lines in 1 file changed: 1 ins; 3 del; 11 mod
  Patch: https://git.openjdk.org/jfx/pull/1378.diff
  Fetch: git fetch https://git.openjdk.org/jfx.git pull/1378/head:pull/1378

PR: https://git.openjdk.org/jfx/pull/1378

Re: RFR: 8314147: Updated the PhongMaterial documentation [v6]

[Nir Lisker]

On Tue, 27 Feb 2024 12:13:09 GMT, Ambarish Rapte  wrote:

>> Nir Lisker has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Fixed typo
>
> modules/javafx.graphics/src/main/java/javafx/scene/paint/PhongMaterial.java 
> line 170:
> 
>> 168:  * V - the vector from the surface to the viewer (camera);
>> 169:  * R - the reflection vector of L from the surface. 
>> R can be calculated from L and N:
>> 170:  * R=2(L⋅N)N - L.
> 
> Should these values be described as respect to point instead of surface ?
> for example: the vector from the surface to the light source -> the vector 
> from the **point** to the light source

The sentence above says "Four normalized vectors are considered for each point 
on the surface", so I think this is clear. The problem with looking at points 
is that they have no direction, unlike a surface, so you can't have a "normal 
of a point", you can only have "normal of a surface", and the reflection vector 
also depends on the directionality of the surface.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1378#discussion_r1506187511

Re: RFR: 8314147: Updated the PhongMaterial documentation [v6]

[Nir Lisker]

On Tue, 27 Feb 2024 11:24:08 GMT, Ambarish Rapte  wrote:

> PhongMaterial is not suitable for surfaces that reflect or refract the 
> incident light.

But it does reflect the incident light as explained in the paragraphs before.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1378#discussion_r1506179403

Re: RFR: 8314147: Updated the PhongMaterial documentation [v6]

[Nir Lisker]

On Tue, 27 Feb 2024 11:53:35 GMT, Ambarish Rapte  wrote:

>> Nir Lisker has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Fixed typo
>
> modules/javafx.graphics/src/main/java/javafx/scene/paint/PhongMaterial.java 
> line 115:
> 
>> 113:  * 
>> 114:  * Important: there is currently a bug that causes objects with 
>> 0 opacity to not render at all (despite having a
>> 115:  * specular or a self-illumination component). Setting the opacity to 
>> 1/255 instead will give the desirable result.
> 
> Is there is JBS issue to track this? The JBS issue should have a pointer for 
> this comment to be removed when fixed.

No, not yet. There a few oddities in the shader that I intend to look at for 
jfx23, this will be one of them.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1378#discussion_r1506020787

Re: RFR: 8314147: Updated the PhongMaterial documentation [v7]

[Nir Lisker]

On Wed, 28 Feb 2024 13:50:38 GMT, Nir Lisker  wrote:

>> Overhaul to the `PhongMaterial` documentation (and a bit to its superclass 
>> `Material`). Except for the introduction, I divided the documentation into 3 
>> sections: qualitative explanation, mathematical model (I wouldn't think it 
>> necessary, but the current doc explains it), and examples.
>> 
>> The reason for the verbosity of the doc is that I envisioned 2 target 
>> audiences for this class. One is a Java developer who wants to understand 
>> the terminology and workings of computer graphics or of the artists who are 
>> already familiar with this domain. (How many Java developers know what 
>> diffuse, specular and normal maps are?) The other is an artist who is 
>> already familiar with the domain, but wants to see how this class compares 
>> with other renderers. For this reason, I looked at the terminology used by 
>> engines like Blender, Maya, UE4 and Unity and tried to mention the 
>> comparisons (like bump vs. height vs. normal maps, or specular vs. 
>> roughness/smoothness).
>> 
>> The examples I chose and some of the schematics are not the best, looking at 
>> it retroactively, but I want to give enough time for reviewers and get this 
>> into 22.
>
> Nir Lisker has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Update images

I generated the new background is an engine. I also enlarged the images a bit 
and added another one in the transparency section with not highlight for 
comparison.

-

PR Comment: https://git.openjdk.org/jfx/pull/1378#issuecomment-1969032540

Re: RFR: 8314147: Updated the PhongMaterial documentation [v7]

[Nir Lisker]

> Overhaul to the `PhongMaterial` documentation (and a bit to its superclass 
> `Material`). Except for the introduction, I divided the documentation into 3 
> sections: qualitative explanation, mathematical model (I wouldn't think it 
> necessary, but the current doc explains it), and examples.
> 
> The reason for the verbosity of the doc is that I envisioned 2 target 
> audiences for this class. One is a Java developer who wants to understand the 
> terminology and workings of computer graphics or of the artists who are 
> already familiar with this domain. (How many Java developers know what 
> diffuse, specular and normal maps are?) The other is an artist who is already 
> familiar with the domain, but wants to see how this class compares with other 
> renderers. For this reason, I looked at the terminology used by engines like 
> Blender, Maya, UE4 and Unity and tried to mention the comparisons (like bump 
> vs. height vs. normal maps, or specular vs. roughness/smoothness).
> 
> The examples I chose and some of the schematics are not the best, looking at 
> it retroactively, but I want to give enough time for reviewers and get this 
> into 22.

Nir Lisker has updated the pull request incrementally with one additional 
commit since the last revision:

  Update images

-

Changes:
  - all: https://git.openjdk.org/jfx/pull/1378/files
  - new: https://git.openjdk.org/jfx/pull/1378/files/60d45bc8..2989624d

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jfx=1378=06
 - incr: https://webrevs.openjdk.org/?repo=jfx=1378=05-06

  Stats: 28 lines in 20 files changed: 4 ins; 1 del; 23 mod
  Patch: https://git.openjdk.org/jfx/pull/1378.diff
  Fetch: git fetch https://git.openjdk.org/jfx.git pull/1378/head:pull/1378

PR: https://git.openjdk.org/jfx/pull/1378

Re: RFR: 8314147: Updated the PhongMaterial documentation [v6]

[Nir Lisker]

> Overhaul to the `PhongMaterial` documentation (and a bit to its superclass 
> `Material`). Except for the introduction, I divided the documentation into 3 
> sections: qualitative explanation, mathematical model (I wouldn't think it 
> necessary, but the current doc explains it), and examples.
> 
> The reason for the verbosity of the doc is that I envisioned 2 target 
> audiences for this class. One is a Java developer who wants to understand the 
> terminology and workings of computer graphics or of the artists who are 
> already familiar with this domain. (How many Java developers know what 
> diffuse, specular and normal maps are?) The other is an artist who is already 
> familiar with the domain, but wants to see how this class compares with other 
> renderers. For this reason, I looked at the terminology used by engines like 
> Blender, Maya, UE4 and Unity and tried to mention the comparisons (like bump 
> vs. height vs. normal maps, or specular vs. roughness/smoothness).
> 
> The examples I chose and some of the schematics are not the best, looking at 
> it retroactively, but I want to give enough time for reviewers and get this 
> into 22.

Nir Lisker has updated the pull request incrementally with one additional 
commit since the last revision:

  Fixed typo

-

Changes:
  - all: https://git.openjdk.org/jfx/pull/1378/files
  - new: https://git.openjdk.org/jfx/pull/1378/files/83bf2eb1..60d45bc8

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jfx=1378=05
 - incr: https://webrevs.openjdk.org/?repo=jfx=1378=04-05

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jfx/pull/1378.diff
  Fetch: git fetch https://git.openjdk.org/jfx.git pull/1378/head:pull/1378

PR: https://git.openjdk.org/jfx/pull/1378

[jfx22] Integrated: 8325550: Grammatical error in AnchorPane.setLeftAnchor (and other setters) javadoc

[Nir Lisker]

On Sat, 24 Feb 2024 09:53:01 GMT, Nir Lisker  wrote:

> Backport of commit 
> [b43c4edf](https://github.com/openjdk/jfx/commit/b43c4edf7590429fd051d1b0e2ccb6dd49a10b8b)
>  from the [openjdk/jfx](https://git.openjdk.org/jfx) repository.

This pull request has now been integrated.

Changeset: 6e1246e8
Author:Nir Lisker 
URL:   
https://git.openjdk.org/jfx/commit/6e1246e870aa6dafe05a3f544bb283aa90a0c078
Stats: 66 lines in 3 files changed: 19 ins; 25 del; 22 mod

8325550: Grammatical error in AnchorPane.setLeftAnchor (and other setters) 
javadoc
8318624: API docs specify incorrect default value for nodeOrientation property

Reviewed-by: kcr
Backport-of: b43c4edf7590429fd051d1b0e2ccb6dd49a10b8b

-

PR: https://git.openjdk.org/jfx/pull/1380

Re: RFR: 8314147: Updated the PhongMaterial documentation [v5]

[Nir Lisker]

> Overhaul to the `PhongMaterial` documentation (and a bit to its superclass 
> `Material`). Except for the introduction, I divided the documentation into 3 
> sections: qualitative explanation, mathematical model (I wouldn't think it 
> necessary, but the current doc explains it), and examples.
> 
> The reason for the verbosity of the doc is that I envisioned 2 target 
> audiences for this class. One is a Java developer who wants to understand the 
> terminology and workings of computer graphics or of the artists who are 
> already familiar with this domain. (How many Java developers know what 
> diffuse, specular and normal maps are?) The other is an artist who is already 
> familiar with the domain, but wants to see how this class compares with other 
> renderers. For this reason, I looked at the terminology used by engines like 
> Blender, Maya, UE4 and Unity and tried to mention the comparisons (like bump 
> vs. height vs. normal maps, or specular vs. roughness/smoothness).
> 
> The examples I chose and some of the schematics are not the best, looking at 
> it retroactively, but I want to give enough time for reviewers and get this 
> into 22.

Nir Lisker has updated the pull request incrementally with one additional 
commit since the last revision:

  Remove redundant image copies

-

Changes:
  - all: https://git.openjdk.org/jfx/pull/1378/files
  - new: https://git.openjdk.org/jfx/pull/1378/files/e367c882..83bf2eb1

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jfx=1378=04
 - incr: https://webrevs.openjdk.org/?repo=jfx=1378=03-04

  Stats: 65 lines in 3 files changed: 0 ins; 65 del; 0 mod
  Patch: https://git.openjdk.org/jfx/pull/1378.diff
  Fetch: git fetch https://git.openjdk.org/jfx.git pull/1378/head:pull/1378

PR: https://git.openjdk.org/jfx/pull/1378

Re: RFR: 8314147: Updated the PhongMaterial documentation [v4]

[Nir Lisker]

> Overhaul to the `PhongMaterial` documentation (and a bit to its superclass 
> `Material`). Except for the introduction, I divided the documentation into 3 
> sections: qualitative explanation, mathematical model (I wouldn't think it 
> necessary, but the current doc explains it), and examples.
> 
> The reason for the verbosity of the doc is that I envisioned 2 target 
> audiences for this class. One is a Java developer who wants to understand the 
> terminology and workings of computer graphics or of the artists who are 
> already familiar with this domain. (How many Java developers know what 
> diffuse, specular and normal maps are?) The other is an artist who is already 
> familiar with the domain, but wants to see how this class compares with other 
> renderers. For this reason, I looked at the terminology used by engines like 
> Blender, Maya, UE4 and Unity and tried to mention the comparisons (like bump 
> vs. height vs. normal maps, or specular vs. roughness/smoothness).
> 
> The examples I chose and some of the schematics are not the best, looking at 
> it retroactively, but I want to give enough time for reviewers and get this 
> into 22.

Nir Lisker has updated the pull request incrementally with one additional 
commit since the last revision:

  Clarifications for the diffuse component

-

Changes:
  - all: https://git.openjdk.org/jfx/pull/1378/files
  - new: https://git.openjdk.org/jfx/pull/1378/files/0130d0f5..e367c882

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jfx=1378=03
 - incr: https://webrevs.openjdk.org/?repo=jfx=1378=02-03

  Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.org/jfx/pull/1378.diff
  Fetch: git fetch https://git.openjdk.org/jfx.git pull/1378/head:pull/1378

PR: https://git.openjdk.org/jfx/pull/1378