Replace boolean parameter to Layout::align with AlignmentOptions struct
#278
Conversation
nicoburns
force-pushed
the
alignment-options-struct
branch
from
2119050 to
e4ecf67
Compare
| /// If set to `true`, "end" and "center" alignment will apply even if the line contents are | ||
| /// wider than the alignment width. If it is set to `false`, all overflowing lines will be | ||
| /// [`Alignment::Start`] aligned. | ||
| pub align_when_overflowing: bool, |
There was a problem hiding this comment.
A nit: it may read more naturally by flipping the meaning of the bool and calling the field start_alignment_on_overflow.
nicoburns
force-pushed
the
alignment-options-struct
branch
from
e4ecf67 to
c5994de
Compare
| container_width: Option<f32>, | ||
| alignment: Alignment, | ||
| align_when_overflowing: bool, | ||
| options: AlignmentOptions, |
There was a problem hiding this comment.
I'm not convinced by this signature. Having a pile of arguments, and then an options struct doesn't quite seem right to me:
fn align(&mut self, alignment: Alignment, options: AlignmentOptions)seems more natural, given that container_width is already optional.
Or even:
fn align(&mut self, options: AlignmentOptions)There was a problem hiding this comment.
One issue with moving container_width into the options is that it cannot account for floated boxes yet, and I don't think we know what that's going to look like yet. (Though I guess floated boxes just subtract from the space provided by container_width.)
I certainly see the logic of moving alignment into AlignmentOptions.
There was a problem hiding this comment.
Hmm... I suppose you could put container_width in the options object. I think alignment ought to stay out. It would also be possible to have separate align and align_with_options functions where the former uses the default options.
There was a problem hiding this comment.
It would also be possible to have separate align and align_with_options functions where the former uses the default options.
That also makes sense.
Just another consideration: perhaps container_width shouldn't be optional. Aligning to the layout's calculated width is not necessarily a sensible default, as that width changes depending on how exactly lines were broken given the container width (max_advance) used for line breaking.
There was a problem hiding this comment.
Just another consideration: perhaps container_width shouldn't be optional. Aligning to the layout's calculated width is not necessarily a sensible default, as that width changes depending on how exactly lines were broken given the container width (max_advance) used for line breaking.
This could also make sense.
| impl Default for AlignmentOptions { | ||
| fn default() -> Self { | ||
| Self { | ||
| align_when_overflowing: false, |
There was a problem hiding this comment.
Is it possible that true is a more sensible default?
There was a problem hiding this comment.
I don't have particularly strong feelings about the default.
There was a problem hiding this comment.
I believe the current default follows what the web does (i.e., when a line overflows, you always see the start of it, in both LTR and RTL text).
nicoburns
force-pushed
the
alignment-options-struct
branch
from
c5994de to
3458d7d
Compare
Signed-off-by: Nico Burns <nico@nicoburns.com>
nicoburns
force-pushed
the
alignment-options-struct
branch
from
3458d7d to
c2b2d56
Compare
|
I think I'm going to merge this as-is, as there seems to be agreement that this is an improvement over what we have. Further API changes and/or default changes can happen in followups. |
Some prior discussion here: #278 (comment). > Aligning to the layout's calculated width is not necessarily a sensible default, as that width changes depending on how exactly lines were broken given the container width (max_advance) used for line breaking.
Some prior discussion here: #278 (comment). > Aligning to the layout's calculated width is not necessarily a sensible default, as that width changes depending on how exactly lines were broken given the container width (max_advance) used for line breaking.
|
I don't know why there was a rush to land this rather than just get it right. I won't include this in my update to masonry since apparently it has to change again (although now that this landed, who will drive actually fixing it?). |
Layout::alignwithAlignmentOptionsStruct #247unjustify, removeLineData::alignment#271 as otherwise these would conflictA small but nice improvement to the public API.