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

Add arrow positioning option #143

Closed
wants to merge 1 commit into from
Closed

Add arrow positioning option #143

wants to merge 1 commit into from

Conversation

natenho
Copy link
Contributor

@natenho natenho commented Oct 7, 2021 •
edited
Loading

Hello, I've just added this option to draw arrows at either different or both sides of the lines.

image
(all lines drawn from left to right)

The configuration is as simple as:

"red Marker end" = PEN (size=6 color="red" arrowsize=1 arrowposition="end");    
"red Marker start" = PEN (size=6 color="red" arrowsize=1 arrowposition="start");    
"red Marker both" = PEN (size=6 color="red" arrowsize=1 arrowposition="both");

I did it in a single commit because it was not a huge change, let me know whether it doesn't fit the contributing guidelines so I can split it up.

I was looking for a tool which mimics drawing features of Microsoft ZoomIt and gromit seems to be the right tool for that, but it lacks some drawing features that users like me would miss when migrating from Windows environment.

Another feature I want to work on is #67, for the same reason.

@bk138 bk138 self-assigned this Jan 23, 2022
bk138
bk138 requested changes Jan 23, 2022
View reviewed changes
Copy link
Owner

@bk138 bk138 left a comment

Choose a reason for hiding this comment

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

First, sorry for the delay, the last months have been pretty wild.

Then, thanks for the contribution! I have a few questions and requests, marked in the patch.

README.md
@@ -218,7 +226,7 @@ The decision which tool to use follows a simple policy:
The same logic holds for the buttons.
5. Slave device config takes precedence over master device config, which
in turn takes precedence over the fallback default config.

Copy link
Owner

Choose a reason for hiding this comment

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

Please remove unrelated whitespace changes from the commit. These make it hard to review :-) and can be done in a separate PR.

data/gromit-mpx.cfg
"red Pen" = PEN (size=5 color="red");
"blue Pen" = "red Pen" (color="blue");
"yellow Pen" = "red Pen" (color="yellow");
"green Marker" = PEN (size=6 color="green" arrowsize=1 arrowposition="end");
Copy link
Owner

Choose a reason for hiding this comment

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

What happens on green Marker" = PEN (size=6 color="green" arrowsize=1); ?

data/gromit-mpx.cfg
@@ -1,21 +1,21 @@
# Default gromit-mpx configuration
# Default gromit-mpx configuration
Copy link
Owner

Choose a reason for hiding this comment

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

same here wrt the whitespace

src/callbacks.c
@@ -1,4 +1,4 @@
/*
/*
Copy link
Owner

Choose a reason for hiding this comment

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

please remove unrelated whitespace changes.

src/callbacks.c
@@ -129,9 +129,9 @@ void on_monitors_changed ( GdkScreen *screen,


data->default_pen = paint_context_new (data, GROMIT_PEN,
data->red, 7, 0, 1, G_MAXUINT);
data->red, 7, 0, GROMIT_ARROW_AT_NONE, 1, G_MAXUINT);
Copy link
Owner

Choose a reason for hiding this comment

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

These should probably be GROMIT_ARROW_AT_END in order to retain the original configless behaviour.

src/drawing.c
@@ -2,14 +2,6 @@
#include <math.h>
#include "drawing.h"

typedef struct
Copy link
Owner

Choose a reason for hiding this comment

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

Why did this one get removed?

src/drawing.c
@@ -31,21 +23,61 @@ void draw_line (GromitData *data,
cairo_set_line_width(devdata->cur_context->paint_ctx, data->maxwidth);
cairo_set_line_cap(devdata->cur_context->paint_ctx, CAIRO_LINE_CAP_ROUND);
cairo_set_line_join(devdata->cur_context->paint_ctx, CAIRO_LINE_JOIN_ROUND);

Copy link
Owner

Choose a reason for hiding this comment

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

Lotsa whitespace changes that make the real changes harder to spot. Please remove those.

src/drawing.c
draw_arrow (data, device, arrow_point->x, arrow_point->y, width, direction);

break;
case GROMIT_ARROW_AT_BOTH:
Copy link
Owner

Choose a reason for hiding this comment

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

Please add a comment about why these are empty for future code readers.

src/main.c
@@ -1,4 +1,4 @@
/*
/*
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove the whitespace changes :-)

src/drawing.c
@@ -153,23 +185,33 @@ void coord_list_free (GromitData *data,
devdata->coordlist = NULL;
}

void cleanup_context(GromitPaintContext *context)
Copy link
Owner

Choose a reason for hiding this comment

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

That sounds like it's free()-ing the whole context ;-) Maybe clear_context and some comment?

Copy link
Owner

Choose a reason for hiding this comment

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

Actually, just inlining this would be most clear, I think.

@bk138 bk138 assigned natenho and unassigned bk138 Jan 23, 2022
@natenho
Copy link
Contributor Author

natenho commented Jan 24, 2022 •
edited
Loading

@bk138 thanks for your review! Actually I did a lot of changes since this PR that are in my fork, which include some cool features (based on MIcrosoft ZoomIt) like rectangles, lines, ellipses, and some other some fixes (like fixing some memory leaks I introduced). Currently I'm using my "version of" gromit-mpx in a daily basis and everything is fine!

I'll try to sync everything back here, but it will probably take a while. Regarding the whitespaces, well, vscode did that. lol. I'll get them fixed aswell.

@bk138
Copy link
Owner

bk138 commented Jan 24, 2022

@bk138 thanks for your review! Actually I did a lot of changes since this PR that are in my fork, which include some cool features (based on MIcrosoft ZoomIt) like rectangles, lines, ellipses, and some other some fixes (like fixing some memory leaks I introduced). Currently I'm using my "version of" gromit-mpx in a daily basis and everything is fine!

I'll try to sync everything back here, but it will probably take a while. Regarding the whitespaces, well, vscode did that. lol. I'll get them fixed aswell.

Super nice - Looking forward to them!

@bk138
Copy link
Owner

bk138 commented Mar 8, 2024

Closing in favour of #195.

@bk138 bk138 closed this Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bk138 bk138 bk138 requested changes

Assignees

@natenho natenho

Labels
answer-needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@natenho @bk138