BOptionPopUp sets value even if not available

[jackburton]

With BOptionPopUp::SetValue() you can set a value which does not exist. As you can see from the implementation, it sets the value of the BControl by calling BControl::SetValue(), but later it could return without setting the correct option, since there might be no option with that value.

Calling Value() will then return the newly set value.

void
BOptionPopUp::SetValue(int32 value)
{
	BControl::SetValue(value);
	BMenu* menu = fMenuField->Menu();
	if (menu == NULL)
		return;

	int32 numItems = menu->CountItems();
	for (int32 i = 0; i < numItems; i++) {
		BMenuItem* item = menu->ItemAt(i);
		if (item && item->Message()) {
			int32 itemValue;
			item->Message()->FindInt32("be:value", &itemValue);
			if (itemValue == value) {
				item->SetMarked(true);
				break;
			}
		}
	}
}

If I remember correctly, this behaviour is inherited from BeOS, but maybe it doesn't make much sense, and could be confusing.

[jackburton]

The proposed change is

void
BOptionPopUp::SetValue(int32 value)
{
-       BControl::SetValue(value);
	BMenu* menu = fMenuField->Menu();
	if (menu == NULL)
		return;

	int32 numItems = menu->CountItems();
	for (int32 i = 0; i < numItems; i++) {
		BMenuItem* item = menu->ItemAt(i);
		if (item && item->Message()) {
			int32 itemValue;
			item->Message()->FindInt32("be:value", &itemValue);
			if (itemValue == value) {
+                        	BControl::SetValue(value);
				item->SetMarked(true);
				break;
			}
		}
	}
}

[waddlesplash]

It looks like you were the one to originally implement this class. I don't see any comments either way or in old commit messages...

[jackburton]

Replying to waddlesplash:

It looks like you were the one to originally implement this class. I don't see any comments either way or in old commit messages...

Yes, I know. Obviously I can't remember for sure if it's the case, but I think that the behaviour is inherited from Beos. Neverthless, it just feels wrong.

[pulkomandy]

I will note that other controls behave similarly, for example you can set arbitrary values for a checkbox or button (besides B_CONTROL_ON and B_CONTROL_OFF).

I imagine it can be useful if subclasses add extra things to the control (for example, making a 3-state checkbox where it can be "partially checked").

In the case of BOptionPopUp this could be for example a subclass adding submenus with more options? Or with custom menu items that do more complex things, for example in ACE I have a menu item with a slider inside it, that can take any value. But I think I build it out of BPopupUpMenu rather than BOptionPopUp. So it's probably fine to restrict BOptionPopUp in this way.

Finally: I think this should be done in BOptionControl and not BOptionPopUp? In case we add other subclasses of BOptionControl, they should have the same protection in place. In fact, BOptionControl already has a SelectOptionFor method which does this validation. It even has a comment wondering why this method exist. It seems we just found why? But now we have to find which one should call the other, and make sure we don't end up with recursion.

[jackburton]

I'm pretty sure I had tested the Beos behaviour at the time, so I am reasonably sure that the way it's implemented (both BOptionControl and BOptionPopUp) mimics the original behaviour.