Patchwork crecord: call prevsibling() and nextsibling() directly

login
register
mail settings
Submitter Anton Shestakov
Date May 6, 2016, 12:12 p.m.
Message ID <01a6efb4dc63e63db93f.1462536777@neuro>
Download mbox | patch
Permalink /patch/14938/
State Accepted
Headers show

Comments

Anton Shestakov - May 6, 2016, 12:12 p.m.
# HG changeset patch
# User Anton Shestakov <av6@dwimlabs.net>
# Date 1462535541 -28800
#      Fri May 06 19:52:21 2016 +0800
# Node ID 01a6efb4dc63e63db93fc922550d34312ed2071b
# Parent  af6d4a49e3615ac6cfa63b9c10811c28520434ea
crecord: call prevsibling() and nextsibling() directly

The 3 classes for items used in crecord (uiheader, uihunk, uihunkline) all have
prevsibling() and nextsibling() methods. The two methods are used to get the
previous/next item of the same type of the same parent element as the current
one: when `a` is a uihunkline instance, a.nextsibling() returns the next line
in this hunk (or None, if `a` is the last line).

There are also two similar methods: previtem() and nextitem(). When called with
constrainlevel=True (the default) they simply returned the result of
prevsibling()/nextsibling(). Only when called with constrainlevel=False they
did something different: they returned previous/next item regardless of its
type (so if `a` is the last line in a hunk, a.nextitem(constrainlevel=False)
could return the next hunk or the next file -- something that is not a line).

Let's simplify this logic and make code call -sibling() methods when only
siblings are needed and -item() methods when any item would do, and then remove
the constrainlevel argument from previtem() and nextitem().
Pierre-Yves David - May 9, 2016, 5:17 p.m.
Looks good to me,
Pushed, thanks.

