Repository: timbrel/gitsavvy
Issue: In the tags dashboard the commit reading is naive

For the text commands we read the whole line and then just `line[4:11]` to get the commit hash.  This **is** naive as the hash length is not a fixed value.  🙄 

Some commands might benefit from reading out the tag name instead as well. 

Changed files:
core/interfaces/tags.py, tests/test_status_dashboard.py, common/ui.py, core/interfaces/status.py

diff --git a/common/ui.py b/common/ui.py
index fff9fbe4..b16b9a06 100644
--- a/common/ui.py
+++ b/common/ui.py
@@ -9,10 +9,13 @@ from . import util
 from ..core.runtime import enqueue_on_worker
 from ..core.settings import GitSavvySettings
 from ..core.utils import focus_view
+from GitSavvy.core.base_commands import GsTextCommand
+from GitSavvy.core.fns import flatten
+
 
 MYPY = False
 if MYPY:
-    from typing import Dict, Optional
+    from typing import Dict, Iterable, Iterator, List, Optional, Tuple, Type
 
 
 interfaces = {}  # type: Dict[sublime.ViewId, Interface]
@@ -212,7 +215,7 @@ class Interface():
 
         return keyed_content
 
-    def update(self, key, content):
+    def update_view_section(self, key, content):
         self.view.run_command("gs_update_region", {
             "key": "git_savvy_interface." + key,
             "content": content
@@ -223,25 +226,6 @@ class Interface():
             self.view.erase_regions("git_savvy_interface." + key)
         self.regions = {}
 
-    def get_view_regions(self, key):
-        return self.view.get_regions("git_savvy_interface." + key)
-
-    def get_selection_line(self):
-        selections = self.view.sel()
-        if not selections or len(selections) > 1:
-            self.view.window().status_message("Please make a selection.")
-            return None
-
-        selection = selections[0]
-        return selection, util.view.get_lines_from_regions(self.view, [selection])[0]
-
-    def get_selection_lines_in_region(self, region):
-        return util.view.get_lines_from_regions(
-            self.view,
-            self.view.sel(),
-            valid_ranges=self.get_view_regions(region)
-        )
-
 
 def partial(key):
     def decorator(fn):
@@ -299,6 +283,70 @@ def get_interface(view_id):
     return interfaces.get(view_id, None)
 
 
+class InterfaceCommand(GsTextCommand):
+    interface_type = None  # type: Type[Interface]
+    interface = None  # type: Interface
+
+    def run_(self, edit_token, args):
+        vid = self.view.id()
+        interface = get_interface(vid)
+        if not interface:
+            raise RuntimeError(
+                "Assertion failed! "
+                "no dashboard registered for {}".format(vid))
+        if not isinstance(interface, self.interface_type):
+            raise RuntimeError(
+                "Assertion failed! "
+                "registered interface `{}` is not of type `{}`"
+                .format(interface, self.interface_type.__name__)
+            )
+        self.interface = interface
+        return super().run_(edit_token, args)
+
+    def region_name_for(self, section):
+        # type: (str) -> str
+        return "git_savvy_interface." + section
+
+
+def region_as_tuple(region):
+    # type: (sublime.Region) -> Tuple[int, int]
+    return region.begin(), region.end()
+
+
+def region_from_tuple(tuple_):
+    # type: (Tuple[int, int]) -> sublime.Region
+    return sublime.Region(*tuple_)
+
+
+def unique_regions(regions):
+    # type: (Iterable[sublime.Region]) -> Iterator[sublime.Region]
+    # Regions are not hashable so we unpack them to tuples,
+    # then use set, finally pack them again
+    return map(region_from_tuple, set(map(region_as_tuple, regions)))
+
+
+def unique_selected_lines(view):
+    # type: (sublime.View) -> List[sublime.Region]
+    return list(unique_regions(flatten(view.lines(s) for s in view.sel())))
+
+
+def extract_by_selector(view, item_selector, within_section=None):
+    # type: (sublime.View, str, str) -> List[str]
+    selected_lines = unique_selected_lines(view)
+    items = view.find_by_selector(item_selector)
+    acceptable_sections = (
+        view.get_regions(within_section)
+        if within_section else
+        [sublime.Region(0, view.size())]
+    )
+    return [
+        view.substr(item)
+        for section in acceptable_sections
+        for line in selected_lines if section.contains(line)
+        for item in items if line.contains(item)
+    ]
+
+
 class GsInterfaceCloseCommand(TextCommand):
 
     """
diff --git a/core/interfaces/status.py b/core/interfaces/status.py
index 2da7d5a0..cf617dc0 100644
--- a/core/interfaces/status.py
+++ b/core/interfaces/status.py
@@ -1,5 +1,4 @@
 from functools import partial, wraps
-from itertools import chain
 import os
 import threading
 
@@ -15,12 +14,27 @@ from GitSavvy.core import store
 from GitSavvy.core.runtime import enqueue_on_worker
 from GitSavvy.core.utils import noop, show_actions_panel
 
-flatten = chain.from_iterable
-
 
 MYPY = False
 if MYPY:
-    from typing import Iterable, Iterator, List, Optional, Tuple
+    from typing import Iterable, List, Optional, TypedDict
+    from ..git_mixins.stash import Stash
+    StatusViewState = TypedDict(
+        "StatusViewState",
+        {
+            "staged_files": List[FileStatus],
+            "unstaged_files": List[FileStatus],
+            "untracked_files": List[FileStatus],
+            "merge_conflicts": List[FileStatus],
+            "clean": bool,
+            "long_status": str,
+            "git_root": str,
+            "show_help": bool,
+            "head": str,
+            "stashes": List[Stash],
+        },
+        total=False
+    )
 
 
 # Expected
@@ -50,7 +64,7 @@ EXTRACT_FILENAME_RE = (
 
 def distinct_until_state_changed(just_render_fn):
     """Custom `lru_cache`-look-alike to minimize redraws."""
-    previous_state = {}
+    previous_state = {}  # type: StatusViewState
 
     @wraps(just_render_fn)
     def wrapper(self, *args, **kwargs):
@@ -193,7 +207,7 @@ class StatusInterface(ui.Interface, GitCommand):
             'show_help': True,
             'head': '',
             'stashes': []
-        }
+        }  # type: StatusViewState
         super().__init__(*args, **kwargs)
 
     def title(self):
@@ -387,46 +401,17 @@ class StatusInterface(ui.Interface, GitCommand):
 ui.register_listeners(StatusInterface)
 
 
-def get_subjects(view, *sections):
-    # type: (sublime.View, str) -> Iterable[sublime.Region]
-    return flatten(
-        view.find_by_selector(
-            'meta.git-savvy.status.section.{} meta.git-savvy.status.subject'.format(section)
-        )
+def _get_subjects_selector(sections):
+    # type: (Iterable[str]) -> str
+    return ", ".join(
+        'meta.git-savvy.status.section.{} meta.git-savvy.status.subject'.format(section)
         for section in sections
     )
 
 
-def region_as_tuple(region):
-    # type: (sublime.Region) -> Tuple[int, int]
-    return region.begin(), region.end()
-
-
-def region_from_tuple(tuple_):
-    # type: (Tuple[int, int]) -> sublime.Region
-    return sublime.Region(*tuple_)
-
-
-def unique_regions(regions):
-    # type: (Iterable[sublime.Region]) -> Iterator[sublime.Region]
-    # Regions are not hashable so we unpack them to tuples,
-    # then use set, finally pack them again
-    return map(region_from_tuple, set(map(region_as_tuple, regions)))
-
-
-def unique_selected_lines(view):
-    # type: (sublime.View) -> List[sublime.Region]
-    return list(unique_regions(flatten(view.lines(s) for s in view.sel())))
-
-
 def get_selected_subjects(view, *sections):
     # type: (sublime.View, str) -> List[str]
-    selected_lines = unique_selected_lines(view)
-    return [
-        view.substr(subject)
-        for subject in get_subjects(view, *sections)
-        if any(line.contains(subject) for line in selected_lines)
-    ]
+    return ui.extract_by_selector(view, _get_subjects_selector(sections))
 
 
 def get_selected_files(view, base_path, *sections):
diff --git a/core/interfaces/tags.py b/core/interfaces/tags.py
index 8d2c53f1..c7c73e18 100644
--- a/core/interfaces/tags.py
+++ b/core/interfaces/tags.py
@@ -2,7 +2,7 @@ from itertools import chain
 import os
 
 import sublime
-from sublime_plugin import WindowCommand, TextCommand
+from sublime_plugin import WindowCommand
 
 from ..commands import GsNavigate
 from ...common import ui
@@ -12,6 +12,23 @@ from GitSavvy.core.fns import filter_
 from GitSavvy.core.runtime import enqueue_on_worker, on_worker
 from GitSavvy.core.utils import flash
 
+
+__all__ = (
+    "gs_show_tags",
+    "gs_tags_toggle_remotes",
+    "gs_tags_refresh",
+    "gs_tags_delete",
+    "gs_tags_push",
+    "gs_tags_view_log",
+    "gs_tags_navigate_tag",
+)
+
+
+MYPY = False
+if MYPY:
+    from typing import List
+
+
 TAG_DELETE_MESSAGE = "Tag(s) deleted."
 
 NO_REMOTES_MESSAGE = "You have not configured any remotes."
@@ -26,16 +43,7 @@ START_PUSH_MESSAGE = "Pushing tag..."
 END_PUSH_MESSAGE = "Push complete."
 
 
-def tag_from_lines(lines):
-    tags = []
-    for line in lines:
-        m = line.strip().split(" ", 2)
-        if len(m) in (2, 3):
-            tags.append(m[1].strip())
-    return tags
-
-
-class GsShowTagsCommand(WindowCommand, GitCommand):
+class gs_show_tags(WindowCommand, GitCommand):
 
     """
     Open a branch dashboard for the active Git repository.
@@ -221,15 +229,47 @@ class TagsInterface(ui.Interface, GitCommand):
 
 ui.register_listeners(TagsInterface)
 
+TAGS_SELECTOR = "meta.git-savvy.tag.name"
+SHA_SELECTOR = "constant.other.git-savvy.tags.sha1"
+
+
+class TagsInterfaceCommand(ui.InterfaceCommand):
+    interface_type = TagsInterface
+    interface = None  # type: TagsInterface
+
+    def selected_local_tags(self):
+        # type: () -> List[str]
+        return ui.extract_by_selector(
+            self.view, TAGS_SELECTOR, self.region_name_for("local_tags"))
 
-class GsTagsToggleRemotesCommand(TextCommand, GitCommand):
+    def selected_local_commits(self):
+        # type: () -> List[str]
+        return ui.extract_by_selector(
+            self.view, SHA_SELECTOR, self.region_name_for("local_tags"))
+
+    def selected_remote_tags(self, remote_name):
+        # type: (str) -> List[str]
+        return ui.extract_by_selector(
+            self.view, TAGS_SELECTOR, self.remote_section_name_for(remote_name))
+
+    def selected_remote_commits(self, remote_name):
+        # type: (str) -> List[str]
+        return ui.extract_by_selector(
+            self.view, SHA_SELECTOR, self.remote_section_name_for(remote_name))
+
+    def remote_section_name_for(self, remote_name):
+        # type: (str) -> str
+        return self.region_name_for("remote_tags_list_" + remote_name)
+
+
+class gs_tags_toggle_remotes(TagsInterfaceCommand):
 
     """
     Toggle display of the remote tags.
     """
 
     def run(self, edit, show=None):
-        interface = ui.get_interface(self.view.id())
+        interface = self.interface
         interface.remotes = None
         if show is None:
             interface.show_remotes = not interface.show_remotes
@@ -238,21 +278,21 @@ class GsTagsToggleRemotesCommand(TextCommand, GitCommand):
         interface.render()
 
 
-class GsTagsRefreshCommand(TextCommand, GitCommand):
+class gs_tags_refresh(TagsInterfaceCommand):
 
     """
     Refresh the tags dashboard.
     """
 
     def run(self, edit, reset_remotes=False):
-        interface = ui.get_interface(self.view.id())
+        interface = self.interface
         if reset_remotes:
             interface.remotes = None
 
         util.view.refresh_gitsavvy(self.view)
 
 
-class GsTagsDeleteCommand(TextCommand, GitCommand):
+class gs_tags_delete(TagsInterfaceCommand):
 
     """
     Delete selected tag(s).
@@ -260,15 +300,13 @@ class GsTagsDeleteCommand(TextCommand, GitCommand):
 
     @on_worker
     def run(self, edit):
-        interface = ui.get_interface(self.view.id())
+        interface = self.interface
         self.delete_local(interface)
         self.delete_remote(interface)
         util.view.refresh_gitsavvy(self.view)
 
     def delete_local(self, interface):
-        lines = interface.get_selection_lines_in_region("local_tags")
-        tags_to_delete = tag_from_lines(lines)
-
+        tags_to_delete = self.selected_local_tags()
         if not tags_to_delete:
             return
 
@@ -283,8 +321,7 @@ class GsTagsDeleteCommand(TextCommand, GitCommand):
             return
 
         for remote_name, remote in interface.remotes.items():
-            lines = interface.get_selection_lines_in_region("remote_tags_list_" + remote_name)
-            tags_to_delete = tag_from_lines(lines)
+            tags_to_delete = self.selected_remote_tags(remote_name)
 
             if tags_to_delete:
                 self.git(
@@ -299,7 +336,7 @@ class GsTagsDeleteCommand(TextCommand, GitCommand):
         util.view.refresh_gitsavvy(self.view)
 
 
-class GsTagsPushCommand(TextCommand, GitCommand):
+class gs_tags_push(TagsInterfaceCommand):
 
     """
     Displays a panel of all remotes defined for the repository, then push
@@ -310,10 +347,10 @@ class GsTagsPushCommand(TextCommand, GitCommand):
     def run(self, edit, push_all=False):
         self.remotes = list(self.get_remotes().keys())
         if not self.remotes:
-            self.view.window().show_quick_panel([NO_REMOTES_MESSAGE], None)
+            self.window.show_quick_panel([NO_REMOTES_MESSAGE], None)
             return
 
-        self.view.window().show_quick_panel(
+        self.window.show_quick_panel(
             self.remotes,
             lambda idx: self.push_async(idx, push_all=push_all),
             flags=sublime.MONOSPACE_FONT
@@ -331,14 +368,13 @@ class GsTagsPushCommand(TextCommand, GitCommand):
             return
         remote = self.remotes[remote_idx]
 
-        interface = ui.get_interface(self.view.id())
-        lines = interface.get_selection_lines_in_region("local_tags")
-        tags_to_push = tag_from_lines(lines)
+        tags_to_push = self.selected_local_tags()
 
         flash(self.view, START_PUSH_MESSAGE)
         self.git("push", remote, *("refs/tags/" + tag for tag in tags_to_push))
         flash(self.view, END_PUSH_MESSAGE)
 
+        interface = self.interface
         interface.remotes = None
         util.view.refresh_gitsavvy(self.view)
 
@@ -351,13 +387,12 @@ class GsTagsPushCommand(TextCommand, GitCommand):
         self.git("push", remote, "--tags")
         flash(self.view, END_PUSH_MESSAGE)
 
-        interface = ui.get_interface(self.view.id())
-        if interface:
-            interface.remotes = None
-            util.view.refresh_gitsavvy(self.view)
+        interface = self.interface
+        interface.remotes = None
+        util.view.refresh_gitsavvy(self.view)
 
 
-class GsTagsViewLogCommand(TextCommand, GitCommand):
+class gs_tags_view_log(TagsInterfaceCommand):
 
     """
     Display the commit for the selected tag's hash.
@@ -365,21 +400,18 @@ class GsTagsViewLogCommand(TextCommand, GitCommand):
 
     @on_worker
     def run(self, edit):
-        interface = ui.get_interface(self.view.id())
-        local_lines = interface.get_selection_lines_in_region("local_tags")
-        commit_hashes = [line[4:11] for line in local_lines if line]
+        interface = self.interface
+        commit_hashes = self.selected_local_commits()
 
         if interface.remotes:
-            for remote_name, remote in interface.remotes.items():
-                lines = interface.get_selection_lines_in_region("remote_tags_list_" + remote_name)
-                commit_hashes.extend(line[4:11] for line in lines if line[:4] == "    ")
+            for remote_name in interface.remotes:
+                commit_hashes += self.selected_remote_commits(remote_name)
 
-        window = self.view.window()
         for commit_hash in commit_hashes:
-            window.run_command("gs_show_commit", {"commit_hash": commit_hash})
+            self.window.run_command("gs_show_commit", {"commit_hash": commit_hash})
 
 
-class GsTagsNavigateTagCommand(GsNavigate):
+class gs_tags_navigate_tag(GsNavigate):
 
     """
     Move cursor to the next (or previous) selectable file in the dashboard.
diff --git a/tests/test_status_dashboard.py b/tests/test_status_dashboard.py
index 4f26d93c..3dbbdb54 100644
--- a/tests/test_status_dashboard.py
+++ b/tests/test_status_dashboard.py
@@ -185,6 +185,6 @@ class TestStatusDashboard(DeferrableTestCase):
 
         view.run_command('gs_status_stage_file')
 
-        verify(GitCommand).stage_file('modified_file', 'new_file', 'conflicting_file', ...)
+        verify(GitCommand).stage_file('new_file', 'modified_file', 'conflicting_file', ...)
         verify(interface).refresh_repo_status_and_render()
         verify(view.window()).status_message("Staged files successfully.")