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

feat: Add convenience functions for printing bold colors #2157

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

fidgetingbits
Copy link
Contributor

Previously if you wanted to use bold colors people would use a lambda or manually use 2 functions, for example:

pwndbg/commands/context.py
934:                line += f" <{pwndbg.color.bold(pwndbg.color.green(symbol))}> "
pwndbg/commands/tls.py
85:    bold_green = lambda text: pwndbg.color.bold(pwndbg.color.green(text))

These would let you just use pwndbg.color.bold_green() in the first case or from pwndbg.color import bold_green for the second case.

A tool I'm porting uses bolded colors for lots of stuff, which I'm keeping the same for now, so am using a bunch of these new functions.

@gsingh93
Copy link
Member

gsingh93 commented May 9, 2024

What if colors took optional arguments like bold=True instead? That seems like a cleaner solution to me.

@fidgetingbits
Copy link
Contributor Author

Ya, that would work too. Could then add underline, italics, etc flags that way too. I'm fine with whatever you all think is best.

@fidgetingbits
Copy link
Contributor Author

One caveat to using flags is, for things like memory coloring, is that pwndbg uses a table like this:

c = ColorConfig(
    "memory",
    [
        ColorParamSpec("stack", "yellow", "color for stack memory"),
        ColorParamSpec("heap", "blue", "color for heap memory"),
        ColorParamSpec("code", "red", "color for executable memory"),
        ColorParamSpec("data", "purple", "color for all other writable memory"),
        ColorParamSpec("rodata", "normal", "color for all read only memory"),
        ColorParamSpec("rwx", "underline", "color added to all RWX memory"),
        ColorParamSpec("gap", "bold_white", "color added for unmapped regions of memory"),
    ],
)

I've added an example "gap" entry to demonstrate someone might want to differentiate the line by using a bold color. Your suggestion wouldn't work there due to the way the generated functions work.

Copy link
Member

@disconnect3d disconnect3d left a comment

Choose a reason for hiding this comment

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

I'm okay with those changes: I am a bit worried, that we introduce more and more redundant func calls where each of them also performs a global fetch but w/e: some of these are probably cached elsewhere.

In the future, we should probably just remove all the BOLD = ... constants if no other code uses it (and it shouldn't) as well as calls to bold(black(x)) and just inline it in within the functions. Kinda a microoptimization, but some of the coloring is called many many times and it may be beneficial on the long run.

@disconnect3d
Copy link
Member

disconnect3d commented May 22, 2024

@gsingh93 if we add bold=... we introduce an unecessary if ... into all the functions, idk, it probably doesn't hurt much but I'm okay with either solution for now. So either merge this or request changes :)

Copy link
Member

@gsingh93 gsingh93 left a comment

Choose a reason for hiding this comment

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

Lets add the bold argument. An additional if statement is better than a number of redundant function calls.

@fidgetingbits
Copy link
Contributor Author

I'll switch it around

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants