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

switches to specify vertical and horizontal char densities #129

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

romangrothausmann
Copy link

This PR (originally based on v1.2a) offers switches to specify vertical and horizontal character densities and the options to change these during run-time.

Copy link
Owner

@abishekvashok abishekvashok left a comment

Choose a reason for hiding this comment

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

These changes make sense. Thank you for your time. Potentially we could add a linter to this project. I can merge this but it would be much more easier if you could fix the merge conflicts. Again, thanks for working on this.

@abishekvashok abishekvashok added the conflicts Contains merge conflicts label Aug 31, 2021
@romangrothausmann
Copy link
Author

These changes make sense. Thank you for your time. Potentially we could add a linter to this project. I can merge this but it would be much more easier if you could fix the merge conflicts. Again, thanks for working on this.

Thanks @abishekvashok, conflicts to master resolved.

Copy link
Owner

@abishekvashok abishekvashok left a comment

Choose a reason for hiding this comment

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

Tiny nits that would make me happy :)

@@ -606,6 +639,16 @@ if (console) {
case '9':
update = keypress - 48;
break;
case 'm':
space += 10;
Copy link
Owner

Choose a reason for hiding this comment

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

The indentation seems off in some places like this line for example. Can you please fix this

@@ -629,7 +672,7 @@ if (console) {
case 'r':
rainbow = 1;
break;
case 'm':
case 'M':
Copy link
Owner

Choose a reason for hiding this comment

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

Instead of modifying an existing option, can we switch the new option to use another letter or capital M if no other letters seem fit for the cause?

@@ -81,6 +81,8 @@ int xwindow = 0;
int lock = 0;
cmatrix **matrix = (cmatrix **) NULL;
int *length = NULL; /* Length of cols in each line */
int space = 50; /* vertical density parameter */
Copy link
Owner

Choose a reason for hiding this comment

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

Can we make them constants? (I am not sure if we modify them later, can you check?)

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

Successfully merging this pull request may close these issues.

2 participants