On Thu, 25 Jun 2026 14:43:31 GMT, Andy Goryachev <[email protected]> wrote:
> ## Summary
>
> This PR changes the behavior of `DataFormat` by allowing multiple instances
> that contain the same set of mime types (ids).
>
>
>
> ## Problem
>
> There seems to be several issues with DataFormat API and implementation
> discovered during review of the `Clipboard`-related code:
>
> 1. `static DataFormat::lookupMimeType(String)` is not thread safe: while
> iterating over previously registered entries in the `DATA_FORMAT_LIST`
> another thread might create a new instance (DataFormat L227)
>
> 2. `public DataFormat(String...)` constructor might throw an
> `IllegalArgumentException` if one of the given mime types is already assigned
> to another `DataFormat`. The origin of this requirement is unclear, but one
> possible issue I can see is if the application has two libraries that both
> attempt to create a `DataFormat` for let's say `"text/css"`. Then, depending
> on the timing or the exact code path, an exception will be thrown for which
> the library(-ies) might not be prepared. The constructor is also not thread
> safe.
>
> 3. To avoid a situation mentioned in bullet 2, a developer would typically
> call `lookupMimeType()` to obtain the already registered instance, followed
> by a constructor call if such an instance has not been found. An example of
> such code can be seen in webkit/UIClientImpl:299 - but even then, despite
> that two-step process being synchronized, the code might still fail if *some
> other* library or the application attempts to create a new instance of
> DataFormat, since the constructor itself is not synchronized.
>
> 4. `DataFormat(new String[] { null })` is allowed but makes no sense!
>
> 5. The current implementation uses the `WeakReferenceQueue` which
> theoretically might, under certain conditions, allow the application to
> create mismatched `DataFormat`s.
>
> Why do we need to have the registry of previously created instances? Unclear.
> My theory is that the DataFormat allows to have multiple mime-types (ids) -
> example being `DataFormat.FILES = new
> DataFormat("application/x-java-file-list", "java.file-list");` - and the
> registry was added to prevent creation of a `DataFormat` with just one id for
> some reason.
>
> Also, I could not find the origin of the multi-id requirement, or the origin
> of the `java.file-list` id itself (it is not a valid mime type).
>
>
>
> ## Solution
>
> The proposed solution is to relax the constraint on the constructor to allow
> multiple instances with the same set of mime types (ids). This might require
> changing the application code from identity compare (`==`) to ...
Added some suggestions, but haven't done a serious review.
modules/javafx.graphics/src/main/java/javafx/scene/input/DataFormat.java line
128:
> 126: public DataFormat(@NamedArg("ids") String... ids) {
> 127: if (ids == null) {
> 128: this.identifier = Collections.<String>emptySet();
Suggestion:
this.identifier = Set.of()
modules/javafx.graphics/src/main/java/javafx/scene/input/DataFormat.java line
131:
> 129: // don't care about registry
> 130: } else {
> 131: this.identifier = Collections.unmodifiableSet(new
> HashSet<>(Arrays.asList(ids)));
Suggestion:
this.identifier = Set.of(ids);
This also throws on `null`s (but NPE, not IAE; you can rethrow). It also throws
on duplicates which we need to do anyway.
modules/javafx.graphics/src/main/java/javafx/scene/input/DataFormat.java line
138:
> 136: if (id == null) {
> 137: throw new IllegalArgumentException("DataFormat
> id must not be null.");
> 138: }
Suggestion:
modules/javafx.graphics/src/main/java/javafx/scene/input/DataFormat.java line
145:
> 143: }
> 144: isNew = false;
> 145: }
Suggestion:
boolean noneExist = Collections.disjoint(this.identifie,
registry.keySet());
modules/javafx.graphics/src/main/java/javafx/scene/input/DataFormat.java line
217:
> 215: return identifier.equals(f.identifier);
> 216: }
> 217: return false;
Suggestion:
}
return (obj instanceof DataFormat df &&
identifier.equals(df.identifier);
-------------
PR Review: https://git.openjdk.org/jfx/pull/2197#pullrequestreview-4574767760
PR Review Comment: https://git.openjdk.org/jfx/pull/2197#discussion_r3477488350
PR Review Comment: https://git.openjdk.org/jfx/pull/2197#discussion_r3477491584
PR Review Comment: https://git.openjdk.org/jfx/pull/2197#discussion_r3477653397
PR Review Comment: https://git.openjdk.org/jfx/pull/2197#discussion_r3477834091
PR Review Comment: https://git.openjdk.org/jfx/pull/2197#discussion_r3477709480
