GNOME Bugzilla – Bug 647488
Required to move to a private structure most of the Atk interfaces data
Last modified: 2021-06-10 11:24:56 UTC
On the atkobject documentation you can find this: "struct AtkObject; The AtkObject structure should not be accessed directly." But the reality is that you can, and in fact it is extensively used on both cally and gail. As was made on Gtk, it should be made on ATK This adds an extra flexibility, as allows to add/remove data without worrying if you will create and API/ABI breakage.
(In reply to comment #0) > On the atkobject documentation you can find this: > > "struct AtkObject; > > The AtkObject structure should not be accessed directly." I wonder how much sense this policy makes. half of me thinks it would make sense to me name and description public fields and get rid of the get_name / set_name methods. Ononehand it makes things neater and gets rid of code, on the other hand it means that we have to deal with sumanticsfor who owns the string that name points to. Which I think is a tricky question in a world where at-spi2 access objects through the same methods. However I think everything will work fine if the application owns the strings so maybe making them publicfields is a workable idea. > But the reality is that you can, and in fact it is extensively used on both > cally and gail. I think that means we should think about what in the api causes that to be a reasonable thing to do and if it means the member in question should just be a public field. I think the other part of this is to just write good code that doesn't do evil things :) > As was made on Gtk, it should be made on ATK > > This adds an extra flexibility, as allows to add/remove data without worrying > if you will create and API/ABI breakage. I'm curious how you can implement this in a good way. having struct AtkObject{ struct myPrivateData data; } doesn't work because atk.h needs to know what a struct myPrivateData is so that sizeof(AtkObject) etc works which means that the code could then just change atkObj->name to atkObj->data.name If instead atkObj holds a pointer to the private data you can make things compile, but you increase fragmentation, and a perf hit since you have to do a little more computation to get the pointer to the name string. So escentially if you have a good technical solution I'm interested to here about it otherwise, I'd just make public what makes sense to be public and use social control to make the rest private. also I wouldn't really consider it an abi change to change data that is not documented, and just consider it too bad if code that touched that data anyway explodes when the data is changed.
(In reply to comment #1) > (In reply to comment #0) > > On the atkobject documentation you can find this: > > > > "struct AtkObject; > > > > The AtkObject structure should not be accessed directly." > > I wonder how much sense this policy makes. half of me thinks it would make > sense to me name and description public fields and get rid of the get_name / > set_name methods. Ononehand it makes things neater and gets rid of code, on the > other hand it means that we have to deal with sumanticsfor who owns the string > that name points to. Which I think is a tricky question in a world where > at-spi2 access objects through the same methods. However I think everything > will work fine if the application owns the strings so maybe making them > publicfields is a workable idea. There are several reasons to do this migration. But as you are talking specifically about get_name/set_name. Take into account that get_name/set_name are virtual methods. Ie: on a gtklabel if your object->name is NULL, get_name implmentation returns the label text. Having those attributes public is, IMHO, error-prone. > > But the reality is that you can, and in fact it is extensively used on both > > cally and gail. > > I think that means we should think about what in the api causes that to be a > reasonable thing to do and if it means the member in question should just be a > public field. I think the other part of this is to just write good code that > doesn't do evil things :) Yes, but it is also good if the API guide developers to write good code ;) > > As was made on Gtk, it should be made on ATK > > > > This adds an extra flexibility, as allows to add/remove data without worrying > > if you will create and API/ABI breakage. > > I'm curious how you can implement this in a good way. > > having struct AtkObject{ > struct myPrivateData data; > } > > doesn't work because atk.h needs to know what a struct myPrivateData is so that > sizeof(AtkObject) etc works which means that the code could then just change > atkObj->name to atkObj->data.name If instead atkObj holds a pointer to the > private data you can make things compile, but you increase fragmentation, and a > perf hit since you have to do a little more computation to get the pointer to > the name string. Well, this wouldn't be a problem because it will be implemented as any private structure on glib related libraries, and specifically, as gtk did (the one I used as example). So instead of the code that you used, it would be something like this: typedef struct _AtkObject AtkObject; typedef struct _AtkObjectClass AtkObjectClass; typedef struct _AtkObjectPrivate AtkObjectPrivate; struct _AtkObject { GObject parent; /* <private> */ AtkObjectPrivate *priv; }; It is: using a pointer, that has a fixed size (so your sizeof issue is solved). And then use private-related methods like: G_TYPE_INSTANCE_GET_PRIVATE g_type_class_add_private And probably set the proper value of priv on the instance init. > So escentially if you have a good technical solution I'm interested to here > about it otherwise, I'd just make public what makes sense to be public and use > social control to make the rest private. also I wouldn't really consider it > an abi change to change data that is not documented, and just consider it too > bad if code that touched that data anyway explodes when the data is changed. As I said, this is the technical solution that other glib related libraries use to solve that. And it is working.
(In reply to comment #2) > (In reply to comment #1) > > (In reply to comment #0) > > > On the atkobject documentation you can find this: > > > > > > "struct AtkObject; > > > > > > The AtkObject structure should not be accessed directly." > > > > I wonder how much sense this policy makes. half of me thinks it would make > > sense to me name and description public fields and get rid of the get_name / > > set_name methods. Ononehand it makes things neater and gets rid of code, on the > > other hand it means that we have to deal with sumanticsfor who owns the string > > that name points to. Which I think is a tricky question in a world where > > at-spi2 access objects through the same methods. However I think everything > > will work fine if the application owns the strings so maybe making them > > publicfields is a workable idea. > > There are several reasons to do this migration. But as you are talking > specifically about get_name/set_name. I'm not trying to be specific I was just thinking if any of these fields actually should be public. > Take into account that get_name/set_name are virtual methods. Ie: on a gtklabel > if your object->name is NULL, get_name implmentation returns the label text. yes, I think it makes sense to have a method to get / set the name since you might want to do some computation to get or set the name in the app. So in that case why do we have the field in the first place? As far as I can see atk always calls get_name() so unless that field serves some purpose I think we can just get rid of it. > Having those attributes public is, IMHO, error-prone. well, I'm not sure its error prone, but it causes problems if a toolkit needs to do some computation to get the name for an object or set it. > > > But the reality is that you can, and in fact it is extensively used on both > > > cally and gail. > > > > I think that means we should think about what in the api causes that to be a > > reasonable thing to do and if it means the member in question should just be a > > public field. I think the other part of this is to just write good code that > > doesn't do evil things :) > > Yes, but it is also good if the API guide developers to write good code ;) yes, what I meant was that once we've added accessors that mean that there's no good reason to touch the private fields directly then there should be no code that directly touches the fields of the object. I don't see how documenting don't touch any field of this object directly supports in any way doing just that and touching fields directly. If there is no incentive for toolkits to touch fields directly then I don't see any real reason to go out of our way to keep them from doing things that they've been told they shouldn't, and have no good reason to do. > > > As was made on Gtk, it should be made on ATK > > > > > > This adds an extra flexibility, as allows to add/remove data without worrying > > > if you will create and API/ABI breakage. > > > > I'm curious how you can implement this in a good way. > > > > having struct AtkObject{ > > struct myPrivateData data; > > } > > > > doesn't work because atk.h needs to know what a struct myPrivateData is so that > > sizeof(AtkObject) etc works which means that the code could then just change > > atkObj->name to atkObj->data.name If instead atkObj holds a pointer to the > > private data you can make things compile, but you increase fragmentation, and a > > perf hit since you have to do a little more computation to get the pointer to > > the name string. > > Well, this wouldn't be a problem because it will be implemented as any private > structure on glib related libraries, and specifically, as gtk did (the one I > used as example). So instead of the code that you used, it would be something > like this: > > typedef struct _AtkObject AtkObject; > typedef struct _AtkObjectClass AtkObjectClass; > typedef struct _AtkObjectPrivate AtkObjectPrivate; > > struct _AtkObject > { > GObject parent; > > /* <private> */ > AtkObjectPrivate *priv; > }; > > It is: using a pointer, that has a fixed size (so your sizeof issue is solved). yes, this is the second option I mentioned. > > And then use private-related methods like: > G_TYPE_INSTANCE_GET_PRIVATE > g_type_class_add_private As I've said a million times I'm not a gobject person :) but imho just doing atkObject->priv->field in private code is fine, and much nicer than fuctions or macros doing the same thing. However as I said before discusing this option its slightly slower and puts a second small object on the heap, which imho is suboptimal. > > So escentially if you have a good technical solution I'm interested to here > > about it otherwise, I'd just make public what makes sense to be public and use > > social control to make the rest private. also I wouldn't really consider it > > an abi change to change data that is not documented, and just consider it too > > bad if code that touched that data anyway explodes when the data is changed. > > As I said, this is the technical solution that other glib related libraries use > to solve that. And it is working. sure, it works, but personally I'm not sure I think the perf issues are justified.
(In reply to comment #3) > > Take into account that get_name/set_name are virtual methods. Ie: on a gtklabel > > if your object->name is NULL, get_name implmentation returns the label text. > > yes, I think it makes sense to have a method to get / set the name since you > might want to do some computation to get or set the name in the app. So in > that case why do we have the field in the first place? As far as I can see atk > always calls get_name() so unless that field serves some purpose I think we can > just get rid of it. Default implementation of get_name, at atkobject returns that name. Usually most of get_name reimplementations always ask for the parent implementation, and only returns a different name if this is NULL. This allows the application or toolkit to override the accessible object default name. So that field it is used, but as I said, I think that it should be a private field. > > And then use private-related methods like: > > G_TYPE_INSTANCE_GET_PRIVATE > > g_type_class_add_private > > As I've said a million times I'm not a gobject person :) but imho just doing > atkObject->priv->field in private code is fine, and much nicer than fuctions or > macros doing the same thing. However as I said before discusing this option > its slightly slower and puts a second small object on the heap, which imho is > suboptimal. Yes those macros are mostly used to define the private structure. But in the practice you access that method with atkObject->priv->field. As you said, there is a good performance reason to do that.
(In reply to comment #4) > (In reply to comment #3) > > > > Take into account that get_name/set_name are virtual methods. Ie: on a gtklabel > > > if your object->name is NULL, get_name implmentation returns the label text. > > > > yes, I think it makes sense to have a method to get / set the name since you > > might want to do some computation to get or set the name in the app. So in > > that case why do we have the field in the first place? As far as I can see atk > > always calls get_name() so unless that field serves some purpose I think we can > > just get rid of it. > > Default implementation of get_name, at atkobject returns that name. Usually > most of get_name reimplementations always ask for the parent implementation, > and only returns a different name if this is NULL. This allows the application > or toolkit to override the accessible object default name. Ok, The only implementation I've ever actually looked at iirc is gecko which looks roughly like getName(atkObj) { geckoAcc = GetGeckoAcc(atkObj); name = atkObj->GetName(); if(name != atkobj->name) atk_set_name(atkObj, name); return atkObj->name; } So I thought nobody was doing anything useful with that field. I also thought from that that nobody actually made use of the virtualness much, so reason 1232145 I think it would be much easier to understand atk if it was based on c++. > So that field it is used, but as I said, I think that it should be a private > field. ok, well, if people use it then I'm not opposed to keeping it, I just thought nobody did anything with it. Imho its already private since the layout ofthe struct isn't documented, and there is documentation saying not to touch the members directly. btw it seems like protected might make more sense here, I haven't really thought much about it though, and I'm not sure that's an option in gobject. > > > And then use private-related methods like: > > > G_TYPE_INSTANCE_GET_PRIVATE > > > g_type_class_add_private > > > > As I've said a million times I'm not a gobject person :) but imho just doing > > atkObject->priv->field in private code is fine, and much nicer than fuctions or > > macros doing the same thing. However as I said before discusing this option > > its slightly slower and puts a second small object on the heap, which imho is > > suboptimal. > > Yes those macros are mostly used to define the private structure. But in the well, its not at all clear to me why you would want to bother with macros, but its not my code, so have fun :) > practice you access that method with atkObject->priv->field. As you said, there > is a good performance reason to do that. I'm not sure I understand you, that is refering to what? I would understand that to mean that you believe atkObj->priv->field is better than atkObj->field performance wise which I have trouble believing.
(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > > > > Take into account that get_name/set_name are virtual methods. Ie: on a gtklabel > > > > if your object->name is NULL, get_name implmentation returns the label text. > > > > > > yes, I think it makes sense to have a method to get / set the name since you > > > might want to do some computation to get or set the name in the app. So in > > > that case why do we have the field in the first place? As far as I can see atk > > > always calls get_name() so unless that field serves some purpose I think we can > > > just get rid of it. > > > > Default implementation of get_name, at atkobject returns that name. Usually > > most of get_name reimplementations always ask for the parent implementation, > > and only returns a different name if this is NULL. This allows the application > > or toolkit to override the accessible object default name. > > Ok, The only implementation I've ever actually looked at iirc is gecko which > looks roughly like > > getName(atkObj) > { > geckoAcc = GetGeckoAcc(atkObj); > name = atkObj->GetName(); > if(name != atkobj->name) > atk_set_name(atkObj, name); > > return atkObj->name; > } Probably I'm missing something on this code. Why do you ask for this geckoAcc object and then you don't use it? > So I thought nobody was doing anything useful with that field. I also thought > from that that nobody actually made use of the virtualness much, so reason > 1232145 I think it would be much easier to understand atk if it was based on > c++. On C++ you also can define virtual methods and then don't use it (But please, lets not start a debate about C++ vs gobject). BTW, atk reference implementation is gail. > > Yes those macros are mostly used to define the private structure. But in the > > well, its not at all clear to me why you would want to bother with macros, but > its not my code, so have fun :) > > > practice you access that method with atkObject->priv->field. As you said, there > > is a good performance reason to do that. > > I'm not sure I understand you, that is refering to what? I would understand > that to mean that you believe atkObj->priv->field is better than atkObj->field > performance wise which I have trouble believing. I mean that you set that atkObj->priv structure and access it directly (ie atkObj->priv->field) instead of getting the private structure all the times with G_TYPE_INSTANCE_GET_PRIVATE.
(In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #4) > > > (In reply to comment #3) > > > > > > > > Take into account that get_name/set_name are virtual methods. Ie: on a gtklabel > > > > > if your object->name is NULL, get_name implmentation returns the label text. > > > > > > > > yes, I think it makes sense to have a method to get / set the name since you > > > > might want to do some computation to get or set the name in the app. So in > > > > that case why do we have the field in the first place? As far as I can see atk > > > > always calls get_name() so unless that field serves some purpose I think we can > > > > just get rid of it. > > > > > > Default implementation of get_name, at atkobject returns that name. Usually > > > most of get_name reimplementations always ask for the parent implementation, > > > and only returns a different name if this is NULL. This allows the application > > > or toolkit to override the accessible object default name. > > > > Ok, The only implementation I've ever actually looked at iirc is gecko which > > looks roughly like > > > > getName(atkObj) > > { > > geckoAcc = GetGeckoAcc(atkObj); > > name = atkObj->GetName(); > > if(name != atkobj->name) > > atk_set_name(atkObj, name); > > > > return atkObj->name; > > } > > Probably I'm missing something on this code. Why do you ask for this geckoAcc > object and then you don't use it? oh, oops, it should be name = geckoObj->GetName(); not name = atkObj->GetName(); > > > So I thought nobody was doing anything useful with that field. I also thought > > from that that nobody actually made use of the virtualness much, so reason > > 1232145 I think it would be much easier to understand atk if it was based on > > c++. > > On C++ you also can define virtual methods and then don't use it (But please, > lets not start a debate about C++ vs gobject). sure, and inho that would belong on the interface bug not this one :) > BTW, atk reference implementation is gail. true, I was only bringing this up as the examples I've seen that made me expect people didn't use the name field.
[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.]
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.