On Wed, 26 Mar 2025 20:13:25 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> Yeah, the reason I was hesitant to change this is that currently you can set
>> the alignment from CSS in two ways:
>>
>> .titled-pane {
>> -fx-alignment: RIGHT;
>> }
>>
>> Or:
>>
>> .titled-pane > .title {
>> -fx-alignment: RIGHT;
>> }
>>
>> Both will work at the moment. I'm not sure which one is more correct.
>>
>> A titled pane naturally has two major regions; its title area, and its
>> content area. The top level alignment could apply to either or none in
>> theory. The top level alignment setting is "inherited" from being a Labeled
>> control, which only has a single alignment to worry about (this also shows
>> using this kind of inheritance is just a bad idea). It would have been much
>> more clear to have a `titleAlignment` and `contentAlignment` property.
>>
>> At the "top" level there is nothing to align, since a titled pane always
>> fill its width.
>
> the original code had some logic around `(pos == null)`, I wonder if we need
> to do the same here, that is, derive the effective alignment from both values?
That was just for deriving the `hpos` and `vpos` fields, which are no longer
needed, in case `pos` was `null`. The `null` check is now handled in
`LabeledSkinBase`.
Of course we could think of some kind of derive option (how to derive LEFT +
RIGHT?) or a way to remove one of these, but I think either way, it's outside
the scope of this change. I added the comment for future maintainers (I could
have left it out, and we'd probably not be having this discussion).
As it is, I don't think we should fix this as part of this change as it is
really completely unrelated (and might need much more discussion and perhaps
even a CSR...)
Perhaps I should remove the comment and instead we describe this in an issue?
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1742#discussion_r2015031232
