Opened 5 years ago

Closed 4 years ago

Last modified 6 months ago

#10734 closed enhancement (fixed)

BOptionPopUp::SetDivider() is ignored/overriden by a call in AllAttached().

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

Description

The AllAttached() hook (i.e. the very last hook called in the setup phase, after ctor and AttachedToWindow()) calls SetDivider(), resetting it to fit the label width, overriding whatever was set earlier:

http://cgit.haiku-os.org/haiku/tree/src/kits/interface/OptionPopUp.cpp#n191

One could theoritically override (augment) that hook to counter-counter override the divider again, but it's not always possible. Think e.g. usage of a GUI framework that aligns the dividers of all widgets in a column in an aesthetically pleasing fashion, which wants to use the class "as-is" without subclassing it or without extending its responsability span to go past child being attached to a parent.

The class could be modified, it might be worth it to call this earlier instead... or even get rid of it completely, since SetDivider() is called in SetLabel() itself and does not seem to be impacted by the view not being attached yet, so why call SetDivider() again as late as in AllAttached()... Food for thought.

Change History (7)

comment:1 Changed 5 years ago by ttcoder

Milestone: R1R1/alpha5

comment:2 Changed 5 years ago by pulkomandy

I'm not sure the initial call in SetLabel is going to work properly, as the view isn't attached to a window yet and it may not be able to access the font metrics. Have you checked that it gives the correct results?

comment:3 Changed 5 years ago by ttcoder

Good point; rather than get rid of the second SetDivider() call it'd be better to move it to AttachedToWindow() (from AllAttached()).

That way an hypothetical gui framework that 'finetunes' its children in AllAttached() is sure to manage optionpopups correctly. I've checked and since this ticket's creation my code has changed so that 1) AddChild() is called immediately on the whole chain (BView arborescence) so that everybody works oneline (rather than offline) from the app_server, 2) AttachedToWindow/AllAttached() are called at once, and thus 3) when my framework finally calls its DivideFromMaxDividers() fine-tuning code, the newly set dividers "stick" as there's nobody to pester them any more =) .. So no need regarding this ticket from me, at this point in time; but in case of future change or for somebody else, why not make the move mentionned above yes. Probably trivial enough that no outside testing is needed?

Another fix that'd be interesting to do in this same ticket (though can create a new one if needed), I'd like to remove this override/hack from my code :-)

void MyOptionPopup::AllAttached()
{
	if( NULL == Label() )
		MenuField()->SetDivider( 0. );
}

In other words, BOptionPopUp (in AllAttached() I think) always does the math the same way, always adds 8 pixels or so of spacing between the label and the menubar.. Eve if the label does not exist.. Contrarily to BMenuField which handles the special case and displays only the menubar (no SetDivider(8)) when the label is NULL..

comment:4 Changed 5 years ago by pulkomandy

Milestone: R1/alpha5R1/beta1

comment:5 Changed 4 years ago by pulkomandy

Milestone: R1/beta1R2

Unfortunately this is not possible. The BeOS version of BOptionPopUp doesn't override AttachedToWindow, so if we do it there, the function may not be called at all by BeOS apps.

I fixed the extra offset in case of an empty label, I can't see a way to handle the initial problem without ABI changes so I'll have to move this to R2 (unless someone has a sugestion).

comment:6 Changed 4 years ago by pulkomandy

Resolution: fixed
Status: newclosed

Well, actually there is no strict ABI breakage here, and well-behaving apps should continue to work fine. Pushed the changes moving this to AttachedToWindow in hrev48388. Let's see if some apps get broken because of this...

comment:7 Changed 6 months ago by pulkomandy

Milestone: R2R1
Note: See TracTickets for help on using tickets.