This is an archive of the discontinued Mercurial Phabricator instance.

log: add a "graphwidth" template variable
ClosedPublic

Authored by hooper on Aug 11 2017, 8:25 PM.

Details

Summary

Wrapping text in templates for 'hg log --graph' can't be done very well,
because the template doesn't know how wide the graph drawing is. The edge
drawing function needs to know the number of lines in the template output, so
we need to also determine how wide that drawing would be before we call the
edgefn or evaluate the template.

This patch adds an optional widthfn callback alongside edgefn so that callers
to displaygraph() can enable it to pass the computed graph width into the
template. The new argument is added so as to avoid breaking any extensions
calling the current displaygraph(). The stock asciiedges() gets a stock
asciiwidth() so that we can do something like this:

COLUMNS=10 hg log --graph --template "{fill(desc, termwidth - graphwidth)}"
@  a a a a
|  a a a a
|  a a a a
o    a a a
|\   a a a
| |  a a a
| |  a a a

Using extensions to do this would be relatively complicated due to a lack of
hooks in this area of the code.

In the future it may make sense to have a more generic "textwidth" that tells
you how many columns you can expect to fill without causing the terminal to
wrap your output. I'm not sure there are other situations to motivate this yet,
or if it is entirely feasible.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

hooper created this revision.Aug 11 2017, 8:25 PM
hooper edited the summary of this revision. (Show Details)Aug 11 2017, 8:27 PM
hooper edited the summary of this revision. (Show Details)Aug 11 2017, 8:34 PM
hooper updated this revision to Diff 822.
hooper edited the summary of this revision. (Show Details)Aug 11 2017, 8:35 PM
yuja added a subscriber: yuja.Aug 13 2017, 6:01 AM

The edge
drawing function needs to know the number of lines in the template output, so
we need to also determine how wide that drawing would be before we call the
edgefn or evaluate the template.

Actually asciiedges() doesn't need the lines to be rendered, so we could do
something as follows:

edges = list(edgefn(type, char, state, rev, parents))
graphwidth = asciiwidth(edges)
displayer.show(...)
lines = displayer.hunk.pop()...
for type, char, coldata in edges:
    ascii(ui, state, type, char, lines, coldata)
    lines = []

This would be better in API point of view, but I'm not sure if asciiwidth() could get
simpler by this change.

mercurial/templatekw.py
768

s/graphwidth/showgraphwidth/, and functions should be sorted alphabetically.

In D360#5639, @yuja wrote:

The edge
drawing function needs to know the number of lines in the template output, so
we need to also determine how wide that drawing would be before we call the
edgefn or evaluate the template.

Actually asciiedges() doesn't need the lines to be rendered, so we could do
something as follows:

edges = list(edgefn(type, char, state, rev, parents))
graphwidth = asciiwidth(edges)
displayer.show(...)
lines = displayer.hunk.pop()...
for type, char, coldata in edges:
    ascii(ui, state, type, char, lines, coldata)
    lines = []

This would be better in API point of view, but I'm not sure if asciiwidth() could get
simpler by this change.

I had avoided this change because it might break extensions with their own edgefns. Do you think that's a concern?

yuja added a comment.Aug 14 2017, 9:25 PM

I had avoided this change because it might break extensions with their own edgefns. Do you think that's a concern?

It's probably okay to change the function interface. I don't think the current edgefn
is any useful for extensions. And I heard no problem after 97cb1aeaca78.

hooper edited the summary of this revision. (Show Details)Aug 15 2017, 2:13 PM
hooper updated this revision to Diff 940.
hooper added inline comments.Aug 15 2017, 2:25 PM
mercurial/templatekw.py
768

Can you explain why it should be renamed? I thought it should mirror termwidth. They both return an integer for computations in templates.

hooper updated this revision to Diff 941.Aug 15 2017, 2:29 PM
yuja added inline comments.Aug 15 2017, 11:44 PM
mercurial/cmdutil.py
2558

Just a nit. Is there any practical benefit to compute edges lazy?
I think edges can be simply converted to a list.

mercurial/templatekw.py
768

That's just a convention of templatekw. termwidth should be renamed too.

hooper marked 2 inline comments as done.Aug 16 2017, 3:12 PM
hooper added inline comments.
mercurial/cmdutil.py
2558

There can be many of them. E.g. in the hg repo it looks like a couple hundred with "hg log --graph -r 'tag()'". It just seems easy enough to hedge against it being expensive, though you might argue premature optimization.

hooper updated this revision to Diff 1009.Aug 16 2017, 3:13 PM
yuja accepted this revision.Aug 17 2017, 9:16 AM

Slightly adjusted the commit message for new version, and queued, thanks.

mercurial/cmdutil.py
2558

Okay, I guess using a list would be slightly faster in common scenario, but I
have no strong opinion about this.

mercurial/graphmod.py
210

Removed this lines in flight.

223

Perhaps the width can be computed from the coldata afterwards, but
that wouldn't make much difference.

width = indentation_level * 2 + 1

# graphmod.ascii()
    indentation_level = max(ncols, ncols + coldiff)
    for (line, logstr) in zip(lines, text):
        ln = "%-*s %s" % (2 * indentation_level, "".join(line), logstr)
This revision is now accepted and ready to land.Aug 17 2017, 9:16 AM
This revision was automatically updated to reflect the committed changes.

it may make sense to have a more generic "textwidth"

Good idea! That seems like what one usually wants. Can we not simply add that (defined as "termwidth - graphwidth")? If we had that, when would one want termwidth or graphwidth themselves?

hooper edited the summary of this revision. (Show Details)Aug 29 2017, 3:26 PM