On 05/06/2016 02:12 PM, Anton Shestakov wrote:
> # HG changeset patch
> # User Anton Shestakov <av6@dwimlabs.net>
> # Date 1462535541 -28800
> #      Fri May 06 19:52:21 2016 +0800
> # Node ID 01a6efb4dc63e63db93fc922550d34312ed2071b
> # Parent  af6d4a49e3615ac6cfa63b9c10811c28520434ea
> crecord: call prevsibling() and nextsibling() directly
>
> The 3 classes for items used in crecord (uiheader, uihunk, uihunkline) all have
> prevsibling() and nextsibling() methods. The two methods are used to get the
> previous/next item of the same type of the same parent element as the current
> one: when `a` is a uihunkline instance, a.nextsibling() returns the next line
> in this hunk (or None, if `a` is the last line).
>
> There are also two similar methods: previtem() and nextitem(). When called with
> constrainlevel=True (the default) they simply returned the result of
> prevsibling()/nextsibling(). Only when called with constrainlevel=False they
> did something different: they returned previous/next item regardless of its
> type (so if `a` is the last line in a hunk, a.nextitem(constrainlevel=False)
> could return the next hunk or the next file -- something that is not a line).
>
> Let's simplify this logic and make code call -sibling() methods when only
> siblings are needed and -item() methods when any item would do, and then remove
> the constrainlevel argument from previtem() and nextitem().
>
> diff --git a/mercurial/crecord.py b/mercurial/crecord.py
> --- a/mercurial/crecord.py
> +++ b/mercurial/crecord.py
> @@ -111,17 +111,12 @@ class patchnode(object):
>       def parentitem(self):
>           raise NotImplementedError("method must be implemented by subclass")
>   
> -    def nextitem(self, constrainlevel=True, skipfolded=True):
> +    def nextitem(self, skipfolded=True):
>           """
> -        If constrainLevel == True, return the closest next item
> -        of the same type where there are no items of different types between
> -        the current item and this closest item.
> +        Try to return the next item closest to this item, regardless of item's
> +        type (header, hunk, or hunkline).
>   
> -        If constrainLevel == False, then try to return the next item
> -        closest to this item, regardless of item's type (header, hunk, or
> -        HunkLine).
> -
> -        If skipFolded == True, and the current item is folded, then the child
> +        If skipfolded == True, and the current item is folded, then the child
>           items that are hidden due to folding will be skipped when determining
>           the next item.
>   
> @@ -131,9 +126,7 @@ class patchnode(object):
>               itemfolded = self.folded
>           except AttributeError:
>               itemfolded = False
> -        if constrainlevel:
> -            return self.nextsibling()
> -        elif skipfolded and itemfolded:
> +        if skipfolded and itemfolded:
>               nextitem = self.nextsibling()
>               if nextitem is None:
>                   try:
> @@ -164,39 +157,31 @@ class patchnode(object):
>               except AttributeError: # parent and/or grandparent was None
>                   return None
>   
> -    def previtem(self, constrainlevel=True):
> +    def previtem(self):
>           """
> -        If constrainLevel == True, return the closest previous item
> -        of the same type where there are no items of different types between
> -        the current item and this closest item.
> -
> -        If constrainLevel == False, then try to return the previous item
> -        closest to this item, regardless of item's type (header, hunk, or
> -        HunkLine).
> +        Try to return the previous item closest to this item, regardless of
> +        item's type (header, hunk, or hunkline).
>   
>           If it is not possible to get the previous item, return None.
>           """
> -        if constrainlevel:
> -            return self.prevsibling()
> -        else:
> -            # try previous sibling's last child's last child,
> -            # else try previous sibling's last child, else try previous sibling
> -            prevsibling = self.prevsibling()
> -            if prevsibling is not None:
> -                prevsiblinglastchild = prevsibling.lastchild()
> -                if ((prevsiblinglastchild is not None) and
> -                    not prevsibling.folded):
> -                    prevsiblinglclc = prevsiblinglastchild.lastchild()
> -                    if ((prevsiblinglclc is not None) and
> -                        not prevsiblinglastchild.folded):
> -                        return prevsiblinglclc
> -                    else:
> -                        return prevsiblinglastchild
> +        # try previous sibling's last child's last child,
> +        # else try previous sibling's last child, else try previous sibling
> +        prevsibling = self.prevsibling()
> +        if prevsibling is not None:
> +            prevsiblinglastchild = prevsibling.lastchild()
> +            if ((prevsiblinglastchild is not None) and
> +                not prevsibling.folded):
> +                prevsiblinglclc = prevsiblinglastchild.lastchild()
> +                if ((prevsiblinglclc is not None) and
> +                    not prevsiblinglastchild.folded):
> +                    return prevsiblinglclc
>                   else:
> -                    return prevsibling
> +                    return prevsiblinglastchild
> +            else:
> +                return prevsibling
>   
> -            # try parent (or None)
> -            return self.parentitem()
> +        # try parent (or None)
> +        return self.parentitem()
>   
>   class patch(patchnode, list): # todo: rename patchroot
>       """
> @@ -603,7 +588,7 @@ class curseschunkselector(object):
>           """
>           currentitem = self.currentselecteditem
>   
> -        nextitem = currentitem.previtem(constrainlevel=False)
> +        nextitem = currentitem.previtem()
>   
>           if nextitem is None:
>               # if no parent item (i.e. currentitem is the first header), then
> @@ -619,8 +604,8 @@ class curseschunkselector(object):
>           parent-item of the currently selected item.
>           """
>           currentitem = self.currentselecteditem
> -        nextitem = currentitem.previtem()
> -        # if there's no previous item on this level, try choosing the parent
> +        nextitem = currentitem.prevsibling()
> +        # if there's no previous sibling, try choosing the parent
>           if nextitem is None:
>               nextitem = currentitem.parentitem()
>           if nextitem is None:
> @@ -641,7 +626,7 @@ class curseschunkselector(object):
>           #self.startprintline += 1 #debug
>           currentitem = self.currentselecteditem
>   
> -        nextitem = currentitem.nextitem(constrainlevel=False)
> +        nextitem = currentitem.nextitem()
>           # if there's no next item, keep the selection as-is
>           if nextitem is None:
>               nextitem = currentitem
> @@ -655,17 +640,16 @@ class curseschunkselector(object):
>           same level as the parent item of the currently selected item.
>           """
>           currentitem = self.currentselecteditem
> -        nextitem = currentitem.nextitem()
> -        # if there's no next item on this level, try choosing the parent's
> -        # nextitem.
> +        nextitem = currentitem.nextsibling()
> +        # if there's no next sibling, try choosing the parent's nextsibling
>           if nextitem is None:
>               try:
> -                nextitem = currentitem.parentitem().nextitem()
> +                nextitem = currentitem.parentitem().nextsibling()
>               except AttributeError:
> -                # parentitem returned None, so nextitem() can't be called
> +                # parentitem returned None, so nextsibling() can't be called
>                   nextitem = None
>           if nextitem is None:
> -            # if no next item on parent-level, then no change...
> +            # if parent has no next sibling, then no change...
>               nextitem = currentitem
>   
>           self.currentselecteditem = nextitem
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Patch

diff --git a/mercurial/crecord.py b/mercurial/crecord.py
--- a/mercurial/crecord.py
+++ b/mercurial/crecord.py
@@ -111,17 +111,12 @@  class patchnode(object):
     def parentitem(self):
         raise NotImplementedError("method must be implemented by subclass")
 
-    def nextitem(self, constrainlevel=True, skipfolded=True):
+    def nextitem(self, skipfolded=True):
         """
-        If constrainLevel == True, return the closest next item
-        of the same type where there are no items of different types between
-        the current item and this closest item.
+        Try to return the next item closest to this item, regardless of item's
+        type (header, hunk, or hunkline).
 
