Code review comment for lp://qastaging/~rodrigo-moya/unity/ref-state-set-for-panel-service

Revision history for this message
Alejandro PiƱeiro (apinheiro) wrote :

Well, probably my mistake for not explaining it.

Normally you don't maintain a private state_set. Normally (except in very few cases) ref_state_set is implemented in a incremental way, to avoid each object providing all the states required, and just filled on the ref_state_set. Take into account that atk_object also have a ref_state_set adding some states, and you are not using it.

So, instead of having a private state_set, our custom ref_state_set should start calling the parent ref_state_set, and then just add the particular states for your object. I know that this means that the state_set can't be filled outside of the ref_state_set implementation, but that was made by purpose. Take a look to this bug [1] (that was more a discussion).

Then you use atk_object_notify_state_change to notify a change on any state. But you don't require to modify the state set on the method (normally a callback) doing that.

You can take a look to the implementation of ref_state_set on nux-object-accessible_ref_state_set

So:

40 + AtkStateSet *state_set;

Don't use a custom state_set, as if you do that, you are not getting the states that comes from the parent.

54 + atk_state_set_add_state (piea->priv->state_set, ATK_STATE_ACTIVE);
55 + atk_state_set_add_state (piea->priv->state_set, ATK_STATE_FOCUSED);

As I said, those calls out of ref_state_set are useless. This apply to all this calls, also on the initialize.

140 +panel_indicator_entry_accessible_ref_state_set (AtkObject *accessible)

Update this method, in order to get the state_set from the parent, and then add the proper states.

[1] https://bugzilla.gnome.org/show_bug.cgi?id=635807

review: Needs Fixing

« Back to merge proposal