config: use shiny, colored graph nodes by default #3381

thoughtpolice · 2024-03-27T17:42:58Z

Checklist

If applicable:

I have updated CHANGELOG.md
I have updated the documentation (README.md, docs/, demos/)

CHANGELOG.md

cli/src/config/templates.toml

CHANGELOG.md

cli/src/config/templates.toml

algmyr · 2024-03-27T19:55:24Z

There was some liking for having a upside down T box drawing character for the root, but maybe that's bad as a default because reversed is a thing.

What about users of the ascii mode? If the code hasn't changed since I introduced this, this will override the symbols used in the ascii mode as well. And (possibly worse) I don't think toml allows you to unset this value to get the ascii defaults.

algmyr · 2024-03-27T20:48:56Z

cli/tests/test_log_command.rs

@@ -1167,16 +1167,16 @@ fn test_graph_styles() {
    @     merge
    |\
    | \
-    |  o  side branch


Yeah, this is the ascii breakage I mentioned.

The default node template is defined in settings.rs for this reason.

necauqua · 2024-03-27T20:55:09Z

There was some liking for having a upside down T box drawing character for the root

Yup I loved that.

Perhaps "log_inverted: bool" of some sorts could be added to templater globals?.
So that whoever cares for edgecases like that (and builtins absolutely do care) can support it.

Maybe in a separate PR

necauqua · 2024-03-27T20:58:28Z

cli/src/config/templates.toml

+  if(!self, label("elided", "⇋"),
+    if(current_working_copy, label("wcc", "@"),
+      if(immutable, label("immutable", "◆"),
+        if(description.starts_with("wip: "), label("wip", "!"),


I don't like ! very much because it kind of looks like a continuation of the line going into it.

I haven't found anything better though

Yuya added `coalesce()` as a built-in function with variable arity in a2a9b7d, and we want to use it with more-than-two arguments in the default `log_node` configuration in order to provide better graph node symbols for users. However, `test_templater` already defined this name as an alias, but with only two parameters; this alias now conflicts with the built-in definition and overrides it, causing the test to fail catastrophically because now the default configuration is considered invalid. Simply renaming it fixes this without invalidating the actual test case. Signed-off-by: Austin Seipp <[email protected]> Change-Id: I18593f64b9168fc334001c2ff5d49ad8d81c006d

cli/src/config/templates.toml

yuja · 2024-03-28T05:03:24Z

cli/src/config/templates.toml

+op_log_node = 'if(current_operation, "@", "◉")'
+
+log_node = '''
+label("node",


nit: perhaps, "node" should be moved to the caller (as we do for "op_log"), but that can be addressed later.

yuja · 2024-03-28T05:06:15Z

cli/tests/test_log_command.rs

@@ -1167,16 +1167,16 @@ fn test_graph_styles() {
    @     merge
    |\
    | \
-    |  o  side branch


The default node template is defined in settings.rs for this reason.

algmyr · 2024-03-28T09:56:58Z

cli/src/config/templates.toml

@@ -39,6 +39,20 @@ log = 'builtin_log_compact'
 op_log = 'builtin_op_log_compact'
 show = 'builtin_log_detailed'

+op_log_node = 'if(current_operation, "@", "◉")'


Do we want to make the "normal" symbol consistent with log_node? Do we want to make labels/colors consistent?

Signed-off-by: Austin Seipp <[email protected]> Change-Id: I4e7e5f6cd3c1afa0b42ee88725bc89d3bfafdc0a

algmyr · 2024-03-29T14:24:30Z

I'm tempted to play around with things a bit more. I have some ideas, let's see how easy it would be to actually make them happen...

Edit: shared some of this in the discord

This should hopefully unblock jj-vcs#3381.

ilyagr · 2024-04-05T04:53:41Z

I would still prefer something like ~ or some form of a unicode ⋯ for "elided". ⇋ might not be in some fonts, and it looks more like "conflicted" to me. It's a busy symbol.

Actually, I quite like the idea of using ⇋ for "conflicted".

martinvonz · 2024-04-05T05:02:26Z

I don't want us to get stuck debating the symbols for too long. I would prefer to see this merged with more conservative choices to start with instead. I would say that the current ◌ (before this PR) works even if we change the default node to ○, especially since elided nodes stand out pretty well based on their text label already. Then we can have a small PR later that just changes the symbol for elided nodes if we all agree on something. What do you think?

ilyagr · 2024-04-05T05:07:32Z

That sounds fine. I don't mean that comment to be blocking, especially as it sounds like this PR is more or less ready to be merged.

ilyagr · 2024-04-05T05:24:37Z

Another non-blocking thought: the diamond is annoyingly not centered vertically with my font. I wonder if something like ⏹ is any better. Or maybe some other Unicode diamond. (Update: Nope. I think that on my screen, ⏹ looks a bit better, but it's definitely not perfectly centered).

(also, another ad for ~)

ilyagr · 2024-04-05T05:31:31Z

And re colors: I really like making @ green.

necauqua · 2024-04-05T14:39:40Z

If/when #3460 gets merged you could also have a different symbol for snapshot operations in the oplog 🙃

noahmayr · 2024-04-06T13:10:08Z

I think ⍭ could work for elided nodes as well or ┋/¦ (or even :) as a dotted line, depending on how various fonts display those.

algmyr · 2024-04-10T13:00:19Z

⍭

Let's stick with the unicode block of box drawing characters for the graph and anything that is supposed to meld with the graph. Those we can depend on actually lining up with each other.

algmyr · 2024-04-10T13:07:47Z

(also, another ad for ~)

I don't mind that actually, it has similar semantics as the ~ at the bottom of a tree, and the color/label makes clear what it is

ilyagr

I've been using this for a bit, and I'm quite used to it now. In particular, I'm now relying on the green tint of @ and the immutable commit marker.

So, I think I'll approve this. If somebody feels this is not ready, please speak up.

I have made two changes to this that I really like. I'd recommend including them in this PR, but I can also make them my own PR; I don't want to slow this down on deciding how controversial my suggestions are.

diff --git a/cli/src/config/templates.toml b/cli/src/config/templates.toml
index 4e9ad79ef8...ad7cfd793d 100644
--- a/cli/src/config/templates.toml
+++ b/cli/src/config/templates.toml
@@ -44,13 +44,13 @@
 log_node = '''
 label("node",
   coalesce(
-    if(!self, label("elided", "⇋")),
+    if(!self, label("elided", "~")),
     if(current_working_copy, label("working_copy", "@")),
-    if(immutable, label("immutable", "◆")),
+    if(immutable, label("immutable", "⏹")),
     label("normal", "○")
   )
 )
'''

ilyagr · 2024-04-12T18:26:12Z

docs/config.md

+  * "Elided nodes" are specified with the "harpoon" symbol `⇋` to show that
+    there are commits between the two nodes that are "cut" out, and not shown.
+  * Commits whose description begins with `wip:` are represented with a yellow
+    `!` symbol.


Is this still true about !? I think it was removed from the PR (and I'd prefer to keep this PR simpler). Or is that already merged?

We also need to remember to update this if we change any of the notes.

algmyr · 2024-04-12T18:26:14Z

First change is fine to me, and kinda helps with keeping ascii and regular similar. Second one is not aligned in my setup. :P

There is some precedence for using the diamond (was it from git branchless?). Worst case you can override it on your own.

The bigger blocker for this change atm is moving the defaults from the config files to being hard coded in the rust code.
(Unless we now have the template infra to support an ascii property)

ilyagr · 2024-04-12T18:31:20Z

Second one is not aligned in my setup. :P

What do you mean?

There is some precedence for using the diamond (was it from git branchless?). Worst case you can override it on your own.

I'm not violently opposed to a diamond, I just think a rectangle looks more solid and better.

The bigger blocker for this change atm is moving the defaults from the config files to being hard coded in the rust code.
(Unless we now have the template infra to support an ascii property)

Ah, I see, this would override the ascii mode too. This does need a solution. My first instinct would be the opposite: move the ASCII config to templates.

I'll un-approve this because of the ASCII issue, thanks for keeping me honest.

ASCII mode issues

algmyr · 2024-04-12T18:39:54Z

What do you mean?

My first instinct would be the opposite: move the ASCII config to templates.

I did have a PR for that, but Martin decided it was not the right path to add an additional set of templates that we would need to remove later.

ilyagr · 2024-04-12T19:32:06Z

I see, thanks.

yuja · 2024-04-13T01:31:27Z

My first instinct would be the opposite: move the ASCII config to templates.

I did have a PR for that, but Martin decided it was not the right path to add an additional set of templates that we would need to remove later.

btw, I think it's okay to add ascii/unicode default "aliases" which the user can freely choose. People like me might want to use the ascii version in unicode graph.

thoughtpolice · 2024-07-16T12:13:06Z

Fixed by #4095, thanks @algmyr!

martinvonz reviewed Mar 27, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

cli/src/config/templates.toml Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

martinvonz reviewed Mar 27, 2024

View reviewed changes

cli/src/config/templates.toml Outdated Show resolved Hide resolved

thoughtpolice force-pushed the aseipp/push-wnxwtqnynlrr branch 4 times, most recently from ca58af7 to c38c930 Compare March 27, 2024 20:45

algmyr reviewed Mar 27, 2024

View reviewed changes

necauqua reviewed Mar 27, 2024

View reviewed changes

thoughtpolice force-pushed the aseipp/push-wnxwtqnynlrr branch from c38c930 to ffa4891 Compare March 28, 2024 03:20

yuja reviewed Mar 28, 2024

View reviewed changes

algmyr reviewed Mar 28, 2024

View reviewed changes

thoughtpolice mentioned this pull request Mar 28, 2024

test: rename coalesce(x,y) alias in test_templater [NFC] #3392

Closed

config: use shiny, colored graph nodes by default

0710bf2

Signed-off-by: Austin Seipp <[email protected]> Change-Id: I4e7e5f6cd3c1afa0b42ee88725bc89d3bfafdc0a

thoughtpolice force-pushed the aseipp/push-wnxwtqnynlrr branch from ffa4891 to 0710bf2 Compare March 28, 2024 20:45

algmyr added a commit to algmyr/jj that referenced this pull request Mar 31, 2024

cli: Split out ascii template versions and migrate config to toml

2f3f3c2

This should hopefully unblock jj-vcs#3381.

algmyr added a commit to algmyr/jj that referenced this pull request Mar 31, 2024

cli: Split out ascii template versions and migrate config to toml

f53c7d6

This should hopefully unblock jj-vcs#3381.

algmyr mentioned this pull request Mar 31, 2024

cli: Split out ascii template versions and migrate config to toml #3411

Closed

4 tasks

algmyr added a commit to algmyr/jj that referenced this pull request Mar 31, 2024

cli: Split out ascii template versions and migrate config to toml

ae2793c

This should hopefully unblock jj-vcs#3381.

ilyagr previously approved these changes Apr 12, 2024

View reviewed changes

ilyagr reviewed Apr 12, 2024

View reviewed changes

martinvonz mentioned this pull request Apr 13, 2024

Shiny default graph symbols, take 2 #3499

Closed

4 tasks

thoughtpolice closed this Jul 16, 2024

thoughtpolice deleted the aseipp/push-wnxwtqnynlrr branch July 16, 2024 12:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

config: use shiny, colored graph nodes by default #3381

config: use shiny, colored graph nodes by default #3381

thoughtpolice commented Mar 27, 2024

algmyr commented Mar 27, 2024

algmyr Mar 27, 2024

yuja Mar 28, 2024

necauqua commented Mar 27, 2024

necauqua Mar 27, 2024

yuja Mar 28, 2024

yuja Mar 28, 2024

algmyr Mar 28, 2024

algmyr commented Mar 29, 2024 •

edited

Loading

ilyagr commented Apr 5, 2024

martinvonz commented Apr 5, 2024

ilyagr commented Apr 5, 2024

ilyagr commented Apr 5, 2024 •

edited

Loading

ilyagr commented Apr 5, 2024

necauqua commented Apr 5, 2024

noahmayr commented Apr 6, 2024 •

edited

Loading

algmyr commented Apr 10, 2024 •

edited

Loading

algmyr commented Apr 10, 2024

ilyagr left a comment •

edited

Loading

ilyagr Apr 12, 2024 •

edited

Loading

algmyr commented Apr 12, 2024 •

edited

Loading

ilyagr commented Apr 12, 2024 •

edited

Loading

algmyr commented Apr 12, 2024 •

edited

Loading

ilyagr commented Apr 12, 2024

yuja commented Apr 13, 2024

thoughtpolice commented Jul 16, 2024

config: use shiny, colored graph nodes by default #3381

config: use shiny, colored graph nodes by default #3381

Conversation

thoughtpolice commented Mar 27, 2024

Checklist

algmyr commented Mar 27, 2024

algmyr Mar 27, 2024

Choose a reason for hiding this comment

yuja Mar 28, 2024

Choose a reason for hiding this comment

necauqua commented Mar 27, 2024

necauqua Mar 27, 2024

Choose a reason for hiding this comment

yuja Mar 28, 2024

Choose a reason for hiding this comment

yuja Mar 28, 2024

Choose a reason for hiding this comment

algmyr Mar 28, 2024

Choose a reason for hiding this comment

algmyr commented Mar 29, 2024 • edited Loading

ilyagr commented Apr 5, 2024

martinvonz commented Apr 5, 2024

ilyagr commented Apr 5, 2024

ilyagr commented Apr 5, 2024 • edited Loading

ilyagr commented Apr 5, 2024

necauqua commented Apr 5, 2024

noahmayr commented Apr 6, 2024 • edited Loading

algmyr commented Apr 10, 2024 • edited Loading

algmyr commented Apr 10, 2024

ilyagr left a comment • edited Loading

Choose a reason for hiding this comment

ilyagr Apr 12, 2024 • edited Loading

Choose a reason for hiding this comment

algmyr commented Apr 12, 2024 • edited Loading

ilyagr commented Apr 12, 2024 • edited Loading

algmyr commented Apr 12, 2024 • edited Loading

ilyagr commented Apr 12, 2024

yuja commented Apr 13, 2024

thoughtpolice commented Jul 16, 2024

algmyr commented Mar 29, 2024 •

edited

Loading

ilyagr commented Apr 5, 2024 •

edited

Loading

noahmayr commented Apr 6, 2024 •

edited

Loading

algmyr commented Apr 10, 2024 •

edited

Loading

ilyagr left a comment •

edited

Loading

ilyagr Apr 12, 2024 •

edited

Loading

algmyr commented Apr 12, 2024 •

edited

Loading

ilyagr commented Apr 12, 2024 •

edited

Loading

algmyr commented Apr 12, 2024 •

edited

Loading