-        If constrainLevel == False, then try to return the next item
-        closest to this item, regardless of item's type (header, hunk, or
-        HunkLine).
-
-        If skipFolded == True, and the current item is folded, then the child
+        If skipfolded == True, and the current item is folded, then the child
         items that are hidden due to folding will be skipped when determining
         the next item.
 
@@ -131,9 +126,7 @@  class patchnode(object):
             itemfolded = self.folded
         except AttributeError:
             itemfolded = False
-        if constrainlevel:
-            return self.nextsibling()
-        elif skipfolded and itemfolded:
+        if skipfolded and itemfolded:
             nextitem = self.nextsibling()
             if nextitem is None:
                 try:
@@ -164,39 +157,31 @@  class patchnode(object):
             except AttributeError: # parent and/or grandparent was None
                 return None
 
-    def previtem(self, constrainlevel=True):
+    def previtem(self):
         """
-        If constrainLevel == True, return the closest previous item
-        of the same type where there are no items of different types between
-        the current item and this closest item.
-
-        If constrainLevel == False, then try to return the previous item
-        closest to this item, regardless of item's type (header, hunk, or
-        HunkLine).
+        Try to return the previous item closest to this item, regardless of
+        item's type (header, hunk, or hunkline).
 
         If it is not possible to get the previous item, return None.
         """
-        if constrainlevel:
-            return self.prevsibling()
-        else:
-            # try previous sibling's last child's last child,
-            # else try previous sibling's last child, else try previous sibling
-            prevsibling = self.prevsibling()
-            if prevsibling is not None:
-                prevsiblinglastchild = prevsibling.lastchild()
-                if ((prevsiblinglastchild is not None) and
-                    not prevsibling.folded):
-                    prevsiblinglclc = prevsiblinglastchild.lastchild()
-                    if ((prevsiblinglclc is not None) and
-                        not prevsiblinglastchild.folded):
-                        return prevsiblinglclc
-                    else:
-                        return prevsiblinglastchild
+        # try previous sibling's last child's last child,
+        # else try previous sibling's last child, else try previous sibling
+        prevsibling = self.prevsibling()
+        if prevsibling is not None:
+            prevsiblinglastchild = prevsibling.lastchild()
+            if ((prevsiblinglastchild is not None) and
+                not prevsibling.folded):
+                prevsiblinglclc = prevsiblinglastchild.lastchild()
+                if ((prevsiblinglclc is not None) and
+                    not prevsiblinglastchild.folded):
+                    return prevsiblinglclc
                 else:
-                    return prevsibling
+                    return prevsiblinglastchild
+            else:
+                return prevsibling
 
-            # try parent (or None)
-            return self.parentitem()
+        # try parent (or None)
+        return self.parentitem()
 
 class patch(patchnode, list): # todo: rename patchroot
     """
@@ -603,7 +588,7 @@  class curseschunkselector(object):
         """
         currentitem = self.currentselecteditem
 
-        nextitem = currentitem.previtem(constrainlevel=False)
+        nextitem = currentitem.previtem()
 
         if nextitem is None:
             # if no parent item (i.e. currentitem is the first header), then
@@ -619,8 +604,8 @@  class curseschunkselector(object):
         parent-item of the currently selected item.
         """
         currentitem = self.currentselecteditem
-        nextitem = currentitem.previtem()
-        # if there's no previous item on this level, try choosing the parent
+        nextitem = currentitem.prevsibling()
+        # if there's no previous sibling, try choosing the parent
         if nextitem is None:
             nextitem = currentitem.parentitem()
         if nextitem is None:
@@ -641,7 +626,7 @@  class curseschunkselector(object):
         #self.startprintline += 1 #debug
         currentitem = self.currentselecteditem
 
-        nextitem = currentitem.nextitem(constrainlevel=False)
+        nextitem = currentitem.nextitem()
         # if there's no next item, keep the selection as-is
         if nextitem is None:
             nextitem = currentitem
@@ -655,17 +640,16 @@  class curseschunkselector(object):
         same level as the parent item of the currently selected item.
         """
         currentitem = self.currentselecteditem
-        nextitem = currentitem.nextitem()
-        # if there's no next item on this level, try choosing the parent's
-        # nextitem.
+        nextitem = currentitem.nextsibling()
+        # if there's no next sibling, try choosing the parent's nextsibling
         if nextitem is None:
             try:
-                nextitem = currentitem.parentitem().nextitem()
+                nextitem = currentitem.parentitem().nextsibling()
             except AttributeError:
-                # parentitem returned None, so nextitem() can't be called
+                # parentitem returned None, so nextsibling() can't be called
                 nextitem = None
         if nextitem is None:
-            # if no next item on parent-level, then no change...
+            # if parent has no next sibling, then no change...
             nextitem = currentitem
 
         self.currentselecteditem = nextitem