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 agree with your sentiment - the current design is the issue. This PR does
not address the design issue but resolves an exception, while keeping things
backward-compatible mostly.
If we set to fix the design issue, I agree with @mstr2 that the semantics
should be around `matches`, possibly choosing a better match.
For example, if the application defines `DataFormat APP_FILES = new
DataFormat("application/x-java-file-list")` it should prefer that over
`DataFormat.FILES` in the clipboard operations when a file list is present in
the clipboard. At the same time, asking for `DataFormat.FILES` should also
work since it "matches" the content.
This way, instead of new API we'll have a new behavior which makes more sense.
I am going to take it back to draft. Thank you @mstr2 @nlisker for a good
discussion!
-------------
PR Comment: https://git.openjdk.org/jfx/pull/2006#issuecomment-4770635959
