Re: [Libguestfs] Checksums and other verification
On Mon, Feb 27, 2023 at 3:56 PM Richard W.M. Jones wrote: > > > https://github.com/kubevirt/containerized-data-importer/issues/1520 > > Hi Eric, > > We had a question from the Kubevirt team related to the above issue. > The question is roughly if it's possible to calculate the checksum of > an image as an nbdkit filter and/or in the qemu block layer. > > Supplemental #1: could qemu-img convert calculate a checksum as it goes > along? > > Supplemental #2: could we detect various sorts of common errors, such > a webserver that is incorrectly configured and serves up an error page > containing ""; or something which is supposed to be a disk image > but does not "look like" (in some ill-defined sense) a disk image, > eg. it has no partition table. > > I'm not sure if qemu has any existing features covering the above (and > I know for sure that nbdkit doesn't). > > One issue is that calculating a checksum involves a linear scan of the > image, although we can at least skip holes. Kubvirt can use blksum https://fosdem.org/2023/schedule/event/vai_blkhash_fast_disk/ But we need to package it for Fedora/CentOS Stream. I also work on "qemu-img checksum", getting more reviews on this can help: Lastest version: https://lists.nongnu.org/archive/html/qemu-block/2022-11/msg00971.html Last reveiw are here: https://lists.nongnu.org/archive/html/qemu-block/2022-12/ More work is needed on the testing framework changes. Nir ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
[jfx20] Integrated: 8293587: Fix mistakes in FX API docs
On Mon, 6 Feb 2023 23:00:17 GMT, Nir Lisker wrote: > Fixes and cleanup in the areas in the linked issue. This pull request has now been integrated. Changeset: 7bf2372b Author: Nir Lisker URL: https://git.openjdk.org/jfx/commit/7bf2372baf8ea719b8b611cd1476596d8c141c50 Stats: 55 lines in 6 files changed: 7 ins; 4 del; 44 mod 8293587: Fix mistakes in FX API docs Reviewed-by: jhendrikx, kcr, angorya - PR: https://git.openjdk.org/jfx/pull/1025
Re: [Libguestfs] [PATCH 1/2] python: Avoid crash if callback parameters cannot be built
On Mon, Feb 20, 2023 at 10:45 AM Laszlo Ersek wrote: > > On 2/17/23 17:52, Eric Blake wrote: > > On Thu, Feb 16, 2023 at 03:09:02PM +0100, Laszlo Ersek wrote: > > >> - Py_BuildValue with the "O" format specifier transfers the new list's > >> *sole* reference (= ownership) to the just-built higher-level object "args" > > > > Reference transfer is done with "N", not "O". That would be an > > alternative to decreasing the refcount of py_array on success, but not > > eliminate the need to decrease the refcount on Py_BuildValue failure. > > > >> > >> - when "args" is killed (decref'd), it takes care of "py_array". > >> > >> Consequently, if Py_BuildValue fails, "py_array" continues owning the > >> new list -- and I believe that, if we take the new error branch, we leak > >> the object pointed-to by "py_array". Is that the case? > > > > Not quite. "O" is different than "N". > > I agree with you *now*, looking up the "O" specification at > <https://docs.python.org/3/c-api/arg.html#building-values>. > > However, when I was writing my email, I looked up Py_BuildValue at that > time as well, just elsewhere. I don't know where. Really. And then that > documentation said that the reference count would *not* be increased. I > distinctly remember that, because it surprised me -- I actually recalled > an *even earlier* experience reading the documentation, which had again > stated that "O" would increase the reference count. Maybe here: https://docs.python.org/2/c-api/arg.html#building-values Looks like another incompatibility between python 2 and 3. Nir ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: RFR: 8290310: ChangeListener events are incorrect or misleading when a nested change occurs [v6]
On Wed, 15 Feb 2023 09:46:22 GMT, John Hendrikx wrote:
>> This contains the following:
>> - Nested changes or invalidations using ExpressionHelper are delayed until
>> the current emission completes
>> - This fixes odd change events being produced (with incorrect oldValue)
>> - Also fixes a bug in ExpressionHelper where a nested change would unlock
>> the listener list early, which could cause a
>> `ConcurrentModificationException` if a nested change was combined with a
>> remove/add listener call
>> - A test for ExpressionHelper to verify the new behavior
>> - A test for all *Property and *Binding classes that verifies correct
>> listener behavior at the API level (this tests gets 85% coverage on
>> ExpressionHelper on its own, the only thing it is not testing is the locking
>> behavior, which is not relevant at the API level).
>> - A fix for `WebColorFieldSkin` which triggered a nested change which used a
>> flag to prevent an event loop (I've changed it now to match how
>> `DoubleFieldSkin` and `IntegerFieldSkin` do it
>
> John Hendrikx has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix no change detection
>
> Old text was uppercased while new text is always lowercase.
With this test
static void with2Changes() {
inv = 0;
var property = new SimpleIntegerProperty(0);
ChangeListener listenerA = (obs, ov, nv) -> {
inv++;
String spaces = spaces();
System.out.println(spaces + " bA " + ov + "->" + nv + " (" +
property.get() + ")");
property.set(5);
System.out.println(spaces + " aA " + ov + "->" + nv + " (" +
property.get() + ")");
};
ChangeListener listenerB = (obs, ov, nv) -> {
inv++;
String spaces = spaces();
System.out.println(spaces + " bB " + ov + "->" + nv + " (" +
property.get() + ")");
property.set(6);
System.out.println(spaces + " aB " + ov + "->" + nv + " (" +
property.get() + ")");
};
property.addListener(listenerA);
property.addListener(listenerB);
property.set(1);
}
I get
1 bA 0->1 (1)
1 aA 0->1 (5)
2 bB 0->1 (5)
2 aB 0->1 (6)
3 bA 1->6 (6)
3 aA 1->6 (5)
4 bB 1->6 (5)
4 aB 1->6 (6)
I think that we are missing a 1->5 event originating in listener A, and maybe a
5->6 event. I'm honestly not sure what the behavior should be here.
-
PR: https://git.openjdk.org/jfx/pull/837
Re: RFR: 8290310: ChangeListener events are incorrect or misleading when a nested change occurs [v6]
On Wed, 15 Feb 2023 09:46:22 GMT, John Hendrikx wrote:
>> This contains the following:
>> - Nested changes or invalidations using ExpressionHelper are delayed until
>> the current emission completes
>> - This fixes odd change events being produced (with incorrect oldValue)
>> - Also fixes a bug in ExpressionHelper where a nested change would unlock
>> the listener list early, which could cause a
>> `ConcurrentModificationException` if a nested change was combined with a
>> remove/add listener call
>> - A test for ExpressionHelper to verify the new behavior
>> - A test for all *Property and *Binding classes that verifies correct
>> listener behavior at the API level (this tests gets 85% coverage on
>> ExpressionHelper on its own, the only thing it is not testing is the locking
>> behavior, which is not relevant at the API level).
>> - A fix for `WebColorFieldSkin` which triggered a nested change which used a
>> flag to prevent an event loop (I've changed it now to match how
>> `DoubleFieldSkin` and `IntegerFieldSkin` do it
>
> John Hendrikx has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix no change detection
>
> Old text was uppercased while new text is always lowercase.
I tried this test:
static void withRemovalAnd2Changes() {
inv = 0;
var property = new SimpleIntegerProperty(0);
ChangeListener listenerA = (obs, ov, nv) -> {
inv++;
String spaces = spaces();
System.out.println(spaces + " bA " + ov + "->" + nv + " (" +
property.get() + ")");
property.set(5);
System.out.println(spaces + " aA " + ov + "->" + nv + " (" +
property.get() + ")");
};
ChangeListener listenerB = (obs, ov, nv) -> {
inv++;
String spaces = spaces();
System.out.println(spaces + " bB " + ov + "->" + nv + " (" +
property.get() + ")");
property.removeListener(listenerA);
property.set(6);
System.out.println(spaces + " aB " + ov + "->" + nv + " (" +
property.get() + ")");
};
property.addListener(listenerA);
property.addListener(listenerB);
property.set(1);
}
which removes a listener in a nested event. With this patch I got the following:
1 bA 0->1 (1)
1 aA 0->1 (5)
2 bB 0->1 (5)
3 bB 5->6 (6)
3 aB 5->6 (6)
2 aB 0->1 (6)
4 bA 1->6 (6)
5 bB 6->5 (5)
6 bB 5->6 (6)
6 aB 5->6 (6)
5 aB 6->5 (6)
4 aA 1->6 (6)
7 bB 1->6 (6)
7 aB 1->6 (6)
Because listener A is removed in B, we hit the inconsistency with 1 listener
again. I need to work through this, but I'm not convinced this is what the
output should be. One glaring question is, are nested listener
additions/removals also deferred, or do they take effect immediately, in which
case they shouldn't receive deferred nested events I think.
-
PR: https://git.openjdk.org/jfx/pull/837
Re: RFR: 8290310: ChangeListener events are incorrect or misleading when a nested change occurs [v5]
On Sat, 18 Feb 2023 19:29:05 GMT, John Hendrikx wrote: > Confusing me again here :-) Did you mean to say "breadth-first" where you > said "depth-first" ? > > Breadth first is for sure a lot easier, as the old values are much easier to > get correct for it. > > I've given depth first some more thought, but every approach I think of > requires some kind of stack or tracking of which listeners weren't notified > yet of the original set that we wanted to notify (and I think we'll need to > remember even more if there is another change before the first and second one > finishes). Yes, sorry, I meant breadth-first. We can go with that one. > @nlisker if we're going for breadth-first, shall I fix the cases for single > listeners as well (finish the notification before calling the same listener > again, instead of recursively calling into it?) I think that we need to be consistent, so barring any special reason for excepting single listeners from this change, I would say that we need to bring them in line with the generic listener. > Perhaps we could limit the explanation to just mentioning that its possible > the "new value" may not be the same as ObservableValue#getValue if nested > changes occured. Also, I'm curious why it would still be a bad practice to > modify the value again (now that the old values are always correct); I think > it's pretty much the standard pattern to veto a change (like a numeric field > rejecting out of range values or non-numeric characters). I'll think about what doc changes to make. We'll see what Ambarish thinks too. Vetoing seems like a normal use case, but maybe there's another way of doing it. I never delved into this. - PR: https://git.openjdk.org/jfx/pull/837
Re: RFR: 8290310: ChangeListener events are incorrect or misleading when a nested change occurs [v5]
On Tue, 14 Feb 2023 23:27:07 GMT, John Hendrikx wrote: > I couldn't quite see which you prefer here; you said "This one makes sense" > but not quite sure which version it refers to (I suppose the depth first > version?) I should have said "this one makes sense **too**". The point was that while your fix is good, it's not the only good fix, and I wasn't sold on choosing the one in this PR just yet. If "breadth-first" is easier to do and requires less memory to remember the old values, then I have no problem with "depth-first". I don't have a strict preference for either approach mostly because I don't do nested modifications myself. I'm not at all sure that we need so specify this behavior, but we need to be consistent with it. @arapte Why do you think we should say what order the nested event is dispatched at? I'm also investigating the effect of this patch in conjunction with addition/removal of listeners in conjunction with a nested event. - PR: https://git.openjdk.org/jfx/pull/837
RFR: 8302816: Refactor sorting-related classes
Most of the changes revolve around unifying the sorting methods for a collection with `Comparable` elements with sorting methods that take an external `Comparator` by passing `Comparator.naturalOrder()` from the former to the latter. This eliminates method duplication and some warnings suppressions. Note that I get 1 failing test: VersionInfoTest.testMajorVersion. This PR is unrelated to this test. - Commit messages: - Initial commit Changes: https://git.openjdk.org/jfx/pull/1041/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1041=00 Issue: https://bugs.openjdk.org/browse/JDK-8302816 Stats: 248 lines in 5 files changed: 27 ins; 125 del; 96 mod Patch: https://git.openjdk.org/jfx/pull/1041.diff Fetch: git fetch https://git.openjdk.org/jfx pull/1041/head:pull/1041 PR: https://git.openjdk.org/jfx/pull/1041
Re: CFV: New OpenJFX Reviewer: Jose Pereda
Vote: Yes On Fri, Feb 17, 2023, 04:54 Philip Race wrote: > Vote: yes > > -phil > >
Re: RFR: 8290310: ChangeListener events are incorrect or misleading when a nested change occurs [v2]
On Sun, 5 Feb 2023 20:11:35 GMT, John Hendrikx wrote:
> I don't think we should care about depth-first, breadth-first. The only thing
> that I think is important here is that the contract of ChangeListener is
> respected. I think that that contract should be:
...
I'll be more concrete. Here is my test program:
public class ListenersTest {
private static int inv = 0;
public static void main(String[] args) {
with1Change();
}
static void with1Change() {
inv = 0;
var property = new SimpleIntegerProperty(0);
ChangeListener listenerA = (obs, ov, nv) -> {
inv++;
String spaces = IntStream.range(1, inv).mapToObj(i -> "
").reduce(String::concat).orElse("") + inv;
System.out.println(spaces + " bA " + ov + "->" + nv + " (" +
property.get() + ")");
property.set(5);
System.out.println(spaces + " aA " + ov + "->" + nv + " (" +
property.get() + ")");
};
property.addListener(listenerA);
ChangeListener listenerB = (obs, ov, nv) -> {
inv++;
String spaces = IntStream.range(1, inv).mapToObj(i -> "
").reduce(String::concat).orElse("") + inv;
System.out.println(spaces + " bB " + ov + "->" + nv + " (" +
property.get() + ")");
System.out.println(spaces + " aB " + ov + "->" + nv + " (" +
property.get() + ")");
};
property.addListener(listenerB);
property.set(1);
System.out.println("-\n");
}
}
With the patch:
1 bA 0->1 (1)
1 aA 0->1 (5)
2 bB 0->1 (5)
2 aB 0->1 (5)
3 bA 1->5 (5)
3 aA 1->5 (5)
4 bB 1->5 (5)
4 aB 1->5 (5)
With your patch, each event finishes its run and only then the next event
happens. This is the "breadth-first" approach.
However, there is another one:
1 bA 0->1 (1)
2 bA 1->5 (5)
2 aA 1->5 (5)
3 bB 1->5 (5)
3 aB 1->5 (5)
1 aA 0->1 (5)
4 bB 0->1 (5)
4 aB 0->1 (5)
This approach starts events before the previous ones finished, and goes back to
the original event later. This is the "depth-first" approach. I don't think
that either is wrong. This one makes sense and it's a behavior I can reason
about: the listener is loyal to the event at the time it happened (and the
"real" value is accessible with `get`).
Without the patch:
1 bA 0->1 (1)
2 bA 1->5 (5)
2 aA 1->5 (5)
3 bB 1->5 (5)
3 aB 1->5 (5)
1 aA 0->1 (5)
4 bB 0->5 (5)
4 aB 0->5 (5)
I agree that at step 4 the 0->5 event is wrong because the events are only 0->1
and 1->5.
If you comment out the line `property.addListener(listenerB);` (only register
A), then both with and without the patch I get
1 bA 0->1 (1)
2 bA 1->5 (5)
2 aA 1->5 (5)
1 aA 0->1 (5)
while with delaying nested events I would expect:
1 bA 0->1 (1)
1 aA 0->1 (5)
2 bA 1->5 (5)
2 aA 1->5 (5)
So this looks inconsistent to me.
The fix for the lock being released is good regardless, it's the behavioral
change that I'm not sold on.
-
PR: https://git.openjdk.org/jfx/pull/837
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages [v5]
On Sun, 12 Feb 2023 21:49:11 GMT, John Hendrikx wrote: >> Packages fixed: >> - com.sun.javafx.binding >> - com.sun.javafx.collections >> - javafx.beans >> - javafx.beans.binding >> - javafx.collections >> - javafx.collections.transformation > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Fix review comments Marked as reviewed by nlisker (Reviewer). - PR: https://git.openjdk.org/jfx/pull/972
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages [v4]
On Sun, 1 Jan 2023 15:25:01 GMT, John Hendrikx wrote:
>> modules/javafx.base/src/main/java/javafx/collections/FXCollections.java line
>> 1640:
>>
>>> 1638: @Override
>>> 1639: public Iterator iterator() {
>>> 1640: return new Iterator<>() {
>>
>> Here the empty `Set` creates a listener on invocation, unlike in the list
>> case. Might want to keep a single pattern. I prefer the one with a singleton
>> iterator because the empty set itself is a singleton. Same comment about
>> considering "inlining" it.
>
> Can make these consistent if the approach is agreed upon.
So let's make the list and the set use an instance singleton pattern, like the
empty list does.
-
PR: https://git.openjdk.org/jfx/pull/972
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages [v4]
On Wed, 8 Feb 2023 15:05:46 GMT, John Hendrikx wrote: >> Packages fixed: >> - com.sun.javafx.binding >> - com.sun.javafx.collections >> - javafx.beans >> - javafx.beans.binding >> - javafx.collections >> - javafx.collections.transformation > > John Hendrikx 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 seven additional > commits since the last revision: > > - Merge branch 'master' of https://git.openjdk.org/jfx into > feature/fix-raw-type-warnings-in-base-2 > - Undo changes in SortHelper > - Simplify MappingChange by using Function > - Clean up expression classes repeated null checks > - Use instanceof with pattern matching in a few spots > - Use static method for no-op map > - Fix raw type warnings in base > >Packages fixed: >- com.sun.javafx.binding >- com.sun.javafx.collections >- javafx.beans >- javafx.beans.binding >- javafx.collections >- javafx.collections.transformation modules/javafx.base/src/main/java/javafx/collections/FXCollections.java line 235: > 233: } > 234: > 235: private static ObservableMap EMPTY_OBSERVABLE_MAP = new > EmptyObservableMap<>(); This can be a `final` field. Same for the list and set ones. Not sure why it wasn't declared as such from the start. modules/javafx.base/src/main/java/javafx/collections/transformation/FilteredList.java line 133: > 131: return getPredicate(); > 132: } > 133: return (Predicate) ALWAYS_TRUE; Do we actually need the constant `ALWAYS_TRUE`? I think that just inlining `return e -> true;` is fine and we can remove the field, the cast, and the warning suppression. What do you think? - PR: https://git.openjdk.org/jfx/pull/972
[ovirt-users] Re: ImageIO Performance
On Thu, Feb 9, 2023 at 7:03 PM Nir Soffer wrote:
>
> On Mon, Feb 6, 2023 at 10:00 AM Jean-Louis Dupond via Users
> wrote:
> >
> > Hi All,
> >
> > We backup our VM's with a custom script based on the
> > https://github.com/oVirt/python-ovirt-engine-sdk4/blob/main/examples/backup_vm.py
> > example.
> > This works fine, but we start to see scaling issues.
> >
> > On VM's where there are a lot of dirty blocks,
>
> We need to see the list of extents returned by the server.
>
> The easiest way would be to enable debug logs - it will be even slower,
> but we will see these logs showing all extents:
>
> log.debug("Copying %s", ext)
> log.debug("Zeroing %s", ext)
> log.debug("Skipping %s", ext)
>
> It will also show other info that can help to understand why it is slow.
>
> > the transfer goes really
> > slow (sometimes only 20MiB/sec).
>
> Seems much slower than expected
>
> > At the same time we see that ovirt-imageio process sometimes uses 100%
> > CPU
>
> This is possible, it shows that you do a lot of requests.
>
> > (its single threaded?).
>
> It uses thread per connection model. When used with backup_vm.py or other
> examples using the ovirt_imageio.client it usually use 4 connections
> per transfer
> so there will be 4 threads on the server size serving the data.
>
> Please share debug log of a slow backup, and info about the backup image
> storage
> for example, is this local file system or NFS?
I opened https://github.com/oVirt/ovirt-imageio/issues/175 to make
debugging such
issue easier.
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/HP2OZ3E3HAZIE2OCUKXSZIVEDYKQO2DY/
[ovirt-users] Re: ImageIO Performance
On Thu, Feb 9, 2023 at 7:03 PM Nir Soffer wrote: > > On Mon, Feb 6, 2023 at 10:00 AM Jean-Louis Dupond via Users > wrote: > The easiest way would be to enable debug logs - it will be even slower, > but we will see these logs showing all extents: Using the --debug option Run backup_vm.py with --help to see all options. ___ 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/HPJVPFQD32Q55ZALOLK6AQ6PFXNGLGMX/
[ovirt-users] Re: ImageIO Performance
On Mon, Feb 6, 2023 at 10:00 AM Jean-Louis Dupond via Users
wrote:
>
> Hi All,
>
> We backup our VM's with a custom script based on the
> https://github.com/oVirt/python-ovirt-engine-sdk4/blob/main/examples/backup_vm.py
> example.
> This works fine, but we start to see scaling issues.
>
> On VM's where there are a lot of dirty blocks,
We need to see the list of extents returned by the server.
The easiest way would be to enable debug logs - it will be even slower,
but we will see these logs showing all extents:
log.debug("Copying %s", ext)
log.debug("Zeroing %s", ext)
log.debug("Skipping %s", ext)
It will also show other info that can help to understand why it is slow.
> the transfer goes really
> slow (sometimes only 20MiB/sec).
Seems much slower than expected
> At the same time we see that ovirt-imageio process sometimes uses 100%
> CPU
This is possible, it shows that you do a lot of requests.
> (its single threaded?).
It uses thread per connection model. When used with backup_vm.py or other
examples using the ovirt_imageio.client it usually use 4 connections
per transfer
so there will be 4 threads on the server size serving the data.
Please share debug log of a slow backup, and info about the backup image storage
for example, is this local file system or NFS?
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/HE6ZKEPEHOYDCIHLUFAW4MDQVXYO2TA6/
Re: [jfx20] RFR: 8293587: Fix mistakes in FX API docs [v5]
> Fixes and cleanup in the areas in the linked issue. Nir Lisker has updated the pull request incrementally with one additional commit since the last revision: Addressed review comments - Changes: - all: https://git.openjdk.org/jfx/pull/1025/files - new: https://git.openjdk.org/jfx/pull/1025/files/a5632000..328bdebc Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1025=04 - incr: https://webrevs.openjdk.org/?repo=jfx=1025=03-04 Stats: 5 lines in 2 files changed: 4 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/1025.diff Fetch: git fetch https://git.openjdk.org/jfx pull/1025/head:pull/1025 PR: https://git.openjdk.org/jfx/pull/1025
Re: [jfx20] RFR: 8293587: Fix mistakes in FX API docs [v4]
> Fixes and cleanup in the areas in the linked issue. Nir Lisker has updated the pull request incrementally with one additional commit since the last revision: Update modules/javafx.graphics/src/main/java/javafx/scene/text/TextFlow.java - Changes: - all: https://git.openjdk.org/jfx/pull/1025/files - new: https://git.openjdk.org/jfx/pull/1025/files/fe9c9889..a5632000 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1025=03 - incr: https://webrevs.openjdk.org/?repo=jfx=1025=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/1025.diff Fetch: git fetch https://git.openjdk.org/jfx pull/1025/head:pull/1025 PR: https://git.openjdk.org/jfx/pull/1025
Re: [jfx20] RFR: 8293587: Fix mistakes in FX API docs [v4]
On Tue, 7 Feb 2023 19:52:14 GMT, Kevin Rushforth wrote: >> A specialized layout... +1 > > +1 from me as well. Suggestion: * A specialized layout for rich text. - PR: https://git.openjdk.org/jfx/pull/1025
Re: [jfx20] RFR: 8293587: Fix mistakes in FX API docs [v3]
On Tue, 7 Feb 2023 13:05:21 GMT, Kevin Rushforth wrote: >> Nir Lisker has updated the pull request incrementally with three additional >> commits since the last revision: >> >> - Update >> modules/javafx.graphics/src/main/java/javafx/scene/text/TextFlow.java >> >>Co-authored-by: John Hendrikx >> - Update >> modules/javafx.graphics/src/main/java/javafx/scene/text/TextFlow.java >> >>Co-authored-by: John Hendrikx >> - Update >> modules/javafx.graphics/src/main/java/javafx/scene/text/TextFlow.java >> >>Co-authored-by: John Hendrikx > > modules/javafx.graphics/src/main/java/javafx/scene/text/TextFlow.java line > 143: > >> 141: * application may set them to other values as needed: >> 142: * >> 143: * textflow.setMaxWidth(500); > > Maybe remove the `` tag while you are at it? All the other panes use this format, for example, https://openjfx.io/javadoc/19/javafx.graphics/javafx/scene/layout/BorderPane.html. Maybe in a Panes doc cleanup issue. I always thought more images were needed in these to show the layouts, so I could look into that. - PR: https://git.openjdk.org/jfx/pull/1025
Re: [jfx20] RFR: 8293587: Fix mistakes in FX API docs [v3]
On Tue, 7 Feb 2023 12:25:14 GMT, John Hendrikx wrote:
>> Nir Lisker has updated the pull request incrementally with three additional
>> commits since the last revision:
>>
>> - Update
>> modules/javafx.graphics/src/main/java/javafx/scene/text/TextFlow.java
>>
>>Co-authored-by: John Hendrikx
>> - Update
>> modules/javafx.graphics/src/main/java/javafx/scene/text/TextFlow.java
>>
>>Co-authored-by: John Hendrikx
>> - Update
>> modules/javafx.graphics/src/main/java/javafx/scene/text/TextFlow.java
>>
>>Co-authored-by: John Hendrikx
>
> modules/javafx.graphics/src/main/java/javafx/scene/text/TextFlow.java line 62:
>
>> 60:
>> 61: /**
>> 62: * A special layout designed to lay out rich text.
>
> What's "special" about it? :-)
> Suggestion:
>
> * A layout designed to lay out rich text.
Maybe it's special in the sense that it's specialized towards displaying text,
unlike other `Pane`s that are multi-purpose for all nodes. "designed to"
already points to that. Maybe "A specialized layout for rich text."?
> modules/javafx.graphics/src/main/java/javafx/scene/text/TextFlow.java line 90:
>
>> 88: * It can be specified by the application by setting the {@code
>> TextFlow}'s preferred
>> 89: * width. If no wrapping is desired, the application can either set the
>> preferred
>> 90: * with to {@code Double.MAX_VALUE} or {@code Region.USE_COMPUTED_SIZE}.
>
> Should these be links? They're the first mention in this doc.
I don't think that linking to `Double.MAX_VALUE` is helpful, but maybe to
`Region.USE_COMPUTED_SIZE` is.
-
PR: https://git.openjdk.org/jfx/pull/1025
Re: [jfx20] RFR: 8293587: Fix mistakes in FX API docs [v3]
On Tue, 7 Feb 2023 12:17:43 GMT, Kevin Rushforth wrote:
>> Nir Lisker has updated the pull request incrementally with three additional
>> commits since the last revision:
>>
>> - Update
>> modules/javafx.graphics/src/main/java/javafx/scene/text/TextFlow.java
>>
>>Co-authored-by: John Hendrikx
>> - Update
>> modules/javafx.graphics/src/main/java/javafx/scene/text/TextFlow.java
>>
>>Co-authored-by: John Hendrikx
>> - Update
>> modules/javafx.graphics/src/main/java/javafx/scene/text/TextFlow.java
>>
>>Co-authored-by: John Hendrikx
>
> modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 3277:
>
>> 3275: private void processDropEnd(DragEvent de) {
>> 3276: if (source == null) {
>> 3277: System.out.println("Scene.DnDGesture.processDropEnd()
>> - UNEXPECTED - source is NULL");
>
> This reminds me... I see that this warning is printed to `System.out`. I plan
> to file a follow-up cleanup bug (not for `jfx20`) to fix this, here and in
> other places, since runtime warnings should be printed to `System.err` (or
> else logged using the platform logger, but that would be a larger change, so
> I expect we'll opt for the simple substitution).
I never understood when something should be logged vs. printed. And do we ever
print to `out`?
-
PR: https://git.openjdk.org/jfx/pull/1025
Re: [jfx20] RFR: 8293587: Fix mistakes in FX API docs [v3]
> Fixes and cleanup in the areas in the linked issue. Nir Lisker has updated the pull request incrementally with three additional commits since the last revision: - Update modules/javafx.graphics/src/main/java/javafx/scene/text/TextFlow.java Co-authored-by: John Hendrikx - Update modules/javafx.graphics/src/main/java/javafx/scene/text/TextFlow.java Co-authored-by: John Hendrikx - Update modules/javafx.graphics/src/main/java/javafx/scene/text/TextFlow.java Co-authored-by: John Hendrikx - Changes: - all: https://git.openjdk.org/jfx/pull/1025/files - new: https://git.openjdk.org/jfx/pull/1025/files/d2155e51..fe9c9889 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1025=02 - incr: https://webrevs.openjdk.org/?repo=jfx=1025=01-02 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jfx/pull/1025.diff Fetch: git fetch https://git.openjdk.org/jfx pull/1025/head:pull/1025 PR: https://git.openjdk.org/jfx/pull/1025
Re: [jfx20] RFR: 8293587: Fix mistakes in FX API docs [v2]
> Fixes and cleanup in the areas in the linked issue. Nir Lisker has updated the pull request incrementally with one additional commit since the last revision: Update modules/javafx.controls/src/main/java/javafx/scene/control/ListView.java Co-authored-by: John Hendrikx - Changes: - all: https://git.openjdk.org/jfx/pull/1025/files - new: https://git.openjdk.org/jfx/pull/1025/files/7d422a9c..d2155e51 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1025=01 - incr: https://webrevs.openjdk.org/?repo=jfx=1025=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/1025.diff Fetch: git fetch https://git.openjdk.org/jfx pull/1025/head:pull/1025 PR: https://git.openjdk.org/jfx/pull/1025
Re: [jfx20] RFR: 8293587: Fix mistakes in FX API docs
On Mon, 6 Feb 2023 23:00:17 GMT, Nir Lisker wrote: > Fixes and cleanup in the areas in the linked issue. Integrating can wait until a bit before the release to allow for more mistakes to be included. - PR: https://git.openjdk.org/jfx/pull/1025
[jfx20] RFR: 8293587: Fix mistakes in FX API docs
Fixes and cleanup in the areas in the linked issue. - Commit messages: - Initial commit Changes: https://git.openjdk.org/jfx/pull/1025/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1025=00 Issue: https://bugs.openjdk.org/browse/JDK-8293587 Stats: 50 lines in 6 files changed: 3 ins; 4 del; 43 mod Patch: https://git.openjdk.org/jfx/pull/1025.diff Fetch: git fetch https://git.openjdk.org/jfx pull/1025/head:pull/1025 PR: https://git.openjdk.org/jfx/pull/1025
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages [v2]
On Sun, 1 Jan 2023 16:08:15 GMT, John Hendrikx wrote: >> Packages fixed: >> - com.sun.javafx.binding >> - com.sun.javafx.collections >> - javafx.beans >> - javafx.beans.binding >> - javafx.collections >> - javafx.collections.transformation > > John Hendrikx has updated the pull request incrementally with two additional > commits since the last revision: > > - Clean up expression classes repeated null checks > - Use instanceof with pattern matching in a few spots I looked at it again. I think it would be best to do the sorting-related changes separately. The rest looks good. - PR: https://git.openjdk.org/jfx/pull/972
Re: RFR: 8290310: ChangeListener events are incorrect or misleading when a nested change occurs [v2]
On Tue, 3 Jan 2023 09:42:05 GMT, John Hendrikx wrote: >> This contains the following: >> - Nested changes or invalidations using ExpressionHelper are delayed until >> the current emission completes >> - This fixes odd change events being produced (with incorrect oldValue) >> - Also fixes a bug in ExpressionHelper where a nested change would unlock >> the listener list early, which could cause a >> `ConcurrentModificationException` if a nested change was combined with a >> remove/add listener call >> - A test for ExpressionHelper to verify the new behavior >> - A test for all *Property and *Binding classes that verifies correct >> listener behavior at the API level (this tests gets 85% coverage on >> ExpressionHelper on its own, the only thing it is not testing is the locking >> behavior, which is not relevant at the API level). >> - A fix for `WebColorFieldSkin` which triggered a nested change which used a >> flag to prevent an event loop (I've changed it now to match how >> `DoubleFieldSkin` and `IntegerFieldSkin` do it > > John Hendrikx has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains two commits: > > - Merge branch 'master' of https://git.openjdk.org/jfx into > feature/delayed-nested-emission > - Delay nested event emission modules/javafx.base/src/test/java/test/javafx/beans/ObservableValueTest.java line 1: > 1: package test.javafx.beans; You will need to add a license header for a new file. - PR: https://git.openjdk.org/jfx/pull/837
Re: RFR: 8290310: ChangeListener events are incorrect or misleading when a nested change occurs [v2]
On Tue, 3 Jan 2023 09:42:05 GMT, John Hendrikx wrote:
>> This contains the following:
>> - Nested changes or invalidations using ExpressionHelper are delayed until
>> the current emission completes
>> - This fixes odd change events being produced (with incorrect oldValue)
>> - Also fixes a bug in ExpressionHelper where a nested change would unlock
>> the listener list early, which could cause a
>> `ConcurrentModificationException` if a nested change was combined with a
>> remove/add listener call
>> - A test for ExpressionHelper to verify the new behavior
>> - A test for all *Property and *Binding classes that verifies correct
>> listener behavior at the API level (this tests gets 85% coverage on
>> ExpressionHelper on its own, the only thing it is not testing is the locking
>> behavior, which is not relevant at the API level).
>> - A fix for `WebColorFieldSkin` which triggered a nested change which used a
>> flag to prevent an event loop (I've changed it now to match how
>> `DoubleFieldSkin` and `IntegerFieldSkin` do it
>
> John Hendrikx has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains two commits:
>
> - Merge branch 'master' of https://git.openjdk.org/jfx into
> feature/delayed-nested-emission
> - Delay nested event emission
The fix for the bug with nested events releasing the lock seems fine. I'm still
not sure if the behavioral change is what we want the result to be even if it's
better than what we have now. I have mentioned this in
https://github.com/openjdk/jfx/pull/108#issuecomment-590878643. we need to
decide on a spec first, and there are several facets to it as discussed in the
mailing list link above
(https://github.com/openjdk/jfx/pull/837#pullrequestreview-1233910264).
I will re-summarize what I think needs to be defined before implementing a
behavior change like the one proposed here:
### Listener order
When listeners are registered, they are added to some collection (an array,
currently). The add/remove methods hint at an order, but don't guarantee one.
Do we specify an order, or say it's unspecified?
### Event dispatch order
The order in which listeners are triggered is not necessarily the order of the
listeners above. Do we specify a dispatch order, or say it's unspecified? If
it's specified, the listener order is probably also specified.
We are also dispatching invalidation events before change events arbitrarily.
Do we dispatch the event to all listeners of one type and then to the other, or
do we want to combine them according to a joint dispatch order? We either say
that they are dispatched separately, together in some order (like
registration), or that it's unspecified.
### Nested listener modifications
If a listener is added/removed during an event dispatch, do we specify it will
affect ("be in time for") the nested event chain, the outer event chain, or
that it's unspecified?
### Nested value modifications
If a listener changes the value of its property during an event dispatch, do we
specify that the new event will trigger first, halting the current event
dispatch (depth-first approach), or that the new event waits until current one
finishes (breadth-first approach), or that it is unspecified? Do we guarantee
that when a listener is invoked it sees the new value that was set by the
latest nested change, or will it see the value that existed at trigger time, or
is it unspecified?
If listeners affect each other with nested events, then the dispatch order
matters.
---
If we answer "unspecified" to the questions above, it allows us more
implementation freedom. It will also mean that listeners should be thought of
as "lone wolves" - they are not intended to talk to each other or depend on
each other; each should do something regardless of what the others are doing
and assume its code "territory" is not affected by other listeners. The user
takes responsibility for coupling them (nested modifications).
On the other hand, no guarantees bring limited usage. The question is, what is
the intended usage?
-
PR: https://git.openjdk.org/jfx/pull/837
Re: RFR: 8290765: Remove parent disabled/treeVisible listeners [v5]
On Fri, 3 Feb 2023 23:31:24 GMT, Michael Strauß wrote: >> `Node` adds InvalidationListeners to its parent's `disabled` and >> `treeVisible` properties and calls its own `updateDisabled()` and >> `updateTreeVisible(boolean)` methods when the property values change. >> >> These listeners are not required, since `Node` can easily call the >> `updateDisabled()` and `updateTreeVisible(boolean)` methods on its children, >> saving the memory cost of maintaining listeners and bindings. > > Michael Strauß has updated the pull request incrementally with one additional > commit since the last revision: > > improved tests, changed foreach loop I missed that you can also change the foreach loop on the treeVisible method too, but it's not important. - Marked as reviewed by nlisker (Reviewer). PR: https://git.openjdk.org/jfx/pull/841
Re: RFR: 8290765: Remove parent disabled/treeVisible listeners [v4]
On Sat, 28 Jan 2023 16:24:30 GMT, Michael Strauß wrote:
>> `Node` adds InvalidationListeners to its parent's `disabled` and
>> `treeVisible` properties and calls its own `updateDisabled()` and
>> `updateTreeVisible(boolean)` methods when the property values change.
>>
>> These listeners are not required, since `Node` can easily call the
>> `updateDisabled()` and `updateTreeVisible(boolean)` methods on its children,
>> saving the memory cost of maintaining listeners and bindings.
>
> Michael Strauß has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Initialize treeVisible field directly
Looks good. I ran the tests and they pass. Left a few optional comments.
Maybe @kevinrushforth will want to have a look too before you merge.
modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 1917:
> 1915: for (Node child : parent.getChildren()) {
> 1916: child.updateDisabled();
> 1917: }
If you like this style, you could do
`parent.getChildren().forEach(Node::updateDisabled);`. I find these more
readable, but it's up to you.
modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 2608:
> 2606: //PerformanceTracker.logEvent("Node.postinit " +
> 2607: //"for [{this}, id=\"{id}\"]
> finished");
> 2608: //}
Not sure if these comments are intended to be used for something. I doubt it
though.
modules/javafx.graphics/src/test/java/test/javafx/scene/NodeTest.java line 2024:
> 2022:
> 2023: }
> 2024:
Since you changed the copyright year on the other files, you might want to do
this one too. There is a script that will do it anyway, so it doesn't matter.
modules/javafx.graphics/src/test/java/test/javafx/scene/NodeTest.java line 2039:
> 2037: assertTrue(n2.isDisabled());
> 2038: assertTrue(n2.isDisabled());
> 2039: }
I suggest to continue this test with a `g.setDisable(false)` to check both
directions. The first group of assertions test the initial state, not a change
to `false`.
Same for the other test.
-
Marked as reviewed by nlisker (Reviewer).
PR: https://git.openjdk.org/jfx/pull/841
Re: RFR: 8301604: Replace Collections.unmodifiableList with List.of [v3]
On Wed, 1 Feb 2023 23:50:07 GMT, Glavo wrote: >> `List.of` is cleaner, and can slightly reduce the memory footprint for lists >> of one or two elements. >> >> Because `List.of` can only store non-null elements, I have only replaced a >> few usage. >> >> Can someone open an Issue on JBS for me? > > Glavo has updated the pull request incrementally with one additional commit > since the last revision: > > reformat Marked as reviewed by nlisker (Reviewer). - PR: https://git.openjdk.org/jfx/pull/1012
Re: RFR: 8301604: Replace Collections.unmodifiableList with List.of [v2]
On Wed, 1 Feb 2023 23:38:57 GMT, Glavo wrote: >> `List.of` is cleaner, and can slightly reduce the memory footprint for lists >> of one or two elements. >> >> Because `List.of` can only store non-null elements, I have only replaced a >> few usage. >> >> Can someone open an Issue on JBS for me? > > Glavo has updated the pull request incrementally with two additional commits > since the last revision: > > - Update FileChooser > - Update getTracks You can put the argument list of the constructors and methods in one line. Also the `IllegalArgumentException` and its message can be in the same line. After you add the empty lines after the `FileChooser` class declaration and the `ExtensionFilter` class declaration this can go in. - PR: https://git.openjdk.org/jfx/pull/1012
Re: RFR: 8301604: Replace Collections.unmodifiableList with List.of
On Wed, 1 Feb 2023 16:54:30 GMT, John Hendrikx wrote:
>> modules/javafx.media/src/main/java/com/sun/media/jfxmedia/Media.java line
>> 103:
>>
>>> 101: }
>>> 102: }
>>> 103: return returnValue;
>>
>> This method can be reduced to
>>
>> public List getTracks() {
>> synchronized (tracks) {
>> return tracks.isEmpty() ? null : List.copyOf(tracks);
>> }
>> }
>>
>> though I find it highly questionable that it returns `null` for an empty
>> list instead of just an empty list. There are 2 use cases of this method and
>> both would do better with just an empty list.
>
> Yeah, I noticed this as well right away, it is documented to do this though.
> The documentation however does seem to suggest it might be possible that
> there are three results:
>
> 1. Tracks haven't been scanned yet -> `null`
> 2. Tracks have been scanned, but none where found -> empty list
> 3. Tracks have been scanned and some were found -> list
>
> Whether case 2 can ever happen is unclear, but distinguishing it from the
> case where nothing has been scanned yet with `null` does not seem
> unreasonable.
It's an internal class and no calling class makes this distinction. I don't
think it's meant to function in the way you described.
-
PR: https://git.openjdk.org/jfx/pull/1012
Re: RFR: 8301604: Replace Collections.unmodifiableList with List.of
On Wed, 1 Feb 2023 15:11:57 GMT, Glavo wrote: > I have considered this, but I didn't make this change because I was worried > that there would be less descriptive information when null was encountered. I think it's fine. The method is documented to throw and it happens immediately on entry. NPEs don't always have a message since they are straightforward. - PR: https://git.openjdk.org/jfx/pull/1012
Re: [External] : Re: Some classes could be sealed
For Material and LightBase it's because they are just facades whose implementation is in native code. You can't extend these without tampering with internals. I think that Camera and Shape3D also requires modifying internal stuff, though not at the native level. On Thu, Feb 2, 2023 at 12:38 AM John Hendrikx wrote: > I'm curious to know why these classes are not allowed to be subclassed > directly, as that may be important in order to decide whether these classes > should really be sealed. > > --John > On 01/02/2023 20:37, Kevin Rushforth wrote: > > I read the spec for sealed classes more carefully, and it turns out we > can't make Node sealed. At least not without API changes to SwingNode and > MediaView (and implementation changes to Printable in the javafx.web > module). All of the classes listed in the "permits" clause must be in the > same module, and SwingNode (javafx.swing) and MediaView (javafx.media) > extend Node directly they would need to be "permitted" subtypes, but there > is no way to specify that. We would also need to do something about the > tests that extend Node and run in the unnamed module. So this doesn't seem > feasible. > > We could still seal Shape, Shape3D, LightBase, and Material, since all > permitted implementation are in the javafx.graphics module. It may or may > not be worth doing that. > > -- Kevin > > > On 2/1/2023 9:45 AM, Nir Lisker wrote: > > I'll add that internal classes, mostly NG___ peers, can also benefit from > sealing. NGLightBase is an example. > > Material is another public class that can be sealed. > > On Wed, Feb 1, 2023 at 7:37 PM Kevin Rushforth > wrote: > >> I agree that we should only seal existing classes that could not have >> been extended by application classes. The ones I listed in my previous >> email fit that bill, since an attempt to subclass them will throw an >> exception when it is used in a scene graph. Each documents that subclassing >> is disallowed. >> >> Btw, we've already started making use of pattern-matching instanceof in >> the implementation anyway. It would be the first API change that relies on >> a JDK 17 feature, but for JavaFX 21, I see no problem in doing that. >> >> -- Kevin >> >> >> On 2/1/2023 9:06 AM, Philip Race wrote: >> >> In the JDK we've only sealed existing classes which provably could not >> have been extended by application classes, >> so I'm not sure about this .. >> >> also I think that might be the first change that absolutely means FX 21 >> can only be built with JDK 17 and later .. >> >> -phil >> >> On 2/1/23 8:59 AM, Thiago Milczarek Sayão wrote: >> >> Yes, sorry, I made the email title in plural, but I meant what Michael >> said, Node would be sealed permitting only what is needed for JavaFx >> internally. >> >> >> -- Thiago >> >> >> Em qua., 1 de fev. de 2023 às 13:48, Michael Strauß < >> michaelstr...@gmail.com> escreveu: >> >>> I don't think that's what Thiago is proposing. Only `Node` would be >>> sealed. >>> The following subclasses would be non-sealed: Parent, SubScene, >>> Camera, LightBase, Shape, Shape3D, Canvas, ImageView. >>> And then there are additional subclasses, which don't fit into this >>> idea since they are in other modules: SwingNode (in javafx.swing), >>> MediaView (in javafx.media), Printable (in javafx.web). >>> >>> >>> >>> On Wed, Feb 1, 2023 at 5:39 PM John Hendrikx >>> wrote: >>> > >>> > I think this may be a bit unclear from this post, but you're proposing >>> I think to make `Node`, `Shape` and `Shape3D` sealed. For those unaware, >>> you're not allowed to extend these classes (despite being public). For >>> example Node says in its documentation: >>> > >>> >* An application should not extend the Node class directly. Doing >>> so may lead to >>> >* an UnsupportedOperationException being thrown. >>> > >>> > Currently this is enforced at runtime in NodeHelper. >>> > >>> > --John >>> > >>> > On 01/02/2023 15:47, Thiago Milczarek Sayão wrote: >>> > >>> > Hi, >>> > >>> > NodeHelper.java has this: >>> > >>> > throw new UnsupportedOperationException( >>> > "Applications should not extend the " >>> > + nodeType + " class directly."); >>> > >>> > >>> > I think it's replaceable with selead classes. Am I right? >>> > >>> > The benefit will be compile time error instead of runtime. >>> > >>> > >>> > -- Thiago. >>> > >>> >> >> >> >
Re: [External] : Re: Some classes could be sealed
I held on proposing sealed class changes because they will be more beneficial once switch patterns are introduced. I also held on refactoring to records (of which I have a branch in my fork) for the reason that record patterns are not out yet. I think that once we have more pieces of the algebraic data types puzzle it will be very beneficial to do a decent amount of refactoring. There's nothing stopping us from starting with what we have for the purpose of upgrading errors from runtime to compile time, but we will need a second iteration anyway in some of these cases. On Wed, Feb 1, 2023 at 9:37 PM Kevin Rushforth wrote: > I read the spec for sealed classes more carefully, and it turns out we > can't make Node sealed. At least not without API changes to SwingNode and > MediaView (and implementation changes to Printable in the javafx.web > module). All of the classes listed in the "permits" clause must be in the > same module, and SwingNode (javafx.swing) and MediaView (javafx.media) > extend Node directly they would need to be "permitted" subtypes, but there > is no way to specify that. We would also need to do something about the > tests that extend Node and run in the unnamed module. So this doesn't seem > feasible. > > We could still seal Shape, Shape3D, LightBase, and Material, since all > permitted implementation are in the javafx.graphics module. It may or may > not be worth doing that. > > -- Kevin > > > On 2/1/2023 9:45 AM, Nir Lisker wrote: > > I'll add that internal classes, mostly NG___ peers, can also benefit from > sealing. NGLightBase is an example. > > Material is another public class that can be sealed. > > On Wed, Feb 1, 2023 at 7:37 PM Kevin Rushforth > wrote: > >> I agree that we should only seal existing classes that could not have >> been extended by application classes. The ones I listed in my previous >> email fit that bill, since an attempt to subclass them will throw an >> exception when it is used in a scene graph. Each documents that subclassing >> is disallowed. >> >> Btw, we've already started making use of pattern-matching instanceof in >> the implementation anyway. It would be the first API change that relies on >> a JDK 17 feature, but for JavaFX 21, I see no problem in doing that. >> >> -- Kevin >> >> >> On 2/1/2023 9:06 AM, Philip Race wrote: >> >> In the JDK we've only sealed existing classes which provably could not >> have been extended by application classes, >> so I'm not sure about this .. >> >> also I think that might be the first change that absolutely means FX 21 >> can only be built with JDK 17 and later .. >> >> -phil >> >> On 2/1/23 8:59 AM, Thiago Milczarek Sayão wrote: >> >> Yes, sorry, I made the email title in plural, but I meant what Michael >> said, Node would be sealed permitting only what is needed for JavaFx >> internally. >> >> >> -- Thiago >> >> >> Em qua., 1 de fev. de 2023 às 13:48, Michael Strauß < >> michaelstr...@gmail.com> escreveu: >> >>> I don't think that's what Thiago is proposing. Only `Node` would be >>> sealed. >>> The following subclasses would be non-sealed: Parent, SubScene, >>> Camera, LightBase, Shape, Shape3D, Canvas, ImageView. >>> And then there are additional subclasses, which don't fit into this >>> idea since they are in other modules: SwingNode (in javafx.swing), >>> MediaView (in javafx.media), Printable (in javafx.web). >>> >>> >>> >>> On Wed, Feb 1, 2023 at 5:39 PM John Hendrikx >>> wrote: >>> > >>> > I think this may be a bit unclear from this post, but you're proposing >>> I think to make `Node`, `Shape` and `Shape3D` sealed. For those unaware, >>> you're not allowed to extend these classes (despite being public). For >>> example Node says in its documentation: >>> > >>> >* An application should not extend the Node class directly. Doing >>> so may lead to >>> >* an UnsupportedOperationException being thrown. >>> > >>> > Currently this is enforced at runtime in NodeHelper. >>> > >>> > --John >>> > >>> > On 01/02/2023 15:47, Thiago Milczarek Sayão wrote: >>> > >>> > Hi, >>> > >>> > NodeHelper.java has this: >>> > >>> > throw new UnsupportedOperationException( >>> > "Applications should not extend the " >>> > + nodeType + " class directly."); >>> > >>> > >>> > I think it's replaceable with selead classes. Am I right? >>> > >>> > The benefit will be compile time error instead of runtime. >>> > >>> > >>> > -- Thiago. >>> > >>> >> >> >> >
Re: Some classes could be sealed
I'll add that internal classes, mostly NG___ peers, can also benefit from sealing. NGLightBase is an example. Material is another public class that can be sealed. On Wed, Feb 1, 2023 at 7:37 PM Kevin Rushforth wrote: > I agree that we should only seal existing classes that could not have been > extended by application classes. The ones I listed in my previous email fit > that bill, since an attempt to subclass them will throw an exception when > it is used in a scene graph. Each documents that subclassing is disallowed. > > Btw, we've already started making use of pattern-matching instanceof in > the implementation anyway. It would be the first API change that relies on > a JDK 17 feature, but for JavaFX 21, I see no problem in doing that. > > -- Kevin > > > On 2/1/2023 9:06 AM, Philip Race wrote: > > In the JDK we've only sealed existing classes which provably could not > have been extended by application classes, > so I'm not sure about this .. > > also I think that might be the first change that absolutely means FX 21 > can only be built with JDK 17 and later .. > > -phil > > On 2/1/23 8:59 AM, Thiago Milczarek Sayão wrote: > > Yes, sorry, I made the email title in plural, but I meant what Michael > said, Node would be sealed permitting only what is needed for JavaFx > internally. > > > -- Thiago > > > Em qua., 1 de fev. de 2023 às 13:48, Michael Strauß < > michaelstr...@gmail.com> escreveu: > >> I don't think that's what Thiago is proposing. Only `Node` would be >> sealed. >> The following subclasses would be non-sealed: Parent, SubScene, >> Camera, LightBase, Shape, Shape3D, Canvas, ImageView. >> And then there are additional subclasses, which don't fit into this >> idea since they are in other modules: SwingNode (in javafx.swing), >> MediaView (in javafx.media), Printable (in javafx.web). >> >> >> >> On Wed, Feb 1, 2023 at 5:39 PM John Hendrikx >> wrote: >> > >> > I think this may be a bit unclear from this post, but you're proposing >> I think to make `Node`, `Shape` and `Shape3D` sealed. For those unaware, >> you're not allowed to extend these classes (despite being public). For >> example Node says in its documentation: >> > >> >* An application should not extend the Node class directly. Doing so >> may lead to >> >* an UnsupportedOperationException being thrown. >> > >> > Currently this is enforced at runtime in NodeHelper. >> > >> > --John >> > >> > On 01/02/2023 15:47, Thiago Milczarek Sayão wrote: >> > >> > Hi, >> > >> > NodeHelper.java has this: >> > >> > throw new UnsupportedOperationException( >> > "Applications should not extend the " >> > + nodeType + " class directly."); >> > >> > >> > I think it's replaceable with selead classes. Am I right? >> > >> > The benefit will be compile time error instead of runtime. >> > >> > >> > -- Thiago. >> > >> > > >
Re: RFR: 8301604: Replace Collections.unmodifiableList with List.of
On Thu, 26 Jan 2023 05:30:56 GMT, Glavo wrote:
> `List.of` is cleaner, and can slightly reduce the memory footprint for lists
> of one or two elements.
>
> Because `List.of` can only store non-null elements, I have only replaced a
> few usage.
>
> Can someone open an Issue on JBS for me?
### FileChooser
`List.of` and `List.copyOf` already check for a `null` argument and `null`
elements. This means that `validateArgs` only needs to check the `length` of
`extensions` and for an empty element. Since the only difference between the
constructors is taking an array vs. taking a list, once a list is created from
the array, the array constructor can call the list constructor.
I suggest the following refactoring:
public ExtensionFilter(String description, String... extensions) {
this(description, List.of(extensions));
}
public ExtensionFilter(String description, List extensions) {
var extensionsList = List.copyOf(extensions);
validateArgs(description, extensionsList);
this.description = description;
this.extensions = extensionsList;
}
Note that `List.copyOf` will not create another copy if it was called from the
array constructor that already created an unmodifiable `List.of`.
Now validation can be reduced to
private static void validateArgs(String description, List
extensions) {
Objects.requireNonNull(description, "Description must not be null");
if (description.isEmpty()) {
throw new IllegalArgumentException("Description must not be
empty");
}
if (extensions.isEmpty()) {
throw new IllegalArgumentException("At least one extension must
be defined");
}
for (String extension : extensions) {
if (extension.isEmpty()) {
throw new IllegalArgumentException("Extension must not be
empty");
}
}
}
Aditionally, please add empty lines after the class declarations of
`FileChooser` and `ExtensionFilter`.
Also please correct the typo in `getTracks`: "The returned List
**is** unmodifiable."
modules/javafx.graphics/src/main/java/com/sun/javafx/iio/common/ImageDescriptor.java
line 41:
> 39: this.extensions = List.of(extensions);
> 40: this.signatures = List.of(signatures);
> 41: this.mimeSubtypes = List.of(mimeSubtypes);
While this is not equivalent since changing the backing array will not change
the resulting list anymore, I would consider the old code a bug and this the
correct fix.
Note that in `FileChooser` care is taken to create a copy of the supplied array
before using it to create a list.
Additionally, care must be taken that all the callers don't have `null`
elements in the arrays they pass. I checked it and it's fine (and should
probably be disallowed, which is good now).
By the way, this class should be a `record`, and all its subtypes, which are
not really subtypes but just configured instances, should be modified
accordingly. This is out of scope though.
modules/javafx.media/src/main/java/com/sun/media/jfxmedia/Media.java line 100:
> 98: returnValue = null;
> 99: } else {
> 100: returnValue = List.copyOf(tracks);
This is fine because `addTrack` checks for `null` elements.
modules/javafx.media/src/main/java/com/sun/media/jfxmedia/Media.java line 103:
> 101: }
> 102: }
> 103: return returnValue;
This method can be reduced to
public List getTracks() {
synchronized (tracks) {
return tracks.isEmpty() ? null : List.copyOf(tracks);
}
}
though I find it highly questionable that it returns `null` for an empty list
instead of just an empty list. There are 2 use cases of this method and both
would do better with just an empty list.
-
Changes requested by nlisker (Reviewer).
PR: https://git.openjdk.org/jfx/pull/1012
Re: RFR: 8301604: Replace Collections.unmodifiableList with List.of
On Wed, 1 Feb 2023 11:21:04 GMT, Glavo wrote: >> Filed: https://bugs.openjdk.org/browse/JDK-8301604. Please let me know if >> the issue is not reported correctly. > > @theaoqi Thank you very much! @Glavo If you can't access JBS, you can submit a report via bugreport.java.com. @theaoqi If you create a JBS issue, fill in the affected version and fix version (`tbd` is fine) fields. The subcomponent is also not only graphics because there are changes to media as well. Use `other` for overarching changes like these (e.g., https://bugs.openjdk.org/browse/JDK-8208761). I can sponsor this fix, but it will need some more changes. Just a drop-in replacement with `List.of` doesn't result in good code. I will review it shortly. - PR: https://git.openjdk.org/jfx/pull/1012
Re: RFR: 8138842: TableViewSelectionModel.selectIndices does not select index 0 [v2]
On Wed, 1 Feb 2023 09:17:33 GMT, Karthik P K wrote: >> I think it is also pretty clear the original author intended to check >> `rows.length == 0` and made the mistake that it would be called with `rows >> == null` when there are no further indices specified, which is incorrect. > > I believe no code change is required as of now. Hence not making any changes > now. I also think that `null` should throw unless otherwise specified. - PR: https://git.openjdk.org/jfx/pull/1018
Re: RFR: 8138842: TableViewSelectionModel.selectIndices does not select index 0
On Tue, 31 Jan 2023 14:40:00 GMT, Karthik P K wrote: > In `selectIndices` method, zero length array is not considered while ignoring > row number given as parameter. > > Updated the code to consider both null and zero length array in the condition > before ignoring the row value given as parameter. > > Added unit test to validate the fix `TreeTableView` has the same problem. - PR: https://git.openjdk.org/jfx/pull/1018
Re: RFR: 8138842: TableViewSelectionModel.selectIndices does not select index 0
On Tue, 31 Jan 2023 14:40:00 GMT, Karthik P K wrote: > In `selectIndices` method, zero length array is not considered while ignoring > row number given as parameter. > > Updated the code to consider both null and zero length array in the condition > before ignoring the row value given as parameter. > > Added unit test to validate the fix I recently looked at https://bugs.openjdk.org/browse/JDK-8120635. Is it related? I managed to reproduce it, so I think it should be reopened. - PR: https://git.openjdk.org/jfx/pull/1018
Re: [Libguestfs] [libnbd PATCH v2 3/3] nbdsh: Improve --help and initial banner contents.
On Tue, Jan 31, 2023 at 12:34 AM Richard W.M. Jones wrote: > > On Fri, Nov 04, 2022 at 04:18:31PM -0500, Eric Blake wrote: > > Document all options in --help output. If -n is not in use, then > > enhance the banner to print the current state of h, and further tailor > > the advice given on useful next steps to take to mention opt_go when > > using --opt-mode. > > I had to partially revert this patch (reverting most of it) because it > unfortunately breaks the implicit handle creation :-( > > https://gitlab.com/nbdkit/libnbd/-/commit/5a02c7d2cc6a201f9e5531c0c20c2f3c22b805a2 > > I'm not actually sure how to do this correctly in Python. I made > several attempts, but I don't think Python is very good about having a > variable which is only defined on some paths -- maybe it's not > possible at all. Can you share the error when it breaks? I'm not sure what is the issue, but usually if you have a global variable created only in some flows, adding: thing = None At the start of the module makes sure that the name exists later, regardless of the flow taken. Code can take the right action based on: if thing is None: ... Nir ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH v2v v2] -o rhv-upload: Improve error message for invalid or missing -os parameter
On Fri, Jan 27, 2023 at 2:26 PM Richard W.M. Jones wrote:
>
> For -o rhv-upload, the -os parameter specifies the storage domain.
> Because the RHV API allows globs when searching for a domain, if you
> used a parameter like -os 'data*' then this would confuse the Python
> code, since it can glob to the name of a storage domain, but then
> later fail when we try to exact match the storage domain we found.
> The result of this was a confusing error in the precheck script:
>
> IndexError: list index out of range
>
> This fix validates the output storage parameter before trying to use
> it. Since valid storage domain names cannot contain glob characters
> or spaces, it avoids the problems above and improves the error message
> that the user sees:
>
> $ virt-v2v [...] -o rhv-upload -os ''
> ...
> RuntimeError: The storage domain (-os) parameter ‘’ is not valid
> virt-v2v: error: failed server prechecks, see earlier errors
>
> $ virt-v2v [...] -o rhv-upload -os 'data*'
> ...
> RuntimeError: The storage domain (-os) parameter ‘data*’ is not valid
> virt-v2v: error: failed server prechecks, see earlier errors
>
Makes sense, the new errors are very helpful.
> Although the IndexError should no longer happen, I also added a
> try...catch around that code to improve the error in case it still
> happens.
Theoretically it can happen if the admin changes the storage domain
name or detaches the domain from the data center in the window
after the precheck completes and before the transfer starts.
>
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1986386
> Reported-by: Junqin Zhou
> Thanks: Nir Soffer
> ---
> output/rhv-upload-precheck.py | 27 +--
> 1 file changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/output/rhv-upload-precheck.py b/output/rhv-upload-precheck.py
> index 1dc1b8498a..ba125611ba 100644
> --- a/output/rhv-upload-precheck.py
> +++ b/output/rhv-upload-precheck.py
> @@ -18,6 +18,7 @@
>
> import json
> import logging
> +import re
> import sys
>
> from urllib.parse import urlparse
> @@ -46,6 +47,15 @@ output_password = output_password.rstrip()
> parsed = urlparse(params['output_conn'])
> username = parsed.username or "admin@internal"
>
> +# Check the storage domain name is valid
> +# (https://bugzilla.redhat.com/show_bug.cgi?id=1986386#c1)
> +# Also this means it cannot contain spaces or glob symbols, so
> +# the search below is valid.
> +output_storage = params['output_storage']
> +if not re.match('^[-a-zA-Z0-9_]+$', output_storage):
The comment in the bug does not point to the docs or code enforcing
the domain name restrictions, but I validated with ovirt 4.5. Trying to
create a domain name with a space or using Hebrew characters is blocked
in the UI, displaying an error. See attached screenshots.
I think it is highly unlikely that this limit will change in the
future since nobody
is working on oVirt now, but if it does change this may prevent uploading to an
existing storage domain.
> +raise RuntimeError("The storage domain (-os) parameter ‘%s’ is not
> valid" %
> + output_storage)
> +
> # Connect to the server.
> connection = sdk.Connection(
> url=params['output_conn'],
> @@ -60,28 +70,33 @@ system_service = connection.system_service()
>
> # Check whether there is a datacenter for the specified storage.
> data_centers = system_service.data_centers_service().list(
> -search='storage.name=%s' % params['output_storage'],
> +search='storage.name=%s' % output_storage,
> case_sensitive=True,
> )
> if len(data_centers) == 0:
> storage_domains = system_service.storage_domains_service().list(
> -search='name=%s' % params['output_storage'],
> +search='name=%s' % output_storage,
> case_sensitive=True,
> )
> if len(storage_domains) == 0:
> # The storage domain does not even exist.
> raise RuntimeError("The storage domain ‘%s’ does not exist" %
> - (params['output_storage']))
> + output_storage)
>
> # The storage domain is not attached to a datacenter
> # (shouldn't happen, would fail on disk creation).
> raise RuntimeError("The storage domain ‘%s’ is not attached to a DC" %
> - (params['output_storage']))
> + output_storage)
> datacenter = data_centers[0]
>
> # Get the storage domain.
> storage_domains = connection.follow_link(datacenter.storage_domains)
> -storage_domain = [sd for sd in storage_domains if sd.name ==
> params['output_storage']][0]
> +try:
> +storage_domain = [sd for sd in s
Re: [Libguestfs] [PATCH v2v] -o rhv-upload: Give a nicer error if the storage domain does not exist
On Fri, Jan 27, 2023 at 1:18 PM Nir Soffer wrote:
>
> On Thu, Jan 26, 2023 at 2:31 PM Richard W.M. Jones wrote:
> >
> > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1986386
> > Reported-by: Junqin Zhou
> > ---
> > output/rhv-upload-precheck.py | 7 ++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/output/rhv-upload-precheck.py b/output/rhv-upload-precheck.py
> > index 1dc1b8498a..35ea021032 100644
> > --- a/output/rhv-upload-precheck.py
> > +++ b/output/rhv-upload-precheck.py
> > @@ -81,7 +81,12 @@ datacenter = data_centers[0]
> >
> > # Get the storage domain.
> > storage_domains = connection.follow_link(datacenter.storage_domains)
> > -storage_domain = [sd for sd in storage_domains if sd.name ==
> > params['output_storage']][0]
> > +try:
> > +storage_domain = [sd for sd in storage_domains \
> > + if sd.name == params['output_storage']][0]
>
> Using `\` may work but it is needed. You can do this:
>
> storage_domain = [sd for sd in storage_domains
>if sd.name == params['output_storage']][0]
>
> This is also the common way to indent list comprehension that
> makes the expression more clear.
>
> > +except IndexError:
> > +raise RuntimeError("The storage domain ‘%s’ does not exist" %
> > + params['output_storage'])
>
> The fix is safe and makes sense.
>
> Not sure why we list all storage domains when we already know the name,
> maybe Albert would like to clean up this mess later.
Like this:
https://github.com/oVirt/python-ovirt-engine-sdk4/blob/2aa50266056b7ee0b72597f346cbf0f006041566/examples/list_storage_domains.py#L93
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH v2v] -o rhv-upload: Give a nicer error if the storage domain does not exist
On Thu, Jan 26, 2023 at 2:31 PM Richard W.M. Jones wrote:
>
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1986386
> Reported-by: Junqin Zhou
> ---
> output/rhv-upload-precheck.py | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/output/rhv-upload-precheck.py b/output/rhv-upload-precheck.py
> index 1dc1b8498a..35ea021032 100644
> --- a/output/rhv-upload-precheck.py
> +++ b/output/rhv-upload-precheck.py
> @@ -81,7 +81,12 @@ datacenter = data_centers[0]
>
> # Get the storage domain.
> storage_domains = connection.follow_link(datacenter.storage_domains)
> -storage_domain = [sd for sd in storage_domains if sd.name ==
> params['output_storage']][0]
> +try:
> +storage_domain = [sd for sd in storage_domains \
> + if sd.name == params['output_storage']][0]
Using `\` may work but it is needed. You can do this:
storage_domain = [sd for sd in storage_domains
if sd.name == params['output_storage']][0]
This is also the common way to indent list comprehension that
makes the expression more clear.
> +except IndexError:
> +raise RuntimeError("The storage domain ‘%s’ does not exist" %
> + params['output_storage'])
The fix is safe and makes sense.
Not sure why we list all storage domains when we already know the name,
maybe Albert would like to clean up this mess later.
Nir
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH v2v] -o rhv-upload: Give a nicer error if the storage domain
On Thu, Jan 26, 2023 at 2:31 PM Richard W.M. Jones wrote: > > https://bugzilla.redhat.com/show_bug.cgi?id=1986386 > > My RHV instance is dead at the moment so I didn't do much more than > check this compiles and passes the one test we have. Also I want to > spend as little time as possible on RHV outputs for virt-v2v since the > RHV product will be discontinued soon. > > I did want to point out some things: > > - The preceeding code is probably wrong. > > https://github.com/libguestfs/virt-v2v/blob/master/output/rhv-upload-transfer.py#L70 > >It attempts to search for the output storage using: > > storage_domains = system_service.storage_domains_service().list( > search='name=%s' % params['output_storage'], > case_sensitive=True, > ) I think the search is correct. This is explained in https://bugzilla.redhat.com/1986386#c1 >I couldn't find any documentation about what can go into that >search string, but it's clearly a lot more complicated than just >pasting in the literal name after "name=". At the very least, >spaces are not permitted, see: > > https://github.com/libguestfs/virt-v2v/blob/master/output/rhv-upload-transfer.py#L70 True, search can be an expression. > - The bug reporter used "data*" as the name and I suspect that is >parsed in some way (wildcard? regexp? I've no idea). It is treated as glob pattern, also explained in comment 1. > - Probably for the same reason, the preceeding code ought to fail >with an error if the output storage domain doesn't exist. The fact >we reach the code patched here at all also indicates some bug, >maybe in the search string. > > As I say above, I don't especially care about any of this. I'm not working on RHV since August 2022. Adding Albert who is current RHV storage maintainer. Nir ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [jfx20] RFR: 8290863: Update the documentation of Virtualized controls to include the best practice of not using Nodes directly in the item list [v3]
On Tue, 24 Jan 2023 09:59:48 GMT, Ajit Ghaisas wrote:
>> modules/javafx.controls/src/main/java/javafx/scene/control/ListView.java
>> line 152:
>>
>>> 150: * Warning: Nodes should not be inserted directly into the items
>>> list
>>> 151: * ListView allows for the items list to contain elements of any type,
>>> including
>>> 152: * {@link Node} instances. Putting nodes into
>>
>> `Node` is already linked above so there's no need for it, but it's fine to
>> leave as is.
>
> The first occurrence of `{@link Node}` is on line 152. I have kept the first
> occurrence and replaced others with `{@code Node}`
It's linked in the type hierarchy, but I'm fine with keeping an inline link.
-
PR: https://git.openjdk.org/jfx/pull/995
Re: [jfx20] RFR: 8290863: Update the documentation of Virtualized controls to include the best practice of not using Nodes directly in the item list [v4]
On Tue, 24 Jan 2023 10:35:36 GMT, Ajit Ghaisas wrote: > > > > @nlisker, Can you please file an issue explaining how exactly you would like > `ListView` documentation to be restructured? Yes, I'll probably work on it during RDP 2. It's not the only control I want to go over. - PR: https://git.openjdk.org/jfx/pull/995
Re: TableViewBuilder idea
Isn't this something that an external library can do? I would think that
writing an adapter that can read JPA annotations is the way to go. After
all, most entities are not written with JavaFX properties.
On Sun, Jan 22, 2023 at 2:08 AM Thiago Milczarek Sayão <
thiago.sa...@gmail.com> wrote:
> Hi,
>
> TableView data usually comes from a database and is mapped to java objects
> (I don't have actual statistics on this but it's probably a true assertion).
> It Would be very useful if we could just copy database mapped objects to
> javafx observables and say: Display it on a tableview.
> Configuration of each column would be done as an annotation, like this:
>
> public class EmployeeObservable {
> @Column(label = "Registration", style = "-fx-alignment: right")
> private IntegerProperty registration = new SimpleIntegerProperty();
>
> @Column(label = "Name")
> private StringProperty name = new SimpleStringProperty();
>
> @Column(label = "Salary", converter = BigDecimalCurrencyConverter.class)
> private ObjectProperty salary = new SimpleObjectProperty<>();
>
> //boilerplate here
> }
>
>
> Other annotation options:
>
> @Column(ignore = true)
>
> @Column(graphicsConverter = CheckBoxGraphicsConverter.class)
>
> And to apply it:
>
> TableViewBuilder builder = new
> TableViewBuilder<>(tableView, EmployeeObservable.class);
> builder.build();
>
>
> I actually have implemented this and it's very useful. I can't share it
> because I've done it for a private company, but it's easy to re-do it.
>
> I'm not sure if it would go in a general purpose toolkit, but I thought it
> would be nice to share (the idea).
>
> -- Thiago.
>
>
Re: RFC: new property in ToggleGroup
We already have ToggleButton:
Unlike RadioButtons, ToggleButtons in a ToggleGroup do not attempt to force
> at least one selected ToggleButton in the group.
That is, if a ToggleButton is selected, clicking on it will cause it to
> become unselected.
With RadioButton, clicking on the selected button in the group will have no
> effect.
Checkboxes tend to allow multiple selection ("select all items that apply
to you"), which means they don't need to toggle each other, they are
independent. They also have indeterminate states, which I don't know how to
address regarding ToggleGroup selection policies.
On Sat, Jan 21, 2023 at 6:33 PM Michael Strauß
wrote:
> My expectation with radio groups is that they may start out initially
> unselected, but once a selection has been made, no user interaction
> can unselect all toggles again.
> I don't see any compelling reason to change that. If the group needs
> to be reset to its initial state, that's as easy as calling
> `toggleGroup.selectToggle(null)`.
>
> However, what you're describing sounds to me like a group of
> checkboxes. I've seen this type of grouping many times in various
> applications, and checkboxes are generally understood to be
> deselectable.
> So my question is: why don't we instead have CheckBox implement
> Toggle, allowing it to be used with ToggleGroup? That would make it
> possible to create a deselectable group of toggles.
>
> On Fri, Jan 20, 2023 at 8:31 PM Andy Goryachev
> wrote:
> >
> > Dear colleagues:
> >
> >
> >
> > In the context of a recent PR
> >
> >
> >
> > https://bugs.openjdk.org/browse/JDK-8237505
> >
> > https://github.com/openjdk/jfx/pull/1002
> >
> >
> https://stackoverflow.com/questions/57911107/javafx-togglegroup-not-functioning-properly-with-accelerators-radiomenuitem
> >
> >
> >
> > where a number of RadioMenuItems belonging to a toggle group are added
> to the menu, we might want to add a new property to the ToggleGroup which
> controls whether all items in a group can be deselected.
> >
> >
> >
> > If this property is set, a selected radio menu item can be deselected
> via either keyboard accelerator or a mouse click. If not, then not only
> this operation cannot be performed, but also the first item added to said
> ToggleGroup gets automatically selected.
> >
> >
> >
> > This should allow for more flexibility in creating menus with
> RadioMenuItems, but eliminate some boilerplate code required in such cases.
> >
> >
> >
> > The new logic would also affect any Toggle, such as ToggleButton.
> >
> >
> >
> > What do you think? Thank you.
> >
> >
> >
> > -andy
>
Re: RFC: new property in ToggleGroup
I don't see it being especially useful. GUI's tend to work this way. I remember it was the same in Swing. On Sat, Jan 21, 2023 at 1:41 AM Kevin Rushforth wrote: > > > On 1/20/2023 2:57 PM, Andy Goryachev wrote: > > I just want to add one thing - the initial state of RadioMenuItems added > to a menu is unselected, even if they belong to a ToggleGroup. > > > > One can say that having all unselected radio buttons in a toggle group > makes no sense, or perhaps it depends on the application requirements - > though I cannot find an example where it might be needed. > > > Yes, it's initially unselected unless the app makes an explicit selection. > HTML radio buttons work the same way [1]. Some apps use that to indicate > that nothing was selected, but then once you select it there will always be > something selected. > > > > > So either we allow two different policies by adding a property to the > ToggleGroup, or we proclaim that adding a radio button to a toggle group > must have a side effect of one (first) button to be automatically selected, > otherwise the whole group is in inconsistent state (or the app developer > must write some code to select one). > > > I wouldn't want to change the default behavior, since I can imagine some > apps relying on being able to tell if the user has ever selected any of the > choices. > > Having two properties would be one solution, presuming we think that we > need to provide a way for the app to indicate that it wants us to enforce > the invariant of ensuring that the app can't ever get the control in a > state where nothing is selected. Although, since it would be an opt-in, the > app could just as easily set the default itself as opposed to setting this > new property. > > -- Kevin > > [1] > https://www.w3schools.com/tags/tryit.asp?filename=tryhtml5_input_type_radio > > > > -andy > > > > > > > > > > *From: *openjfx-dev > on behalf of Kevin Rushforth > > *Date: *Friday, 2023/01/20 at 12:27 > *To: *openjfx-dev@openjdk.org > > *Subject: *Re: RFC: new property in ToggleGroup > > How common a UI feature is being able to deselect the selected item in a > ToggleGroup via the UI such that no item is selected? I don't normally see > that in various apps or toolkits that I am familiar with. What I do see is > that either a default item is selected or no item is selected initially > (which is the one and only time that there will be no item selected), but > in both case, once you make a selection, there is no way via the UI to > deselect the current item. Absent a compelling need, I think the current > behavior (once the fix for JDK-8237505 is integrated) is sufficient. > > What do other developers think? > > -- Kevin > > On 1/20/2023 11:31 AM, Andy Goryachev wrote: > > Dear colleagues: > > > > In the context of a recent PR > > > > https://bugs.openjdk.org/browse/JDK-8237505 > > https://github.com/openjdk/jfx/pull/1002 > > > https://stackoverflow.com/questions/57911107/javafx-togglegroup-not-functioning-properly-with-accelerators-radiomenuitem > > > > where a number of RadioMenuItems belonging to a toggle group are added to > the menu, we might want to add a new property to the ToggleGroup which > controls whether all items in a group can be deselected. > > > > If this property is set, a selected radio menu item can be deselected via > either keyboard accelerator or a mouse click. If not, then not only this > operation cannot be performed, but also the first item added to said > ToggleGroup gets automatically selected. > > > > This should allow for more flexibility in creating menus with > RadioMenuItems, but eliminate some boilerplate code required in such cases. > > > > The new logic would also affect any Toggle, such as ToggleButton. > > > > What do you think? Thank you. > > > > -andy > > > > >
Re: [jfx20] RFR: 8290863: Update the documentation of Virtualized controls to include the best practice of not using Nodes directly in the item list [v3]
On Wed, 18 Jan 2023 09:25:08 GMT, Ajit Ghaisas wrote:
>> This PR adds a warning about inserting Nodes directly into the virtualized
>> containers such as ListView, TreeView, TableView and TreeTableView. It also
>> adds code snippets showing the recommended pattern of using a custom cell
>> factory for each of the virtualized control.
>
> Ajit Ghaisas has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove extra spaces
modules/javafx.controls/src/main/java/javafx/scene/control/ListView.java line
151:
> 149: *
> 150: * Warning: Nodes should not be inserted directly into the items
> list
> 151: * ListView allows for the items list to contain elements of any type,
> including
`ListView` should be in `@code`
modules/javafx.controls/src/main/java/javafx/scene/control/ListView.java line
152:
> 150: * Warning: Nodes should not be inserted directly into the items
> list
> 151: * ListView allows for the items list to contain elements of any type,
> including
> 152: * {@link Node} instances. Putting nodes into
`Node` is already linked above so there's no need for it, but it's fine to
leave as is.
modules/javafx.controls/src/main/java/javafx/scene/control/ListView.java line
163:
> 161: * Avoid creating new {@link Node}s in custom {@link
> #cellFactoryProperty() cell factory} {@code updateItem} method.
> 162: *
> 163: * The following minimal example shows how to create a custom cell
> factory for {@code ListView} containing {@link Node}s:
No need to link `Node` here again.
-
PR: https://git.openjdk.org/jfx/pull/995
Re: [jfx20] RFR: 8290863: Update the documentation of Virtualized controls to include the best practice of not using Nodes directly in the item list [v4]
On Fri, 20 Jan 2023 11:16:04 GMT, Ajit Ghaisas wrote: >> This PR adds a warning about inserting Nodes directly into the virtualized >> containers such as ListView, TreeView, TableView and TreeTableView. It also >> adds code snippets showing the recommended pattern of using a custom cell >> factory for each of the virtualized control. > > Ajit Ghaisas has updated the pull request incrementally with one additional > commit since the last revision: > > Additional review fixes Left some minor inline comments that are relevant for the other classes here as well. Looking at the current `ListView` docs, I think it's not very informative. The main paragraph talks about generics in Java: > A ListView is able to have its generic type set to represent the type of data > in the backing model. Doing this has the benefit of making various methods in > the ListView, as well as the supporting classes (mentioned below), type-safe. > In addition, making use of the generic type supports substantially simplified > development of applications making use of ListView, as all modern IDEs are > able to auto-complete far more successfully with the additional type > information. I think that all of that should be removed, and your explanation on how to actually use `ListView` should be added instead. While it's tied to the "Customizing Visuals" section, the points of `ListView` is for it contain data/model instances which are interpreted as a visual `Node` by the `ListView`, so it should belong in the main paragraph. I can come up with a redistribution of the paragraphs if you want and if you don't consider this out of scope. - PR: https://git.openjdk.org/jfx/pull/995
Re: [jfx20] RFR: 8290863: Update the documentation of Virtualized controls to include the best practice of not using Nodes directly in the item list
On Fri, 13 Jan 2023 12:32:53 GMT, Ajit Ghaisas wrote: > This PR adds a warning about inserting Nodes directly into the virtualized > containers such as ListView, TreeView, TableView and TreeTableView. It also > adds code snippets showing the recommended pattern of using a custom cell > factory for each of the virtualized control. Instead of repeating this in every class, maybe writing it once in `Cell` and linking to it from these classes will be better? - PR: https://git.openjdk.org/jfx/pull/995
Re: [jfx20] RFR: 8290863: Update the documentation of Virtualized controls to include the best practice of not using Nodes directly in the item list
On Fri, 13 Jan 2023 12:32:53 GMT, Ajit Ghaisas wrote:
> This PR adds a warning about inserting Nodes directly into the virtualized
> containers such as ListView, TreeView, TableView and TreeTableView. It also
> adds code snippets showing the recommended pattern of using a custom cell
> factory for each of the virtualized control.
modules/javafx.controls/src/main/java/javafx/scene/control/ListView.java line
196:
> 194: * This example has an anonymous custom {@code ListCell} class in
> the custom cell factory.
> 195: * Note that the Rectangle (Node) object needs to be created in the
> custom {@code ListCell} class
> 196: * or in it's constructor and updated/used in it's updateItem method.
* it's -> its (both places)
* `updateItem` should be in `@code`.
Same for the other classes.
-
PR: https://git.openjdk.org/jfx/pull/995
Re: RFR: 8217853: Cleanup in the D3D native pipeline [v11]
On Sun, 25 Dec 2022 04:04:40 GMT, Nir Lisker wrote: >> Refactoring and renaming changes to some of the D3D pipeline files and a few >> changes on the Java side. These are various "leftovers" from previous issues >> that we didn't want to touch at the time in order to confine the scope of >> the changes. They will make future work easier. >> >> Since there are many small changes, I'm giving a full list here: >> >> **Java** >> >> * `NGShape3D.java` >> * Extracted methods to help with the cumbersome lighting loop: one method >> per light type + empty light (reset light) + default point light. This >> section of the code would benefit from the upcoming pattern matching on >> `switch`. >> * Normalized the direction here instead of in the native code. >> * Ambient light is now only set when it exists (and is not black). >> * `NGPointLight,java` - removed unneeded methods that were used by >> `NGShape3D` before the per-lighting methods were extracted (point light >> doesn't need spotlight-specific methods since they each have their own "add" >> method). >> * `NGSpotLight.java` - removed `@Override` annotations as a result of the >> above change. >> >> **Native C++** >> >> * Initialized the class members of `D3DLight`, `D3DMeshView` and >> `D3DPhongMaterial` in the header file instead of a more cumbersome >> initialization in the constructor (this is allowed since C++ 11). >> * `D3DLight` >> * Commented out unused methods. Were they supposed to be used at some >> point? >> * Renamed the `w` component to `lightOn` since it controls the number of >> lights for the special pixel shader variant and having it in the 4th >> position of the color array was confusing. >> * `D3DPhongShader.h` >> * Renamed some of the register constants for more clarity. >> * Moved the ambient light color constant from the vertex shader to the >> pixel shader (see the shader section below). I don't understand the >> calculation of the number of registers in the comment there: "8 ambient >> points + 2 coords = 10". There is 1 ambient light, what are the ambient >> points and coordinates? In `vsConstants` there is `gAmbinetData[10]`, but it >> is unused. >> * Reduced the number of assigned vertex register for the `VSR_LIGHTS` >> constant since it included both position and color, but color was unused >> there (it was used directly in the pixel shader), so now it's only the >> position. >> * `D3DMeshView.cc` >> * Unified the lighting loop that prepares the lights' 4-vetors that are >> passed to the shaders. >> * Removed the direction normalization as stated in the change for >> `NGShape3D.java`. >> * Reordered the shader constant assignment to be the same order as in >> `D3DPhongShader.h`. >> >> **Native shaders** >> * Renamed many of the variables to what I think are more descriptive names. >> This includes noting in which space they exist as some calculations are done >> in model space, some in world space, and we might need to do some in view >> space. For vectors, also noted if the vector is to or from (`eye` doesn't >> tell me if it's from or to the camera). >> * Commented out many unused functions. I don't know what they are for, so I >> didn't remove them. >> * `vsConstants` >> * Removed the light color from here since it's unused, only the position >> is. >> * Removed the ambient light color constant from here since it's unused, >> and added it to `psConstants` instead. >> * `vs2ps` >> * Removed the ambient color interpolation, which frees a register (no >> change in performance). >> * Simplified the structure (what is `LocalBumpOut` and why is it called >> `light` and contains `LocalBump`?). >> * `Mtl1PS` and `psMath` >> * Moved the shader variant constants (`#ifndef`) to `Mtl1PS` where they >> are used for better clarity. >> * Moved the lights loop to `Mtl1PS`. The calculation itself remains in >> `psMath`. >> >> No changes in performance were measured and the behavior stayed the same. > > Nir Lisker has updated the pull request incrementally with two additional > commits since the last revision: > > - Changed comment as suggested > - Removed unused fields The change that moved `float2 texD : texcoord0;` from the beginning to the end is the one that fixed it for me, and apparently for you. If you can move it back to the beginning of the struct and check if it fails, it would be helpful. I will add a comment there in a later PR. - PR: https://git.openjdk.org/jfx/pull/789
Re: RFR: 8217853: Cleanup in the D3D native pipeline [v11]
On Sun, 25 Dec 2022 04:04:40 GMT, Nir Lisker wrote: >> Refactoring and renaming changes to some of the D3D pipeline files and a few >> changes on the Java side. These are various "leftovers" from previous issues >> that we didn't want to touch at the time in order to confine the scope of >> the changes. They will make future work easier. >> >> Since there are many small changes, I'm giving a full list here: >> >> **Java** >> >> * `NGShape3D.java` >> * Extracted methods to help with the cumbersome lighting loop: one method >> per light type + empty light (reset light) + default point light. This >> section of the code would benefit from the upcoming pattern matching on >> `switch`. >> * Normalized the direction here instead of in the native code. >> * Ambient light is now only set when it exists (and is not black). >> * `NGPointLight,java` - removed unneeded methods that were used by >> `NGShape3D` before the per-lighting methods were extracted (point light >> doesn't need spotlight-specific methods since they each have their own "add" >> method). >> * `NGSpotLight.java` - removed `@Override` annotations as a result of the >> above change. >> >> **Native C++** >> >> * Initialized the class members of `D3DLight`, `D3DMeshView` and >> `D3DPhongMaterial` in the header file instead of a more cumbersome >> initialization in the constructor (this is allowed since C++ 11). >> * `D3DLight` >> * Commented out unused methods. Were they supposed to be used at some >> point? >> * Renamed the `w` component to `lightOn` since it controls the number of >> lights for the special pixel shader variant and having it in the 4th >> position of the color array was confusing. >> * `D3DPhongShader.h` >> * Renamed some of the register constants for more clarity. >> * Moved the ambient light color constant from the vertex shader to the >> pixel shader (see the shader section below). I don't understand the >> calculation of the number of registers in the comment there: "8 ambient >> points + 2 coords = 10". There is 1 ambient light, what are the ambient >> points and coordinates? In `vsConstants` there is `gAmbinetData[10]`, but it >> is unused. >> * Reduced the number of assigned vertex register for the `VSR_LIGHTS` >> constant since it included both position and color, but color was unused >> there (it was used directly in the pixel shader), so now it's only the >> position. >> * `D3DMeshView.cc` >> * Unified the lighting loop that prepares the lights' 4-vetors that are >> passed to the shaders. >> * Removed the direction normalization as stated in the change for >> `NGShape3D.java`. >> * Reordered the shader constant assignment to be the same order as in >> `D3DPhongShader.h`. >> >> **Native shaders** >> * Renamed many of the variables to what I think are more descriptive names. >> This includes noting in which space they exist as some calculations are done >> in model space, some in world space, and we might need to do some in view >> space. For vectors, also noted if the vector is to or from (`eye` doesn't >> tell me if it's from or to the camera). >> * Commented out many unused functions. I don't know what they are for, so I >> didn't remove them. >> * `vsConstants` >> * Removed the light color from here since it's unused, only the position >> is. >> * Removed the ambient light color constant from here since it's unused, >> and added it to `psConstants` instead. >> * `vs2ps` >> * Removed the ambient color interpolation, which frees a register (no >> change in performance). >> * Simplified the structure (what is `LocalBumpOut` and why is it called >> `light` and contains `LocalBump`?). >> * `Mtl1PS` and `psMath` >> * Moved the shader variant constants (`#ifndef`) to `Mtl1PS` where they >> are used for better clarity. >> * Moved the lights loop to `Mtl1PS`. The calculation itself remains in >> `psMath`. >> >> No changes in performance were measured and the behavior stayed the same. > > Nir Lisker has updated the pull request incrementally with two additional > commits since the last revision: > > - Changed comment as suggested > - Removed unused fields > Btw, I can confirm that yes, this fixed it for me. Specifically, commit > [55fe2dc](https://github.com/openjdk/jfx/commit/55fe2dc7371f6dcb12c414c5d672728e47e9c504) > has resolved my issue. If you have time, it would be interesting check if it breaks for you by changing the order of the semantics. It might be worth adding a comment there because it's a rather surprising side effect. - PR: https://git.openjdk.org/jfx/pull/789
Integrated: 8217853: Cleanup in the D3D native pipeline
On Mon, 2 May 2022 16:05:08 GMT, Nir Lisker wrote: > Refactoring and renaming changes to some of the D3D pipeline files and a few > changes on the Java side. These are various "leftovers" from previous issues > that we didn't want to touch at the time in order to confine the scope of the > changes. They will make future work easier. > > Since there are many small changes, I'm giving a full list here: > > **Java** > > * `NGShape3D.java` > * Extracted methods to help with the cumbersome lighting loop: one method > per light type + empty light (reset light) + default point light. This > section of the code would benefit from the upcoming pattern matching on > `switch`. > * Normalized the direction here instead of in the native code. > * Ambient light is now only set when it exists (and is not black). > * `NGPointLight,java` - removed unneeded methods that were used by > `NGShape3D` before the per-lighting methods were extracted (point light > doesn't need spotlight-specific methods since they each have their own "add" > method). > * `NGSpotLight.java` - removed `@Override` annotations as a result of the > above change. > > **Native C++** > > * Initialized the class members of `D3DLight`, `D3DMeshView` and > `D3DPhongMaterial` in the header file instead of a more cumbersome > initialization in the constructor (this is allowed since C++ 11). > * `D3DLight` > * Commented out unused methods. Were they supposed to be used at some point? > * Renamed the `w` component to `lightOn` since it controls the number of > lights for the special pixel shader variant and having it in the 4th position > of the color array was confusing. > * `D3DPhongShader.h` > * Renamed some of the register constants for more clarity. > * Moved the ambient light color constant from the vertex shader to the > pixel shader (see the shader section below). I don't understand the > calculation of the number of registers in the comment there: "8 ambient > points + 2 coords = 10". There is 1 ambient light, what are the ambient > points and coordinates? In `vsConstants` there is `gAmbinetData[10]`, but it > is unused. > * Reduced the number of assigned vertex register for the `VSR_LIGHTS` > constant since it included both position and color, but color was unused > there (it was used directly in the pixel shader), so now it's only the > position. > * `D3DMeshView.cc` > * Unified the lighting loop that prepares the lights' 4-vetors that are > passed to the shaders. > * Removed the direction normalization as stated in the change for > `NGShape3D.java`. > * Reordered the shader constant assignment to be the same order as in > `D3DPhongShader.h`. > > **Native shaders** > * Renamed many of the variables to what I think are more descriptive names. > This includes noting in which space they exist as some calculations are done > in model space, some in world space, and we might need to do some in view > space. For vectors, also noted if the vector is to or from (`eye` doesn't > tell me if it's from or to the camera). > * Commented out many unused functions. I don't know what they are for, so I > didn't remove them. > * `vsConstants` > * Removed the light color from here since it's unused, only the position is. > * Removed the ambient light color constant from here since it's unused, and > added it to `psConstants` instead. > * `vs2ps` > * Removed the ambient color interpolation, which frees a register (no > change in performance). > * Simplified the structure (what is `LocalBumpOut` and why is it called > `light` and contains `LocalBump`?). > * `Mtl1PS` and `psMath` > * Moved the shader variant constants (`#ifndef`) to `Mtl1PS` where they are > used for better clarity. > * Moved the lights loop to `Mtl1PS`. The calculation itself remains in > `psMath`. > > No changes in performance were measured and the behavior stayed the same. This pull request has now been integrated. Changeset: 4a278013 Author:Nir Lisker URL: https://git.openjdk.org/jfx/commit/4a2780134b88e86e437ae2bdffe06b145788e61d Stats: 685 lines in 22 files changed: 232 ins; 227 del; 226 mod 8217853: Cleanup in the D3D native pipeline Reviewed-by: arapte, kcr - PR: https://git.openjdk.org/jfx/pull/789
Re: CheckBoxTreeItem behavior - independent property
I would like to create an issue for changing the behavior of the independent property to be independent both as a source and as a target. That is, an independent item would no longer update when a parent or child of it is updated. Can I go forward with it or is there a risk of breaking current code that makes it not worth doing this? To me it looks like a bug, but the docs don't say anything about this point. On Sat, Nov 19, 2022 at 9:28 AM Nir Lisker wrote: > Hi, > > Another issue I stumbled across is the usage of the Independent property > on CheckBoxTreeItem. The docs read: > > A BooleanProperty used to represent the independent state of this >> CheckBoxTreeItem. The independent state is used to represent whether >> changes to a single CheckBoxTreeItem should influence the state of its >> parent and children. > > > By default, the independent property is false, which means that when a >> CheckBoxTreeItem has state changes to the selected or indeterminate >> properties, the state of related CheckBoxTreeItems will possibly be >> changed. If the independent property is set to true, the state of related >> CheckBoxTreeItems will never change. > > > It makes it clear that changes to this checkbox don't influence its > children/parents, and in terms of usage, it means that clicking on the > checkbox doesn't change the state of its parents/children. However: > > 1. Does it also include stopping the propagation of selection updates > through it? If an independent item has a child and a parent, and the parent > is selected, does the item stop its child from being selected? I think that > the answer is yes, but I want to make sure the independent property wasn't > just meant for direct clicks or direct changes to its own properties alone. > > 2. Do the parents/children affect this item? The docs only mention one > direction: item -> parents/children, it doesn't mention parents/children -> > item. As it stands currently, the item seems to be affected by other > selections. I find it odd because this doesn't make the item really > independent, and I can't think of a use case for this one-sided independent > behavior. > > In any case, I think that the documentation should clarify these points, > so I would like to know what the behavior should be. > > - Nir >
Re: RFR: 8217853: Cleanup in the D3D native pipeline [v11]
On Sun, 25 Dec 2022 04:04:40 GMT, Nir Lisker wrote: >> Refactoring and renaming changes to some of the D3D pipeline files and a few >> changes on the Java side. These are various "leftovers" from previous issues >> that we didn't want to touch at the time in order to confine the scope of >> the changes. They will make future work easier. >> >> Since there are many small changes, I'm giving a full list here: >> >> **Java** >> >> * `NGShape3D.java` >> * Extracted methods to help with the cumbersome lighting loop: one method >> per light type + empty light (reset light) + default point light. This >> section of the code would benefit from the upcoming pattern matching on >> `switch`. >> * Normalized the direction here instead of in the native code. >> * Ambient light is now only set when it exists (and is not black). >> * `NGPointLight,java` - removed unneeded methods that were used by >> `NGShape3D` before the per-lighting methods were extracted (point light >> doesn't need spotlight-specific methods since they each have their own "add" >> method). >> * `NGSpotLight.java` - removed `@Override` annotations as a result of the >> above change. >> >> **Native C++** >> >> * Initialized the class members of `D3DLight`, `D3DMeshView` and >> `D3DPhongMaterial` in the header file instead of a more cumbersome >> initialization in the constructor (this is allowed since C++ 11). >> * `D3DLight` >> * Commented out unused methods. Were they supposed to be used at some >> point? >> * Renamed the `w` component to `lightOn` since it controls the number of >> lights for the special pixel shader variant and having it in the 4th >> position of the color array was confusing. >> * `D3DPhongShader.h` >> * Renamed some of the register constants for more clarity. >> * Moved the ambient light color constant from the vertex shader to the >> pixel shader (see the shader section below). I don't understand the >> calculation of the number of registers in the comment there: "8 ambient >> points + 2 coords = 10". There is 1 ambient light, what are the ambient >> points and coordinates? In `vsConstants` there is `gAmbinetData[10]`, but it >> is unused. >> * Reduced the number of assigned vertex register for the `VSR_LIGHTS` >> constant since it included both position and color, but color was unused >> there (it was used directly in the pixel shader), so now it's only the >> position. >> * `D3DMeshView.cc` >> * Unified the lighting loop that prepares the lights' 4-vetors that are >> passed to the shaders. >> * Removed the direction normalization as stated in the change for >> `NGShape3D.java`. >> * Reordered the shader constant assignment to be the same order as in >> `D3DPhongShader.h`. >> >> **Native shaders** >> * Renamed many of the variables to what I think are more descriptive names. >> This includes noting in which space they exist as some calculations are done >> in model space, some in world space, and we might need to do some in view >> space. For vectors, also noted if the vector is to or from (`eye` doesn't >> tell me if it's from or to the camera). >> * Commented out many unused functions. I don't know what they are for, so I >> didn't remove them. >> * `vsConstants` >> * Removed the light color from here since it's unused, only the position >> is. >> * Removed the ambient light color constant from here since it's unused, >> and added it to `psConstants` instead. >> * `vs2ps` >> * Removed the ambient color interpolation, which frees a register (no >> change in performance). >> * Simplified the structure (what is `LocalBumpOut` and why is it called >> `light` and contains `LocalBump`?). >> * `Mtl1PS` and `psMath` >> * Moved the shader variant constants (`#ifndef`) to `Mtl1PS` where they >> are used for better clarity. >> * Moved the lights loop to `Mtl1PS`. The calculation itself remains in >> `psMath`. >> >> No changes in performance were measured and the behavior stayed the same. > > Nir Lisker has updated the pull request incrementally with two additional > commits since the last revision: > > - Changed comment as suggested > - Removed unused fields The ambient light issue was on the Java side. The purple light is probably just a false observation because it starts in a position where it doesn't apply its light. The real mystery is the problem with the two point lights. I have an AMD 470. - PR: https://git.openjdk.org/jfx/pull/789
Re: RFR: 8217853: Cleanup in the D3D native pipeline [v11]
On Sun, 25 Dec 2022 04:04:40 GMT, Nir Lisker wrote: >> Refactoring and renaming changes to some of the D3D pipeline files and a few >> changes on the Java side. These are various "leftovers" from previous issues >> that we didn't want to touch at the time in order to confine the scope of >> the changes. They will make future work easier. >> >> Since there are many small changes, I'm giving a full list here: >> >> **Java** >> >> * `NGShape3D.java` >> * Extracted methods to help with the cumbersome lighting loop: one method >> per light type + empty light (reset light) + default point light. This >> section of the code would benefit from the upcoming pattern matching on >> `switch`. >> * Normalized the direction here instead of in the native code. >> * Ambient light is now only set when it exists (and is not black). >> * `NGPointLight,java` - removed unneeded methods that were used by >> `NGShape3D` before the per-lighting methods were extracted (point light >> doesn't need spotlight-specific methods since they each have their own "add" >> method). >> * `NGSpotLight.java` - removed `@Override` annotations as a result of the >> above change. >> >> **Native C++** >> >> * Initialized the class members of `D3DLight`, `D3DMeshView` and >> `D3DPhongMaterial` in the header file instead of a more cumbersome >> initialization in the constructor (this is allowed since C++ 11). >> * `D3DLight` >> * Commented out unused methods. Were they supposed to be used at some >> point? >> * Renamed the `w` component to `lightOn` since it controls the number of >> lights for the special pixel shader variant and having it in the 4th >> position of the color array was confusing. >> * `D3DPhongShader.h` >> * Renamed some of the register constants for more clarity. >> * Moved the ambient light color constant from the vertex shader to the >> pixel shader (see the shader section below). I don't understand the >> calculation of the number of registers in the comment there: "8 ambient >> points + 2 coords = 10". There is 1 ambient light, what are the ambient >> points and coordinates? In `vsConstants` there is `gAmbinetData[10]`, but it >> is unused. >> * Reduced the number of assigned vertex register for the `VSR_LIGHTS` >> constant since it included both position and color, but color was unused >> there (it was used directly in the pixel shader), so now it's only the >> position. >> * `D3DMeshView.cc` >> * Unified the lighting loop that prepares the lights' 4-vetors that are >> passed to the shaders. >> * Removed the direction normalization as stated in the change for >> `NGShape3D.java`. >> * Reordered the shader constant assignment to be the same order as in >> `D3DPhongShader.h`. >> >> **Native shaders** >> * Renamed many of the variables to what I think are more descriptive names. >> This includes noting in which space they exist as some calculations are done >> in model space, some in world space, and we might need to do some in view >> space. For vectors, also noted if the vector is to or from (`eye` doesn't >> tell me if it's from or to the camera). >> * Commented out many unused functions. I don't know what they are for, so I >> didn't remove them. >> * `vsConstants` >> * Removed the light color from here since it's unused, only the position >> is. >> * Removed the ambient light color constant from here since it's unused, >> and added it to `psConstants` instead. >> * `vs2ps` >> * Removed the ambient color interpolation, which frees a register (no >> change in performance). >> * Simplified the structure (what is `LocalBumpOut` and why is it called >> `light` and contains `LocalBump`?). >> * `Mtl1PS` and `psMath` >> * Moved the shader variant constants (`#ifndef`) to `Mtl1PS` where they >> are used for better clarity. >> * Moved the lights loop to `Mtl1PS`. The calculation itself remains in >> `psMath`. >> >> No changes in performance were measured and the behavior stayed the same. > > Nir Lisker has updated the pull request incrementally with two additional > commits since the last revision: > > - Changed comment as suggested > - Removed unused fields > I tested this behavior at commit > [bb9f802](https://github.com/openjdk/jfx/commit/bb9f80263d4565cad1f485a59228064ca53d2d96), > and can't observe any visual difference with regards to the order of `texD` > in the `PsInput` struct. Adding two points lights works fi
Re: RFR: 8209017: CheckBoxTreeCell: graphic on TreeItem not always showing [v3]
On Tue, 3 Jan 2023 23:56:12 GMT, Nir Lisker wrote: >> A simple fix for the graphics of a `CheckBoxTreeCell` not showing after >> expand/collapse events. The graphics node needs to be released from the >> previous checkbox so that it doesn't serve as the graphics of more than one >> checkbox at once. If it does, only one checkbox (its skin) can actually use >> it as a scenegraph child, and it's not always the correct one. >> >> Added also a manual test app as I'm unsure about how to do an automatic test >> for this. The test app will be useful for later fixes, so I suggest adding >> it anyway. To test the fix, simply expand and collapse the root note >> repeatedly while it has a child. Before the fix the graphic will disappear >> after a few tries, after the fix it won't. You can test other hierarchies as >> well by adding children to the tree. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Removed thrown exception Thanks for the quick review! - PR: https://git.openjdk.org/jfx/pull/970
Integrated: 8209017: CheckBoxTreeCell: graphic on TreeItem not always showing
On Tue, 6 Dec 2022 20:10:16 GMT, Nir Lisker wrote: > A simple fix for the graphics of a `CheckBoxTreeCell` not showing after > expand/collapse events. The graphics node needs to be released from the > previous checkbox so that it doesn't serve as the graphics of more than one > checkbox at once. If it does, only one checkbox (its skin) can actually use > it as a scenegraph child, and it's not always the correct one. > > Added also a manual test app as I'm unsure about how to do an automatic test > for this. The test app will be useful for later fixes, so I suggest adding it > anyway. To test the fix, simply expand and collapse the root note repeatedly > while it has a child. Before the fix the graphic will disappear after a few > tries, after the fix it won't. You can test other hierarchies as well by > adding children to the tree. This pull request has now been integrated. Changeset: 012fa16f Author:Nir Lisker URL: https://git.openjdk.org/jfx/commit/012fa16faccaf9b6a1f56cd3b5450ab200bdec9c Stats: 135 lines in 5 files changed: 134 ins; 0 del; 1 mod 8209017: CheckBoxTreeCell: graphic on TreeItem not always showing Reviewed-by: jhendrikx, angorya, kcr - PR: https://git.openjdk.org/jfx/pull/970
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages [v2]
On Sun, 1 Jan 2023 16:08:15 GMT, John Hendrikx wrote:
>> Packages fixed:
>> - com.sun.javafx.binding
>> - com.sun.javafx.collections
>> - javafx.beans
>> - javafx.beans.binding
>> - javafx.collections
>> - javafx.collections.transformation
>
> John Hendrikx has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Clean up expression classes repeated null checks
> - Use instanceof with pattern matching in a few spots
I took a closer look at the sorting-related classes. I think that the design
there is not ideal. Fixing it might be out of scope for this PR. I wouldn't
mind fixing it myself, by the way.
The way the JDK treats soring on lists is with a single `sort(Comparable c)`
method that, when receiving `null`, assumes that the elements are `Comparable`
and throws if not. `Collections` has 2 static sorting methods that help enforce
this condition, where the one that doesn't take a `Comparator` passes `null` to
the one that does.
JavaFX tries to emulate this. `FXCollections` has the same 2 methods for
`ObservableList`, but one doesn't call the other. The asymmetry comes from
`SortableList` (which doesn't live up to its name - non-sortable lists can also
implement it). It defines a 0-argument method and a `Comparator`-argument
method as different behaviors, instead of one calling the other. This is
deferred to its implementations, `ObservableListWrapper` and
`ObservableSequentialListWrapper`, which make the differentiation by, again,
calling 2 different methods on `SortHelper`.
I suggest that the argument check be made at the lowest level, like in the JDK
(inside `Arrays` sort method). First, `FXCollections` can change to:
public static > void sort(ObservableList
list) {
sort(list, null); // or Comparator.naturalOrder() instead of null
}
public static void sort(ObservableList list, Comparator
c) {
if (list instanceof SortableList) {
list.sort(c);
} else {
List newContent = new ArrayList<>(list);
newContent.sort(c);
list.setAll(newContent);
}
}
`SortableList` then removes its `sort()` method, and now it's just overriding
`List::sort(Comparator)` without doing anything with it, so it can be removed
too, and it's left as a marker interface for the special sorting in
`SortHelper` (I haven't understood yet why it's better , I need to dig there
more):
public interface SortableList {}
Now `ObservableListWrapper` and `ObservableSequentialListWrapper` also remove
`sort()`, and override `List::sort(Comparator)` directly and in accordance with
its contract, passing the (possibly `null`) comparator to `SortHelper`, which
is akin to the JDK's `Arrays::sort` method.
Now I need to look into `SortHelper` to see what it does exactly and how to
change it. I think it doesn't need the `sort(List list)` method too now
because it doesn't really enforce that the elements are `Comparable`, it will
naturally throw if not.
---
In my opinion, this is the design flaw: `ObservableList` should have overridden
`List`'s sort method to specify that its sorting is done as one change:
public interface ObservableList extends List, Observable {
@Override
default void sort(Comparator c) {
var newContent = new ArrayList<>(this);
newContent.sort(c);
setAll(newContent);
}
Then we wouldn't need the `SortableList` marker because `FXCollections` could
just call this method directly (like `Collections` does, which means that
`FXCollections`'s methods are not needed anymore, though we can't remove them),
and whichever implementation wants to do a more efficient sorting, like
`ObservableListWrapper`, can override again.
This will be a behavioral change, though, because the generated change events
differ.
-
PR: https://git.openjdk.org/jfx/pull/972
Re: RFR: 8209017: CheckBoxTreeCell: graphic on TreeItem not always showing [v3]
> A simple fix for the graphics of a `CheckBoxTreeCell` not showing after > expand/collapse events. The graphics node needs to be released from the > previous checkbox so that it doesn't serve as the graphics of more than one > checkbox at once. If it does, only one checkbox (its skin) can actually use > it as a scenegraph child, and it's not always the correct one. > > Added also a manual test app as I'm unsure about how to do an automatic test > for this. The test app will be useful for later fixes, so I suggest adding it > anyway. To test the fix, simply expand and collapse the root note repeatedly > while it has a child. Before the fix the graphic will disappear after a few > tries, after the fix it won't. You can test other hierarchies as well by > adding children to the tree. Nir Lisker has updated the pull request incrementally with one additional commit since the last revision: Removed thrown exception - Changes: - all: https://git.openjdk.org/jfx/pull/970/files - new: https://git.openjdk.org/jfx/pull/970/files/4d89b0da..abb71bd4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=970=02 - incr: https://webrevs.openjdk.org/?repo=jfx=970=01-02 Stats: 3 lines in 1 file changed: 0 ins; 2 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/970.diff Fetch: git fetch https://git.openjdk.org/jfx pull/970/head:pull/970 PR: https://git.openjdk.org/jfx/pull/970
Re: RFR: 8209017: CheckBoxTreeCell: graphic on TreeItem not always showing [v2]
On Tue, 3 Jan 2023 23:39:36 GMT, Andy Goryachev wrote: >> Nir Lisker has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Added copyright header and changed package name > > tests/performance/checkboxTreeView/src/main/java/main/CheckBoxTreeEditor.java > line 26: > >> 24: */ >> 25: >> 26: package main; > > I think the package name "main" is ok here, considering this code will be > folded into the tester. There is only one package with 1 class. I didn't see any meaningful name for the package. - PR: https://git.openjdk.org/jfx/pull/970
Re: RFR: 8209017: CheckBoxTreeCell: graphic on TreeItem not always showing [v2]
> A simple fix for the graphics of a `CheckBoxTreeCell` not showing after > expand/collapse events. The graphics node needs to be released from the > previous checkbox so that it doesn't serve as the graphics of more than one > checkbox at once. If it does, only one checkbox (its skin) can actually use > it as a scenegraph child, and it's not always the correct one. > > Added also a manual test app as I'm unsure about how to do an automatic test > for this. The test app will be useful for later fixes, so I suggest adding it > anyway. To test the fix, simply expand and collapse the root note repeatedly > while it has a child. Before the fix the graphic will disappear after a few > tries, after the fix it won't. You can test other hierarchies as well by > adding children to the tree. Nir Lisker has updated the pull request incrementally with one additional commit since the last revision: Added copyright header and changed package name - Changes: - all: https://git.openjdk.org/jfx/pull/970/files - new: https://git.openjdk.org/jfx/pull/970/files/b1808afe..4d89b0da Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=970=01 - incr: https://webrevs.openjdk.org/?repo=jfx=970=00-01 Stats: 203 lines in 2 files changed: 114 ins; 89 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/970.diff Fetch: git fetch https://git.openjdk.org/jfx pull/970/head:pull/970 PR: https://git.openjdk.org/jfx/pull/970
Re: RFR: 8209017: CheckBoxTreeCell: graphic on TreeItem not always showing
On Tue, 3 Jan 2023 23:11:08 GMT, John Hendrikx wrote: > Looks good to me, releasing the graphic for empty cells. I'm left wondering > if it should also be releasing the bindings it makes in that case, although > that's less likely to cause (visible) issues. I could make that change. I don't see any issues either way. - PR: https://git.openjdk.org/jfx/pull/970
Re: RFR: 8209017: CheckBoxTreeCell: graphic on TreeItem not always showing
On Tue, 3 Jan 2023 23:07:19 GMT, Andy Goryachev wrote: >> A simple fix for the graphics of a `CheckBoxTreeCell` not showing after >> expand/collapse events. The graphics node needs to be released from the >> previous checkbox so that it doesn't serve as the graphics of more than one >> checkbox at once. If it does, only one checkbox (its skin) can actually use >> it as a scenegraph child, and it's not always the correct one. >> >> Added also a manual test app as I'm unsure about how to do an automatic test >> for this. The test app will be useful for later fixes, so I suggest adding >> it anyway. To test the fix, simply expand and collapse the root note >> repeatedly while it has a child. Before the fix the graphic will disappear >> after a few tries, after the fix it won't. You can test other hierarchies as >> well by adding children to the tree. > > tests/performance/checkboxTreeView/.classpath line 1: > >> 1: > > minor: should this project be moved to manual tests hierarchy? > > BTW, I am planning to create a monkey tester application which will include > many tests, to simplify the testing/verification of fixes JDK-8299335 > This code be added to the tester, either as a separate page, or as a data > source option for one of the TreeTableView page. I wasn't sure exactly where to put it, but it's going to be used in a fix for performance issue I found when a node has many children. If it will fit into a grater app I don't mind. - PR: https://git.openjdk.org/jfx/pull/970
Re: RFR: 8209017: CheckBoxTreeCell: graphic on TreeItem not always showing
On Tue, 6 Dec 2022 20:10:16 GMT, Nir Lisker wrote: > A simple fix for the graphics of a `CheckBoxTreeCell` not showing after > expand/collapse events. The graphics node needs to be released from the > previous checkbox so that it doesn't serve as the graphics of more than one > checkbox at once. If it does, only one checkbox (its skin) can actually use > it as a scenegraph child, and it's not always the correct one. > > Added also a manual test app as I'm unsure about how to do an automatic test > for this. The test app will be useful for later fixes, so I suggest adding it > anyway. To test the fix, simply expand and collapse the root note repeatedly > while it has a child. Before the fix the graphic will disappear after a few > tries, after the fix it won't. You can test other hierarchies as well by > adding children to the tree. @hjohn @kevinrushforth Can you review this in time for RDP1? It's a simple fix. - PR: https://git.openjdk.org/jfx/pull/970
Re: CFV: New OpenJFX Committer: John Hendrikx
Vote: YES On Tue, Jan 3, 2023 at 5:18 PM Kevin Rushforth wrote: > Vote: YES > > On 1/3/2023 7:17 AM, Kevin Rushforth wrote: > > I hereby nominate John Hendrikx [1] to OpenJFX Committer. > >
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages [v2]
On Sun, 1 Jan 2023 15:04:17 GMT, John Hendrikx wrote:
>> modules/javafx.base/src/main/java/com/sun/javafx/binding/BidirectionalContentBinding.java
>> line 81:
>>
>>> 79: if ((obj1 instanceof ObservableList list1) && (obj2
>>> instanceof ObservableList list2)) {
>>> 80: @SuppressWarnings("unchecked")
>>> 81: final ListContentBinding binding = new
>>> ListContentBinding<>((ObservableList) list1,
>>> (ObservableList) list2);
>>
>> Although the previous code has the same problem, this is sketchy. The two
>> lists can be of different types while `ListContentBinding` requires the same
>> type. This is a result of the `Bindings` public API that takes two
>> `Objects`, so all type information is lost. Is it worth adding a comment
>> about this since suppressing the warning can be understood as "trust me,
>> this is fine".
>
> What would go wrong if they're not the same type? `ListContentBinding`
> doesn't (and can't) enforce it and doesn't care either way. The whole
> function fails silently if types don't match. Also `ListContentBinding` is a
> private class and so I'd expect this code to be aware of how it works and
> what is/isn't safe to do.
>
> I personally think this entire class is unfinished. It fails miserably in
> edge cases without so much as a warning to the user. Take this for example:
>
> public static void main(String[] args) {
> ObservableList a = FXCollections.observableArrayList();
> ObservableList b = FXCollections.observableArrayList();
> Bindings.bindContentBidirectional(a, b);
> Bindings.bindContentBidirectional(a, b);
> a.add("A");
> System.out.println(a + " : " + b);
> }
>
> Prints:
>
> [A, A, A, A] : [A, A, A]
>
> No mention about this in the API docs at all. It breaks even worse when you
> make circular bindings `[a,b], [b,c], [c,a]` (stack traces get logged to the
> console complaining about `IndexOutOfBoundsException`).
>
> I've created a solution that rejects double bindings and circular bindings,
> but it's a bit out of scope for this. I think however that it is worth
> adding, and considering that the current behavior is broken when doing any of
> such things, not a big API break if instead we throw an exception.
You are right, nothing would go wrong.
I agree that the behavior is undesired and should be fixed in another issue. I
was thinking of adding more specific overloads that make sense to the public
API and deprecating the method that takes everything, making it throw.
>> modules/javafx.base/src/main/java/com/sun/javafx/binding/ContentBinding.java
>> line 89:
>>
>>> 87: ListContentBinding binding = new
>>> ListContentBinding<>((List) list1);
>>> 88:
>>> 89: list2.removeListener(binding);
>>
>> Another problem inherited from the existing code. What if the `obj2` is a
>> `List` and `obj1` is an `ObservableList`? The docs don't say anything about
>> the order.
>>
>> Same question as before about adding a comment addressing the case that the
>> two lists are not of the same type.
>
> Yes, looks like this is quite broken. This wouldn't have gone unnoticed so
> long if unbind would just throw an exception when nothing could be unbound;
> silently failing is never a good idea.
Can you file an issue for this if it's not filed already?
-
PR: https://git.openjdk.org/jfx/pull/972
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages [v2]
On Sun, 1 Jan 2023 15:23:16 GMT, John Hendrikx wrote:
>> modules/javafx.base/src/main/java/javafx/collections/FXCollections.java line
>> 709:
>>
>>> 707: private static class EmptyObservableList extends
>>> AbstractList implements ObservableList {
>>> 708:
>>> 709: private static final ListIterator iterator = new
>>> ListIterator<>() {
>>
>> Isn't a better fix is to not make this class static and use ``? Other
>> lists create a new iterator on each invocation, but this is effectively a
>> singleton.
>> In fact, `EmptyObservableList` doesn't need to be declared because it's
>> called only in one place, so it can be "inlined" when declaring the
>> `EMPTY_LIST` field. Maybe this is out of scope.
>
> I considered all that, and don't mind changing it if we're in agreement.
>
> The reason I didn't is that it would subtly change the iterator (it would
> have an unnecessary reference to its containing class).
In my opinion that's fine. The iterator might not use its reference to the
list, but it's using its type information.
As for creating a new iterator on every invocation (like empty set does) or
returning the same instance (like empty list does), I don't mind that much and
I can't imagine it mattering in terms of performance.
-
PR: https://git.openjdk.org/jfx/pull/972
Re: RFR: 8209017: CheckBoxTreeCell: graphic on TreeItem not always showing
On Tue, 6 Dec 2022 20:10:16 GMT, Nir Lisker wrote: > A simple fix for the graphics of a `CheckBoxTreeCell` not showing after > expand/collapse events. The graphics node needs to be released from the > previous checkbox so that it doesn't serve as the graphics of more than one > checkbox at once. If it does, only one checkbox (its skin) can actually use > it as a scenegraph child, and it's not always the correct one. > > Added also a manual test app as I'm unsure about how to do an automatic test > for this. The test app will be useful for later fixes, so I suggest adding it > anyway. To test the fix, simply expand and collapse the root note repeatedly > while it has a child. Before the fix the graphic will disappear after a few > tries, after the fix it won't. You can test other hierarchies as well by > adding children to the tree. @aghaisas @kleopatra Since you are the assignee and reported of the original issue, can you have a look? I also need advice for how to write a test for it as I'm not too familiar with virtual flow. - PR: https://git.openjdk.org/jfx/pull/970
RFR: 8209017: CheckBoxTreeCell: graphic on TreeItem not always showing
A simple fix for the graphics of a `CheckBoxTreeCell` not showing after expand/collapse events. The graphics node needs to be released from the previous checkbox so that it doesn't serve as the graphics of more than one checkbox at once. If it does, only one checkbox (its skin) can actually use it as a scenegraph child, and it's not always the correct one. Added also a manual test app as I'm unsure about how to do an automatic test for this. The test app will be useful for later fixes, so I suggest adding it anyway. To test the fix, simply expand and collapse the root note repeatedly while it has a child. Before the fix the graphic will disappear after a few tries, after the fix it won't. You can test other hierarchies as well by adding children to the tree. - Commit messages: - Removed whitespaces - Name and whitespace fixes to the test app - Manual test utility - Fix Changes: https://git.openjdk.org/jfx/pull/970/files Webrev: https://webrevs.openjdk.org/?repo=jfx=970=00 Issue: https://bugs.openjdk.org/browse/JDK-8209017 Stats: 112 lines in 5 files changed: 111 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/970.diff Fetch: git fetch https://git.openjdk.org/jfx pull/970/head:pull/970 PR: https://git.openjdk.org/jfx/pull/970
Re: Concatenated observable list
We can do better than that. Instead of introducing methods for specific
operations, like concat and retain, we can offer bulk operations on
collections. When John H. proposed the fluent bindings additions, we
discussed doing the same for collections after that.
At the very least, we can have a method that takes an observable collection
and a function that specifies the operation you want to perform. This will
be a lot more flexible.
I have been playing around with adding ObservableCollection as a first step
so that we don't need to multiply the methods for sets and lists. It's also
useful for an observableValues() on ObservableMap. I hit a few backwards
compatibility issues there.
I can share later some mock API I created as a sort of end goal, but it's
incomplete.
On Sat, Dec 31, 2022 at 8:15 PM Michael Strauß
wrote:
> FXCollections.concat(ObservableList...) can be used to create a new
> ObservableList that contains the concatenation of all elements of the
> source lists. This is useful to initialize the contents of the new
> ObservableList, but the returned list is not kept in sync with the
> source lists.
>
> I'm proposing to add a new method:
> FXCollections.concatenatedObservableList(ObservableList...)
>
> Like FXCollections.concat, it returns a list that contains the
> concatenation of all elements of the source lists. However, in this
> case the returned list is unmodifiable and is always kept in sync with
> the source lists. This can be useful to create a list where only parts
> of the list are under the control of the author, and other parts are
> derived from other lists.
>
> I'm interested to hear your thoughts on this. Here's the proposed
> specification of the new method:
>
> /**
> * Creates a new unmodifiable {@code ObservableList} that contains
> * the concatenation of the contents of the specified observable lists.
> * In contrast to {@link #concat(ObservableList[])}, the list returned from
> * this method is updated when any of the source lists are changed.
> *
> * @param lists the source lists
> * @param the element type
> * @return an {@code ObservableList} that contains the concatenation
> * of the contents of {@code lists}
> * @throws IndexOutOfBoundsException if the sum of the number of
> * elements contained in all source lists exceeds
> * {@link Integer#MAX_VALUE}
> */
> @SafeVarargs
> public static ObservableList concatenatedObservableList(
> ObservableList... lists)
>
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages
On Sun, 11 Dec 2022 20:12:17 GMT, John Hendrikx wrote:
> Packages fixed:
> - com.sun.javafx.binding
> - com.sun.javafx.collections
> - javafx.beans
> - javafx.beans.binding
> - javafx.collections
> - javafx.collections.transformation
Second part of the review. I'm still looking at the changes to sorting. I think
that the problem stems from a more basic flaw that we can possible fix, and
that is that any `ObservableListWrapper` is a `SortedList` even when it's not.
A `SortedList` should require that its elements implement `Comparable`. The
only other class affected is the `Sequential` variant, so the scope here is
small.
While the current fix removes the warnings, I suspect we are just hiding the
flaw that these warnings exist for.
In addition `FXCollections::sort(ObservableList)` can change to
if (list instanceof SortableList sortableList) {
sortableList.sort();
and remove the `@SuppressWarnings("unchecked")`.
modules/javafx.base/src/main/java/javafx/collections/FXCollections.java line
709:
> 707: private static class EmptyObservableList extends AbstractList
> implements ObservableList {
> 708:
> 709: private static final ListIterator iterator = new
> ListIterator<>() {
Isn't a better fix is to not make this class static and use ``? Other lists
create a new iterator on each invocation, but this is effectively a singleton.
In fact, `EmptyObservableList` doesn't need to be declared because it's called
only in one place, so it can be "inlined" when declaring the `EMPTY_LIST`
field. Maybe this is out of scope.
modules/javafx.base/src/main/java/javafx/collections/FXCollections.java line
1640:
> 1638: @Override
> 1639: public Iterator iterator() {
> 1640: return new Iterator<>() {
Here the empty `Set` creates a listener on invocation, unlike in the list case.
Might want to keep a single pattern. I prefer the one with a singleton iterator
because the empty set itself is a singleton. Same comment about considering
"inlining" it.
modules/javafx.base/src/main/java/javafx/collections/transformation/TransformationList.java
line 94:
> 92: List currentSource = source;
> 93: while(currentSource instanceof TransformationList) {
> 94: currentSource = ((TransformationList ?>)currentSource).source;
Can use pattern matching for `instanceof`.
Also, can add a space after `while`.
modules/javafx.base/src/main/java/javafx/collections/transformation/TransformationList.java
line 143:
> 141: int idx = getSourceIndex(index);
> 142: while(currentSource != list && currentSource instanceof
> TransformationList) {
> 143: final TransformationList tSource =
> (TransformationList) currentSource;
Sam here.
-
PR: https://git.openjdk.org/jfx/pull/972
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages
On Mon, 26 Dec 2022 14:32:52 GMT, John Hendrikx wrote:
>> modules/javafx.base/src/main/java/javafx/beans/binding/ListExpression.java
>> line 238:
>>
>>> 236: public Iterator iterator() {
>>> 237: final ObservableList list = get();
>>> 238: return (list == null)?
>>> ListExpression.emptyList().iterator() : list.iterator();
>>
>> You're using three slightly different ways of referring to the empty list:
>> * `ListExpression.emptyList()`
>> * `emptyList()`
>> * `EMPTY_LIST`
>>
>> What do you think about using the first option in all cases?
>
> I'm fine with that; the first two are equivalent, but in some cases I need to
> add the type witness to avoid a warning and you can only do that by adding
> the class name as well (ie. `emptyList()` is not allowed, but
> `ListExpression.emptyList()` is.
Why not use `FXCollections.emptyObservableList()` directly? If it's too
long, a convenvenience method can be used:
private static ObservableList emptyList() {
return FXCollections.emptyObservableList();
}
No need to hold the instance here again in a way that makes it lose its type
and do the same cast (and supress the warning) that `FXCollections` already
does.
Same with the other collections.
-
PR: https://git.openjdk.org/jfx/pull/972
Re: RTOS inmate misses interrupts
Hi Ralf,
Do you know of a difference in the way SPI and PPI are treated by the
hypervisor?
A timer that is specific for a certain core is wired to GIC by PPI. Would
its processing be influenced by heavy load running on another core?
The GIC is facing the same load, but handles PPI instead of SPI. What would
be the implications on performance?
Thanks a lot,
Nir.
On Wed, Dec 14, 2022 at 11:09 AM Rasty Slutsker wrote:
> Hi,
> Exactly I reference to 'jailhouse cell stats'
> We enabled TSC in Hypervisor init, and printed value to dmsg.
> But *after* start of Linux it gets disabled again.
> *** Is there a place in hypervisor that runs at kernel space after start
> of linux where we can put re-enabling of TSC?
>
> We would like to measure duration of Hypervisor ISR with TSC and look for
> anomalies.
> Anomalies like unusual duration of ISR processing and Irregularities in
> ISR entry for particular request number.
>
> Other suggestions would be highly appreciated.
>
> Thanks
> Rasty
>
> On Tuesday, December 13, 2022 at 11:27:50 PM UTC+2 Ralf Ramsauer wrote:
>
>> Hi Rasty,
>>
>> (reply-to-all :) )
>>
>> On 13/12/2022 19:31, Rasty Slutsker wrote:
>> > We learned how you export some statistics from Jailhouse as you guys do
>> > and added 3 variables
>>
>> You reference to 'jailhouse cell stats'?
>>
>> > 1. At the entry Jailhouse IRQ (if irq==xxx counter ++)
>> > 2. At injection point of the same IRQ to inmate, still in Jailhouse
>> > 3. At the beginning of ISR in inmate (RTOS).
>>
>> Ok, but that just counts the interrupts, and not the occuring delays,
>> right?
>>
>> >
>> > We let system run, introduce some load to linux. We see physical
>> effects
>> > that suggest that we lose interrupts.
>> > We confirm it with difference in performance counters (gaps in quanta
>> of
>> > 30 uSecs) that we sample in inmate ISR.
>> > Than we kill interrupt.
>> > All 3 counters are the same. Amount matches interrupt rate.
>> > My conclusion that Interrupt request is lost.
>>
>> Could be the case. I don't know what happens if the jitter gets too long
>> between interrupts.
>>
>> >
>> > Another question.
>> > We try to read CPU time stamp counter from Jailhouse ISR . We get 0.
>> > mrc p15, #0, r0, c9, c13, #0
>>
>> That's the PMCNTR, right?
>>
>> >
>> > Any idea why? Do you have some code for that?
>>
>> Uhm, I would have to read the reference manual as well. Does reading the
>> TSC work in Linux? Maybe it has to be activated or enabled for the
>> hypervisor?
>>
>>
>> https://developer.arm.com/documentation/ddi0406/b/Debug-Architecture/Debug-Registers-Reference/Performance-monitor-registers/c9--Count-Enable-Set-Register--PMCNTENSET-?lang=en
>>
>> Thanks,
>> Ralf
>>
>> >
>> > Best regards
>> > Rasty
>> >
>> >
>> >
>> > On Tuesday, December 13, 2022 at 3:47:25 PM UTC+2 Ralf Ramsauer wrote:
>> >
>> > Hi Rasty,
>> >
>> > Please reply-to-all, then your reply will also pop up in my Inbox.
>> >
>> > On 10/12/2022 08:52, Rasty Slutsker wrote:
>> > > Hi,
>> > > We did some performance measurements.
>> > > Added counters in 3 places (per Irq source)
>> > > 1. entry to jailhouse ISR
>> > > 2. dispatch of interrupt to particular vector to particular core
>> > > 3. in RTOS isr.
>> >
>> > Okay. How do you read and dump them? I hope after everything is done.
>> >
>> > Take care that if you dump them immediately to the uart, this will
>> > consume a lot of time and cause significant delay. ('heavy' printk
>> > logic
>> > + busy waiting for the uart)
>> >
>> > >
>> > > *We see that all 3 counters have the same value*, but we measure
>> > time
>> >
>> > huh? What counters do you access? There's something odd if they hold
>> > the
>> > same value at different places.
>> >
>> > > gaps in RTOS in ISR invocation times, sometimes upto 60 uSec.
>> > >
>> > > It means that either
>> > > a) interrupt request is lost. But, according to setup it is
>> > > edge-triggered, it cannot be lost, just delayed.
>> > > b) there is a delay of more than 60 usec in jailhouse ISR.
>> > >
>> > > questions:
>> > > 1. Is it possible that jailhouse interrupt dispatching routine
>> > enters
>> >
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages
On Sun, 11 Dec 2022 20:12:17 GMT, John Hendrikx wrote:
> Packages fixed:
> - com.sun.javafx.binding
> - com.sun.javafx.collections
> - javafx.beans
> - javafx.beans.binding
> - javafx.collections
> - javafx.collections.transformation
This is the first half of the review. Will finish the classes under the
`javafx` package in the 2nd iteration.
modules/javafx.base/src/main/java/com/sun/javafx/binding/BidirectionalContentBinding.java
line 81:
> 79: if ((obj1 instanceof ObservableList list1) && (obj2 instanceof
> ObservableList list2)) {
> 80: @SuppressWarnings("unchecked")
> 81: final ListContentBinding binding = new
> ListContentBinding<>((ObservableList) list1, (ObservableList)
> list2);
Although the previous code has the same problem, this is sketchy. The two lists
can be of different types while `ListContentBinding` requires the same type.
This is a result of the `Bindings` public API that takes two `Objects`, so all
type information is lost. Is it worth adding a comment about this since
suppressing the warning can be understood as "trust me, this is fine".
modules/javafx.base/src/main/java/com/sun/javafx/binding/BidirectionalContentBinding.java
line 81:
> 79: if ((obj1 instanceof ObservableList list1) && (obj2 instanceof
> ObservableList list2)) {
> 80: @SuppressWarnings("unchecked")
> 81: final ListContentBinding binding = new
> ListContentBinding<>((ObservableList) list1, (ObservableList)
> list2);
By the way, for these cases I usually use `var` as I find it has less clutter:
final var binding = new ListContentBinding(...)
You don't have to change this.
modules/javafx.base/src/main/java/com/sun/javafx/binding/ContentBinding.java
line 87:
> 85: if ((obj1 instanceof List list1) && (obj2 instanceof
> ObservableList list2)) {
> 86: @SuppressWarnings("unchecked")
> 87: ListContentBinding binding = new
> ListContentBinding<>((List) list1);
Another place where I would personally use `var`.
modules/javafx.base/src/main/java/com/sun/javafx/binding/ContentBinding.java
line 89:
> 87: ListContentBinding binding = new
> ListContentBinding<>((List) list1);
> 88:
> 89: list2.removeListener(binding);
Another problem inherited from the existing code. What if the `obj2` is a
`List` and `obj1` is an `ObservableList`? The docs don't say anything about the
order.
Same question as before about adding a comment addressing the case that the two
lists are not of the same type.
modules/javafx.base/src/main/java/com/sun/javafx/collections/MappingChange.java
line 38:
> 36: private List removed;
> 37:
> 38: private static final Map NOOP_MAP = new Map<>() {
I think that we can do better with a bit of refactoring. The `Map` interface
here is just `java.util.Function`. We can get rid of it and use `Function`
instead. The `map` method call in `getRemoved` will be replaced with `apply`.
The call sites needs just a little adjustment:
* In `TableView::TableViewArrayListSelectionModel` the `cellToIndicesMap` needs
to change its type to `Function`, but it is also unused (didn't look what it
was supposed to be for), so no issue there.
* In `TableView`, the method `fireCustomSelectedCellsListChangeEvent` needs to
change its 2nd argument in the `MappingChange` constructor to
`Function.indentity()` or something like `f -> f`.
* Same changes for `TreeTableView`.
I think that we can also use JavaFX's `Callback`, though I never use it if I
can use `Function`.
modules/javafx.base/src/main/java/javafx/beans/binding/ListExpression.java line
238:
> 236: public Iterator iterator() {
> 237: final ObservableList list = get();
> 238: return (list == null)? ListExpression.emptyList().iterator()
> : list.iterator();
Might as well add a space before `?` in these lines if you want.
modules/javafx.base/src/main/java/javafx/beans/binding/ListExpression.java line
406:
> 404: private static ObservableList emptyList() {
> 405: return (ObservableList) EMPTY_LIST;
> 406: }
I would move this below the declaration of `EMPTY_LIST`.
modules/javafx.base/src/main/java/javafx/beans/binding/MapExpression.java line
326:
> 324: private static ObservableMap emptyMap() {
> 325: return (ObservableMap) EMPTY_MAP;
> 326: }
Same
modules/javafx.base/src/main/java/javafx/beans/binding/SetExpression.java line
331:
> 329: private static ObservableSet emptySet() {
> 330: return (ObservableSet) EMPTY_SET;
> 331: }
Same
-
PR: https://git.openjdk.org/jfx/pull/972
Re: RFR: 8217853: Cleanup in the D3D native pipeline [v7]
On Thu, 1 Sep 2022 15:03:18 GMT, Ambarish Rapte wrote: >> Nir Lisker has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Addressed review comment > > LGTM, > suggested a minor correction in comment. > I will re-approve if any more changes. @arapte Please re-review. - PR: https://git.openjdk.org/jfx/pull/789
Re: RFR: 8217853: Cleanup in the D3D native pipeline [v11]
> Refactoring and renaming changes to some of the D3D pipeline files and a few > changes on the Java side. These are various "leftovers" from previous issues > that we didn't want to touch at the time in order to confine the scope of the > changes. They will make future work easier. > > Since there are many small changes, I'm giving a full list here: > > **Java** > > * `NGShape3D.java` > * Extracted methods to help with the cumbersome lighting loop: one method > per light type + empty light (reset light) + default point light. This > section of the code would benefit from the upcoming pattern matching on > `switch`. > * Normalized the direction here instead of in the native code. > * Ambient light is now only set when it exists (and is not black). > * `NGPointLight,java` - removed unneeded methods that were used by > `NGShape3D` before the per-lighting methods were extracted (point light > doesn't need spotlight-specific methods since they each have their own "add" > method). > * `NGSpotLight.java` - removed `@Override` annotations as a result of the > above change. > > **Native C++** > > * Initialized the class members of `D3DLight`, `D3DMeshView` and > `D3DPhongMaterial` in the header file instead of a more cumbersome > initialization in the constructor (this is allowed since C++ 11). > * `D3DLight` > * Commented out unused methods. Were they supposed to be used at some point? > * Renamed the `w` component to `lightOn` since it controls the number of > lights for the special pixel shader variant and having it in the 4th position > of the color array was confusing. > * `D3DPhongShader.h` > * Renamed some of the register constants for more clarity. > * Moved the ambient light color constant from the vertex shader to the > pixel shader (see the shader section below). I don't understand the > calculation of the number of registers in the comment there: "8 ambient > points + 2 coords = 10". There is 1 ambient light, what are the ambient > points and coordinates? In `vsConstants` there is `gAmbinetData[10]`, but it > is unused. > * Reduced the number of assigned vertex register for the `VSR_LIGHTS` > constant since it included both position and color, but color was unused > there (it was used directly in the pixel shader), so now it's only the > position. > * `D3DMeshView.cc` > * Unified the lighting loop that prepares the lights' 4-vetors that are > passed to the shaders. > * Removed the direction normalization as stated in the change for > `NGShape3D.java`. > * Reordered the shader constant assignment to be the same order as in > `D3DPhongShader.h`. > > **Native shaders** > * Renamed many of the variables to what I think are more descriptive names. > This includes noting in which space they exist as some calculations are done > in model space, some in world space, and we might need to do some in view > space. For vectors, also noted if the vector is to or from (`eye` doesn't > tell me if it's from or to the camera). > * Commented out many unused functions. I don't know what they are for, so I > didn't remove them. > * `vsConstants` > * Removed the light color from here since it's unused, only the position is. > * Removed the ambient light color constant from here since it's unused, and > added it to `psConstants` instead. > * `vs2ps` > * Removed the ambient color interpolation, which frees a register (no > change in performance). > * Simplified the structure (what is `LocalBumpOut` and why is it called > `light` and contains `LocalBump`?). > * `Mtl1PS` and `psMath` > * Moved the shader variant constants (`#ifndef`) to `Mtl1PS` where they are > used for better clarity. > * Moved the lights loop to `Mtl1PS`. The calculation itself remains in > `psMath`. > > No changes in performance were measured and the behavior stayed the same. Nir Lisker has updated the pull request incrementally with two additional commits since the last revision: - Changed comment as suggested - Removed unused fields - Changes: - all: https://git.openjdk.org/jfx/pull/789/files - new: https://git.openjdk.org/jfx/pull/789/files/856a9db4..bb9f8026 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=789=10 - incr: https://webrevs.openjdk.org/?repo=jfx=789=09-10 Stats: 4 lines in 2 files changed: 0 ins; 3 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/789.diff Fetch: git fetch https://git.openjdk.org/jfx pull/789/head:pull/789 PR: https://git.openjdk.org/jfx/pull/789
Re: RFR: 8217853: Cleanup in the D3D native pipeline [v10]
> Refactoring and renaming changes to some of the D3D pipeline files and a few > changes on the Java side. These are various "leftovers" from previous issues > that we didn't want to touch at the time in order to confine the scope of the > changes. They will make future work easier. > > Since there are many small changes, I'm giving a full list here: > > **Java** > > * `NGShape3D.java` > * Extracted methods to help with the cumbersome lighting loop: one method > per light type + empty light (reset light) + default point light. This > section of the code would benefit from the upcoming pattern matching on > `switch`. > * Normalized the direction here instead of in the native code. > * Ambient light is now only set when it exists (and is not black). > * `NGPointLight,java` - removed unneeded methods that were used by > `NGShape3D` before the per-lighting methods were extracted (point light > doesn't need spotlight-specific methods since they each have their own "add" > method). > * `NGSpotLight.java` - removed `@Override` annotations as a result of the > above change. > > **Native C++** > > * Initialized the class members of `D3DLight`, `D3DMeshView` and > `D3DPhongMaterial` in the header file instead of a more cumbersome > initialization in the constructor (this is allowed since C++ 11). > * `D3DLight` > * Commented out unused methods. Were they supposed to be used at some point? > * Renamed the `w` component to `lightOn` since it controls the number of > lights for the special pixel shader variant and having it in the 4th position > of the color array was confusing. > * `D3DPhongShader.h` > * Renamed some of the register constants for more clarity. > * Moved the ambient light color constant from the vertex shader to the > pixel shader (see the shader section below). I don't understand the > calculation of the number of registers in the comment there: "8 ambient > points + 2 coords = 10". There is 1 ambient light, what are the ambient > points and coordinates? In `vsConstants` there is `gAmbinetData[10]`, but it > is unused. > * Reduced the number of assigned vertex register for the `VSR_LIGHTS` > constant since it included both position and color, but color was unused > there (it was used directly in the pixel shader), so now it's only the > position. > * `D3DMeshView.cc` > * Unified the lighting loop that prepares the lights' 4-vetors that are > passed to the shaders. > * Removed the direction normalization as stated in the change for > `NGShape3D.java`. > * Reordered the shader constant assignment to be the same order as in > `D3DPhongShader.h`. > > **Native shaders** > * Renamed many of the variables to what I think are more descriptive names. > This includes noting in which space they exist as some calculations are done > in model space, some in world space, and we might need to do some in view > space. For vectors, also noted if the vector is to or from (`eye` doesn't > tell me if it's from or to the camera). > * Commented out many unused functions. I don't know what they are for, so I > didn't remove them. > * `vsConstants` > * Removed the light color from here since it's unused, only the position is. > * Removed the ambient light color constant from here since it's unused, and > added it to `psConstants` instead. > * `vs2ps` > * Removed the ambient color interpolation, which frees a register (no > change in performance). > * Simplified the structure (what is `LocalBumpOut` and why is it called > `light` and contains `LocalBump`?). > * `Mtl1PS` and `psMath` > * Moved the shader variant constants (`#ifndef`) to `Mtl1PS` where they are > used for better clarity. > * Moved the lights loop to `Mtl1PS`. The calculation itself remains in > `psMath`. > > No changes in performance were measured and the behavior stayed the same. Nir Lisker 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 17 additional commits since the last revision: - Merge remote-tracking branch 'origin/master' into 8217853_Cleanup_in_the_D3D_native_pipeline - Changed the order of semantics and moved constant out of struct - Add reserved registers comment - Change xyz to rgb - Added a default light mode - Reverted a change that breaks ambient lighting - Addressed review comment - Addressed review comments - undo debugging - Merge remote-tracking branch 'origin/master' into 8217853_Cleanup_in_the_D3D_native_pipeline - ... and 7 more: https://git.openjdk.org/jfx/compare/ab956ab5...856a9db4 - Changes: - all: https://git.openjdk.org/j
Re: RFR: 8217853: Cleanup in the D3D native pipeline [v9]
On Sun, 25 Dec 2022 03:28:27 GMT, Nir Lisker wrote: >> Refactoring and renaming changes to some of the D3D pipeline files and a few >> changes on the Java side. These are various "leftovers" from previous issues >> that we didn't want to touch at the time in order to confine the scope of >> the changes. They will make future work easier. >> >> Since there are many small changes, I'm giving a full list here: >> >> **Java** >> >> * `NGShape3D.java` >> * Extracted methods to help with the cumbersome lighting loop: one method >> per light type + empty light (reset light) + default point light. This >> section of the code would benefit from the upcoming pattern matching on >> `switch`. >> * Normalized the direction here instead of in the native code. >> * Ambient light is now only set when it exists (and is not black). >> * `NGPointLight,java` - removed unneeded methods that were used by >> `NGShape3D` before the per-lighting methods were extracted (point light >> doesn't need spotlight-specific methods since they each have their own "add" >> method). >> * `NGSpotLight.java` - removed `@Override` annotations as a result of the >> above change. >> >> **Native C++** >> >> * Initialized the class members of `D3DLight`, `D3DMeshView` and >> `D3DPhongMaterial` in the header file instead of a more cumbersome >> initialization in the constructor (this is allowed since C++ 11). >> * `D3DLight` >> * Commented out unused methods. Were they supposed to be used at some >> point? >> * Renamed the `w` component to `lightOn` since it controls the number of >> lights for the special pixel shader variant and having it in the 4th >> position of the color array was confusing. >> * `D3DPhongShader.h` >> * Renamed some of the register constants for more clarity. >> * Moved the ambient light color constant from the vertex shader to the >> pixel shader (see the shader section below). I don't understand the >> calculation of the number of registers in the comment there: "8 ambient >> points + 2 coords = 10". There is 1 ambient light, what are the ambient >> points and coordinates? In `vsConstants` there is `gAmbinetData[10]`, but it >> is unused. >> * Reduced the number of assigned vertex register for the `VSR_LIGHTS` >> constant since it included both position and color, but color was unused >> there (it was used directly in the pixel shader), so now it's only the >> position. >> * `D3DMeshView.cc` >> * Unified the lighting loop that prepares the lights' 4-vetors that are >> passed to the shaders. >> * Removed the direction normalization as stated in the change for >> `NGShape3D.java`. >> * Reordered the shader constant assignment to be the same order as in >> `D3DPhongShader.h`. >> >> **Native shaders** >> * Renamed many of the variables to what I think are more descriptive names. >> This includes noting in which space they exist as some calculations are done >> in model space, some in world space, and we might need to do some in view >> space. For vectors, also noted if the vector is to or from (`eye` doesn't >> tell me if it's from or to the camera). >> * Commented out many unused functions. I don't know what they are for, so I >> didn't remove them. >> * `vsConstants` >> * Removed the light color from here since it's unused, only the position >> is. >> * Removed the ambient light color constant from here since it's unused, >> and added it to `psConstants` instead. >> * `vs2ps` >> * Removed the ambient color interpolation, which frees a register (no >> change in performance). >> * Simplified the structure (what is `LocalBumpOut` and why is it called >> `light` and contains `LocalBump`?). >> * `Mtl1PS` and `psMath` >> * Moved the shader variant constants (`#ifndef`) to `Mtl1PS` where they >> are used for better clarity. >> * Moved the lights loop to `Mtl1PS`. The calculation itself remains in >> `psMath`. >> >> No changes in performance were measured and the behavior stayed the same. > > Nir Lisker has updated the pull request incrementally with three additional > commits since the last revision: > > - Changed the order of semantics and moved constant out of struct > - Add reserved registers comment > - Change xyz to rgb For reasons that I don't understand, the problem was the order of the semantics declared in the `vs2ps` struct. Moving `float2 texD : texcoord0;` down fixed the issue. I played a bit with the ordering out of curiosity and got different results. I didn't see anything in the documentation that talks about the order of the interpolated variables. This fixed problem 2 for me. - PR: https://git.openjdk.org/jfx/pull/789
Re: RFR: 8217853: Cleanup in the D3D native pipeline [v9]
> Refactoring and renaming changes to some of the D3D pipeline files and a few > changes on the Java side. These are various "leftovers" from previous issues > that we didn't want to touch at the time in order to confine the scope of the > changes. They will make future work easier. > > Since there are many small changes, I'm giving a full list here: > > **Java** > > * `NGShape3D.java` > * Extracted methods to help with the cumbersome lighting loop: one method > per light type + empty light (reset light) + default point light. This > section of the code would benefit from the upcoming pattern matching on > `switch`. > * Normalized the direction here instead of in the native code. > * Ambient light is now only set when it exists (and is not black). > * `NGPointLight,java` - removed unneeded methods that were used by > `NGShape3D` before the per-lighting methods were extracted (point light > doesn't need spotlight-specific methods since they each have their own "add" > method). > * `NGSpotLight.java` - removed `@Override` annotations as a result of the > above change. > > **Native C++** > > * Initialized the class members of `D3DLight`, `D3DMeshView` and > `D3DPhongMaterial` in the header file instead of a more cumbersome > initialization in the constructor (this is allowed since C++ 11). > * `D3DLight` > * Commented out unused methods. Were they supposed to be used at some point? > * Renamed the `w` component to `lightOn` since it controls the number of > lights for the special pixel shader variant and having it in the 4th position > of the color array was confusing. > * `D3DPhongShader.h` > * Renamed some of the register constants for more clarity. > * Moved the ambient light color constant from the vertex shader to the > pixel shader (see the shader section below). I don't understand the > calculation of the number of registers in the comment there: "8 ambient > points + 2 coords = 10". There is 1 ambient light, what are the ambient > points and coordinates? In `vsConstants` there is `gAmbinetData[10]`, but it > is unused. > * Reduced the number of assigned vertex register for the `VSR_LIGHTS` > constant since it included both position and color, but color was unused > there (it was used directly in the pixel shader), so now it's only the > position. > * `D3DMeshView.cc` > * Unified the lighting loop that prepares the lights' 4-vetors that are > passed to the shaders. > * Removed the direction normalization as stated in the change for > `NGShape3D.java`. > * Reordered the shader constant assignment to be the same order as in > `D3DPhongShader.h`. > > **Native shaders** > * Renamed many of the variables to what I think are more descriptive names. > This includes noting in which space they exist as some calculations are done > in model space, some in world space, and we might need to do some in view > space. For vectors, also noted if the vector is to or from (`eye` doesn't > tell me if it's from or to the camera). > * Commented out many unused functions. I don't know what they are for, so I > didn't remove them. > * `vsConstants` > * Removed the light color from here since it's unused, only the position is. > * Removed the ambient light color constant from here since it's unused, and > added it to `psConstants` instead. > * `vs2ps` > * Removed the ambient color interpolation, which frees a register (no > change in performance). > * Simplified the structure (what is `LocalBumpOut` and why is it called > `light` and contains `LocalBump`?). > * `Mtl1PS` and `psMath` > * Moved the shader variant constants (`#ifndef`) to `Mtl1PS` where they are > used for better clarity. > * Moved the lights loop to `Mtl1PS`. The calculation itself remains in > `psMath`. > > No changes in performance were measured and the behavior stayed the same. Nir Lisker has updated the pull request incrementally with three additional commits since the last revision: - Changed the order of semantics and moved constant out of struct - Add reserved registers comment - Change xyz to rgb - Changes: - all: https://git.openjdk.org/jfx/pull/789/files - new: https://git.openjdk.org/jfx/pull/789/files/42994e9c..55fe2dc7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=789=08 - incr: https://webrevs.openjdk.org/?repo=jfx=789=07-08 Stats: 21 lines in 5 files changed: 5 ins; 5 del; 11 mod Patch: https://git.openjdk.org/jfx/pull/789.diff Fetch: git fetch https://git.openjdk.org/jfx pull/789/head:pull/789 PR: https://git.openjdk.org/jfx/pull/789
Re: [Puppet Users] Create a variable out of a bash command and notify it
@Martin,
I have tried your approach; I put the facter file underneath:
/opt/puppetlabs/puppet/lib/ruby/vendor_ruby/facter/custom_facts/core/execution/
*file:*
username.rb
*content:*
*# username.rb*
*Facter.add(:username) do*
* setcode do*
*Facter::Core::Execution.execute("/usr/bin/hostname |/usr/bin/awk -F'-'
{'print $1'}")*
* end*
*end*
But I keep getting an error:
Error: Evaluation Error: Empty string title at 0. Title strings must have a
length greater than zero. (line: 1, column: 8) on node
puppet-master.domain.com
what am I missing here?
On Monday, December 5, 2022 at 10:02:09 AM UTC+2 Nir Fishler wrote:
> Thanks a lot guys for your prompt reply!
>
> Gonna try it out later on.
>
> On Monday, December 5, 2022 at 9:47:52 AM UTC+2 dhei...@opentext.com
> wrote:
>
>> Am Sonntag, dem 04.12.2022 um 07:43 -0800 schrieb Nir Fishler:
>>
>> Main goal: get a list of upgrade-able packages and notify the user about
>> it.
>>
>>
>> The puppetlabs-apt module from the Puppet Forge has various facts for
>> this.
>>
>> HTH...
>>
>> Dirk
>>
>> --
>>
>> *Dirk Heinrichs*
>> Senior Systems Engineer, Delivery Pipeline
>> OpenText ™ Discovery | Recommind
>> *Phone*: +49 2226 15966 18 <+49%202226%201596618>
>> *Email*: dhei...@opentext.com
>> *Website*: www.recommind.de
>> Recommind GmbH, Von-Liebig-Straße 1, 53359 Rheinbach
>> Vertretungsberechtigte Geschäftsführer Gordon Davies, Madhu Ranganathan,
>> Christian Waida, Registergericht Amtsgericht Bonn, Registernummer HRB 10646
>> This e-mail may contain confidential and/or privileged information. If
>> you are not the intended recipient (or have received this e-mail in error)
>> please notify the sender immediately and destroy this e-mail. Any
>> unauthorized copying, disclosure or distribution of the material in this
>> e-mail is strictly forbidden
>> Diese E-Mail enthält vertrauliche und/oder rechtlich geschützte
>> Informationen. Wenn Sie nicht der richtige Adressat sind oder diese E-Mail
>> irrtümlich erhalten haben, informieren Sie bitte sofort den Absender und
>> vernichten Sie diese Mail. Das unerlaubte Kopieren sowie die unbefugte
>> Weitergabe dieser Mail sind nicht gestattet.
>>
>
--
You received this message because you are subscribed to the Google Groups
"Puppet Users" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to puppet-users+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/puppet-users/703212ce-9dd7-426a-bb8d-20a0058fc3f1n%40googlegroups.com.
Re: [Puppet Users] Puppet filters
Perfect!
Thanks Martin.
On Sunday, December 18, 2022 at 10:35:59 AM UTC+2 Martin Alfke wrote:
> Hi Nir,
>
> do you only need the capacity from the root file system?
>
> You can access the data hash directly:
> $facts['mountpoints']['/']['capacity']
>
> hth,
> Martin
>
>
> > On 18. Dec 2022, at 08:44, Nir Fishler wrote:
> >
> > Hey Guys,
> >
> > I'm trying to pull data from an array using the 'mountpoints' facter
> >
> > $root_partition = $mountpoints.filter |$values| { $values[0] =~ "/$" }
> >
> > the results I get are:
> >
> > Notice: {/ => {device => /dev/mapper/ubuntu--vg-ubuntu--lv, filesystem
> => ext4, options => [rw, relatime], size_bytes => 262901354496,
> available_bytes => 240302788608, used_bytes => 11734274048, capacity =>
> 4.66%, size => 244.85 GiB, available => 223.80 GiB, used => 10.93 GiB}}
> > Notice: /Stage[main]/Disk_usage/Notify[{/ => {device =>
> /dev/mapper/ubuntu--vg-ubuntu--lv, filesystem => ext4, options => [rw,
> relatime], size_bytes => 262901354496, available_bytes => 240302788608,
> used_bytes => 11734274048, capacity => 4.66%, size => 244.85 GiB, available
> => 223.80 GiB, used => 10.93 GiB}}]/message: defined 'message' as '{/ =>
> {device => /dev/mapper/ubuntu--vg-ubuntu--lv, filesystem => ext4, options
> => [rw, relatime], size_bytes => 262901354496, available_bytes =>
> 240302788608, used_bytes => 11734274048, capacity => 4.66%, size => 244.85
> GiB, available => 223.80 GiB, used => 10.93 GiB}}'
> >
> > What I need is the get the 'capacity' value out of it.
> >
> >
> > Thanks!
> > Nir.
> >
> > --
> > You received this message because you are subscribed to the Google
> Groups "Puppet Users" group.
> > To unsubscribe from this group and stop receiving emails from it, send
> an email to puppet-users...@googlegroups.com.
> > To view this discussion on the web visit
> https://groups.google.com/d/msgid/puppet-users/a3de8dab-287c-40da-be6c-c8036bba38c5n%40googlegroups.com
> .
>
>
--
You received this message because you are subscribed to the Google Groups
"Puppet Users" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to puppet-users+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/puppet-users/0cd95451-576d-44e9-b55a-a37d522ddfa4n%40googlegroups.com.
[Puppet Users] Puppet filters
Hey Guys,
I'm trying to pull data from an array using the 'mountpoints' facter
$root_partition = $mountpoints.filter |$values| { $values[0] =~ "/$" }
the results I get are:
Notice: {/ => {device => /dev/mapper/ubuntu--vg-ubuntu--lv, filesystem =>
ext4, options => [rw, relatime], size_bytes => 262901354496,
available_bytes => 240302788608, used_bytes => 11734274048, capacity =>
4.66%, size => 244.85 GiB, available => 223.80 GiB, used => 10.93 GiB}}
Notice: /Stage[main]/Disk_usage/Notify[{/ => {device =>
/dev/mapper/ubuntu--vg-ubuntu--lv, filesystem => ext4, options => [rw,
relatime], size_bytes => 262901354496, available_bytes => 240302788608,
used_bytes => 11734274048, capacity => 4.66%, size => 244.85 GiB, available
=> 223.80 GiB, used => 10.93 GiB}}]/message: defined 'message' as '{/ =>
{device => /dev/mapper/ubuntu--vg-ubuntu--lv, filesystem => ext4, options
=> [rw, relatime], size_bytes => 262901354496, available_bytes =>
240302788608, used_bytes => 11734274048, capacity => 4.66%, size => 244.85
GiB, available => 223.80 GiB, used => 10.93 GiB}}'
What I need is the get the 'capacity' value out of it.
Thanks!
Nir.
--
You received this message because you are subscribed to the Google Groups
"Puppet Users" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to puppet-users+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/puppet-users/a3de8dab-287c-40da-be6c-c8036bba38c5n%40googlegroups.com.
Re: Setting graphics of a Labeled does not show the Label correctly
I tried to change the behavior of setting a graphic for Labeled to one that removes the graphics from its previous labeled (if exists). This resulted in test failures in a few places: * MenuButton: * MenuButtonTest::testSetContentDisplayGraphicOnly - deliberately sets the same graphic to two MenuButtons. Doesn't look correct. * LabeledImpl.Shuttler::invalidated - used by MenuButtonSkinBase, sets the graphics of the Labeled to that of its own Label. That is, it doesn't use the MenuButton's own Label, but creates a different one to use as a child and copies the properties to it. The test that fails is LabeledImplOtherTest. test_RT_21357 [1]. Also seems like an odd implementation choice. Also note that other controls that are not a Labeled have graphic properties, like Tab (as mstr mentioned), so this change doesn't solve all the cases, only those within Labeled. [1] https://bugs.openjdk.org/browse/JDK-8119175 On Mon, Dec 12, 2022 at 6:10 PM Nir Lisker wrote: > Another idea is to use a static map that maps a node to its "graphic > parent". That will save on memory at the expense of a lookup. > > On Thu, Dec 1, 2022 at 11:53 PM Nir Lisker wrote: > >> Are we convinced that a node can't be both a graphic and a clip, or >> something else? The docs for clip specify that the node is not a child in >> the scenegraph. >> >> On Thu, Dec 1, 2022 at 11:41 PM John Hendrikx >> wrote: >> >>> Adding another field doesn't seem ideal, would it be possible to >>> generalize the clipParent field to function for both (the ownedBy or owner >>> field that I suggested earlier)? >>> >>> --John >>> On 01/12/2022 20:26, Nir Lisker wrote: >>> >>> Michael's idea could solve the problem if it's about more than just >>> traversing, it needs to set rules for allowing a node to serve only 1 >>> logical role (or 1 role type, like clip and graphic?) at the same time. In >>> any case, these rules need to be decided upon before starting to work on >>> anything. I can do a quick fix for now that can be reverted later >>> if needed. From what I gather, I will need to add a graphicsParent field >>> like clipParent does. >>> >>> On Thu, Dec 1, 2022 at 8:47 PM Nir Lisker wrote: >>> >>>> By the way, these issues are caused by this inconsistent behavior (they >>>> are probably duplicates): >>>> >>>> https://bugs.openjdk.org/browse/JDK-8209017 >>>> https://bugs.openjdk.org/browse/JDK-8190331 >>>> >>>> The graphic of the checkbox of a CheckBoxTreeItem is not set correctly >>>> on the new CheckBox that is provided with the cell when virtual flow >>>> switches it. It might happen with other controls that use virtual flow. >>>> >>>> On Thu, Dec 1, 2022 at 8:40 PM Kevin Rushforth < >>>> kevin.rushfo...@oracle.com> wrote: >>>> >>>>> This seems related, but somewhat tangential. A Control's "graphic" >>>>> isn't >>>>> a child node, just like a Shape's "clip" isn't a child node. >>>>> >>>>> Creating a separate "document graph" (or "logical graph") sounds like >>>>> an >>>>> interesting idea, but it would bring its own set of challenges. And it >>>>> wouldn't directly solve this case anyway. >>>>> >>>>> -- Kevin >>>>> >>>>> >>>>> On 12/1/2022 9:42 AM, Michael Strauß wrote: >>>>> > There's a larger picture here: from a user perspective, there's a >>>>> > difference between the scene graph and the "document graph". >>>>> > The document graph is what users actually work with, for example by >>>>> > setting the `Labeled.graphic` property. In some cases, document nodes >>>>> > don't correspond to scene nodes at all (`MenuItem` or `Tab` come to >>>>> > mind). >>>>> > The document graph is later inflated into a scene graph of unknown >>>>> > structure (because skins are mostly black boxes with regards to their >>>>> > internal structure). >>>>> > >>>>> > I've proposed an enhancement that would make the document graph a >>>>> > first-class citizen: >>>>> > https://mail.openjdk.org/pipermail/openjfx-dev/2022-June/034417.html >>>>> > >>>>> > With this in place, we could simply disallow the same node appearing >>>>> > multiple times in the docume
Re: [PATCH v2 2/5] Support format or cache specific out file
On Tue, Dec 13, 2022 at 8:09 PM Hanna Reitz wrote:
>
> On 13.12.22 16:56, Nir Soffer wrote:
> > On Mon, Dec 12, 2022 at 12:38 PM Hanna Reitz wrote:
> >> On 28.11.22 15:15, Nir Soffer wrote:
> >>> Extend the test finder to find tests with format (*.out.qcow2) or cache
> >>> specific (*.out.nocache) out file. This worked before only for the
> >>> numbered tests.
> >>> ---
> >>>tests/qemu-iotests/findtests.py | 10 --
> >>>1 file changed, 8 insertions(+), 2 deletions(-)
> >> This patch lacks an S-o-b, too.
> >>
> >>> diff --git a/tests/qemu-iotests/findtests.py
> >>> b/tests/qemu-iotests/findtests.py
> >>> index dd77b453b8..f4344ce78c 100644
> >>> --- a/tests/qemu-iotests/findtests.py
> >>> +++ b/tests/qemu-iotests/findtests.py
> >>> @@ -38,31 +38,37 @@ def chdir(path: Optional[str] = None) ->
> >>> Iterator[None]:
> >>>os.chdir(saved_dir)
> >>>
> >>>
> >>>class TestFinder:
> >>>def __init__(self, test_dir: Optional[str] = None) -> None:
> >>>self.groups = defaultdict(set)
> >>>
> >>>with chdir(test_dir):
> >>>self.all_tests = glob.glob('[0-9][0-9][0-9]')
> >>>self.all_tests += [f for f in glob.iglob('tests/*')
> >>> - if not f.endswith('.out') and
> >>> - os.path.isfile(f + '.out')]
> >>> + if self.is_test(f)]
> >> So previously a file was only considered a test file if there was a
> >> corresponding reference output file (`f + '.out'`), so files without
> >> such a reference output aren’t considered test files...
> >>
> >>>for t in self.all_tests:
> >>>with open(t, encoding="utf-8") as f:
> >>>for line in f:
> >>>if line.startswith('# group: '):
> >>>for g in line.split()[2:]:
> >>>self.groups[g].add(t)
> >>>break
> >>>
> >>> +def is_test(self, fname: str) -> bool:
> >>> +"""
> >>> +The tests directory contains tests (no extension) and out files
> >>> +(*.out, *.out.{format}, *.out.{option}).
> >>> +"""
> >>> +return re.search(r'.+\.out(\.\w+)?$', fname) is None
> >> ...but this new function doesn’t check that. I think we should check it
> >> (just whether there’s any variant of `/{fname}\.out(\.\w+)?/` to go with
> >> `fname`) so that behavior isn’t changed.
> > This means that you cannot add a test without a *.out* file, which may
> > be useful when you don't use the out file for validation, but we can
> > add this later if needed.
>
> I don’t think tests work without a reference output, do they? At least
> a couple of years ago, the ./check script would refuse to run tests
> without a corresponding .out file.
This may be true, but most tests do not really need an out file and better be
verified by asserting. There are some python tests that have pointless out
file with the output of python unittest:
$ cat tests/qemu-iotests/tests/nbd-multiconn.out
...
--
Ran 3 tests
OK
This is not only unhelpful (update the output when adding a 4th test)
but fragile.
if unitests changes the output, maybe adding info about skipped tests, or
changing "---" to "", the test will break.
But for now I agree the test framework should keep the current behavior.
Nir
Re: [PATCH v2 2/5] Support format or cache specific out file
On Tue, Dec 13, 2022 at 8:09 PM Hanna Reitz wrote:
>
> On 13.12.22 16:56, Nir Soffer wrote:
> > On Mon, Dec 12, 2022 at 12:38 PM Hanna Reitz wrote:
> >> On 28.11.22 15:15, Nir Soffer wrote:
> >>> Extend the test finder to find tests with format (*.out.qcow2) or cache
> >>> specific (*.out.nocache) out file. This worked before only for the
> >>> numbered tests.
> >>> ---
> >>>tests/qemu-iotests/findtests.py | 10 --
> >>>1 file changed, 8 insertions(+), 2 deletions(-)
> >> This patch lacks an S-o-b, too.
> >>
> >>> diff --git a/tests/qemu-iotests/findtests.py
> >>> b/tests/qemu-iotests/findtests.py
> >>> index dd77b453b8..f4344ce78c 100644
> >>> --- a/tests/qemu-iotests/findtests.py
> >>> +++ b/tests/qemu-iotests/findtests.py
> >>> @@ -38,31 +38,37 @@ def chdir(path: Optional[str] = None) ->
> >>> Iterator[None]:
> >>>os.chdir(saved_dir)
> >>>
> >>>
> >>>class TestFinder:
> >>>def __init__(self, test_dir: Optional[str] = None) -> None:
> >>>self.groups = defaultdict(set)
> >>>
> >>>with chdir(test_dir):
> >>>self.all_tests = glob.glob('[0-9][0-9][0-9]')
> >>>self.all_tests += [f for f in glob.iglob('tests/*')
> >>> - if not f.endswith('.out') and
> >>> - os.path.isfile(f + '.out')]
> >>> + if self.is_test(f)]
> >> So previously a file was only considered a test file if there was a
> >> corresponding reference output file (`f + '.out'`), so files without
> >> such a reference output aren’t considered test files...
> >>
> >>>for t in self.all_tests:
> >>>with open(t, encoding="utf-8") as f:
> >>>for line in f:
> >>>if line.startswith('# group: '):
> >>>for g in line.split()[2:]:
> >>>self.groups[g].add(t)
> >>>break
> >>>
> >>> +def is_test(self, fname: str) -> bool:
> >>> +"""
> >>> +The tests directory contains tests (no extension) and out files
> >>> +(*.out, *.out.{format}, *.out.{option}).
> >>> +"""
> >>> +return re.search(r'.+\.out(\.\w+)?$', fname) is None
> >> ...but this new function doesn’t check that. I think we should check it
> >> (just whether there’s any variant of `/{fname}\.out(\.\w+)?/` to go with
> >> `fname`) so that behavior isn’t changed.
> > This means that you cannot add a test without a *.out* file, which may
> > be useful when you don't use the out file for validation, but we can
> > add this later if needed.
>
> I don’t think tests work without a reference output, do they? At least
> a couple of years ago, the ./check script would refuse to run tests
> without a corresponding .out file.
This may be true, but most tests do not really need an out file and better be
verified by asserting. There are some python tests that have pointless out
file with the output of python unittest:
$ cat tests/qemu-iotests/tests/nbd-multiconn.out
...
--
Ran 3 tests
OK
This is not only unhelpful (update the output when adding a 4th test)
but fragile.
if unitests changes the output, maybe adding info about skipped tests, or
changing "---" to "", the test will break.
But for now I agree the test framework should keep the current behavior.
Nir
Re: [PATCH v2 2/5] Support format or cache specific out file
On Mon, Dec 12, 2022 at 12:38 PM Hanna Reitz wrote:
>
> On 28.11.22 15:15, Nir Soffer wrote:
> > Extend the test finder to find tests with format (*.out.qcow2) or cache
> > specific (*.out.nocache) out file. This worked before only for the
> > numbered tests.
> > ---
> > tests/qemu-iotests/findtests.py | 10 --
> > 1 file changed, 8 insertions(+), 2 deletions(-)
>
> This patch lacks an S-o-b, too.
>
> > diff --git a/tests/qemu-iotests/findtests.py
> > b/tests/qemu-iotests/findtests.py
> > index dd77b453b8..f4344ce78c 100644
> > --- a/tests/qemu-iotests/findtests.py
> > +++ b/tests/qemu-iotests/findtests.py
> > @@ -38,31 +38,37 @@ def chdir(path: Optional[str] = None) -> Iterator[None]:
> > os.chdir(saved_dir)
> >
> >
> > class TestFinder:
> > def __init__(self, test_dir: Optional[str] = None) -> None:
> > self.groups = defaultdict(set)
> >
> > with chdir(test_dir):
> > self.all_tests = glob.glob('[0-9][0-9][0-9]')
> > self.all_tests += [f for f in glob.iglob('tests/*')
> > - if not f.endswith('.out') and
> > - os.path.isfile(f + '.out')]
> > + if self.is_test(f)]
>
> So previously a file was only considered a test file if there was a
> corresponding reference output file (`f + '.out'`), so files without
> such a reference output aren’t considered test files...
>
> > for t in self.all_tests:
> > with open(t, encoding="utf-8") as f:
> > for line in f:
> > if line.startswith('# group: '):
> > for g in line.split()[2:]:
> > self.groups[g].add(t)
> > break
> >
> > +def is_test(self, fname: str) -> bool:
> > +"""
> > +The tests directory contains tests (no extension) and out files
> > +(*.out, *.out.{format}, *.out.{option}).
> > +"""
> > +return re.search(r'.+\.out(\.\w+)?$', fname) is None
>
> ...but this new function doesn’t check that. I think we should check it
> (just whether there’s any variant of `/{fname}\.out(\.\w+)?/` to go with
> `fname`) so that behavior isn’t changed.
This means that you cannot add a test without a *.out* file, which may
be useful when you don't use the out file for validation, but we can
add this later if needed.
I'll change the code to check both conditions.
Re: [PATCH v2 2/5] Support format or cache specific out file
On Mon, Dec 12, 2022 at 12:38 PM Hanna Reitz wrote:
>
> On 28.11.22 15:15, Nir Soffer wrote:
> > Extend the test finder to find tests with format (*.out.qcow2) or cache
> > specific (*.out.nocache) out file. This worked before only for the
> > numbered tests.
> > ---
> > tests/qemu-iotests/findtests.py | 10 --
> > 1 file changed, 8 insertions(+), 2 deletions(-)
>
> This patch lacks an S-o-b, too.
>
> > diff --git a/tests/qemu-iotests/findtests.py
> > b/tests/qemu-iotests/findtests.py
> > index dd77b453b8..f4344ce78c 100644
> > --- a/tests/qemu-iotests/findtests.py
> > +++ b/tests/qemu-iotests/findtests.py
> > @@ -38,31 +38,37 @@ def chdir(path: Optional[str] = None) -> Iterator[None]:
> > os.chdir(saved_dir)
> >
> >
> > class TestFinder:
> > def __init__(self, test_dir: Optional[str] = None) -> None:
> > self.groups = defaultdict(set)
> >
> > with chdir(test_dir):
> > self.all_tests = glob.glob('[0-9][0-9][0-9]')
> > self.all_tests += [f for f in glob.iglob('tests/*')
> > - if not f.endswith('.out') and
> > - os.path.isfile(f + '.out')]
> > + if self.is_test(f)]
>
> So previously a file was only considered a test file if there was a
> corresponding reference output file (`f + '.out'`), so files without
> such a reference output aren’t considered test files...
>
> > for t in self.all_tests:
> > with open(t, encoding="utf-8") as f:
> > for line in f:
> > if line.startswith('# group: '):
> > for g in line.split()[2:]:
> > self.groups[g].add(t)
> > break
> >
> > +def is_test(self, fname: str) -> bool:
> > +"""
> > +The tests directory contains tests (no extension) and out files
> > +(*.out, *.out.{format}, *.out.{option}).
> > +"""
> > +return re.search(r'.+\.out(\.\w+)?$', fname) is None
>
> ...but this new function doesn’t check that. I think we should check it
> (just whether there’s any variant of `/{fname}\.out(\.\w+)?/` to go with
> `fname`) so that behavior isn’t changed.
This means that you cannot add a test without a *.out* file, which may
be useful when you don't use the out file for validation, but we can
add this later if needed.
I'll change the code to check both conditions.
Re: RFR: 8217853: Cleanup in the D3D native pipeline [v8]
> Refactoring and renaming changes to some of the D3D pipeline files and a few > changes on the Java side. These are various "leftovers" from previous issues > that we didn't want to touch at the time in order to confine the scope of the > changes. They will make future work easier. > > Since there are many small changes, I'm giving a full list here: > > **Java** > > * `NGShape3D.java` > * Extracted methods to help with the cumbersome lighting loop: one method > per light type + empty light (reset light) + default point light. This > section of the code would benefit from the upcoming pattern matching on > `switch`. > * Normalized the direction here instead of in the native code. > * Ambient light is now only set when it exists (and is not black). > * `NGPointLight,java` - removed unneeded methods that were used by > `NGShape3D` before the per-lighting methods were extracted (point light > doesn't need spotlight-specific methods since they each have their own "add" > method). > * `NGSpotLight.java` - removed `@Override` annotations as a result of the > above change. > > **Native C++** > > * Initialized the class members of `D3DLight`, `D3DMeshView` and > `D3DPhongMaterial` in the header file instead of a more cumbersome > initialization in the constructor (this is allowed since C++ 11). > * `D3DLight` > * Commented out unused methods. Were they supposed to be used at some point? > * Renamed the `w` component to `lightOn` since it controls the number of > lights for the special pixel shader variant and having it in the 4th position > of the color array was confusing. > * `D3DPhongShader.h` > * Renamed some of the register constants for more clarity. > * Moved the ambient light color constant from the vertex shader to the > pixel shader (see the shader section below). I don't understand the > calculation of the number of registers in the comment there: "8 ambient > points + 2 coords = 10". There is 1 ambient light, what are the ambient > points and coordinates? In `vsConstants` there is `gAmbinetData[10]`, but it > is unused. > * Reduced the number of assigned vertex register for the `VSR_LIGHTS` > constant since it included both position and color, but color was unused > there (it was used directly in the pixel shader), so now it's only the > position. > * `D3DMeshView.cc` > * Unified the lighting loop that prepares the lights' 4-vetors that are > passed to the shaders. > * Removed the direction normalization as stated in the change for > `NGShape3D.java`. > * Reordered the shader constant assignment to be the same order as in > `D3DPhongShader.h`. > > **Native shaders** > * Renamed many of the variables to what I think are more descriptive names. > This includes noting in which space they exist as some calculations are done > in model space, some in world space, and we might need to do some in view > space. For vectors, also noted if the vector is to or from (`eye` doesn't > tell me if it's from or to the camera). > * Commented out many unused functions. I don't know what they are for, so I > didn't remove them. > * `vsConstants` > * Removed the light color from here since it's unused, only the position is. > * Removed the ambient light color constant from here since it's unused, and > added it to `psConstants` instead. > * `vs2ps` > * Removed the ambient color interpolation, which frees a register (no > change in performance). > * Simplified the structure (what is `LocalBumpOut` and why is it called > `light` and contains `LocalBump`?). > * `Mtl1PS` and `psMath` > * Moved the shader variant constants (`#ifndef`) to `Mtl1PS` where they are > used for better clarity. > * Moved the lights loop to `Mtl1PS`. The calculation itself remains in > `psMath`. > > No changes in performance were measured and the behavior stayed the same. Nir Lisker has updated the pull request incrementally with two additional commits since the last revision: - Added a default light mode - Reverted a change that breaks ambient lighting - Changes: - all: https://git.openjdk.org/jfx/pull/789/files - new: https://git.openjdk.org/jfx/pull/789/files/1f66f613..42994e9c Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=789=07 - incr: https://webrevs.openjdk.org/?repo=jfx=789=06-07 Stats: 56 lines in 4 files changed: 12 ins; 17 del; 27 mod Patch: https://git.openjdk.org/jfx/pull/789.diff Fetch: git fetch https://git.openjdk.org/jfx pull/789/head:pull/789 PR: https://git.openjdk.org/jfx/pull/789
Re: RFR: 8217853: Cleanup in the D3D native pipeline [v7]
On Thu, 1 Sep 2022 11:23:44 GMT, Nir Lisker wrote: >> Refactoring and renaming changes to some of the D3D pipeline files and a few >> changes on the Java side. These are various "leftovers" from previous issues >> that we didn't want to touch at the time in order to confine the scope of >> the changes. They will make future work easier. >> >> Since there are many small changes, I'm giving a full list here: >> >> **Java** >> >> * `NGShape3D.java` >> * Extracted methods to help with the cumbersome lighting loop: one method >> per light type + empty light (reset light) + default point light. This >> section of the code would benefit from the upcoming pattern matching on >> `switch`. >> * Normalized the direction here instead of in the native code. >> * Ambient light is now only set when it exists (and is not black). >> * `NGPointLight,java` - removed unneeded methods that were used by >> `NGShape3D` before the per-lighting methods were extracted (point light >> doesn't need spotlight-specific methods since they each have their own "add" >> method). >> * `NGSpotLight.java` - removed `@Override` annotations as a result of the >> above change. >> >> **Native C++** >> >> * Initialized the class members of `D3DLight`, `D3DMeshView` and >> `D3DPhongMaterial` in the header file instead of a more cumbersome >> initialization in the constructor (this is allowed since C++ 11). >> * `D3DLight` >> * Commented out unused methods. Were they supposed to be used at some >> point? >> * Renamed the `w` component to `lightOn` since it controls the number of >> lights for the special pixel shader variant and having it in the 4th >> position of the color array was confusing. >> * `D3DPhongShader.h` >> * Renamed some of the register constants for more clarity. >> * Moved the ambient light color constant from the vertex shader to the >> pixel shader (see the shader section below). I don't understand the >> calculation of the number of registers in the comment there: "8 ambient >> points + 2 coords = 10". There is 1 ambient light, what are the ambient >> points and coordinates? In `vsConstants` there is `gAmbinetData[10]`, but it >> is unused. >> * Reduced the number of assigned vertex register for the `VSR_LIGHTS` >> constant since it included both position and color, but color was unused >> there (it was used directly in the pixel shader), so now it's only the >> position. >> * `D3DMeshView.cc` >> * Unified the lighting loop that prepares the lights' 4-vetors that are >> passed to the shaders. >> * Removed the direction normalization as stated in the change for >> `NGShape3D.java`. >> * Reordered the shader constant assignment to be the same order as in >> `D3DPhongShader.h`. >> >> **Native shaders** >> * Renamed many of the variables to what I think are more descriptive names. >> This includes noting in which space they exist as some calculations are done >> in model space, some in world space, and we might need to do some in view >> space. For vectors, also noted if the vector is to or from (`eye` doesn't >> tell me if it's from or to the camera). >> * Commented out many unused functions. I don't know what they are for, so I >> didn't remove them. >> * `vsConstants` >> * Removed the light color from here since it's unused, only the position >> is. >> * Removed the ambient light color constant from here since it's unused, >> and added it to `psConstants` instead. >> * `vs2ps` >> * Removed the ambient color interpolation, which frees a register (no >> change in performance). >> * Simplified the structure (what is `LocalBumpOut` and why is it called >> `light` and contains `LocalBump`?). >> * `Mtl1PS` and `psMath` >> * Moved the shader variant constants (`#ifndef`) to `Mtl1PS` where they >> are used for better clarity. >> * Moved the lights loop to `Mtl1PS`. The calculation itself remains in >> `psMath`. >> >> No changes in performance were measured and the behavior stayed the same. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Addressed review comment At first I couldn't reproduce these, then I found out the launch command of the test app points to the wrong build artifacts, so any changes I made were not reflected in my visual checks. It's the same for other test apps, so it's a good thing it was discovered. Thanks! 1. Right, I reverted this line. 2. Right, it seems to be a problem in the native level, will have to check carefully. 3. I think it just looks like that because of the position of the central lights (the magenta ones). The sphere intersects the light upon creation, so there is no lighting of the sphere with this geometry. Try to animate the sphere, or use the boxes/meshes instead that are created further away from the light. - PR: https://git.openjdk.org/jfx/pull/789
Re: RFR: 8290040: Provide simplified deterministic way to manage listeners [v14]
On Mon, 12 Dec 2022 22:42:16 GMT, John Hendrikx wrote: >> This PR adds a new (lazy*) property on `Node` which provides a boolean which >> indicates whether or not the `Node` is currently part of a `Scene`, which in >> turn is part of a currently showing `Window`. >> >> It also adds a new fluent binding method on `ObservableValue` dubbed `when` >> (open for discussion, originally I had `conditionOn` here). >> >> Both of these together means it becomes much easier to break strong >> references that prevent garbage collection between a long lived property and >> one that should be shorter lived. A good example is when a `Label` is bound >> to a long lived property: >> >> >> label.textProperty().bind(longLivedProperty.when(label::isShowingProperty)); >> >> The above basically ties the life cycle of the label to the long lived >> property **only** when the label is currently showing. When it is not >> showing, the label can be eligible for GC as the listener on >> `longLivedProperty` is removed when the condition provided by >> `label::isShowingProperty` is `false`. A big advantage is that these >> listeners stop observing the long lived property **immediately** when the >> label is no longer showing, in contrast to weak bindings which may keep >> observing the long lived property (and updating the label, and triggering >> its listeners in turn) until the next GC comes along. >> >> The issue in JBS also describes making the `Subscription` API public, but I >> think that might best be a separate PR. >> >> Note that this PR contains a bugfix in `ObjectBinding` for which there is >> another open PR: https://github.com/openjdk/jfx/pull/829 -- this is because >> the tests for the newly added method would fail otherwise; once it has been >> integrated to jfx19 and then to master, I can take the fix out. >> >> (*) Lazy means here that the property won't be creating any listeners unless >> observed itself, to avoid problems creating too many listeners on >> Scene/Window. > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Fix formatting in test Marked as reviewed by nlisker (Reviewer). - PR: https://git.openjdk.org/jfx/pull/830
Re: Setting graphics of a Labeled does not show the Label correctly
Another idea is to use a static map that maps a node to its "graphic parent". That will save on memory at the expense of a lookup. On Thu, Dec 1, 2022 at 11:53 PM Nir Lisker wrote: > Are we convinced that a node can't be both a graphic and a clip, or > something else? The docs for clip specify that the node is not a child in > the scenegraph. > > On Thu, Dec 1, 2022 at 11:41 PM John Hendrikx > wrote: > >> Adding another field doesn't seem ideal, would it be possible to >> generalize the clipParent field to function for both (the ownedBy or owner >> field that I suggested earlier)? >> >> --John >> On 01/12/2022 20:26, Nir Lisker wrote: >> >> Michael's idea could solve the problem if it's about more than just >> traversing, it needs to set rules for allowing a node to serve only 1 >> logical role (or 1 role type, like clip and graphic?) at the same time. In >> any case, these rules need to be decided upon before starting to work on >> anything. I can do a quick fix for now that can be reverted later >> if needed. From what I gather, I will need to add a graphicsParent field >> like clipParent does. >> >> On Thu, Dec 1, 2022 at 8:47 PM Nir Lisker wrote: >> >>> By the way, these issues are caused by this inconsistent behavior (they >>> are probably duplicates): >>> >>> https://bugs.openjdk.org/browse/JDK-8209017 >>> https://bugs.openjdk.org/browse/JDK-8190331 >>> >>> The graphic of the checkbox of a CheckBoxTreeItem is not set correctly >>> on the new CheckBox that is provided with the cell when virtual flow >>> switches it. It might happen with other controls that use virtual flow. >>> >>> On Thu, Dec 1, 2022 at 8:40 PM Kevin Rushforth < >>> kevin.rushfo...@oracle.com> wrote: >>> >>>> This seems related, but somewhat tangential. A Control's "graphic" >>>> isn't >>>> a child node, just like a Shape's "clip" isn't a child node. >>>> >>>> Creating a separate "document graph" (or "logical graph") sounds like >>>> an >>>> interesting idea, but it would bring its own set of challenges. And it >>>> wouldn't directly solve this case anyway. >>>> >>>> -- Kevin >>>> >>>> >>>> On 12/1/2022 9:42 AM, Michael Strauß wrote: >>>> > There's a larger picture here: from a user perspective, there's a >>>> > difference between the scene graph and the "document graph". >>>> > The document graph is what users actually work with, for example by >>>> > setting the `Labeled.graphic` property. In some cases, document nodes >>>> > don't correspond to scene nodes at all (`MenuItem` or `Tab` come to >>>> > mind). >>>> > The document graph is later inflated into a scene graph of unknown >>>> > structure (because skins are mostly black boxes with regards to their >>>> > internal structure). >>>> > >>>> > I've proposed an enhancement that would make the document graph a >>>> > first-class citizen: >>>> > https://mail.openjdk.org/pipermail/openjfx-dev/2022-June/034417.html >>>> > >>>> > With this in place, we could simply disallow the same node appearing >>>> > multiple times in the document graph, which would not only solve the >>>> > problem for `Labeled`, but for all controls with a similar problem. >>>> > >>>> > >>>> > On Thu, Dec 1, 2022 at 6:17 PM John Hendrikx >>>> wrote: >>>> >> The mechanism does seem like it is a bit poorly designed, as it is >>>> easy to create inconsistencies. >>>> >> >>>> >> Luckily it seems that you can't remove a graphic yourself from a >>>> Control (getChildren is protected). >>>> >> >>>> >> I don't think there is an easy solution though... >>>> >> >>>> >> I think that once you set a graphic on a Labeled, you need to >>>> somehow mark it as "in use". Normally you could just check parent != null >>>> for this, but it is trickier than that. The Skin ultimately determines if >>>> it adds the graphic as child, which may be delayed or may even be disabled >>>> (content display property is set to showing TEXT only). >>>> >> >>>> >> Perhaps Skins should always add the graphic and just hide it >>>> (visible false, managed false), but that's going to be hard to enforce. >>>> >> >>>> >> Marking the graphic as "in use" could be done with: >>>> >> >>>> >> - a property in `getProperties` >>>> >> - a new "owner" or "ownedBy" property >>>> >> - allowing "parent" to be set without adding it to children >>>> (probably going to mess up stuff) >>>> >> >>>> >> --John >>>> >>>>
Re: RFR: 8290040: Provide simplified deterministic way to manage listeners [v12]
On Sun, 11 Dec 2022 20:31:24 GMT, John Hendrikx wrote: >> This PR adds a new (lazy*) property on `Node` which provides a boolean which >> indicates whether or not the `Node` is currently part of a `Scene`, which in >> turn is part of a currently showing `Window`. >> >> It also adds a new fluent binding method on `ObservableValue` dubbed `when` >> (open for discussion, originally I had `conditionOn` here). >> >> Both of these together means it becomes much easier to break strong >> references that prevent garbage collection between a long lived property and >> one that should be shorter lived. A good example is when a `Label` is bound >> to a long lived property: >> >> >> label.textProperty().bind(longLivedProperty.when(label::isShowingProperty)); >> >> The above basically ties the life cycle of the label to the long lived >> property **only** when the label is currently showing. When it is not >> showing, the label can be eligible for GC as the listener on >> `longLivedProperty` is removed when the condition provided by >> `label::isShowingProperty` is `false`. A big advantage is that these >> listeners stop observing the long lived property **immediately** when the >> label is no longer showing, in contrast to weak bindings which may keep >> observing the long lived property (and updating the label, and triggering >> its listeners in turn) until the next GC comes along. >> >> The issue in JBS also describes making the `Subscription` API public, but I >> think that might best be a separate PR. >> >> Note that this PR contains a bugfix in `ObjectBinding` for which there is >> another open PR: https://github.com/openjdk/jfx/pull/829 -- this is because >> the tests for the newly added method would fail otherwise; once it has been >> integrated to jfx19 and then to master, I can take the fix out. >> >> (*) Lazy means here that the property won't be creating any listeners unless >> observed itself, to avoid problems creating too many listeners on >> Scene/Window. > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Remove example referencing Node#shownProperty Marked as reviewed by nlisker (Reviewer). - PR: https://git.openjdk.org/jfx/pull/830
