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

# 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().

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