After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 649575 - Need to clarify focus management status
Need to clarify focus management status
Status: RESOLVED FIXED
Product: atk
Classification: Platform
Component: atk
unspecified
Other Linux
: Normal normal
: ---
Assigned To: ATK maintainer(s)
ATK maintainer(s)
: 651367 (view as bug list)
Depends on:
Blocks: 638537
 
 
Reported: 2011-05-06 14:56 UTC by Alejandro Piñeiro Iglesias (IRC: infapi00)
Modified: 2015-06-29 08:44 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Alejandro Piñeiro Iglesias (IRC: infapi00) 2011-05-06 14:56:20 UTC
IMHO, current status of the focus management on ATK is not clear (of just messy). There are several ATK API methods that it is not clear if it should be implemented by an implementor to be used by any client library or are utility methods to be implemented insed the ATK implementation.

IMHO, they should be simplified (methods removal) or clarified (documentation).

Specifically I think that it is not clear the meaning/purpose of the atk focus tracker methods:

atk_add_focus_tracker
atk_remove_focus_tracker
atk_focus_tracker_init
atk_focus_tracker_notify

IMHO all this functions are superfluous. They mostly seems as utility methods to help the focus management implementation. They seem that were added during a specific ATK implementation (probably GAIK) as utility methods. In the case of GAIL the definition of those methods are mostly used by GAIL.

As far as I understand ATK, it should be an abstract library and his API should be mostly methods and signals required by his library client (in this case, at-spi2-atk or directly gtk apps). 

The main reason I think that those methods are superfluous is because you can get the same functionality with other things that already exists on ATK:

AtkObject::focus_event
AtkUtil::atk_add_global_event_listener
AtkUtil::atk_remove_global_event_listener

An in fact, right now at-spi2-atk just uses his focus tracker to re-emit the focus event. Using the global event support would allow to manage this in a more homogeneous way. In summary I don't know why the event "AtkObject::focus-event" should be managed in a different way to other ATK event.

The only missing thing would be the required focus initialization, but I think that this could be solved on the initialization of any ATK implementation, and that it wouldn't require to expose any API on the general purpose ATK API.

Additionally, we also have some focus tracking on AtkComponent (not saying anything about atk_grab_focus):

atk_component_add_focus_handler
atk_component_remove_focus_handler

The only advantage of this methods is that you can specify the object you want to track. *But* I don't see the difference between those methods and a g_signal_connect to AtkObject::focus-event, so I also vote to remove them.

IMHO, cleaning this would make things easier to understand and to implement.

Anyway, probably I'm wrong on some of my conclusions, because the focus management is something complex, and I could be missing something from the full picture. But in this case, documentation should be improved.

Anyway, in any case, it would be good to write a document clarifying what it is expected from the ATK focus management implementation, and a "good practice" document.
Comment 1 Joanmarie Diggs (IRC: joanie) 2011-05-10 09:47:53 UTC
Related hackfest discussion/consensus:

1. We have redundant events: object:state-changed:focused and focus:

