-
Notifications
You must be signed in to change notification settings - Fork 57
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
separated pointer and touch selection states #55
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! :)
Here are a few comments.
include/slurp.h
Outdated
int32_t x, y; | ||
int32_t anchor_x, anchor_y; | ||
struct slurp_box selection; | ||
bool has_selection; | ||
bool has_selection;*/ | ||
struct slurp_selection pointer_selection; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style: we use tabs, not spaces
include/slurp.h
Outdated
@@ -101,4 +111,5 @@ struct slurp_seat { | |||
}; | |||
|
|||
bool box_intersect(const struct slurp_box *a, const struct slurp_box *b); | |||
struct slurp_selection* seat_get_current_selection(struct slurp_seat* seat); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style: *
on the right
main.c
Outdated
seat_get_current_selection(seat); | ||
if (current_selection == NULL) { | ||
//return; | ||
current_selection = &seat->pointer_selection; //still need to track the mouse coordinates; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style: we use tabs, not spaces
render.c
Outdated
(color >> (1*8) & 0xFF) / 255.0, | ||
(color >> (0*8) & 0xFF) / 255.0); | ||
static void set_source_u32(cairo_t *cairo, uint32_t color) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style: {
on the same line as the function prototype
main.c
Outdated
state->edit_anchor = false; | ||
state->running = false; | ||
break; | ||
|
||
case XKB_KEY_space: | ||
if (!seat->has_selection) { | ||
if (!seat->pointer_selection.has_selection && | ||
!seat->touch_selection.has_selection) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style: we don't alignm, we indent with two tabs instead (to make a clear difference between the if condition and block).
render.c
Outdated
{ | ||
struct slurp_selection *current_selection = | ||
seat_get_current_selection(seat); | ||
if (current_selection == NULL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style: braces are mandatory (I see the previous code had a mistake).
render.c
Outdated
continue; | ||
if (!seat->wl_pointer) | ||
continue; | ||
if (!current_selection->has_selection) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should already be enforced by seat_get_current_selection
, right?
render.c
Outdated
seat_get_current_selection(seat); | ||
if (current_selection == NULL) | ||
continue; | ||
if (!seat->wl_pointer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the seat has a touch device but no pointer device, we should still draw the selection. So I think we should drop this check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think by this and previous comment I'll rewrite or even remove the function entirely.
b03c1bb
to
d03a8ef
Compare
render.c
Outdated
if (!seat->has_selection) { | ||
struct slurp_selection *current_selection = | ||
(seat->touch_selection.has_selection) ? | ||
(&seat->touch_selection) : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style: unnecessary parentheses
main.c
Outdated
int x = wl_fixed_to_int(surface_x) + seat->current_output->logical_geometry.x; | ||
int y = wl_fixed_to_int(surface_y) + seat->current_output->logical_geometry.y; | ||
wl_fixed_t surface_y, | ||
struct slurp_selection *current_selection) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style: indent arguments with one extra tab to be able to tell them apart from the function body
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from the remaining style nits, this is ready for being merged :)
This closes emersion#54
b3be2eb
to
f3c35f9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Added separated structs as per discussion in #50.