-
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
Implement strip_workspace_name. #3083
Conversation
b9d8d7f
to
7927b24
Compare
I'm almost certain that I have put a
It fails to find the handler, even though I've defined it:
I've also tried an |
The handlers need to be in alphabetical order. Just reverse the two lines and it should work. |
Can't believe I missed that, thanks a lot. |
3f7ac47
to
7927b24
Compare
c380a75
to
5f0fcdd
Compare
I have committed another solution. I'm not sure how picky i3 is about the |
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.
Since strtok
alters the original string, strip_workspace_name
is forced when strip_workspace_numbers
is set to no
. Additionally, anything else that uses ws->name
will have just the number (regardless of the settings), instead of the full name.
Also, when strip_workspace_numbers
is set to yes
and the workspace is not of the form n:name
, then (null)
is displayed.
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.
Just needs documentation. Aside from that LGTM.
Let me know before you merge and I will do a squash. |
This commit fixes all the edge cases I'm aware of. I noticed some unrelated behaviour with custom workspace names - if you rename a workspace so that the EDIT - I've squashed the commits together, but the relevant changes are in |
35febd5
to
9ae314d
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.
Code LGTM but I haven't tested it.
BTW, my original intention was for the swaybar_workspace
struct to have something like name
and label
rather than name
, stripped_name
and stripped_number
. You could then make render_workspace_button
use the label
field without having to check the stripping config. But I'm not fussed about this.
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.
The variable names stripped_name
and stripped_number
keep confusing me because I keep forgetting which way round they are. But that might just be me.
cd1630f
to
d0622bd
Compare
@RyanDwyer I think this commit should do it the way you suggested. |
I have managed to get There's some kind of memory bug I can't figure out. I'm allocating 2 bytes, one for the first character and one for the null terminator, and then calling |
Ready for review again, it has the same behaviour as i3 AFAIK from testing. |
7f0ffd3
to
b92bf9c
Compare
e0e8919
to
7090cd9
Compare
I have done some testing and these cases now work:
|
strip_workspace_numbers |
Dismissing my review. There's been heaps of changes since I last looked at this.
@ddevault It looks like something has gone wrong with the CI. |
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.
Implementation all works now, just (mostly) style fixes.
if (bar->config->strip_workspace_name) { | ||
free(ws->label); | ||
ws->label = malloc(len_offset + 1 * sizeof(char)); | ||
ws->label[len_offset] = '\0'; |
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 just realised you should be able to remove the three other lines in this block and this will still work, since it'll terminate the string in the correct place, even if there's leftover characters after it.
} else if (bar->config->strip_workspace_numbers) { | ||
len_offset += ws->label[len_offset] == ':'; | ||
if (strlen(ws->name) > len_offset) { | ||
free(ws->label); |
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.
You could remove this line and use strcpy
below, though it's less of a fun solution as the one above.
ecffabd
to
e16346d
Compare
e16346d
to
4bd46fb
Compare
Thanks! |
#3082.
For some reason, thestrip_workspace_name
is deemed invalid in the config file. Still trying to find out why.