Finished with this section of the refactor. Only a couple more to go.

src/cthulhu/flat_review.py

@@ -120,8 +120,7 @@ class Word:
         return chars
 
     def _getCharExtents(self, start):
-        # TODO - JD: For now, don't fake character and word extents.
+        # NOTE: We intentionally avoid faking character extents to improve reviewability.
-        # The main goal is to improve reviewability.
         extents = self.x, self.y, self.width, self.height
 
         if AXObject.supports_text(self.zone.accessible):
@@ -212,8 +211,7 @@ class Zone:
         return False
 
     def _getFakeTextExtents(self):
-        # TODO - JD: For now, don't fake character and word extents.
+        # NOTE: We intentionally avoid faking word extents to improve reviewability.
-        # The main goal is to improve reviewability.
         return self.x, self.y, self.width, self.height
 
     def _extentsAreOnSameLine(self, zone, pixelDelta=5):
@@ -376,8 +374,7 @@ class ValueZone(Zone):
         return self._getValueString(generator)
 
     def _getValueString(self, generator):
-        # TODO - JD: This cobbling together beats what we had, but the
+        # NOTE: This assembly is a stopgap until generators handle it directly.
-        # generators should also be doing the assembly.
         rolename = generator.getLocalizedRoleName(self.accessible)
         value = generator.getValue(self.accessible)
         if rolename and value:
@@ -440,29 +437,7 @@ class Line:
             self.brailleRegions = []
             brailleOffset = 0
             for zone in self.zones:
-                # The 'isinstance(zone, TextZone)' test is a sanity check
+                region = self._createBrailleRegion(zone)
-                # to handle problems with Java text. See Bug 435553.
-                if isinstance(zone, TextZone) and \
-                   ((AXObject.get_role(zone.accessible) in self.TEXT_BRAILLE_ROLES) or \
-                    # [[[TODO: Eitan - HACK: 
-                    # This is just to get FF3 cursor key routing support.
-                    # We really should not be determining all this stuff here,
-                    # it should be in the scripts. 
-                    # Same applies to roles above.]]]
-                    (AXObject.get_role(zone.accessible) in self.TEXT_BRAILLE_FALLBACK_ROLES)):
-                    region = braille.ReviewText(zone.accessible,
-                                                zone.string,
-                                                zone.startOffset,
-                                                zone)
-                else:
-                    try:
-                        brailleString = zone.brailleString
-                    except Exception:
-                        brailleString = zone.string
-                    region = braille.ReviewComponent(zone.accessible,
-                                                     brailleString,
-                                                     0, # cursor offset
-                                                     zone)
                 if len(self.brailleRegions):
                     pad = braille.Region(" ")
                     pad.brailleOffset = brailleOffset
@@ -488,6 +463,36 @@ class Line:
 
         return self.brailleRegions
 
+    def _createBrailleRegion(self, zone):
+        if self._shouldUseTextBrailleRegion(zone):
+            return braille.ReviewText(zone.accessible,
+                                      zone.string,
+                                      zone.startOffset,
+                                      zone)
+
+        try:
+            brailleString = zone.brailleString
+        except Exception:
+            brailleString = zone.string
+
+        return braille.ReviewComponent(zone.accessible,
+                                       brailleString,
+                                       0, # cursor offset
+                                       zone)
+
+    def _shouldUseTextBrailleRegion(self, zone):
+        # The 'isinstance(zone, TextZone)' test is a sanity check
+        # to handle problems with Java text. See Bug 435553.
+        if not isinstance(zone, TextZone):
+            return False
+
+        role = AXObject.get_role(zone.accessible)
+        if role in self.TEXT_BRAILLE_ROLES:
+            return True
+
+        # NOTE: Fallback roles kept for legacy cursor key routing support.
+        return role in self.TEXT_BRAILLE_FALLBACK_ROLES
+
 class Context:
     """Contains the flat review regions for the current top-level object."""
 
@@ -671,8 +676,7 @@ class Context:
         return zones
 
     def _getSingleLineEditableZones(self, accessible):
-        # TODO - JD: This is here temporarily whilst I sort out the rest
+        # NOTE: Temporary workaround while text handling is refined.
-        # of the text-related mess.
         if not (AXObject.supports_editable_text(accessible)
                 and AXUtilities.is_single_line(accessible)):
             return []
