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

Impliment i3-style marks #1145

Merged
merged 1 commit into from
Apr 3, 2017
Merged

Impliment i3-style marks #1145

merged 1 commit into from
Apr 3, 2017

Conversation

4e554c4c
Copy link
Contributor

@4e554c4c 4e554c4c commented Apr 2, 2017

This commit adds three commands to sway: show_marks, mark and
unmark. Marks are displayed right-aligned in the window border as i3
does. Marks may be found using criteria.

Fixes #1007
I may want to add some commits that improve criteria handling before this gets merged.

list_add(view->marks, mark);
}
}
else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Join your elses on the same line as your closing braces

free(view->marks->items[index]);
list_del(view->marks, index);

if (0 == view->marks->length) {
Copy link
Contributor

Choose a reason for hiding this comment

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

No yoda conditionals please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What are Yoda conditionals?

Copy link
Member

Choose a reason for hiding this comment

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

Constant value before the variable in the comparison.
E.g: "0 equals mark's length" instead of "mark's length equals 0"

@ddevault
Copy link
Contributor

ddevault commented Apr 2, 2017

Looks good at first glance. Will review more in depth tomorrow. Nice work, thanks!

@ddevault
Copy link
Contributor

ddevault commented Apr 3, 2017

Found an issue:

  1. show_marks on
  2. mark a
  3. show_marks off
  4. show_marks on

The marks do not re-appear.

sway/border.c Outdated
strcpy(cursor + 1, mark);
*(cursor + len + 1) = ']';
cursor += len + 2;
*cursor = '\0';
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer snprintf to all of this manual string slinging

sway/border.c Outdated
size += strlen((char *)view->marks->items[i]) + 2;
}
}
marks = (size ? (char *)malloc(size + 1) : strdup("\0"));
Copy link
Contributor

Choose a reason for hiding this comment

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

You can also use snprintf to measure the final string here

sway/border.c Outdated
get_text_size(cr, config->font, &width, &height, 1, false, "%s", marks);
cairo_move_to(cr, MAX((int)tb->size.w + x - (width + 2), x + 2), y + 2);
cairo_set_source_u32(cr, colors->text);
pango_printf(cr, config->font, 1, false, "%s", marks);
Copy link
Contributor

Choose a reason for hiding this comment

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

Better yet, how about just doing this in a loop and avoiding allocation and string slinging alltogether?


swayc_t *view = get_focused_container(&root_container);
int add = false;
int toggle = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

bool add = false

include stdbool.h too

@4e554c4c
Copy link
Contributor Author

4e554c4c commented Apr 3, 2017

Looks like the show_marks issue was because of some dumb code copying. These problems should be fixed now.

This commit adds three commands to sway: `show_marks`, `mark` and
`unmark`. Marks are displayed right-aligned in the window border as i3
does. Marks may be found using criteria.

Fixes #1007
@ddevault
Copy link
Contributor

ddevault commented Apr 3, 2017

Thanks!

@ddevault ddevault merged commit 7d43a76 into swaywm:master Apr 3, 2017
@ddevault
Copy link
Contributor

ddevault commented Apr 4, 2017

Right, so this change is eligible for a bounty. Please follow up via sir@cmpwn.com to claim it, or you can redonate it to another bounty if you wish.

@4e554c4c
Copy link
Contributor Author

4e554c4c commented Apr 4, 2017

@SirCmpwn Please redonate my bounty to #892. Thanks for the review!

@4e554c4c 4e554c4c deleted the marks branch April 4, 2017 04:11
@ddevault ddevault mentioned this pull request Apr 25, 2017
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