Layouting should have default spacing

[axeld]

When using layouts like BGroupLayout their default item spacing is 0 pixels.

While it certainly makes sense to be able to set such a spacing for specific use cases, the typical way one would use a layout is to have a certain spacing that you currently have to set manually everytime (via BControlLook::DefaultItemSpacing()).

This spacing should be the default instead.

[yourpalal]

*default spacing introduced in hrev38512
*fixed ActivityMonitor regression in hrev38513
*fixed OpenGL preflet regression in hrev38514

Leaving this ticket open for reporting of regressions/grievances

[yourpalal]

*fixed CodyCam in hrev38519
*fixed AboutSystem in hrev38520

I noticed Expander has a regression as well, I'll look into this tomorrow at the same time as #5289.

[yourpalal]

fixed Expander, Icon-O-Matic, Tracker & Screenshot regressions in hrev38538.

[yourpalal]

There doesn't seem to be any more regressions, so I'm closing this now.

[axeld]

Thanks for having implemented this! I don't see where the default spacing is still 0, 0, 0, 0 as you mention in the commit message of hrev38512.

One thing I don't like as much is that the default spacing is "composed" in the constructor, as this means the spacing is not updated if you relayout after a global font size change (we don't yet broadcast those IIRC, though), or if you change the font size afterwards.

Also, I guess we can now go over the sources, and remove all the custom spacing setters?

[yourpalal]

Replying to axeld:

Thanks for having implemented this! I don't see where the default spacing is still 0, 0, 0, 0 as you mention in the commit message of hrev38512.

Welcome :) Although the spacing defaults to B_USE_DEFAULT_SPACING, the insets for layouts that support them (BTwoDimensionalLayout & BSplitLayout) still default to 0, 0, 0, 0. I think this is the most sane behaviour, since otherwise, nesting layouts would lead to very large spaces in the GUI. eg.

------------------------------------------|
| insets |-----------------------| insets |
|        | insets -------- insets|        |
|        |      |usable space|   |        |
           ...    etc   ...    ... 

That example isn't very good, but hopefully you get the idea :P

I did however make it possible to pass in B_USE_DEFAULT_SPACING to SetInsets, so it is still possible to make use of 'default' insets.

One thing I don't like as much is that the default spacing is "composed" in the constructor, as this means the spacing is not updated if you relayout after a global font size change (we don't yet broadcast those IIRC, though), or if you change the font size afterwards.

Yes, this is something I flip-flopped a bit on, but decided to compose it at construction/setting time for now since currently the font size is used and BControlLook::DefaultItemSpacing is (currently) non-virtual, it was nearly equivalent. I also felt it kept the code a bit cleaner.

That being said, it could be nice to have these values update on the fly, especially once DefaultItemSpacing is virtual (there is a TODO for it, but I'm not sure what is stopping it). The nice thing is that moving the composing to 'Get time' should not break anything. (Although it should be finalized before the API goes public, of course)

Also, I guess we can now go over the sources, and remove all the custom spacing setters?

Yes, that would make a good "easy ticket", I think.

[stippi]

In BControlLook, I marked some supposed to be virtual methods that I left non-virtual for the time being, since there were already third-party GCC4 packages that would have been broken by introducing more virtuals. I intended to break those when more changes had piled up. Don't know if that is what you meant.

[yourpalal]

Replying to stippi:

In BControlLook, I marked some supposed to be virtual methods that I left non-virtual for the time being, since there were already third-party GCC4 packages that would have been broken by introducing more virtuals. I intended to break those when more changes had piled up. Don't know if that is what you meant.

Yes, I think that is probably it.

[yourpalal]

I was thinking today and realized a good reason not to have the spacing/insets calculated on the fly: If this information changes, it will likely change the size constraints of the layout, which should trigger an invalidation. Unfortunately, this would be very hard to detect in a reasonable way. Something could be done with BHandlers and StartWatching I suppose, but I think that cure would be worse than the illness. I'm not actually sure if this is part of why I didn't have this calculated on the fly in the first place, but is a good reason not to do so now, I think.