Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cli: replace o as graph node by ● #1359

Merged
merged 3 commits into from
Mar 13, 2023
Merged

cli: replace o as graph node by ● #1359

merged 3 commits into from
Mar 13, 2023

Conversation

martinvonz
Copy link
Member

I'll update the screenshots if people like this change.

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (src/config-schema.json)
  • I have added tests to cover my changes

@martinvonz martinvonz mentioned this pull request Mar 11, 2023
Open
@yuja
Copy link
Contributor

yuja commented Mar 12, 2023

The character ● (U+25CF) has a historical problem, which may occupy 2 ascii-character width depending on environment.
https://www.unicode.org/reports/tr11-2/

It's okay for unicode graph (since some of the graph characters have this problem anyway), but better to not use non-ascii character for ascii graphs.

@martinvonz
Copy link
Member Author

The character ● (U+25CF) has a historical problem, which may occupy 2 ascii-character width depending on environment. https://www.unicode.org/reports/tr11-2/

It's okay for unicode graph (since some of the graph characters have this problem anyway), but better to not use non-ascii character for ascii graphs.

Makes sense. I've updated it to still use o for the ascii graph styles.

@martinvonz
demos: clarify command for finding operation to undo
e2a0108 
The command grepped for 'o ' and picked the third line. That was meant
to match the graph nodes only, but it also matched the 'jj co master'
line. Let's match only 'o' at the beginning of the line, and throw in
another space for good measure (since that's what we get from the new
default graph style from Sapling).
@martinvonz
cli: let graphlog provide a default node symbol
31b1434 
I'm about to make the default (non-working-copy) node symbol be a
unicode symbol, but we only want that when using a unicode graph, so
users with a terminal that doesn't support unicode can get plain ASCII
output by setting e.g. `ui.graph.style = "ascii"`.
@martinvonz
cli: replace o as graph node by ● when using unicode graph
96258b4 
@joyously found `o` confusing because it's a valid change id prefix. I
don't have much preference, but `●` seems fine. The "ascii",
"ascii-large", and "legacy" graph styles still use "o".

I didn't change `@` since it seems useful to have that match the
symbol used on the CLI. I don't think we want to have users do
something like `jj co ◎-`.
@martinvonz
Copy link
Member Author

I didn't hear any objections, so I updated the screenshots.

@martinvonz martinvonz merged commit 5c703ae into main Mar 13, 2023
@martinvonz martinvonz deleted the push-xnstvkqlqpyu branch March 13, 2023 06:21
@necauqua
Copy link
Contributor

necauqua commented Mar 22, 2023 •
edited
Loading

Too late to the party, but I've just noticed this after updating jj - and this slightly clashes mentally for people (like me) who used git-branchless:
The ● (or ◆) symbol there means "current head" and ◯/◇ is used for other commits - in jj all the commits are ● and the wc is @ which is kind of looks like ◯, so it's backwards and the intuition is super-wrong now and confusing for a 0.1s on every jj log :)

In the end those symbols should probably be customizable by a config

Also another (very slight) collision is the unicode symbol being very geometric and unicode-y and plain boring @ contrasts with it (slightly) unpleasantly as well 🤷, maybe thats just me though

@martinvonz
Copy link
Member Author

In the end those symbols should probably be customizable by a config

Yes, that makes sense.

Also another (very slight) collision is the unicode symbol being very geometric and unicode-y and plain boring @ contrasts with it (slightly) unpleasantly as well 🤷, maybe thats just me though

It's not just you - I feel the same way. I really don't want to change the @ symbol to something else, because I think it's useful to help reinforce the association between @ and the working-copy commit we have in revsets. So I guess the other choice would be to use a plain ASCII symbol like * (I think that's what Git does). The mismatch doesn't bother me enough to do that, however :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ralith Ralith Ralith approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@martinvonz @yuja @necauqua @Ralith