iced icon indicating copy to clipboard operation
iced copied to clipboard

Superfluous alignment functions

Open Plecra opened this issue 4 years ago • 6 comments

The current api exposes multiple ways for alignment to be expressed:

  • Container::center_x is equivalent to Container::align_x(Center)
  • HorizonalAlignment, VerticalAlignement, and Align all express the same concept.

These introduce more names and relationships for users to learn about in the api, and I think reducing them would be positive. could we remove them?

Plecra avatar Apr 11 '20 14:04 Plecra

Yes, I believe these methods need to be redesigned and unified somehow. Some thoughts:

  • Container is a very vague name for a widget.
  • Container::center_x doesn't really express the intent that it will align the contents instead of the Container itself.
  • HorizontalAlignment and VerticalAlignment are very long names that are hard to type and read. We could abbreviate them, but that would be working around the issue. I'd prefer to find better, more intuitive names. Should we simply use Align instead?

Overall, I think layouting needs some rethinking in general.

hecrj avatar Apr 11 '20 22:04 hecrj

HorizonalAlignment and VerticalAlignment are only used by the Text::*__alignment functions, which don't seem to have any point of comparison. None of the other "end-nodes" have an idea of internal alignment (you'd need a Row, Column, or Container).

I'd like to consider separating alignment from any of the visual widgets, and relegating it to the layout widgets only. This would mean align_ functions could always refer to the contents of the widget, and hopefully make things more predictable.

I think this shouldn't be too hard with Text - the rendered string will have a fixed size that can be handled by layout.

On the other hand, I'm not sure how Image should be handled in this scenario. It's not even clear how it should behave with different sizes today (I tried Length::Fill in the pokedex example, and the image disappeared?), and we'll want to support things like Scaling::{Fill, Fit} down the line

Plecra avatar Apr 12 '20 12:04 Plecra

You can look at the flutter. How about this type:

pub struct Alignment(pub f32, pub f32); // (x, y)

impl Alignment {
    const TOP_LEFT: Self   = Self(-1.0, -1.0);
    const TOP_CENTER: Self = Self(-1.0, 0.0);
    const TOP_RIGHT: Self  = Self(-1.0, 1.0);

    const CENTER_LEFT: Self  = Self(0.0, -1.0);
    const CENTER: Self       = Self(0.0, 0.0);
    const CENTER_RIGHT: Self = Self(0.0, 1.0);

    const BOTTOM_LEFT: Self   = Self(1.0, -1.0);
    const BOTTOM_CENTER: Self = Self(1.0, 0.0);
    const BOTTOM_RIGHT: Self  = Self(1.0, 1.0);

    // ....
}

// Possible usage:

let element = Container::new(inner).align(Alignment::CENTER_LEFT);
let element = Container::align(Alignment::CENTER_LEFT, inner);

// You can add a special widget for alignment only:
let element = Align::new(Alignment::CENTER_LEFT, inner);

lain-dono avatar Apr 13 '20 10:04 lain-dono

I like the idea of just using Align for everything. For things that have a natural axis Align works fine and you should always have the extra information about which axis the alignment is relative to.

Flutter has some good insights, some bloody confusing stuff as well though. It has things like CrossAxisAlignment which just seem unnecessary.

krooq avatar May 12 '20 10:05 krooq

Here are some notes I have gathered about alignment from daily use:

I understand the convenience of center_x, in fact I currently always utilize it over the former whenever centering something.

However, this means for me, that the align_x method is only used for 2/3 of it's available functionality. Mixing both of these methods makes alignment harder to read in a code-base and I am not a fan of the duplication of partial functionality.

I am also looking for cleanest way to approach alignment, so here are some (expected) examples to look at:


Using align with Start, End, ...

This is hard to understand.

container(column()
  .align_x(Align::Start)
  .align_y(Align::End)
  .push(text("Making the example a bit busier..."))
  .push(horizontal_rule(1))
)

