On Thu, 23 Oct 2025 21:03:20 GMT, Roger Riggs <[email protected]> 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
