Page MenuHomePhabricator

beautifygraph: add configs for customizing the characters
Needs RevisionPublic

Authored by hooper on Oct 14 2018, 8:22 AM.


Group Reviewers

I figure we should keep this out of and for now.
Moving the extension functionality out of hgext is a separate issue.

Calling ui.config() inside getgraphnode seems like it might slow things down,
but it needs to happen there because of the template keyword. We probably want
to honor config overrides, so we can't cache glyphs at this level.

Diff Detail

rHG Mercurial
Lint Skipped
Unit Tests Skipped

Event Timeline

hooper created this revision.Oct 14 2018, 8:22 AM
hooper updated this revision to Diff 12138.Oct 14 2018, 10:17 AM

hooper and I talked at the sprint, and they wrote up the following for why they feel we should keep this out of graphmod/templatekw for now:

I don't think configurable graph log characters in core is a great idea right now, since it makes the empty promise that you can make logs look correct by using common fonts. E.g. the problem that pipe characters are used in several situations that just can't be handled well by one character.

As a specific example, there's no unicode line drawing characters that are fully-extended lines at 0 degrees (straight up) and 135 degrees (down to the right to the corner of the character box), so there will always be cases that can't really be represented "perfectly".

I don't generally agree with this reasoning - limitations of unicode should not be a reason to require that we go through a separate extension like this. The reasoning for the extension instead of just making the actual graph characters customizable was two-fold, if I remember correctly:

  1. there is currently some awkward code to handle extending one of the characters used to draw parent/grandparent lines for a few lines and then switch to the other. ( shows this in action) - the code to implement this is not unicode safe (at least in python2)
  2. the current graphlog code does not currently have config knobs to control every character, I think.

I think we should fix the code issue in #1 (I just sent D5112 for this), and then we can extended graphlog to have whatever character(s) we need, instead of attempting to do the replacements in an extension like we're currently doing? The extension could just become a set of "recommended" values for these config knobs.

I'll elaborate a bit. I think the idea of using unicode to make the graph good looking is flawed. There are no characters that properly serve common cases like this:


That's part of why there was some disagreement in early discussion of this extension.

I think it's a bad user experience, because if the graph looks 95% like a connected drawing, these broken areas are going to look like malfunctions (whereas we started with a stylized ascii drawing where the limitations are obvious to the user).

Additionally, the existing graphlog output is lossy. The two pipes in this example do not have the same meaning:


Even if we had the unicode characters/custom font needed to make this work, we would need at least 5 configurable variants of what is currently just a pipe. You would need hundreds of distinct characters to support all combinations of the 8 horizontal and diagonal edges in each character/cell of the graph rendering. More than double that to support dashed lines. Maybe exclude some unused characters. It's hard to imagine making that easy to configure.

You can also imagine having the graphlog output optionally use only horizontal/vertical edges so this can be done properly with box drawing characters. This is would complicate graphlog itself considerably, regardless of configuration.

Meanwhile, this patch would be useful to some of our users, and it keeps the problems contained within the extension.

baymax requested changes to this revision.Jan 24 2020, 12:32 PM

There seems to have been no activities on this Diff for the past 3 Months.

By policy, we are automatically moving it out of the need-review state.

Please, move it back to need-review without hesitation if this diff should still be discussed.


This revision now requires changes to proceed.Jan 24 2020, 12:32 PM