From 701fcafc70f18c6f1982a742019e875beeb258d7 Mon Sep 17 00:00:00 2001 From: Ryan Dwyer Date: Wed, 15 Aug 2018 15:14:35 +1000 Subject: [PATCH] Use list_find in more places and refactor/fix workspace prev_next functions The original purpose of this commit is to replace some for loops with list_find. But while doing this I found the workspace_prev_next_impl functions to be difficult to read and also contained a bug, so I refactored them and fixed the bug. To reproduce the bug: * Have two outputs, where the left output has workspaces 1, 2, 3 and the right output has workspaces 4, 5, 6. Make workspace 2 focused_inactive and workspace 4 focused. * Run `workspace prev`. * Previously it would visit the left output, then apply `workspace prev` to workspace 2, which focuses workspace 1. * Now it will focus the rightmost workspace on the left output (workspace 3). The refactoring I made to the workspace functions are: * Added the static keyword. * They now accept an int dir rather than bool, to avoid an unnecessary conversion. * Rather than preparing start and end variables for the purpose of iterating, just iterate everything. * Replace for loops with list_find. * Don't call workspace_output_prev_next_impl (this fixes the bug). --- common/list.c | 2 +- include/list.h | 2 +- sway/tree/layout.c | 17 +++------- sway/tree/root.c | 8 ++--- sway/tree/workspace.c | 78 ++++++++++++++++--------------------------- 5 files changed, 37 insertions(+), 70 deletions(-) diff --git a/common/list.c b/common/list.c index a3a22d8f..ee268c9a 100644 --- a/common/list.c +++ b/common/list.c @@ -77,7 +77,7 @@ int list_seq_find(list_t *list, int compare(const void *item, const void *data), return -1; } -int list_find(list_t *list, void *item) { +int list_find(list_t *list, const void *item) { for (int i = 0; i < list->length; i++) { if (list->items[i] == item) { return i; diff --git a/include/list.h b/include/list.h index 7c0e4bd2..03851a82 100644 --- a/include/list.h +++ b/include/list.h @@ -20,7 +20,7 @@ void list_qsort(list_t *list, int compare(const void *left, const void *right)); // Return index for first item in list that returns 0 for given compare // function or -1 if none matches. int list_seq_find(list_t *list, int compare(const void *item, const void *cmp_to), const void *cmp_to); -int list_find(list_t *list, void *item); +int list_find(list_t *list, const void *item); // stable sort since qsort is not guaranteed to be stable void list_stable_sort(list_t *list, int compare(const void *a, const void *b)); // swap two elements in a list diff --git a/sway/tree/layout.c b/sway/tree/layout.c index 38e14d00..2b710403 100644 --- a/sway/tree/layout.c +++ b/sway/tree/layout.c @@ -20,14 +20,7 @@ #include "log.h" static int index_child(const struct sway_container *child) { - struct sway_container *parent = child->parent; - for (int i = 0; i < parent->children->length; ++i) { - if (parent->children->items[i] == child) { - return i; - } - } - // This happens if the child is a floating container - return -1; + return list_find(child->parent->children, child); } static void container_handle_fullscreen_reparent(struct sway_container *con, @@ -125,11 +118,9 @@ struct sway_container *container_remove_child(struct sway_container *child) { } struct sway_container *parent = child->parent; - for (int i = 0; i < parent->children->length; ++i) { - if (parent->children->items[i] == child) { - list_del(parent->children, i); - break; - } + int index = index_child(child); + if (index != -1) { + list_del(parent->children, index); } child->parent = NULL; container_notify_subtree_changed(parent); diff --git a/sway/tree/root.c b/sway/tree/root.c index 79f2194e..a974a461 100644 --- a/sway/tree/root.c +++ b/sway/tree/root.c @@ -84,11 +84,9 @@ void root_scratchpad_remove_container(struct sway_container *con) { return; } con->scratchpad = false; - for (int i = 0; i < root_container.sway_root->scratchpad->length; ++i) { - if (root_container.sway_root->scratchpad->items[i] == con) { - list_del(root_container.sway_root->scratchpad, i); - break; - } + int index = list_find(root_container.sway_root->scratchpad, con); + if (index != -1) { + list_del(root_container.sway_root->scratchpad, index); } } diff --git a/sway/tree/workspace.c b/sway/tree/workspace.c index 0177068b..cd2a7a04 100644 --- a/sway/tree/workspace.c +++ b/sway/tree/workspace.c @@ -286,8 +286,8 @@ struct sway_container *workspace_by_name(const char *name) { * the end and beginning. If next is false, the previous workspace is returned, * otherwise the next one is returned. */ -struct sway_container *workspace_output_prev_next_impl( - struct sway_container *output, bool next) { +static struct sway_container *workspace_output_prev_next_impl( + struct sway_container *output, int dir) { if (!output) { return NULL; } @@ -302,27 +302,17 @@ struct sway_container *workspace_output_prev_next_impl( focus : container_parent(focus, C_WORKSPACE)); - int i; - for (i = 0; i < output->children->length; i++) { - if (output->children->items[i] == workspace) { - return output->children->items[ - wrap(i + (next ? 1 : -1), output->children->length)]; - } - } - - // Doesn't happen, at worst the for loop returns the previously active - // workspace - return NULL; + int index = list_find(output->children, workspace); + size_t new_index = wrap(index + dir, output->children->length); + return output->children->items[new_index]; } /** * Get the previous or next workspace. If the first/last workspace on an output - * is active, proceed to the previous/next output's previous/next workspace. If - * next is false, the previous workspace is returned, otherwise the next one is - * returned. + * is active, proceed to the previous/next output's previous/next workspace. */ -struct sway_container *workspace_prev_next_impl( - struct sway_container *workspace, bool next) { +static struct sway_container *workspace_prev_next_impl( + struct sway_container *workspace, int dir) { if (!workspace) { return NULL; } @@ -331,52 +321,40 @@ struct sway_container *workspace_prev_next_impl( return NULL; } - struct sway_container *current_output = workspace->parent; - int offset = next ? 1 : -1; - int start = next ? 0 : 1; - int end; - if (next) { - end = current_output->children->length - 1; + struct sway_container *output = workspace->parent; + int index = list_find(output->children, workspace); + int new_index = index + dir; + + if (new_index >= 0 && new_index < output->children->length) { + return output->children->items[index + dir]; + } + + // Look on a different output + int output_index = list_find(root_container.children, output); + new_index = wrap(output_index + dir, root_container.children->length); + output = root_container.children->items[new_index]; + + if (dir == 1) { + return output->children->items[0]; } else { - end = current_output->children->length; + return output->children->items[output->children->length - 1]; } - int i; - for (i = start; i < end; i++) { - if (current_output->children->items[i] == workspace) { - return current_output->children->items[i + offset]; - } - } - - // Given workspace is the first/last on the output, jump to the - // previous/next output - int num_outputs = root_container.children->length; - for (i = 0; i < num_outputs; i++) { - if (root_container.children->items[i] == current_output) { - struct sway_container *next_output = root_container.children->items[ - wrap(i + offset, num_outputs)]; - return workspace_output_prev_next_impl(next_output, next); - } - } - - // Doesn't happen, at worst the for loop returns the previously active - // workspace on the active output - return NULL; } struct sway_container *workspace_output_next(struct sway_container *current) { - return workspace_output_prev_next_impl(current, true); + return workspace_output_prev_next_impl(current, 1); } struct sway_container *workspace_next(struct sway_container *current) { - return workspace_prev_next_impl(current, true); + return workspace_prev_next_impl(current, 1); } struct sway_container *workspace_output_prev(struct sway_container *current) { - return workspace_output_prev_next_impl(current, false); + return workspace_output_prev_next_impl(current, -1); } struct sway_container *workspace_prev(struct sway_container *current) { - return workspace_prev_next_impl(current, false); + return workspace_prev_next_impl(current, -1); } bool workspace_switch(struct sway_container *workspace,