Created attachment 159095 [details] [review]
Orientative patch (not working)

This is a not working patch for what I'm pretending to do.

The patch adds a CALLY.py toolkit script (and it is called properly).

In the patch I use the method processKeyboardEvent to try to fill the event_string. Anyway, for any reason the method is not called (this is the reason I added the debug.println).

Previously I tried to use consumeKeyboardEvent, and although the method was called and the translation was succesful, the key was not speeched out.

After reviewing were the code is called, I think that the ideal place to call this event conversion is on _processKeyboardEvent, when the orca KeyboardEvent is created from the pyatspi event struct (other changes, like the event printed and so on should be modified as well).

But this place is outside of the script logic, so I'm not sure if we could get that working.

Ideas are welcome.

> But this place is outside of the script logic, so I'm not sure if we could get
                                   ^^^^^^^^^^^^ script *toolkit* logic I mean

I still need to get caught up on this (the removal of the gdkish code surprises me). But having said that.... We need gnome-shell to work with Orca. So we need to deal with this one way or another....

Alex, I'll be much happier if I can see what you're seeing rather than merely speaking to the questions you ask theoretically. :-)

So....

I just set up a system with a fresh install of Ubuntu Lucid and gnome-shell. I also pulled Cally from git master and applied the four patches from http://bugzilla.o-hand.com/show_bug.cgi?id=2072. And I'm using Orca from git master, along with your patch from this bug.