Using align with Left, Top, ...

This is much more readable than the first example.

container(column()
  .align_x(Align::Left)
  .align_y(Align::Bottom)
  .push(text("Making the example a bit busier..."))
  .push(horizontal_rule(1))
)

Using custom methods for each alignment

container(column()
  .left()
  .middle()
  .push(text("Making the example a bit busier..."))
  .push(horizontal_rule(1))
)
left()
center()
right()

top()
middle() // the best I can think of
bottom()

Some other potential APIs:

left_x(), center_x(), right_x()
top_y(), center_y(), bottom_y()

// I actually prefer this to using `left` and `right`
start_x(), center_x(), end_x()
start_y(), center_y(), end_y()

If I had a preference, I would actually stop using center_x and would migrate to using align_x(Align::Center) assuming that we could use Align::Left, Align::Top, etc. and deprecate alignment::Horizontal and alignment::Vertical.

I am sharing this because I originally thought having distinct methods for left and right was the best approach, but I believe having the word align in the call is the most readable.

So I wound up agreeing with @krooq. I do think aligning horizontal and vertical in the same method as described by @lain-dono is unnecessary as I find most things don't require a specified alignment at all.

asvln avatar Aug 15 '22 19:08 asvln

@asvln What I really meant was using f32 instead of enum. Among other things, it allows you to animate the alignment.

align_size_within_range(size, range, -1.0) // align start (left or top)
align_size_within_range(size, range,  0.0) // align center
align_size_within_range(size, range, -1.0) // align end (right or bottom)

fn align_size_within_range(size: f32, range: Range<f32>, align: f32) -> Range<f32> {
    let half_delta = (range.end - range.start - size) * 0.5;
    let start = range.start + align.mul_add(half_delta, half_delta);
    start..start + size
}

/// Find the position that corresponds to the alignment
fn align(range: Range<f32>, align: f32) -> f32 {
    let half = (range.end - range.start) * 0.5;
    range.start + align.mul_add(half, half)
}

Note: f32::mul_add is just self * a + b, but faster

With this math you can get an api like this:

struct Align(f32);


impl From<f32> for Align {
    fn from(align: f32) -> Self {
        Self(align)
    }
}

impl Align {
    const START: Self = Self(-1.0);
    const END: Self = Self(1.0);
    // ...
}

type HorizontalAlign = Align;
impl HorizontalAlign { // Yes, it's legal. And it also gives you convenient documentation.
    const LEFT: Self = Self(-1.0);
    const RIGHT: Self = Self(1.0);
    // ...
}

type VerticalAlign = Align; // Personally, I think AlignY is the best name. But who cares.
impl VerticalAlign {
    const TOP: Self = Self(-1.0);
    const BOTTOM: Self = Self(1.0);
    // ...
}

fn align(self, x: impl Into<Align>, y: impl Into<Align>);
fn align_x(self, x: impl Into<Align>);
fn align_y(self, y: impl Into<Align>);

// All options below work simultaneously

container(column()
  .align(0.0, 0.0)
  .push(text("Making the example a bit busier..."))
  .push(horizontal_rule(1))
)

container(column()
  .align_x(Align::START)
  .align_y(Align::END)
  .push(text("Making the example a bit busier..."))
  .push(horizontal_rule(1))
)

container(column()
  .align_x(HorizontalAlign::LEFT)
  .align_y(VerticalAlign::BOTTOM)
  .push(text("Making the example a bit busier..."))
  .push(horizontal_rule(1))
)

container(column()
  .align(Align::LEFT, Align::BOTTOM)
  .push(text("Making the example a bit busier..."))
  .push(horizontal_rule(1))
)

Keep in mind, if you want to support bi-direction, you should use start/end instead of left/right. Also, for text or flex you may still need separate specialized options. Something like SpaceBetween or Justify does not fit the concept.

lain-dono avatar Aug 16 '22 01:08 lain-dono