2. The object:state-changed:focused events are preferred if one of the two events is to be removed. (i.e. we're proposing to remove/deprecate focus:)

3. It would be very beneficial for ATs to have a new method by which the currently-focused object could be requested. This will need to be added to AT-SPI.
Comment 2 Alejandro Piñeiro Iglesias (IRC: infapi00) 2011-05-10 09:51:04 UTC
4. That would mean that as my conclusions on the description were right, and most of that ATK methods are superfluous, so ATK API in relation with the focus can be simplified.

5. It would be good to write a "Focus management guide" (ala "What ATs waits from  ATK/AT-SPI2 in relation to the focus") to have this clear.
Comment 3 Joanmarie Diggs (IRC: joanie) 2011-05-29 02:37:43 UTC
Spawning new bugs for the tasks identified.

(In reply to comment #1)
> 1. We have redundant events: object:state-changed:focused and focus:
> 
> 2. The object:state-changed:focused events are preferred if one of the two
> events is to be removed. (i.e. we're proposing to remove/deprecate focus:)

Bug 651367 opened for the deprecation of the focus: signal.

> 3. It would be very beneficial for ATs to have a new method by which the
> currently-focused object could be requested. This will need to be added to
> AT-SPI.

Bug 651368 opened for this new method.

(In reply to comment #2)
> 4. That would mean that as my conclusions on the description were right, and
> most of that ATK methods are superfluous, so ATK API in relation with the focus
> can be simplified.

Alex: What needs to be done here, specifically?

> 5. It would be good to write a "Focus management guide" (ala "What ATs waits
> from  ATK/AT-SPI2 in relation to the focus") to have this clear.

Bug 651369 opened for the doc writing.
Comment 4 Alejandro Piñeiro Iglesias (IRC: infapi00) 2011-05-29 17:21:21 UTC
> > 4. That would mean that as my conclusions on the description were right, and
> > most of that ATK methods are superfluous, so ATK API in relation with the focus
> > can be simplified.
> 
> Alex: What needs to be done here, specifically?

Means that some methods would be required to be marked as deprecated. Specifically:

From AtkUtil:
 atk_add_focus_tracker
 atk_remove_focus_tracker
 atk_focus_tracker_init
 atk_focus_tracker_notify

From AtkComponent:
 atk_component_add_focus_handler
 atk_component_remove_focus_handler

Anyway, probably we can't just set them as deprecated as some others components are using it (at-spi2, gail, cally) ... well, or we can just deprecate them, and create bugs for each component about using deprecated methods (almost what bug 650559 is). Some further investigation would be required.
Comment 5 Joanmarie Diggs (IRC: joanie) 2011-05-29 18:17:29 UTC
(In reply to comment #4)

[...]

> Anyway, probably we can't just set them as deprecated as some others components
> are using it (at-spi2, gail, cally) ... well, or we can just deprecate them,
> and create bugs for each component about using deprecated methods (almost what
> bug 650559 is). Some further investigation would be required.

I personally like the idea of the latter: If these methods need deprecating, then they need deprecating. Therefore, I opened bug 651407 for that. (And apologies if I seem a bit anal-retentive about splitting issues up into discrete bugs, but I think that will help us in terms of planning and in allocating resources to the non-trivial number of hackfest-identified tasks which now need doing.)

So.... Anything else which remains to be done w.r.t. this original bug?
Comment 6 André Klapper 2011-06-23 22:06:16 UTC
[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.]
Comment 7 Alejandro Piñeiro Iglesias (IRC: infapi00) 2011-07-05 15:05:34 UTC
(In reply to comment #0)

> Specifically I think that it is not clear the meaning/purpose of the atk focus
> tracker methods:
> 
> atk_add_focus_tracker
> atk_remove_focus_tracker
> atk_focus_tracker_init
> atk_focus_tracker_notify
> 
> IMHO all this functions are superfluous. They mostly seems as utility methods
> to help the focus management implementation. They seem that were added during a
> specific ATK implementation (probably GAIK) as utility methods. In the case of
> GAIL the definition of those methods are mostly used by GAIL.

Checking ATK implementation due this deprecation work I realized that some of those methods are used internally by ATK itself. Specifically atk_focus_tracker_notify.

This method not only executes the trackers. It also updates internally the current focused object, the one that you can access using atk_get_focus_object. Probably this is the only AtkUtil method that will survive in the long term. So this is related to bug 651368.

Anyway, IMHO, that is not enough to maintain that public API, as we can get the same behaviour with the current global listeners and connecting to state-change:focus as we already commented.

So in general this is a FYI and a reminder that after deprecate those methods, ATK implementation itself would require to be updated (not only ATK implementors)
Comment 8 Alejandro Piñeiro Iglesias (IRC: infapi00) 2013-08-12 19:17:10 UTC
AtkObject::focus_event
atk_add_focus_tracker
atk_remove_focus_tracker
atk_focus_tracker_init
atk_focus_tracker_notify
atk_component_add_focus_handler
atk_component_remove_focus_handler

Deprecated with this commit:
commit c568794a6f75cc9cc297b24978bc7480ee689bea
Author: Alejandro Piñeiro <apinheiro@igalia.com>
Date:   Mon Aug 12 16:47:06 2013 +0200

    Deprecate AtkObject::focus-event signal and all related methods

Now you only have state-change:focused

Note to myself: open a bug on GTK in order to remove all that stuff there (btw: Clutter has that stuff removed like one year ago)
Note to myself2: remove all the code at ATK that is using atk_focus_tracker_notify to tracker the current focus object

Closing bug
Comment 9 Alejandro Piñeiro Iglesias (IRC: infapi00) 2013-12-11 12:37:33 UTC
*** Bug 651367 has been marked as a duplicate of this bug. ***
Comment 10 Murray Cumming 2015-06-29 07:16:10 UTC
The documentation mentions "state-changed", but the AtkObject signal is called state-change. I've corrected this in git master, but feel free to revert it if I'm wrong:
https://git.gnome.org/browse/atk/commit/?id=30f2cca3ed1c626c0b3656dea4744123f4263511

The wording is still rather vague - it's really talking about checking the parameter for the "focused" string, I think, but that signal is sparsely documented in general. I guess it should mention AtkStateType in its documentation.
Comment 11 Alejandro Piñeiro Iglesias (IRC: infapi00) 2015-06-29 08:44:14 UTC
(In reply to Murray Cumming from comment #10)
> The documentation mentions "state-changed", but the AtkObject signal is
> called state-change. I've corrected this in git master, but feel free to
> revert it if I'm wrong:
> https://git.gnome.org/browse/atk/commit/
> ?id=30f2cca3ed1c626c0b3656dea4744123f4263511

Seems ok, thanks for the patch.

> The wording is still rather vague - it's really talking about checking the
> parameter for the "focused" string, I think, but that signal is sparsely
> documented in general. I guess it should mention AtkStateType in its
> documentation.

Well, it is sparsely documented because most of that documentation is about the deprecation of focus related methods. And it is already mentioned on AtkStateType:

"ATK_STATE_FOCUSED
	
Indicates this object currently has the keyboard focus"

What it is missing? That now state-chage:focused signal is the way to track focus changes?