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

Add ruler to histogram output #4776

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

Conversation

DimitrisVita
Copy link

@DimitrisVita DimitrisVita commented Dec 17, 2024

Your checklist for this pull request

  • I've read the guidelines for contributing to this repository
  • I made sure to follow the project's coding style
  • I've documented or updated the documentation of every function and struct this PR changes. If not so I've explained why.
  • I've added tests that prove my fix is effective or that my feature works (if possible)
  • I've updated the rizin book with the relevant information (if needed)

Detailed description

This pull request adds a vertical ruler to the horizontal histogram output in librz/cons/histogram.c. The ruler includes value ranges and uses a vertical Unicode line character if scr.utf8=true.

I've added y-axis numbering, which is computed as 255 - threshold, where the threshold is calculated for each row. Please let me know if this approach is acceptable or if any adjustments are needed. Should it be added to the interactive histogram too?

Here are two images showing the expected output:

  • Image 1: p== 50 with scr.utf8=true

peqeq50_true

  • Image 2: p== 50 with scr.utf8=false

peqeq50_false

Test plan

  1. Enable scr.utf8=true in the configuration.
  2. Generate a horizontal histogram using the p== command.
  3. Verify that the vertical ruler is displayed correctly with value ranges.
  4. Disable scr.utf8=true and repeat the test to ensure the ruler is displayed correctly without Unicode characters.

Closing issues

Addresses #129 but might need improvement

@DimitrisVita DimitrisVita marked this pull request as draft December 17, 2024 12:16
@DimitrisVita DimitrisVita marked this pull request as ready for review December 17, 2024 15:06
@XVilka
Copy link
Member

XVilka commented Dec 17, 2024

In such PRs (related to UI) please attach screenshots.

@XVilka
Copy link
Member

XVilka commented Dec 18, 2024

@DimitrisVita I think such ruler should be moved to the left, not the right. Or this can be one more histogram option

Copy link
Member

@XVilka XVilka left a comment

Choose a reason for hiding this comment

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

I also think, that while vertical ruler should be added to horizontal histogram, horitzontal ruler should be added to vertical histogram.

Rot127
Rot127 previously approved these changes Dec 18, 2024
@DimitrisVita DimitrisVita force-pushed the enhance-histogram-y-axis-numbering branch from b45d2c6 to ab9a6cf Compare December 20, 2024 18:54
@XVilka
Copy link
Member

XVilka commented Jan 4, 2025

@DimitrisVita hi, have you had some time to continue working on this?

@notxvilka
Copy link
Contributor

@DimitrisVita if you have any questions, don't hesitate to ask them on the Rizin Dev channel on our Mattermost.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RzCons UX/UI User Interface/User experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants