*LayoutBuilder::DefaultInsets()

[jonas.kirilla]

How about a DefaultInsets() or SetDefaultInsets() method for the layout builders?

[bonefish]

Replying to jonas.kirilla:

How about a DefaultInsets() or SetDefaultInsets() method for the layout builders?

The defaults are the defaults. Why would we need a method to explicitly set them?

BTW, the *LayoutBuilder classes are obsolete. I would phase them out and only improve the templatized builders.

[jonas.kirilla]

A misunderstanding on my part. I thought the default behaviour was without insets.

[bonefish]

Replying to jonas.kirilla:

A misunderstanding on my part. I thought the default behaviour was without insets.

The default is 0 insets. I assume you want a method to set the insets to something other than the default. :-) In the source there seems to be explicit support for setting the insets to the default spacing (a bit weird IMO). I would rather name the method according to the context the spacing is normally used in. E.g. {Set,Apply}WindowContentSpacing() or something like that.

[jonas.kirilla]

Yes, some shorthand for BControlLook-based insets. (But I'm not sure if the item spacing BControlLook provides applies to insets.)

There are some uses of (BTwoDimensionalLayout::)SetInsets(B_USE_DEFAULT_SPACING, B_USE_DEFAULT_SPACING, B_USE_DEFAULT_SPACING, B_USE_DEFAULT_SPACING) which could perhaps be helped by a single-argument version - SetInsets(float side) - or a no-argument version.

Perhaps a BInsets class, grouping a quad of floats, sort of BRect-like, defaulting to non-zero values?

(Or one could make the layout classes default to non-zero insets. I don't know what makes the most sense.)

[bonefish]

Replying to jonas.kirilla:

Yes, some shorthand for BControlLook-based insets. (But I'm not sure if the item spacing BControlLook provides applies to insets.)

Maybe a set of constants for different inset situations could be defined and BControlLook could provide the respective inset values. Maybe that even depends on the kind of frame around the inset area.

There are some uses of (BTwoDimensionalLayout::)SetInsets(B_USE_DEFAULT_SPACING, B_USE_DEFAULT_SPACING, B_USE_DEFAULT_SPACING, B_USE_DEFAULT_SPACING) which could perhaps be helped by a single-argument version - SetInsets(float side) - or a no-argument version.

Yep, a single argument version of SetInsets() can certainly be added for convenience to both BTwoDimensionalLayout and the builders and also a version with two arguments (horizontal/vertical). I'm not in favor of a no-argument version, as it is not clear what inset values it would set.

Perhaps a BInsets class, grouping a quad of floats, sort of BRect-like, defaulting to non-zero values?

Introducing a BInsets class is possible (since the layout management API was inspired by Java's I considered it back then), but not for this purpose. We should introduce such class only, if we decide that it is passed around and used enough to improve convenience. I didn't add it, because it turned out that there were really only the {Set,Get}Insets() methods where it would have been used.

(Or one could make the layout classes default to non-zero insets. I don't know what makes the most sense.)

Indeed. :-) The default constructor should initialize the object to 0 insets. Global constants or getters (e.g. in BControlLook) could be added to provide insets for different situations. That's also what should be analyzed first: What are the situations and how do we want to provide the insets? Whether a BInsets class is introduced can be decided when that is clearer.

[yourpalal]

Firstly, sorry to perform ticket-necromancy, but I'd like to tackle this now :)

Replying to bonefish:

Replying to jonas.kirilla:

Yes, some shorthand for BControlLook-based insets. (But I'm not sure if the item spacing BControlLook provides applies to insets.)

Maybe a set of constants for different inset situations could be defined and BControlLook could provide the respective inset values. Maybe that even depends on the kind of frame around the inset area.

This is the idea I would like to go with, much like the current B_USE_DEFAULT_SPACING. For the record, the reason that constant has such a name, is that it can be passed to a spacing function or an inset function, and is equivalent to using be_control_look->DefaultItemSpacing(). I put the 'USE' in the name to distinguish the fact that it is a symbolic constant (-2 or something), not the actual spacing. The BControlLook::ComposeItemSpacing() method will be extended (and renamed to ComposeSpacing()) to support the new constants.

There are some uses of (BTwoDimensionalLayout::)SetInsets(B_USE_DEFAULT_SPACING, B_USE_DEFAULT_SPACING, B_USE_DEFAULT_SPACING, B_USE_DEFAULT_SPACING) which could perhaps be helped by a single-argument version - SetInsets(float side) - or a no-argument version.

B_USE_DEFAULT_SPACING should probably be renamed, maybe to B_USE_ITEM_SPACING.

Yep, a single argument version of SetInsets() can certainly be added for convenience to both BTwoDimensionalLayout and the builders and also a version with two arguments (horizontal/vertical). I'm not in favor of a no-argument version, as it is not clear what inset values it would set.

This is the other portion of the strategy I would like to use. 3 versions of SetInsets(): 4 arguments, 2 arguments, 1 argument.

Perhaps a BInsets class, grouping a quad of floats, sort of BRect-like, defaulting to non-zero values?

Introducing a BInsets class is possible (since the layout management API was inspired by Java's I considered it back then), but not for this purpose. We should introduce such class only, if we decide that it is passed around and used enough to improve convenience. I didn't add it, because it turned out that there were really only the {Set,Get}Insets() methods where it would have been used.

(Or one could make the layout classes default to non-zero insets. I don't know what makes the most sense.)

Indeed. :-) The default constructor should initialize the object to 0 insets. Global constants or getters (e.g. in BControlLook) could be added to provide insets for different situations. That's also what should be analyzed first: What are the situations and how do we want to provide the insets? Whether a BInsets class is introduced can be decided when that is clearer.

I think that for now, the insets will need to remain defaulted to 0. This is assumed all over the place in our repo, and is actually a pretty sane default. Here's what I'm thinking for different spacing constants:

• B_USE_ITEM_SPACING
• B_USE_HALF_ITEM_SPACING (this is a common use case I have seen in our tree)
• B_USE_WINDOW_INSETS (INSETS because this shouldn't be used from spacing between items)
• B_USE_SMALL_SPACING
• B_USE_BIG_SPACING

I don't know how we would choose values the SMALL/BIG spacing, but it could be nice to offer a bit of consistency to developers. For now, they could be some multiple of be_control_look->DefaultItemSpacing(). The values these constants map to will all be available from BControlLook as well.

As for a BInsets class, I don't think this is required. I don't really see it being very useful, and if some developer really wants to use something like that, it would be trivial to write a template method that called SetInsets() on some object with the values from a BRect.

Anyway, if nobody objects, I'll implement this in a day or two.

[bonefish]

Replying to yourpalal:

Firstly, sorry to perform ticket-necromancy, but I'd like to tackle this now :)

If you're referring to the age of the ticket, compared to a lot of other tickets it is really under age. :-)

Anyway, if nobody objects, I'll implement this in a day or two.

Sounds excellent! Just one remark regarding the naming: For each *_SPACING constant that can be passed to an inset function as well, I'd introduce a similarly named *_INSETS constant with the same value. That makes it possible to map them to the same code internally, if necessary, but still allows the user to use the "correct" name for the respective context.

[yourpalal]

Replying to bonefish:

Sounds excellent! Just one remark regarding the naming: For each *_SPACING constant that can be passed to an inset function as well, I'd introduce a similarly named *_INSETS constant with the same value. That makes it possible to map them to the same code internally, if necessary, but still allows the user to use the "correct" name for the respective context.

That sounds good to me :)

[yourpalal]

I was originally planning on removing the B_USE_DEFAULT_SPACING constant after hrev42077, but I think I will leave it. The reason I would like to leave it is that it is given as the default argument in some constructors, and it makes it very obvious how to return a layout to the default spacing. Currently however, B_USE_DEFAULT_SPACING == B_USE_ITEM_SPACING, which I think should be changed if B_USE_DEFAULT_SPACING is sticking around, to give the option of changing the mapping of these two values independently of one another. Another plus of keeping it around is that B_USE_DEFAULT_SPACING is already used a bit, so this saves me a bit of work ;)

[yourpalal]

Fixed with hrev42025, hrev42077 and hrev42222.