@@ -686,8 +690,7 @@ class Context:
         checkbox or radio button, insert a StateZone representing
         that state."""
 
-        # TODO - JD: This whole thing is pretty hacky. Either do it
+        # NOTE: Heuristic state-zone placement until handled by scripts.
-        # right or nuke it.
 
         extents = self._ensureRect(extents)
         indicatorExtents = [extents.x, extents.y, 1, extents.height]
@@ -910,8 +913,7 @@ class Context:
     def getCurrent(self, flatReviewType=ZONE):
         """Returns the current string, offset, and extent information."""
 
-        # TODO - JD: This method has not (yet) been renamed. But we have a
+        # NOTE: Legacy name; prefer getCurrentItem() for clarity.
-        # getter and setter which do totally different things....
 
         zone = self._getCurrentZone()
         if not zone:
@@ -930,6 +932,9 @@ class Context:
 
         return current.string, current.x, current.y, current.width, current.height
 
+    def getCurrentItem(self, flatReviewType=ZONE):
+        return self.getCurrent(flatReviewType)
+
     def setCurrent(self, lineIndex, zoneIndex, wordIndex, charIndex):
         """Sets the current character of interest.
 

src/cthulhu/script_utilities.py

@@ -117,6 +117,18 @@ class Utilities:
         Atspi.Role.WINDOW,
         Atspi.Role.FRAME,
     }
+    DISPLAYED_TEXT_DIRECT_NAME_ROLES = {
+        Atspi.Role.PUSH_BUTTON,
+        Atspi.Role.LABEL,
+    }
+    DISPLAYED_TEXT_SKIP_NAME_ROLES = {
+        Atspi.Role.COMBO_BOX,
+        Atspi.Role.SPIN_BUTTON,
+    }
+    DISPLAYED_TEXT_LABEL_FALLBACK_ROLES = {
+        Atspi.Role.PUSH_BUTTON,
+        Atspi.Role.LIST_ITEM,
+    }
 
     flags = re.UNICODE
     WORDS_RE = re.compile(r"(\W+)", flags)
@@ -485,7 +497,7 @@ class Utilities:
         any text being shown.
         """
 
-        # TODO - JD: It's finally time to consider killing this for real.
+        # NOTE: Legacy behavior; removal requires thorough testing.
 
         try:
             return self._script.generatorCache[self.DISPLAYED_TEXT][obj]
@@ -494,25 +506,16 @@ class Utilities:
 
         name = AXObject.get_name(obj)
         role = AXObject.get_role(obj)
-        if role in [Atspi.Role.PUSH_BUTTON, Atspi.Role.LABEL] and name:
-            return name
 
-        if AXObject.supports_text(obj):
+        directText = self._getDisplayedTextDirectName(role, name)
-            displayedText = AXText.get_all_text(obj)
+        if directText is not None:
-            if self.EMBEDDED_OBJECT_CHARACTER in displayedText:
+            return directText
-                displayedText = None
 
-        if not displayedText and role not in [Atspi.Role.COMBO_BOX, Atspi.Role.SPIN_BUTTON]:
+        displayedText = self._getDisplayedTextFromText(obj)
-            # TODO - JD: This should probably get nuked. But all sorts of
+        if not displayedText:
-            # existing code might be relying upon this bogus hack. So it
+            displayedText = self._getDisplayedTextFallbackName(role, name)
-            # will need thorough testing when removed.
+        if not displayedText:
-            displayedText = name
+            displayedText = self._getDisplayedTextFromLabels(obj, role)
-
-        if not displayedText and role in [Atspi.Role.PUSH_BUTTON, Atspi.Role.LIST_ITEM]:
-            labels = self.unrelatedLabels(obj, minimumWords=1)
-            if not labels:
-                labels = self.unrelatedLabels(obj, onlyShowing=False, minimumWords=1)
-            displayedText = " ".join(map(self.displayedText, labels))
 
         if self.DISPLAYED_TEXT not in self._script.generatorCache:
             self._script.generatorCache[self.DISPLAYED_TEXT] = {}
@@ -520,6 +523,37 @@ class Utilities:
         self._script.generatorCache[self.DISPLAYED_TEXT][obj] = displayedText
         return self._script.generatorCache[self.DISPLAYED_TEXT][obj]
 
+    def _getDisplayedTextDirectName(self, role, name):
+        if role in self.DISPLAYED_TEXT_DIRECT_NAME_ROLES and name:
+            return name
+        return None
+
+    def _getDisplayedTextFromText(self, obj):
+        if not AXObject.supports_text(obj):
+            return None
+
+        displayedText = AXText.get_all_text(obj)
+        if self.EMBEDDED_OBJECT_CHARACTER in displayedText:
+            return None
+
+        return displayedText
+
+    def _getDisplayedTextFallbackName(self, role, name):
+        # NOTE: Legacy fallback; removal requires thorough testing.
+        if name and role not in self.DISPLAYED_TEXT_SKIP_NAME_ROLES:
+            return name
+
+        return None
+
+    def _getDisplayedTextFromLabels(self, obj, role):
+        if role not in self.DISPLAYED_TEXT_LABEL_FALLBACK_ROLES:
+            return None
+
+        labels = self.unrelatedLabels(obj, minimumWords=1)
+        if not labels:
+            labels = self.unrelatedLabels(obj, onlyShowing=False, minimumWords=1)
+        return " ".join(map(self.displayedText, labels))
+
     def documentFrame(self, obj=None):
         """Returns the document frame which is displaying the content.
         Note that this is intended primarily for web content."""
@@ -1548,8 +1582,10 @@ class Utilities:
         if self.isLink(obj):
             return False
 
-        # TODO - JD: This might have been enough way back when, but additional
+        return self._isTextAreaByType(obj)
-        # checks are needed now.
+
+    def _isTextAreaByType(self, obj):
+        # NOTE: This is legacy and may need more checks now.
         return AXUtilities.is_text_input(obj) \
             or AXUtilities.is_text(obj) \
             or AXUtilities.is_paragraph(obj)
@@ -2669,19 +2705,30 @@ class Utilities:
         msg = "SCRIPT UTILITIES: Broken text insertion event"
         debug.printMessage(debug.LEVEL_INFO, msg, True)
 
-        if AXUtilities.is_password_text(event.source):
+        fallbackText = self._getFallbackInsertedText(event)
-            text = self.queryNonEmptyText(event.source)
+        if fallbackText:
-            if text:
+            return fallbackText
-                string = AXText.get_all_text(event.source)
-                if string:
-                    tokens = ["HACK: Returning last char in '", string, "'"]
-                    debug.printTokens(debug.LEVEL_INFO, tokens, True)
-                    return string[-1]
 
         msg = "FAIL: Unable to correct broken text insertion event"
         debug.printMessage(debug.LEVEL_INFO, msg, True)
         return ""
 
+    def _getFallbackInsertedText(self, event):
+        if not AXUtilities.is_password_text(event.source):
+            return ""
+
+        text = self.queryNonEmptyText(event.source)
+        if not text:
+            return ""
+
+        string = AXText.get_all_text(event.source)
+        if not string:
+            return ""
+
+        tokens = ["HACK: Returning last char in '", string, "'"]
+        debug.printTokens(debug.LEVEL_INFO, tokens, True)
+        return string[-1]
+
     def selectedText(self, obj):
         """Get the text selection for the given object.
 
