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.

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

Issue #17 Add rendering mode using blocks of columns instead of bars #18

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

zivanfi
Copy link

@zivanfi zivanfi commented Sep 10, 2022

No description provided.

@zivanfi
Copy link
Author

zivanfi commented Sep 10, 2022

Screenshots:

duviz-columns

duviz-columns-colored

@zivanfi
Copy link
Author

zivanfi commented Sep 10, 2022

Closes #17

@zivanfi zivanfi marked this pull request as draft October 4, 2022 17:49
Copy link
Owner

@soxofaan soxofaan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a first quick look at your PR

duviz.py Outdated
depth: int,
) -> List[str]:
"""Render a single node"""
def _truncate(s, maxlen):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be a top-level helper function I think

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

):
super().__init__(max_depth, size_formatter)
self.height = height
self.color_mode = color_mode
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly the non color-mode is also using color escape codes ("monochrome rendering").
Isn't it possible to have a real non-color pure-ASCII renderer, e.g. to allow usage in context where there is no color support (e.g. a text/markdown file, ...)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, will upload new revision soon (hopefully).

duviz.py Outdated
else:
prefix = ""
suffix = ""
left = "█"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this special character? can't this just be done with pure ASCII, or even just a space?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A space could also work as a separator, but it would need to be inverted. While the "█" character is outside of seven-bit ASCII, it is present in all monospace fonts I encountered, even in Linux's text-mode console, probably because it was a part of CP437 of the original IBM PC.

On the other hand, I realized that the "▁" character (which was also used in this revision) is missing from CP437 and has no corresponding glyph in Linux's text-mode console either. For this reason I have been working on a new revision that produces a less attractive but more compatible output that looks like this:

duviz-columns-ascii

I still need to do some finishing touches before I can upload this new revision though.

duviz.py Outdated
size_str,
]
else:
assert False
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assert False will not give a very helpful error message. I think this could at least be raise ValueError(text_lines)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@zivanfi
Copy link
Author

zivanfi commented Oct 16, 2022

Thanks for the review. A quick status update, since I couldn't find the time to finish the new revision, sorry about that:
I modified the code to use characters covered by most fonts and no ANSI sequences in monochrome mode but I still need to make some finishing touches.

Copy link
Author

@zivanfi zivanfi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Today I was finally able to finish the next revision, sorry about the delay. I got rid of the "▁" characters as they did not show up in the text-mode linux console and also of the ANSI sequences in non-color mode. On the other hand, I added "▒" as a background filling pattern to make the content stand out more. While this character is out of the 7-bit ASCII range, it is available in every fixed-width font I encountered probably due to it being part of IBM's CP437 encoding.

I also rebased to the latest committed revision which renamed the class attributes of the Colorizer class to start with an underscore and therefore be private by convention. My code, however, needs to use these class attributes directly as the current Colorizer API only makes sense for line-based rendering and not for column-based. Please let me know what you would consider the nicest solution to this problem.

duviz.py Outdated
depth: int,
) -> List[str]:
"""Render a single node"""
def _truncate(s, maxlen):
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

duviz.py Outdated
size_str,
]
else:
assert False
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@zivanfi zivanfi marked this pull request as ready for review October 27, 2022 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants