On Mon, 8 Jun 2026 14:57:53 GMT, Andy Goryachev <[email protected]> wrote:
>> ### Summary
>>
>> This PR makes deprecates the `DataFormat` constructor (for removal):
>>
>> public DataFormat(@NamedArg("ids") String... ids)
>>
>>
>> and replaces it with
>>
>> public static DataFormat of(String ... ids)
>>
>>
>> ### Problem
>>
>> There seems to be several issues with DataFormat API and implementation
>> discovered during a Clipboard-related code review:
>>
>> 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 is
>> typically call `lookupMimeType()` to obtain an 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!
>>
>> 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.
>>
>> What should be done?
>> - find out why we need this registry in the first place i.e. what could
>> happen if we have multiple DataFormat instances with overlapping ids.
>> - if the registry is needed add a new factory method, something like
>> `DataFormat::of(String ...)` which is properly synchronized. This method
>> will be called by the constructor to retain the backward compatibility.
>> - deprecate (possibly for removal) `DataFormat::l...
>
> Andy Goryachev 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 nine additional
> commits since the last revision:
>
> - Merge branch 'master' into 8373452.data.format
> - Merge branch 'master' into 8373452.data.format
> - 2026
> - Merge branch 'master' into 8373452.data.format
> - Merge branch 'master' into 8373452.data.format
> - junit
> - whitespace
> - javadoc
> - data format
I've been staring at this code, scratching my head and wondering what is going
on.
First of all, the global `DataFormat` registry does indeed expose some real
problems. However, replacing the constructor with an interned
`DataFormat.of(String...)` does not address the underlying design problem. It
seems to me that the main question here is not whether `DataFormat` should be
created with a constructor or a factory method, the main question is what
`DataFormat` is supposed to mean.
The proposed `DataFormat.of(...)` still requires a global ownership model for
identifiers. That model is ambiguous for overlapping formats:
DataFormat a = DataFormat.of("text/foo");
DataFormat b = DataFormat.of("text/foo", "text/bar");
There is no universally correct interned result here:
* returning `a` loses the "text/bar" information
* returning `b` changes what earlier callers meant by "text/foo"
* throwing keeps the current failure mode
* mutating `a` to include "text/bar" would break immutability and hash-based
collections
So the factory does not actually solve the semantic problem. It also does not
remove the need to support existing public API, as both `DataFormat(String...)`
and `DataFormat.lookupMimeType(String)` are already public.
The current implementation relies too much on identity and registry lookup. For
example, `QuantumClipboard` maps native MIME strings back through
`DataFormat.lookupMimeType(...)`, which makes clipboard behavior depend on
unrelated application initialization order.
I think we should treat `DataFormat` primarily as an immutable value
descriptor: `new DataFormat("text/foo", "text/bar")` means that this format can
be represented by any of the specified identifiers.
Then we define another operation, let's call it `boolean
DataFormat.matches(String)`. This method returns `true` if any of the
identifiers is equal to the given string. This is the question that clipboard
code _actually_ needs to answer. (Note that this operation is not strictly
required, as the identifiers are already exposed with
`DataFormat.getIdentifiers()`, so anyone can answer the question that a
`matches` method would answer.)
`Clipboard.getContentTypes()` should report what the clipboard _actually_
advertises, not an arbitrary alias group that an application may have
registered globally. If the native clipboard reports only `text/foo` then
`getContentTypes()` can reasonably return `Set.of(new DataFormat("text/foo"))`.
But this must still work:
clipboard.hasContent(new DataFormat("text/foo", "text/bar")) // true
clipboard.getContent(new DataFormat("text/foo", "text/bar")) // reads text/foo
These methods must be implemented in terms of `matches`. The clipboard
implementation should be changed as follows:
* `getContentTypes()` discovers the actual identifiers on the clipboard (for
example "text/foo")
* `hasContent(format)` and `getContent(format)` match a user-provided
`DataFormat`, which may be an alias group for one of the identifiers on the
clipboard (for example ["text/foo", "text/bar"])
* `QuantumClipboard` should stop using `lookupMimeType()` to decide whether
content exists
* `QuantumClipboard` should stop depending on identity comparisons for built-in
formats like image, URL, files, RTF, etc.
This would fix the synchronization issue without relying on global interning.
-------------
PR Comment: https://git.openjdk.org/jfx/pull/2006#issuecomment-4759398192