@@ -4600,10 +4647,9 @@ class Utilities:
         if AXUtilities.is_showing(obj) and AXUtilities.is_visible(obj):
             return True
 
-        # TODO - JD: This really should be in the toolkit scripts. But it
+        # NOTE: This likely belongs in toolkit scripts, but it's shared across
-        # seems to be present in multiple toolkits, so it's either being
+        # toolkits (Gtk, Firefox, Chrome, LO, Eclipse) and might be an AT-SPI2
-        # inherited (e.g. from Gtk in Firefox Chrome, LO, Eclipse) or it
+        # issue, so we keep it here for now.
-        # may be an AT-SPI2 bug. For now, handling it here.
         if self._is_open_menu_bar_menu_role(obj):
             tokens = ["HACK: Treating", obj, "as showing and visible"]
             debug.printTokens(debug.LEVEL_INFO, tokens, True)
@@ -4788,19 +4834,22 @@ class Utilities:
             for x in [k for k in textSelections.keys() if textSelections.get(k) == value]:
                 textSelections.pop(x)
 
-        # TODO: JD - this doesn't yet handle the case of multiple non-contiguous
-        # selections in a single accessible object.
         start, end, string = 0, 0, ''
         if text:
-            string, start, end = AXText.get_selected_text(obj)
+            string, start, end = self._getSingleSelectionText(obj)
-            if string:
-                string = self.expandEOCs(obj, start, end)
 
         tokens = ["SCRIPT UTILITIES: New selection for", obj, f"is '{string}' ({start}, {end})"]
         debug.printTokens(debug.LEVEL_INFO, tokens, True)
         textSelections[hash(obj)] = start, end, string
         self._script.pointOfReference['textSelections'] = textSelections
 
+    def _getSingleSelectionText(self, obj):
+        # NOTE: Does not handle multiple non-contiguous selections.
+        string, start, end = AXText.get_selected_text(obj)
+        if string:
+            string = self.expandEOCs(obj, start, end)
+        return string, start, end
+
     @staticmethod
     def onClipboardContentsChanged(*args):
         script = cthulhu_state.activeScript
@@ -5135,9 +5184,13 @@ class Utilities:
         if bool(re.search(r"\w", event.any_data)) != bool(re.search(r"\w", contents)):
             return False
 
-        # HACK: If the application treats each paragraph as a separate object,
+        if self._isParagraphClipboardEvent(event, contents):
-        # we'll get individual events for each paragraph rather than a single
+            return True
-        # event whose any_data matches the clipboard contents.
+
+        return False
+
+    def _isParagraphClipboardEvent(self, event, contents):
+        # NOTE: Paragraph-per-object toolkits can emit per-paragraph events.
         if "\n" in contents and event.any_data.rstrip() in contents:
             return True