GNOME Bugzilla – Bug 467366
event.type does not conform to IDL
Last modified: 2019-03-27 20:11:50 UTC
Today event.type is a struct styled class. It seems like this stayed for historical purposes from pyLinAcc. What are the benefits of it being a struct? Anyway, a backwork compatable way for proceeding with this is to make sure event.type could be inspected like a string, including __cmp__, __getattr__, and __getitem__. A patch is coming up.
Created attachment 93798 [details] [review] Proposed patch Allows EventType to be inspected as a string.
Interesting! This seems like a nice way to support assistive technologies that expect the event.type to be a string (as is defined in the AT-SPI IDL) and only do string comparisons with event.type. Any other use, such as looking at substrings or prefixes, however, seems like it would fail. :-( Since GNOME 2.20 is the first official release of pyatspi, I really think we need to think this through seriously and get it right. I think co-opting the 'type' field is a bad thing, and I'm not sure what useful purpose the EventType class really has. It also seems to violate the design goal of "To keep naming similar to AT-SPI" for pyatspi (http://live.gnome.org/GAP/PythonATSPI). I propose that EventType goes away and that event.type always be a string as is defined in the AT-SPI IDL. If splitting out class:major:minor is desired (which seems to be about the only function EventType serves), then perhaps an [klass, major, minor] = event.splitType() method could be added.
I mostly agree. Although substrings and prefixes work :)
> I mostly agree. > Although substrings and prefixes work :) Cool! The semantics of __cmp__ and __getitem__ weren't obvoius to me when it came to dealing with strings. Neat. I'd still like to see EventType disappear, though, since we're still in a stage where this is a brandy new API. :-)
Created attachment 93803 [details] [review] Proposed patch This is even less invasive, and for all intents and purposes, EventType is a string.
(In reply to comment #5) > Created an attachment (id=93803) [edit] > Proposed patch > > This is even less invasive, and for all intents and purposes, EventType is a > string. > Neat. What is the purpose of stripping off the trailing ":"? If the AT-SPI is giving those to us, I'm not sure they should be stripped.
I'm neutral on whatever you decide. The splitting of the string was useful in LSR where knowing the more general class of event allowed us to optimize the dispatch. We needed to split in all cases, so we just did it on every event. Eitan's patch makes this feature non-invasive. If you really want to remove it entirely, check with Eitan first to see if he's using it in Accerciser. If not, banishing it should be fine. The trailing ":" is always a bother. Why does it exist on some events (e.g., focus:) but not on others (e.g., object:active-descendant). Always ignoring the trailing ":" removes burden from the programmer who has to remember whether it's there or not. I believe the code also strips it during registration, so the two are standardized, and the registration code works whether you give it "focus:" or "focus", for instance. Again, you're the ones having to deal such headaches, so it's up to you if you want to remove the safety. :)
> I'm neutral on whatever you decide. The splitting of the string was useful in > LSR where knowing the more general class of event allowed us to optimize the > dispatch. We needed to split in all cases, so we just did it on every event. > Eitan's patch makes this feature non-invasive. If you really want to remove it > entirely, check with Eitan first to see if he's using it in Accerciser. If not, > banishing it should be fine. I think we want to support the notion that pyatspi is as close to the metal as we can get it. Eitan's patch for event.type seems to get us there for that area, so that's cool. > The trailing ":" is always a bother. Why does it exist on some events (e.g., > focus:) but not on others (e.g., object:active-descendant). Always ignoring the > trailing ":" removes burden from the programmer who has to remember whether > it's there or not. I believe the code also strips it during registration, so > the two are standardized, and the registration code works whether you give it > "focus:" or "focus", for instance. Again, you're the ones having to deal such > headaches, so it's up to you if you want to remove the safety. :) If this is indeed a bug, it should be fixed at the AT-SPI infrastructure level rather than a layer on top of it. In looking at http://www.gnome.org/~billh/at-spi-idl/html/structAccessibility_1_1Event.html, there seems to be conflict in what is the right thing to do. We first see: "The string can be interpreted as class:type:subtype" Then we see references to "focus:", "window:", "object:", "document:", etc., events. I also looked at at-spi, atk, and GAIL code, and don't see the stripping referred to above. Instead, I see specific references to "focus:". Thus, I suspect the current implementation of including the trailing ":" on event types consisting solely of an event class (I think focus: is the only one) is probably the thing we should stick with. In other words, we probably shouldn't do the stripping.
Created attachment 95170 [details] [review] Patch to limit splitting In response to some work being proposed at https://bugzilla.mozilla.org/show_bug.cgi?id=392061, and a stack trace Aaron was seeing at https://bugzilla.mozilla.org/show_bug.cgi?id=392061#c15, I came digging here to see if I could work up a patch. Looks like you already did the work for me. :-) It turns out there is a 'details' field in pyatspi/event.py:EventType that can be used to support the proposed work, and I'm guessing Aaron is using an older version of accerciser that has its own copy of pyatspi. In digging, however, I just suggest maybe this minor patch -- it limits the splitting and allows the 'details' field to be the remainder of the event type string. As an aside, it took me a while to figure out the mapping between the fields in pyatspi/event.py:EventType and the 'class:type:subtype' description at http://www.gnome.org/~billh/at-spi-new-idl/html/html/structAccessibility_1_1Event.html#o0. It looks like EventType.klass=class, EventType.major=type and EventType.minor=subtype. It's probably too late, but it would be nice to make the EventType fields more closely match the AT-SPI spec. Given that the EventType fields are merely convenience fields, though, this is just a marginal request.
BTW, it looks like the 2007-08-16 18:22 patch at http://bugzilla.gnome.org/attachment.cgi?id=93803 was committed. Is it OK to mark it as such in this bug?
(In reply to comment #9) > Created an attachment (id=95170) [edit] > Patch to limit splitting Li - Eitan and I think this one is worth a freeze break request.
Created attachment 95561 [details] [review] Fix for new FF event type This fixes a bug in the dispatch code.
So the patch doesn't break API/ABI and string freeze, right?
Nope, it doesn't.
There are two patches below. I guess one of them should be obsolete?
Please send mail to release team to explain the reason to break the freeze. I assume you have tested the patch and the patch for bug 472301 well.
Looks like the patch (http://bugzilla.gnome.org/attachment.cgi?id=95561) was committed. Is this bug closable?
Yup.