[lworld] RFR: 8368333: [lworld] Add preview mode to ImageReader and JRT file-system [v6]

David Beaumont duke at openjdk.org
Fri Oct 24 14:25:00 UTC 2025


On Thu, 23 Oct 2025 21:03:20 GMT, Roger Riggs <rriggs at openjdk.org> wrote:

>> Oh I see what you mean. The outer API is `isPreviewOnly()` because that matches the semantics of the flags it is used to create in the parent directory (ImageLocation flags). However, as stated in the comments, the private (completely encapsulated) flag values inside the class are best defined in terms of additive flags. This is so you can merge entries easily.
>> 
>> If the inner flag was an "IS_PREVIEW_ONLY" flag, you'd have it *set* and later *cleared* when a new resource entry that *wasn't* a preview entry was processed. By making the flags "IS_XXX_VERSION", you just set them for each resource, completely independently, and then, when the references are merged, you just OR the flags together.
>> 
>> I mean I could change the public API to be talking about "has normal version", but then the code that calculates the parent directory's flags will look less clear than it does now. Instead of "parent dir is preview only if all entries are preview only" it would be "parent directory is preview only if no child entries have normal versions".
>> 
>> This is just an example of the internal working of an encapsulated class not matching its public API for technical reasons. But that's why you encapsulate things, so the inner working can do what they do best, and the outer API can be friendly to the callers.
>
> Leave it alone, maybe it will ultimately show its value.

I did remove the (single use) `hasNormalVersion()` method. It was previously also use in the sorting of entries, but that changed and left it dangling a bit. Thanks for making me look at the code again.

-------------

PR Review Comment: https://git.openjdk.org/valhalla/pull/1619#discussion_r2460344263


More information about the valhalla-dev mailing list