Fix commands: criteria, layout, move, workspace #2392
Conversation
sway/criteria.c
Outdated
| return NULL; | ||
| } | ||
| size_t id = view->swayc->id; | ||
| int len = snprintf(NULL, 0, "%zu", id) + 1; |
There was a problem hiding this comment.
Is this the standard way to compute the string's length?
There was a problem hiding this comment.
Yes. It's not a length (as in number of chars), it's a size (as in number of bytes that will get malloc'ed) so it probably makes more sense to name it id_size. Also it should be a size_t, not an int.
There was a problem hiding this comment.
Also it should be a size_t, not an int.
*printf returns an int and so it makes sense to leave it as it is. It really is splitting hairs, though.
There was a problem hiding this comment.
fwiw I found that function after using snprintf like that myself, but for integer string length conversion we have this in common/util.c (declared in include/util.h):
int numlen(int n) {
if (n == 0) {
return 1;
}
return log10(n) + 1;
}
(It doesn't handle negative integers nor the full range of size_t, but both are fixable by just changing the prototype of the function. We only have one user though, it's a shame but it might be just as well to remove that function...)
sway/sway.5.scd
Outdated
|
|
||
| *move* left|right|up|down [<px>] | ||
| *layout* toggle [split|tabbed|stacking|splitv|splith] [split|tabbed|stacking|splitv|splith]... |
There was a problem hiding this comment.
Your code doesn't seem to handle split here
|
Can we work on this a few commands at a time? I'd rather not do a mega-PR which attempts to close #2336 all in one go. |
|
Got it, I'll add a checklist in the description |
ianyfan
changed the title
ianyfan
force-pushed
the
commands
branch
2 times, most recently
from
3272167
to
750f38c
Compare
ianyfan
changed the title
ianyfan
changed the title
|
Ready for review, though not sure if I've missed any edge cases in testing. |
sway/tree/workspace.c
Outdated
| return current_workspace; | ||
|
|
||
| char *name_cpy = strdup(name); | ||
| char *first_word = strtok(name_cpy, " "); |
There was a problem hiding this comment.
What's the reasoning for getting and checking against the first word in this function?
There was a problem hiding this comment.
I did it to consolidate logic, treating commands '[move container to] workspace next thing' as just the next workspace without having to check for each case again in their respective functions. However, thinking about it, I guess it overcorrects if the name is quoted: 'workspace "next thing"' also gets treated as the next workspace. So I'm not opposed to changing it.
sway/tree/workspace.c
Outdated
| ws = current_workspace; | ||
| } else if (strcasecmp(first_word, "back_and_forth") == 0) { | ||
| if (prev_workspace_name) { | ||
| ws = container_find(&root_container, _workspace_by_name, |
There was a problem hiding this comment.
Should this recreate the workspace if it doesn't exist anymore?
There was a problem hiding this comment.
It's recreated in the commands that call it, but I wasn't sure whether it should be recreated in all cases (there's a few other functions that call it), so I didn't.
|
Just needs the long lines split and then it will LGTM. |
|
The logic is all done now, I think; I'll have a look at splitting the long lines later, and maybe squash some commits together. |
Allow optional --no-auto-back-and-forth flag, as well as refactoring some logic
|
Thanks! |
Ref #2336
Fixes #1518
Criteria:
Commands:
Also prevents renaming workspace to "back_and_forth", as well as making it clear that they cannot be named to "special" workspace names.