Overhaul swaybar protocol handling (+fixes) #2640
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ascent12
reviewed
Sep 16, 2018
|
I'll have a look into it. |
ianyfan
changed the title
Agree |
ianyfan
force-pushed
the
swaybar
branch
2 times, most recently
from
3c4ba1d
to
72008bd
Compare
|
Addressed comments; the rewriting makes me want to squash some commits together. |
ianyfan
force-pushed
the
swaybar
branch
2 times, most recently
from
17548d2
to
926e2ab
Compare
This now uses getline to correctly handle multiple or long statuses. It also removes the struct text_protocol_state and moves its members into the status_line struct.
This now uses the getline function to receive the header, replacing read_line_buffer, which has been deleted since it is otherwise unused. Furthermore, once the protocol has been determined, the current status is handled immediately to be shown (though this has not been added for the i3bar protocol since it has not yet been rewritten to handle this).
This now correctly handles an incoming json infinite array by shifting most of the heavy listing to the json-c parser, as well as sending multiple statuses at once. It also removes the struct i3bar_protocol_state and moves its members into the status_line struct, allowing the same buffer to be used for both protocols.
|
Ready for more review |
|
This is looking pretty good. Is it still WIP? If not I'll start running it as my daily driver. |
|
Nope, should be good to go. |
ianyfan
changed the title
|
Works for me, thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Should fix #2177
This replaces the somewhat dodgy old implementation with something a bit more sensible, I think.
Both protocols now properly handle long and multiple statuses. The old i3bar handling also allowed dodgy JSON to be sent, as long as it starts with a valid array, which this fixes.
I've tried it with many different status commands, but I may have missed something so please test if you have anything unconventional. Might also want to test performance as well, seems fine but I haven't tested it thoroughly.
One annoying thing is that since everything works in the one buffer, it is not trivial to print received JSON for debugging (since we cannot simply add a null-terminator). Ideas for adding it back in:1. copy the string each time a valid array is encountered, then print it at the end; good if we assume that most of the time, only one status with be sent at a time;2. don't drop the last json object, keep it around in the buffer, then add a null-terminator at the end and print it; good if we assume that most of the time, the buffer won't in fact overflow.Thoughts? I think the second option is probably better.In the end, I didn't go with either of the above options, instead, I printed each object as they came (using some buffer shenanigans to avoid copying the string), but printed another message for what actually gets rendered.
Also leave comments about
function/variable names, they may not be the best.One thing I did realise afterwards is that
read_line_bufferskips over\rbut I haven't bothered to do that, not sure if that's wanted.Note: there is a small issue with the bar immediately exiting due to invalid JSON which I thought I had fixed but still occurs occasionally. I'm also not sure why it exits rather than just display the error message. I think it might have something to do with receiving null bytes from the stream.