Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Superfluous alignment functions #286

Open
Plecra opened this issue Apr 11, 2020 · 6 comments
Open

Superfluous alignment functions #286

Plecra opened this issue Apr 11, 2020 · 6 comments
Labels
good first issue Good for newcomers help wanted Extra attention is needed layout question Further information is requested
Milestone

Comments

@Plecra
Copy link
Contributor

Plecra commented Apr 11, 2020

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?

@hecrj hecrj added help wanted Extra attention is needed question Further information is requested labels Apr 11, 2020
@hecrj hecrj added this to the 0.2.0 milestone Apr 11, 2020
@hecrj
Copy link
Member

hecrj commented Apr 11, 2020

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.

@Plecra
Copy link
Contributor Author

Plecra commented Apr 12, 2020

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

@lain-dono
Copy link
Contributor

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);

@krooq
Copy link

krooq commented May 12, 2020

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.

@hecrj hecrj modified the milestones: 0.2.0, 0.3.0 Nov 26, 2020
@hecrj hecrj modified the milestones: 0.3.0, 0.4.0 Mar 31, 2021
@hecrj hecrj added layout good first issue Good for newcomers labels Jan 18, 2022
@hecrj hecrj modified the milestones: 0.4.0, 0.5.0 Apr 12, 2022
@ghost
Copy link

ghost commented Aug 15, 2022

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.

@lain-dono
Copy link
Contributor

@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.

@hecrj hecrj modified the milestones: 0.5.0, 0.6.0 Nov 9, 2022
@hecrj hecrj modified the milestones: 0.6.0, 0.7.0 Dec 7, 2022
@hecrj hecrj modified the milestones: 0.7.0, 0.8.0 Jan 14, 2023
@hecrj hecrj modified the milestones: 0.8.0, 0.9.0 Feb 18, 2023
@hecrj hecrj modified the milestones: 0.9.0, 0.10.0 Apr 13, 2023
@hecrj hecrj modified the milestones: 0.10.0, 0.11.0 Jul 28, 2023
@hecrj hecrj removed this from the 0.12 milestone Feb 11, 2024
@hecrj hecrj added this to the 0.13 milestone Feb 11, 2024
@hecrj hecrj modified the milestones: 0.13, 0.14 Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed layout question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants