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 504742 - Gecko.py should not call getLineContentsAtOffset() twice unnecessarily
Gecko.py should not call getLineContentsAtOffset() twice unnecessarily
Status: RESOLVED FIXED
Product: orca
Classification: Applications
Component: general
2.21.x
Other All
: Normal normal
: 2.22.0
Assigned To: Joanmarie Diggs (IRC: joanie)
Orca Maintainers
Depends on:
Blocks: 404403 491756
 
 
Reported: 2007-12-20 20:40 UTC by Joanmarie Diggs (IRC: joanie)
Modified: 2008-07-22 19:33 UTC
See Also:
GNOME target: ---
GNOME version: 2.21/2.22

Attachments
proposed patch (10.99 KB, patch)
2007-12-20 20:43 UTC, Joanmarie Diggs (IRC: joanie) 		none Details | Review
revised version (11.29 KB, patch)
2007-12-20 20:54 UTC, Joanmarie Diggs (IRC: joanie) 		accepted-commit_now Details | Review
made the changes Will indicated (11.00 KB, patch)
2007-12-21 01:18 UTC, Joanmarie Diggs (IRC: joanie) 		committed Details | Review
minor tweak (1.26 KB, patch)
2007-12-27 02:07 UTC, Joanmarie Diggs (IRC: joanie) 		committed Details | Review

Description Joanmarie Diggs (IRC: joanie) 2007-12-20 20:40:28 UTC 
Currently, Gecko.py has a number of places where it makes two back-to-back calls:

  updateBraille()
  speakContents()

speakContents() is being passed what is returned by getLineContentsAtOffset()
updateBraille() turns around and calls getLineContentsAtOffset() again.

We should store the contents so we don't have to call the method twice.
Comment 1 Joanmarie Diggs (IRC: joanie) 2007-12-20 20:43:35 UTC 
Created attachment 101354 [details] [review]
proposed patch

Mike please test.  Especially without the other performance patch you've been testing.
Comment 2 Joanmarie Diggs (IRC: joanie) 2007-12-20 20:54:35 UTC 
Created attachment 101356 [details] [review]
revised version

Mike please test.
Comment 3 Mike Pedersen 2007-12-20 23:27:38 UTC 
Even without the patch to bug 500016 this patch does improve arrowing by line.
Comment 4 Willie Walker 2007-12-20 23:46:20 UTC 
I think this looks good as is, especially since it's been tested by you and Mike. If I were to be a little anal, it would be to make currentLineContents be _currentLineContents, and I'd move the "contents = self.currentLineContents" line in updateBraille closer to the first use of contents in the method (e.g., just before the "if not contents or needToRefresh" line).  Anyhow, great to see this done!  Thanks!
Comment 5 Joanmarie Diggs (IRC: joanie) 2007-12-21 01:18:43 UTC 
Created attachment 101372 [details] [review]
made the changes Will indicated

Patch committed.  Moving to pending.
Comment 6 Joanmarie Diggs (IRC: joanie) 2007-12-27 02:07:20 UTC 
Created attachment 101654 [details] [review]
minor tweak

In redoing the Firefox regression tests to use assertions, I caught the fact that I was a little too enthusiastic with my patch for this bug: go{Next,Previous}Chunk() should present the object contents; not the line contents. My bad.

Patch pylinted, regression tested, committed.
Comment 7 Mike Pedersen 2008-01-04 01:43:00 UTC 
This one seems to be working well.
Comment 8 Joanmarie Diggs (IRC: joanie) 2008-01-04 04:23:34 UTC 
Thanks!  Closing as FIXED.