GNOME Bugzilla – Bug 620320
Speaking of indentation should be handled by the speech generator
Last modified: 2010-09-20 10:53:48 UTC
The information should be calculated and returned to caller. This will be useful because then we can use the result in formatting strings amongst other things.
Created attachment 162510 [details] [review] Fix for bgo#620320 - default.py:speakIndentation should be generalized.
Hi Joanie, does this look ok? soffice.script.py and evolution.script.py dont pylint to 10, but it isnt due to these changes. Thanks.
Review of attachment 162510 [details] [review]: I agree that this shouldn't be in default.py. I'm not convinced it belongs as a script utility, however. To me, the presentation of indentation seems more like the job for the generators.
The reason I put it in utilities, is so that it would be overwridden in certain applications. For example text editors would have the current behaviour (spaces and tabs) whereas in wiziwig (oowriter) we could overwride this to provide 0.5 inch, 1 inch etc, as required by bug #502101 Although saying this, maybe i shouldnt be calling self._script.utilities if the function is to be overwridden by getTextIndentation in local script_utilities?
Script utilities can be overridden. That's no problem -- other than the fact that you'd want to lose the @staticmethod you currently have. The purpose of the generators is to generate the stuff to be spoken, the stuff to be brailled, (future) audio tones, whatever. Scripts shouldn't be piecing this stuff together. That's why we have bugs like bug 616879 and bug 586634. As for bug 502101, that's specific to what should happen in response to an input event. In other words, it's feedback. The current speakTextIndentation() is, I believe, primarily intended for presenting the current line (i.e. in response to a caret-moved or focus event). As for braille, right now, we just display a Tab char, but we might want an option whereby users can display whitespace as whitespace and configure how many spaces should be used for Tab. (I can think of one screen reader which does this.) So are you going to have another script utility which does the braille string?
(In reply to comment #2) > Hi Joanie, does this look ok? > soffice.script.py and evolution.script.py dont pylint to 10, but it isnt > due to these changes. I beg to differ. When I manually run pylint on those two files without having applied your patch, they pylint to a 10. With your patch, they do not. The offending line in the soffice.script.py is: result = self._script.utilities.getTextIndentation(event.source, You have a very, very similar line in evolution.script.py. In script.py, self is an instance of Script. The Script class does not have any member called _script. Other classes do have a _script member (SpeechGenerator, BrailleGenerator, StructuralNavigation, Chat, Utilities, etc.) The _script member of those other classes gives us the corresponding Script instance. Did you not get a traceback when you tested your patch? Anyhoo, if we were going to make this a script utility (which I don't think we want to do), the correct call would be self.utilities.getTextIndentation().
Created attachment 162574 [details] [review] Rev 2, Fix for bgo#620320 - default.py:speakIndentation should be generalized. Hi Joanie, as discussed. things now live in speech_generator.py. I think my pylint is playing up more than usual, pylint output to be attached. Hope you dont mind the sneeking in of additional "\n"'s in generator.py, just makes the debug.out much easier to navigate.
Created attachment 162577 [details] evolution.pylint
Created attachment 162578 [details] soffice.pylint
Looking at the patch, it seems like the right thing to do. Thanks! I'd like to regression test it to be sure. Stay tuned.... (Also, I hope you don't mind the summary line change, but I think it more accurately describes what is being done, which is handy for logs and release notes.)
Looking at the regression tests for gtk-demo, this change is turning things from two utterances into one. It might just be my imagination, but I think that I'm hearing a slight pause with the current, two-utterance version. If it's not my imagination, will any users be bothered by the removal of this slight pause?
Created attachment 162739 [details] [review] Fix for bgo#620320 - Speaking of indentation should be handled by the speech generator. I see what you mean with not having the pause there, personally i dont mind, but I guess others might do. This should return the regression tests to pass as usual. Sorry for the number of iterations on this bug.
Review of attachment 162739 [details] [review]: Awesome. Thanks! Pylints, regression-tests nicely. Please commit.
Oops. I suppose this is an excellent example of why we need Evolution regression tests. Jon, would you mind taking a gander at this please? It looks like a regression from your fix here. Sorry I didn't catch it myself before. Thanks! Traceback (most recent call last):
+ Trace 222262
s.processObjectEvent(event)
self.listeners[key](event)
default.Script.onFocus(self, event)
orca.setLocusOfFocus(event, newFocus)
orca_state.locusOfFocus)
newLocusOfFocus)
self.presentMessageLine(event.source, newLocusOfFocus)
result = self.speechGenerator.generateTextIndentation(obj, string)
Created attachment 162826 [details] [review] Fixing regression introduced by bgo#620320 for evolution. Sorry Joanie, :( I will be more careful. Commited to master.