-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Restore workspaces to output when re-enabled #2115
Conversation
c0543ca
to
7b753bb
Compare
This has been rebased and is ready for review |
Not a fan of this approach. I'd rather have each workspace just keep a track of the outputs it has been on in a list of priority. That way, you can remove N (where N>1) monitors and plug them back in and have everything as it was. |
Sounds good to me. I'll update the PR. I'll need some help testing the implementation for that approach though since I only have two monitors. |
7b753bb
to
d1c7403
Compare
sway/tree/container.c
Outdated
@@ -433,6 +445,9 @@ void container_descendants(struct sway_container *root, | |||
func(item, data); | |||
} | |||
container_descendants(item, type, func, data); | |||
if (i < root->children->length && root->children->items[i] != item) { |
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.
Hmm, this is a bit hacky, and only works if the current container is removed. I'd rather copy the list when doing this sort of thing.
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 would prefer the PR be changed to use a double/nested for
loop to iterate the workspaces, rather than use container_descendants
. We don't need to walk the entire tree.
If we weren't going to go with that suggestion then my next preference would be to copy container_descendants
into a new function called container_descendants_safe
and go with the list copy idea.
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 would prefer the PR be changed to use a double/nested for loop to iterate the workspaces
Well, that doesn't change the fact that the inner loop will iterate over a list that will be mutated. But I agree it's a better idea.
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 switched it to using a nested for loop. Since it's only ever removes the current element, it should be fine.
list_insert(ws->output_priority, old_index, strdup(output->name)); | ||
} else if (new_index > old_index) { | ||
char *name = ws->output_priority->items[new_index]; | ||
list_del(ws->output_priority, new_index); |
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.
Do you need to free the name in the output_priority list before deleting it?
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.
No, because instead of using strdup(output->name)
like I do if it's not in the list, I'm just re-inserting the same copy on the following line. I could do the following, but I felt it was a waste to make a duplicate after freeing a duplicate:
char *name = ws->output_priority->items[new_index];
list_del(ws->output_priority, new_index);
free(name);
list_insert(ws->output_priority, old_index, strdup(output->name));
```
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.
Ah, yep. I had a derp moment. It was 12:30am local time.
sway/tree/container.c
Outdated
@@ -433,6 +445,9 @@ void container_descendants(struct sway_container *root, | |||
func(item, data); | |||
} | |||
container_descendants(item, type, func, data); | |||
if (i < root->children->length && root->children->items[i] != item) { |
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 would prefer the PR be changed to use a double/nested for
loop to iterate the workspaces, rather than use container_descendants
. We don't need to walk the entire tree.
If we weren't going to go with that suggestion then my next preference would be to copy container_descendants
into a new function called container_descendants_safe
and go with the list copy idea.
d1c7403
to
e2b2fb0
Compare
Thanks! |
This is forked from #2108 so that needs to be merged first
In #2108, it was requested that workspaces be restored to their outputs when re-enabled as discussed in swaywm/wlroots#479. This PR implements that request.