What in particular are you using to test, causing the new CALLY.py to be used? (gnome-shell --replace ain't doing it for me)

Thanks!

Aha. Cally's examples cause CALLY.py to be loaded. :-)

Blurgh.

I need to give this some more thought (when I'm well-rested :-) ), but at the moment I'm wondering if we want to deal with this well before _processKeyboardEvent (i.e. do so input_event.py).

(In reply to comment #3)
> I still need to get caught up on this (the removal of the gdkish code surprises
> me). But having said that.... 

The idea is integrate Cally with Clutter, instead of have Cally as a isolated module loaded on runtime. So cally would be compiled with clutter (as cogl), and sharing the dependencies. As a gdk/gtk dependency is not viable for clutter (CCing Emmanuele Bassi to confirm what I think it is obvious), we are removing that.

Although this move, right now it is not really clear. Read the last comment in this bug [1]

(In reply to comment #5)
> Aha. Cally's examples cause CALLY.py to be loaded. :-)

Yes, I'm testing this with the Cally examples, really more easy that with the whole gnome-shell.

(In reply to comment #6)
> I need to give this some more thought (when I'm well-rested :-) ), but at the
> moment I'm wondering if we want to deal with this well before
> _processKeyboardEvent (i.e. do so input_event.py).

Yes, that was I mean with "when the orca KeyboardEvent is created from the pyatspi event struct" on comment 1. _processKeyboardEvent creates a new KeyboardEvent, class defined on input_event.py. My idea would be that the method __init__ on KeyboardEvent could made a toolkit specific call, in order to fill event_string if necessary.

But, as I'm not really used to orca code, I don't know if this would mean mess up the code, and the reason I'm asking for the orca developers wisdom. Anyway, I would continue with this idea.

BTW: in fact, in the patch attached I already modified the KeyboardEvent, as I include the keyval emitted by the pyatspi structure, as this have more gdk-information that the hw_code.

[1] http://bugzilla.o-hand.com/show_bug.cgi?id=2072#c5

(In reply to comment #7)
 
> BTW: in fact, in the patch attached I already modified the KeyboardEvent, as I
> include the keyval emitted by the pyatspi structure, as this have more
> gdk-information that the hw_code.

Yeah, I saw that. And looking at the id's output for Gail and Java, I think that addition (combined with the changes you're describing) is going solve our Java problem. (Yay! Yay! Yay!) You *so* rock!

On another note: Down the road, I anticipate there might need to be a Cally/Clutter speech generator, braille generator, etc. So rather than do a CALLY.py, I'd go ahead and make a CALLY/script.py. That way, it'll be easier to add needed stuff later.

Created attachment 159161 [details] [review]
Add keyval_name to keyboard events

Alex, I'm wondering if this change will help you out. (And if it doesn't help, let me know if it hurts. If not, I'm really tempted to commit this to see what we can do around the Java issues.)

As a temporary hack, what I think you could then do in CALLY/script.py is overide consumesKeyboardEvent (similar to what was done in the Java toolkit's script).

Let me know what you think. Thanks again!!

(In reply to comment #9)
> Created an attachment (id=159161) [details] [review]
> Add keyval_name to keyboard events
> 
> Alex, I'm wondering if this change will help you out. (And if it doesn't help,
> let me know if it hurts. If not, I'm really tempted to commit this to see what
> we can do around the Java issues.)

Well, now I somewhat lost. As far as I see this patch adds a new field keyval_name, and I'm not sure how you will use it.

The issue is that we still lack a proper event_string (at least in CALLY), that should be filled as it is used in other places.

How keyval_name will be used? As a fallback to event_string if it is not present? As a fallback to event_string in some specific toolkits?

> As a temporary hack, what I think you could then do in CALLY/script.py is
> overide consumesKeyboardEvent (similar to what was done in the Java toolkit's
> script).

And in the same way, it is still required to recompute the field is_text. is_text is computed on at-spi using keyval and event_string, so it is also received empty. This can be seen as something general enough to be used always, but as the others toolkits are receiving properly this method, my idea was compute that just on the cally toolkit script.

> Let me know what you think. Thanks again!!

And about the CALLY directory, this was the first thing that I attempted, but the script toolkit was not loaded, so I change that for just a CALLY.py. Anyway I would try it again.

(In reply to comment #10)

> Well, now I somewhat lost. As far as I see this patch adds a new field
> keyval_name, and I'm not sure how you will use it.

As just one example of why I am going down this road, check out what was recently done in the java-atk-wrapper:
http://git.gnome.org/browse/java-atk-wrapper/commit/?id=44021c6b890c305e2d10da43d38f19a988e852df

This was done largely to make Orca do the right thing w.r.t. commands. (Not my idea; in fact, once I learned it had been done, I suggested it be undone. But that's another story entirely. If you're really curious, see [1] and the next few comments. Anyhoo....)

The java-atk-wrapper hack had a side effect of causing us to no longer echo any of those characters when typing because when we decide if a character is a printable key (for the purpose of deciding whether or not to speak it according to the user's preferences) we expect the actual punctuation mark (".") as the event_string. Due to the above java-atk-wrapper hack, we get ("period"). "." is a printable key; "period" is not. So now we need to add another hack in Orca to hack around Ke's hack in order to get "." back so we can speak the right thing. (Fun, eh?)

So.... In the case of Java, and the current state of affairs in Orca, when we want to standardize keycodes, we want a key name; when we want to decide what to echo, we need the symbol which was actually typed. I believe the change in my patch helps by giving us access to both of these items. As a result, Orca can do the right thing and Ke can remove the hack he was asked to add to his module on our behalf. :-/

[1] http://defect.opensolaris.org/bz/show_bug.cgi?id=14365#c17

> The issue is that we still lack a proper event_string (at least in CALLY), that
> should be filled as it is used in other places.

Understood. This patch will give you access to the id and the name. What I was thinking you could do at least for now in your script is use the id and/or name to come up with the event_string in consumesKeyboardEvent. (Right? Or do I need more sleep and more coffee?)
 
> And in the same way, it is still required to recompute the field is_text.
> is_text is computed on at-spi using keyval and event_string, so it is also
> received empty. This can be seen as something general enough to be used always,
> but as the others toolkits are receiving properly this method, my idea was
> compute that just on the cally toolkit script.

To be honest, I think Gail might be telling us is_text is True event when it's False. But that's another issue....
 
> And about the CALLY directory, this was the first thing that I attempted, but
> the script toolkit was not loaded, so I change that for just a CALLY.py. Anyway
> I would try it again.

Cool. If it fails again, send me a patch and I'll see what's up.

Created attachment 159175 [details] [review]
Provisional patch

Ok, this is a provisional patch. It implements the issue how we were discussing. It still misses to fill the is_text field, and apply the modifiers to the keyval, as right now is just using the keyval received. But we can review it step by step. Anyway, this patch seems to work (now I get speeched

After all, this patch also make a lot of different changes. Summary:

I added a new script.py method called checkKeyboardEventData (the name is horrible, we can change it)

The purpose of this method is just check the KeyboardEvent fields, checking if some of them are required to be filled in some way.

It also adds a new debug method called keyboardEventToString, in order to show the fields of the orca KeyboardEvent class. The problem is that the name is confusing. We could just use the keyEventString, as the field name are the same, but not sure if in the future both structures will use the same fields.

There are a place where I have doubts: the way to call the script. I use orca_state, and use the activeScript, but not sure if it is the proper way to do that. It is correct or I should do something similar to orca.py with the _PRESENTATION_MANAGERS?

BTW: it places the CALLY in a proper directory as suggested in comment 8

(In reply to comment #11)
> (In reply to comment #10)

Sorry I uploaded my patch without realizing this comment.

> So.... In the case of Java, and the current state of affairs in Orca, when we
> want to standardize keycodes, we want a key name; when we want to decide what
> to echo, we need the symbol which was actually typed. I believe the change in
> my patch helps by giving us access to both of these items. As a result, Orca
> can do the right thing and Ke can remove the hack he was asked to add to his
> module on our behalf. :-/

Ok, now I understand, in some cases you would require the keyvalname and the event_string as different things. Point taken.

Anyway, in the Cally case, it would be the same.

> Understood. This patch will give you access to the id and the name. What I was
> thinking you could do at least for now in your script is use the id and/or name
> to come up with the event_string in consumesKeyboardEvent. (Right? Or do I need
> more sleep and more coffee?)

Well, I would prefer to go just for the final solution, and do that previously, in the "correct place" (at least the place where we both conclude that it is best to do the conversion).

> > And in the same way, it is still required to recompute the field is_text.
> > is_text is computed on at-spi using keyval and event_string, so it is also
> > received empty. This can be seen as something general enough to be used always,
> > but as the others toolkits are receiving properly this method, my idea was
> > compute that just on the cally toolkit script.
> 
> To be honest, I think Gail might be telling us is_text is True event when it's
> False. But that's another issue....

Hmmm, so you think that is_text should be always True?

Anyway, as I said this is_text field is not managed by Gail or Cally. As I said this is computed on at-spi and it is a field that only exists in the pyatspi key event struct (defined on idl/device.didl at at-spi2-core, filled on at-adaptor/event.c ad at-spi2-atk). This is the reason I said on comment 0 that this is too much ad-hoc for my taste. Not sure if any application should require that, and in the same way, as it is not define on AtkKeyEventStruct, not sure why at-spi concludes that it should be exported as well.

*But* as right now it exists and it is in use, we would just maintain that.
 
> > And about the CALLY directory, this was the first thing that I attempted, but
> > the script toolkit was not loaded, so I change that for just a CALLY.py. Anyway
> > I would try it again.
> 
> Cool. If it fails again, send me a patch and I'll see what's up.

The patch that I just uploaded have the CALLY script on the proper directory, so issue solved.

Review of attachment 159175 [details] [review]:

(In reply to comment #12)
 
> It still misses to fill the is_text field, and apply the modifiers
> to the keyval, as right now is just using the keyval received. But we can
> review it step by step.

Exactly. One thing at a time. And like you point out and I have verified, it does work. :-)

> I added a new script.py method called checkKeyboardEventData (the name is
> horrible, we can change it)

To be honest, that name isn't any worse than our other method names. :-) And it describes exactly what it's doing. So I say leave it unless you've got something better.
 
> It also adds a new debug method called keyboardEventToString, in order to show
> the fields of the orca KeyboardEvent class. The problem is that the name is
> confusing.

Agreed.

> We could just use the keyEventString, as the field name are the
> same, but not sure if in the future both structures will use the same fields.

Would it make sense to create a method (maybe toString()) within the KeyboardEvent class?

> There are a place where I have doubts: the way to call the script. I use
> orca_state, and use the activeScript, but not sure if it is the proper way to
> do that. It is correct or I should do something similar to orca.py with the
> _PRESENTATION_MANAGERS?

The thing is.... Orca's presentation managers were created early on (well before my joining the team), back when we thought we'd have multiple presentation managers. :-) It's cruft that we should probably remove one of these days....

I suppose one could make a good argument for doing something similar to orca.py and waiting until we actually get around to cruft removal to change how we handle these sorts of things. Having said that, I'm personally not bothered by your use of activeScript because it makes it very clear what's taking place. So I'll leave it up to you. However, if you would prefer to do something similar to orca.py, please add a comment in input_event.py explaining that what is happening functionally is that the activeScript is being used.

Nits:
* Lines should be under 80 chars (Cally script, input_event)
* No Tab chars unless required by code or file (you have one in
  your Cally script)
* Try to avoid unused imports (Cally script)

Best/fastest way to catch that stuff (I make those mistakes too!):
* Install pylint and dependencies
* git-add any new files
* run_pylint.sh (in the top level orca dir)

Sometimes pylint is wrong/confused, but often it's spot on. (And sometimes it finds existing issues/errors that we missed; only worry about issues/errors you've added. :-) )

(In reply to comment #13)
> (In reply to comment #11)
> > (In reply to comment #10)
> 
> Sorry I uploaded my patch without realizing this comment.

And I commented on it at the same time you were writing this response. :-)

> Ok, now I understand, in some cases you would require the keyvalname and the
> event_string as different things. Point taken.
> 
> Anyway, in the Cally case, it would be the same.

Yup. If you always base the the event_string on the value of the keyvalname, Cally is going to suffer from the same punctuation problem ("." is typed, and you change that into the name "period") and then Orca doesn't treat it as a printable char.

> Well, I would prefer to go just for the final solution, and do that previously,
> in the "correct place" (at least the place where we both conclude that it is
> best to do the conversion).

I like the final solution idea. The question is: Where is the best place to do the conversion?

If the goal is "the final solution": The one place it does NOT belong IMHO is consumeKeyboardEvent(). :-) I only proposed that as an interim hack so that you'd have something working.

To be clear: The question Orca's consumeKeyboardEvent() exists to answer is: "Is this KeyboardEvent an Orca command that a) should be executed by Orca and b) NOT passed along to the desktop environment/app?" It is not the place to be modifying the event data and I want to stop the Java toolkit script from doing that.

I like what you've done in your patch:

* It's not in consumeKeyboardEvent()

* It's at the script level and hence easy to override should some other toolkit come out and require some different solution to checkKeyboardEventData().

So what I'm thinking currently -- while multitasking on my DayJob and eating lunch at my desk :-) -- is this: We follow your model and I see if I can make Java start doing the right thing by giving it its own checkKeyboardEventData(). I'll try to add this to my after-work to-do list tonight, unless you have a better idea.
  
> Hmmm, so you think that is_text should be always True?

No. I think that is_text should be True if the event is... well... text. :-) Earlier when I was doing some debugging, I dumped out the value of is_text for gnome-terminal and gedit. It didn't matter what key I pressed (Alt and F1 for example) or where I was, is_text was always True. I think that's wrong. (Don't you??)
 
> this is computed on at-spi and it is a field that only exists in the pyatspi
> key event struct (defined on idl/device.didl at at-spi2-core, filled on
> at-adaptor/event.c ad at-spi2-atk).

I'm still using the old CORBA stuff, but I'll look later. Thanks for the clarification/explanation!

Review of attachment 159175 [details] [review]:

Oops, just noticed something I missed before.

I think checkKeyboardEventData() should check the keyboard event data :-) -- and only check the keyboard event data. That you have included this line:

       return default.Script.processKeyboardEvent(self, keyboardEvent)

seems out of place/unexpected.

(In reply to comment #14)

> To be honest, that name isn't any worse than our other method names. :-) And it
> describes exactly what it's doing. So I say leave it unless you've got
> something better.

My imagination is limited, so we can keep this name.

> Would it make sense to create a method (maybe toString()) within the
> KeyboardEvent class?

Good idea.

> I suppose one could make a good argument for doing something similar to orca.py
> and waiting until we actually get around to cruft removal to change how we
> handle these sorts of things. Having said that, I'm personally not bothered by
> your use of activeScript because it makes it very clear what's taking place. 

Ok, as it is working, and it is clear what is being done, we can kept his solution.

> Nits:
> * Lines should be under 80 chars (Cally script, input_event)
> * No Tab chars unless required by code or file (you have one in
>   your Cally script)
> * Try to avoid unused imports (Cally script)

In my defense I can say that it was just a provisional patch ;) The idea was start to review how the functionality was implemented. Thanks for pointing the errors.

> Best/fastest way to catch that stuff (I make those mistakes too!):
> * Install pylint and dependencies
> * git-add any new files
> * run_pylint.sh (in the top level orca dir)

Ok, thanks for the advice.

(In reply to comment #15)

> Yup. If you always base the the event_string on the value of the keyvalname,
> Cally is going to suffer from the same punctuation problem ("." is typed, and
> you change that into the name "period") and then Orca doesn't treat it as a
> printable char.

Hmm, right now Cally is setting event_string as NULL. I will check if a option would be fill event_string with the printable char and NULL otherwise. So the script would try to fill event_string only if a NULL is received.

> I like what you've done in your patch:

Ok, so lets kept with that, at least for the moment.

> > Hmmm, so you think that is_text should be always True?
> 
> No. I think that is_text should be True if the event is... well... text. :-)
> Earlier when I was doing some debugging, I dumped out the value of is_text for
> gnome-terminal and gedit. It didn't matter what key I pressed (Alt and F1 for
> example) or where I was, is_text was always True. I think that's wrong. (Don't
> you??)

Bad new, my idea would be just use the at-spi code as reference to fill is_text or not.

> > this is computed on at-spi and it is a field that only exists in the pyatspi
> > key event struct (defined on idl/device.didl at at-spi2-core, filled on
> > at-adaptor/event.c ad at-spi2-atk).
> 
> I'm still using the old CORBA stuff, but I'll look later. Thanks for the
> clarification/explanation!

In the CORBA case:
  * defined on idl/Accessibility_Registry.idl
  * filled on atk-bridge/bridge.c:spi_init_keystroke_from_atk_key_event


(In reply to comment #16)

> I think checkKeyboardEventData() should check the keyboard event data :-) --
> and only check the keyboard event data. That you have included this line:
> 
>        return default.Script.processKeyboardEvent(self, keyboardEvent)
> 
> seems out of place/unexpected.

Ouch. This is legacy, from the times I was defining processKeyboardEvent. The idea was call the default checkKeyboardEventData. I will correct that, thanks.

Created attachment 159256 [details] [review]
Updated patch

This patch solves (I hope) the issues commented by Joanmarie in the previous comments.

It also compute event_string only if NULL is received. I hope that this would solve the '.' vs 'period' issue (new patch uploaded on bug http://bugzilla.o-hand.com/show_bug.cgi?id=2072)

(In reply to comment #18)
> Created an attachment (id=159256) [details] [review]
> Updated patch
> 
> This patch solves (I hope) the issues commented by Joanmarie in the previous
> comments.

I'll hopefully have time to take a look in a bit. In the meantime, I wanted to address this:

> It also compute event_string only if NULL is received. I hope that this would
> solve the '.' vs 'period' issue (new patch uploaded on bug
> http://bugzilla.o-hand.com/show_bug.cgi?id=2072)

I have mixed feelings about that...

Pro: One less thing for Orca to have to worry about.

Con: It's inconsistent in terms of the toolkit implementing it. It seems to me what that's saying is, "Yeah, we know the string field is deprecated and according to the docs, 'should never be used'. And we agree, unless it happens to be a mark of punctuation. In which case it's inconvenient for us, so please fix it on your end." And, to make matters worse, once other toolkits get around to removing their implementation of this deprecated field, we're going to have to approach them with the same request.

I don't have a problem with your change (only compute it if NULL is received) because that should work with the current Cally/Clutter, and it will work if your new patch on that bug is implemented, and it won't hurt anything.

But when I made the comment about the '.' vs 'period' issue, all I meant was that the issue isn't just in Java. (Read, "And we should probably come up with a reliable, cross-toolkit solution in Orca.") In that spirit, I just did some quick playing around in a Python console:

>>> val = gdk.keyval_from_name("period")
>>> val
46L

>>> symbol = unichr(val)
>>> symbol
u'.'

>>> char = symbol.encode("UTF-8")
>>> char
'.'

So it would appear that Orca can handle the conversion on its end without our having to ask various toolkits to change things on theirs. What do you think?

(In reply to comment #19)

> > It also compute event_string only if NULL is received. I hope that this would
> > solve the '.' vs 'period' issue (new patch uploaded on bug
> > http://bugzilla.o-hand.com/show_bug.cgi?id=2072)
> 
> I have mixed feelings about that...
> 
> Pro: One less thing for Orca to have to worry about.
> 
> Con: It's inconsistent in terms of the toolkit implementing it. It seems to me
> what that's saying is, "Yeah, we know the string field is deprecated and
> according to the docs, 'should never be used'.

Well, this field is deprecated but just on the GdkEventStruct [1], not in Atk. And take into account that gailutil is already calling gdk_keyval_name when this field is not present on the gdk event struct. The issue here is that Clutter doesn't have any gdk_keyval_name [3] equivalent, and now, in order to integrate cally in clutter, we want to avoid calling gdk_keyval_name.

I mean that I know that with this change Cally is doing it in a inconsistent way, *but* I think that just emit a NULL in the event->string always is more inconsistent that the other way, emit NULL when I don't have any other possibility.

> And we agree, unless it happens
> to be a mark of punctuation. In which case it's inconvenient for us, so please
> fix it on your end." And, to make matters worse, once other toolkits get around
> to removing their implementation of this deprecated field, we're going to have
> to approach them with the same request.

Well, right now there aren't too many toolkits, AFAIK.

> I don't have a problem with your change (only compute it if NULL is received)
> because that should work with the current Cally/Clutter, and it will work if
> your new patch on that bug is implemented, and it won't hurt anything.
> 
> But when I made the comment about the '.' vs 'period' issue, all I meant was
> that the issue isn't just in Java. (Read, "And we should probably come up with
> a reliable, cross-toolkit solution in Orca.") In that spirit, I just did some

Well, it was a pity that thread [2] had 0 success.

> quick playing around in a Python console:
> 
> >>> val = gdk.keyval_from_name("period")
> >>> val
> 46L
> 
> >>> symbol = unichr(val)
> >>> symbol
> u'.'
> 
> >>> char = symbol.encode("UTF-8")
> >>> char
> '.'
> 
> So it would appear that Orca can handle the conversion on its end without our
> having to ask various toolkits to change things on theirs. What do you think?

So you mean that it was not required my last change on Cally, and it can just keep emitting a NULL on event.string? That as it can't handle all possibilities, just simplify and not handle any one?

BTW: adding dependency with bug 616459

[1] http://library.gnome.org/devel/gdk/unstable/gdk-Event-Structures.html#GdkEventKey
[2] http://mail.gnome.org/archives/gnome-accessibility-devel/2010-March/msg00005.html
[3] http://bugzilla.openedhand.com/show_bug.cgi?id=1952

Created attachment 159342 [details] [review]
Updated patch

Updated patch:

  * Some of the changes of the previous patch are already on master due bug 616459 commit, so now it is thiner.
  * It applies the modifiers to the keyval. Remember that GDK emit the event applying modifiers (ie SHIFT) to keyval. Clutter don't do that. So this patch apply the modifiers.
  * This updated keyval is used to compute keyval_name in the default script
  * try and except added to be more error clean

What it is missing to be complete? Compute is_text.

Created attachment 159439 [details] [review]
Compute is_text

Compute is_text

It is implemented using as reference what at-spi does, although using python funcs instead.

With this and the previous patch, I think that this feature is complete.

Now it only remains the usual revision (as probably there are some errors) and wait until apply related Cally bug [1] (pity we don't have a cross-bugzillas feature).

Anyway, probably this patch will be applied on Cally master although the integration with clutter is not applied finally.

[1] http://bugzilla.o-hand.com/show_bug.cgi?id=2072

Review of attachment 159342 [details] [review]:

I just tested this, and it seems to be doing the right thing. Awesome.

Please pylint it and correct the nits found (e.g. you should have a space after a comma, keep lines under 80 chars, avoid unused imports). Then I say commit it to master.

Review of attachment 159439 [details] [review]:

I am really sleep-deprived, so please forgive me if this is a dumb question....

+
+        # Set with non printable unicode categories. Full table:
+        # http://www.fileformat.info/info/unicode/category/index.htm
+        #
+        global non_printable_set
+        non_printable_set = ('Cc', 'Cf', 'Cn', 'Co', 'Cs')
+
         default.Script.__init__(self, app)

What purpose does this serve?

Review of attachment 159439 [details] [review]:

I knew it was the sleep deprivation... :-)

+    try:
+        import unicodedata
+        category = unicodedata.category (unichar)
+        result = category not in printable_set

here you've got printable_set (which I don't see defined). So should that last line instead be:

         result = category not in non_printable_set

?

If so, then what you should do is turn this:

+ global non_printable_set
+ non_printable_set = ('Cc', 'Cf', 'Cn', 'Co', 'Cs')
+

into:

self.non_printable_set = ('Cc', 'Cf', 'Cn', 'Co', 'Cs')

I really, really am tired. I'm sorry.

From the patch I failed to notice that your new methods are outside of the Script class. D'oh! So, hopefully the third time will be charm. :-)

This:

+        non_printable_set = ('Cc', 'Cf', 'Cn', 'Co', 'Cs')

should only be in the Script.__init__() if the script will have a need to change what the non_printable_set is. If you do not anticipate that, move that line outside of the Script class.

And if I still don't get it, I'll just give up. :-/

(In reply to comment #25)
> Review of attachment 159439 [details] [review]:
> 
> I knew it was the sleep deprivation... :-)
> 
> +    try:
> +        import unicodedata
> +        category = unicodedata.category (unichar)
> +        result = category not in printable_set
> 
> here you've got printable_set (which I don't see defined). So should that last
> line instead be:
> 
>          result = category not in non_printable_set

Urgh, yes a stupid typo. At the beginning it was a printable_set. I changed it because the non_printable_set was smaller.

In the same way I have just realized that the "return result" is not properly indented.

(In reply to comment #26)
> I really, really am tired. I'm sorry.

Don't worry, thank you very much to the review.
> 
> From the patch I failed to notice that your new methods are outside of the
> Script class. D'oh! So, hopefully the third time will be charm. :-)

Yes, at first they were methods in the class, but for any reason, when I called them it failed as were not found.

> This:
> 
> +        non_printable_set = ('Cc', 'Cf', 'Cn', 'Co', 'Cs')
> 
> should only be in the Script.__init__() if the script will have a need to
> change what the non_printable_set is. If you do not anticipate that, move that
> line outside of the Script class.

Ok, in fact it would became the patch more readable.

Thanks for the review, I will work to clear this issues.

Created attachment 159854 [details] [review]
Updated is_text compute patch

It solves the issues detected by Joanmarie Diggs

I also executed run_pylint. Although I normally avoid to mix things, as it was detect only some details, I included the clean up in this patch.

Review of attachment 159854 [details] [review]:

Looks good to me and seems to be working nicely. Please commit to master.

Solucion applied. Setting bug as FIXED.

I finally applied the changes on cally bug CB#2072, so this patch now works against the master cally.

I decided to make just a commit, so I merge previous two patches in just one, and I made a more complete commit description.

commit fa58dad41d577f13efabc91f2bae9e47e087dc50
Author: Alejandro Piñeiro <apinheiro@igalia.com>
Date:   Thu May 6 16:08:53 2010 +0200

    Add new specific toolkit script CALLY
    
    It adds a new toolkit script, CALLY, that manages specific details
    from clutter based applications.
    
    This patch adds the infrastructure of the new toolkit and redefines
    checkKeyboardEventData:
    
      * cally doesn't emit a key event with a event_string if this is
        not a printable character, like Ctrl, etc, so the toolkit script
        fill its using gdk_keyval_name
    
      * As at-spi uses event_string to check if the character is a text
        or not, it also make sure that this field is correctly filled
    
      * In order to compute properly the keyval name it apply the key
        modifiers, as clutter keyval is emitted without the modifiers,
        in opposite to gtk
    
    Fixes bgo#616206