Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#5614 closed enhancement (fixed)

Layouting should have default spacing

Reported by: axeld Owned by: yourpalal
Priority: normal Milestone: R1
Component: Kits/Interface Kit Version: R1/Development
Keywords: Cc:
Blocked By: Blocking: #5524
Has a Patch: no Platform: All

Description

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.

Change History (11)

comment:1 Changed 9 years ago by yourpalal

Blocking: 5524 added
Owner: changed from axeld to yourpalal
Status: newassigned

comment:2 Changed 9 years ago by yourpalal

Status: assignedin-progress

comment:3 Changed 9 years ago by 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

comment:4 Changed 9 years ago by 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.

comment:5 Changed 9 years ago by yourpalal

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

comment:6 Changed 9 years ago by yourpalal

Resolution: fixed
Status: in-progressclosed

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

comment:7 Changed 9 years ago by 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?

comment:8 in reply to:  7 Changed 9 years ago by 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.

comment:9 Changed 9 years ago by 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.

comment:10 in reply to:  9 Changed 9 years ago by 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.

comment:11 Changed 9 years ago by 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.

Note: See TracTickets for help on using tickets.