GNOME Bugzilla – Bug 649031
issues with atkstateset
Last modified: 2021-06-10 11:26:56 UTC
currently the atk representation of a state set is a 64 bit field. states are added with some thing morally equivelent to field |=1 << stateEnum Which works fine so long as the compiler cooperates when assigning integers to enum values, but iirc how the compiler chooses to do this is undefined, so I believe this code is incorrect. I think my prefered solution here is to say that a state set is simply a public 64 bit field making each type a constant with value 0x1 0x2 0x4 etc/ then instead of calling atk_state_set_add_state(set, state) an implementor could just do set |= ATK_STATE_ACTIVe; etc. Some other styleish things that people are free to ignore if they like: having AtkStateSt and RealStateSet seems odd to me, can we just have one and stop the casting? why do we want to support adding states at run time, and how can that possibly do something useful?
(In reply to comment #0) I didn't think too much about the first part of the description of the bug, but I want to add a comment to one sentence: > why do we want to support adding states at run time, and how can that possibly > do something useful? Could you elaborate that question? Because as far as I understand it, you are asking why we want to add a state on runtime to a stateset. And the answer seems obvious. At this moment you ask the state set via atk_object->ref_state_set. And at this moment, a state_set is created and filled, adding the proper states (ie: if the object is visible, it adds ATK_STATE_VISIBLE). And it is required to add the state on runtime, when the state is asked. But I have the feeling that I'm not properly understanding that question.
(In reply to comment #1) > > why do we want to support adding states at run time, and how can that possibly > > do something useful? > > Could you elaborate that question? sure, I was thinking about atk_state_type_register() and other functions inn atkstate.c > Because as far as I understand it, you are asking why we want to add a state on > runtime to a stateset. And the answer seems obvious. At this moment you ask the > state set via atk_object->ref_state_set. And at this moment, a state_set is > created and filled, adding the proper states (ie: if the object is visible, it > adds ATK_STATE_VISIBLE). And it is required to add the state on runtime, when > the state is asked. yeah, no, that all makes sense but if you look at atkstate.c you see atk_state_type_register() which seems to allow you to create a ew state like atk_state_special_visible so it would seem that you know have a special state which no at is aware of so no at can use this new state you created, but you can add this state to a state set you create. > But I have the feeling that I'm not properly understanding that question. make more sense now?
(In reply to comment #2) > > But I have the feeling that I'm not properly understanding that question. > > make more sense now? Yes, thank you. You were talking about adding *new* states. Well, the first answer that came to my mind is "why not?" I guess that if they added this method, is because original atk developers needed that. For example, that would allow to a app to add a non-defined state. But it is true that it would be good to debate if it is worth add this extra complexity.
> Yes, thank you. You were talking about adding *new* states. YES, SORRY, i THOUGHT WHAT i SAID MADE THAT CLEAR. > Well, the first answer that came to my mind is "why not?" well, its not the best answer, but imho getting rid of unusable code is a good idea, if only to prevent having to read it in from disk each time the library gets loaded. > I guess that if they added this method, is because original atk developers > needed that. I'd like to believe that, but I'm having trouble seeing how you could do anything useful with it. I mean ok, you've added atk_state_my_new_special_state to your state set after creating the state at run time now you hand this stateset to orca or some other at, but it has no way to know anything about atk_state_my_new_special_state so what have you achieved by creating your new state? You'll notice that atkrelationtype has a similar method that in a different bug I mentioned is equally useless. So my thought is that there's some sort of boiler plate for enums in the gobject world where you add methods to add values to the enum and convert values to and from strings. It seems a bit silly to me, but then I'd say the same thing about a lot of gobject so whatever. > For example, that would allow to a app to add a non-defined state. yes, but as I said how is that useful?
[Mass-reassigning open atk bug reports for better trackability as requested in https://bugzilla.gnome.org/show_bug.cgi?id=653179 . PLEASE NOTE: If you have watched the previous assignee of this bug report as a workaround for actually getting notified of changes in atk bugs, you yourself will now have to add atk-maint@gnome.bugs to your watchlist at the bottom of https://bugzilla.gnome.org/userprefs.cgi?tab=email to keep watching atk bug reports in GNOME Bugzilla. Sorry for the noise: Feel free to filter for this comment in order to mass-delete the triggered bugmail.]
FWIW, I have found and described a new AtkStateSet issue on bug 667920 (In reply to comment #0) > I think my prefered solution here is to say that a state set is simply a public > 64 bit field making each type a constant with value 0x1 0x2 0x4 etc/ then > instead of calling atk_state_set_add_state(set, state) an implementor could > just do set |= ATK_STATE_ACTIVe; etc. As I said on that bug I prefer the option of maintain AtkStateSet. Call atk_state_set_add_state is, IMHO, more legible than set != ATK_STATE_ACTIVE. In the same way, we already have some methods like atk_state_set_contains_states or atk_state_set_add_states, that it is more logical to implement having the set type.
(In reply to comment #6) > FWIW, I have found and described a new AtkStateSet issue on bug 667920 yeah, I've seen that before and wondered about it, but didn't spend much time puzzling it out. > (In reply to comment #0) > > > I think my prefered solution here is to say that a state set is simply a public > > 64 bit field making each type a constant with value 0x1 0x2 0x4 etc/ then > > instead of calling atk_state_set_add_state(set, state) an implementor could > > just do set |= ATK_STATE_ACTIVe; etc. > > As I said on that bug I prefer the option of maintain AtkStateSet. Call > atk_state_set_add_state is, IMHO, more legible than set != ATK_STATE_ACTIVE. In honestly I think operations on a bit field are perfectly readable. So I'm not really sure what to say here. btw I assume you meant |= not != > the same way, we already have some methods like atk_state_set_contains_states > or atk_state_set_add_states, that it is more logical to implement having the > set type. why? adding a set is just cur |= state1 | state2 and contains is cur & (state1 | state2) While I'd prefer that implementors could access the bit field directly so gecko can just munge its own bit field into the atk one without calling functions I see no reason to oppose adding inline convenience methods to atk object to add / remove / check present states.
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version of atk, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a ticket at https://gitlab.gnome.org/GNOME/atk/-/issues/ Thank you for your understanding and your help.