Suggested fix lp:#213718 (bazaar pager)

[Daniel Vedder]

Hello Richard,

I like your suggestions, they make the code a lot more elegant. I'll
include them when I upload the branch.

@Joshua: thanks for the link. I'll have a read through the documentation
and do the Launchpad push as soon as I can.

Two other questions: is it OK to hard-code the pager-threshold to 23
lines? Should we choose another value or make it configurable (or even
self-adjusting, if that is possible)? And secondly, the 'editor' option
also reads in the environmental variable BZR_EDITOR - should BZR_PAGER
also be made available?

Greetings,
Daniel

On 11.04.2015 16:03, Richard Wilbur wrote:
> Greetings Daniel,
>
> Thanks for the cleaned up patch.  Much easier reading!
>
> I would hope that uploading your branch wouldn't require sending
> everything that is common between your branch and the baseline from
> which you diverged, but mainly just the differences--in other words
> mostly just what you already sent in your patch.  (Here I have to
> defer to those who know the innards of bzr better than I.)  If it
> requires sending the whole thing, this sounds like fodder for some
> architectural discussions.
>
> A comment on bzrlib/ui/text.py:TextUIOutputStream._is_long():
> +    def _is_long(self, text):
> +        if "list" in repr(type(text)):
> +            if len(text) > self.pager_cutoff:
> +                return True
> +            else:
> +                return False
> +        else:
> +            number_of_lines = 0
> +            while "\n" in text:
> +                text = text[text.find("\n")+1:]
> +                number_of_lines = number_of_lines + 1
> +            if number_of_lines > self.pager_cutoff:
> +                return True
> +            else:
> +                return False
>
> What if we simplify the 'list' case to return the boolean comparison, as in:
> +        if "list" in repr(type(text)):
> +            return len(text) > self.pager_cutoff
>
> Now, on the string case, I like the fact that we are processing
> incrementally.  We could stop looking as soon as we reach the pager
> threshold.
> +        else:
> +            number_of_lines = 0
> +            start = 0
> +            while (start = text.find("\n", start)+1) > 0:
> +                number_of_lines = number_of_lines + 1
> +                if number_of_lines > self.pager_cutoff:
> +                    return True
> +            return False
>
> I haven't tested this but string.find returns -1 when it fails.
>
> I'll be offline most of today till this evening.
>
> Sincerely,
> Richard
>