Skip to content
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

fix(streamable): Append read stream buffer to ongoing response for long items #372

Merged
merged 1 commit into from
Apr 14, 2023

Conversation

peanball
Copy link
Contributor

@peanball peanball commented Apr 8, 2023

Process output is streamed into a buffer. By default this buffer fits 4k characters, which is too short for image data.

With this change, if this block does not contain a stream separator, the complete block is appended to the in-progress content stored so far.

Fixes #371.

@peanball peanball force-pushed the streamable-allow-large-payloads branch 2 times, most recently from 4d6947a to 7c14349 Compare April 8, 2023 14:00
…ng items

Process output is streamed into a buffer. By default this buffer fits 4k characters, which is too short for image data.
With this change, if this block does not contain a stream separator, the complete block is appended to the in-progress content stored so far.
Fixes swiftbar#371.
@peanball peanball force-pushed the streamable-allow-large-payloads branch from 7c14349 to f6e8988 Compare April 8, 2023 14:02
@peanball
Copy link
Contributor Author

peanball commented Apr 8, 2023

Added conditional check for a future "SwiftBar 1.5.0" in https://github.com/peanball/music-swiftbar. The plugin falls back to not putting the artwork if SwiftBar is older than that. I will adapt to the correct version, in which this functionality is included.

@melonamin
Copy link
Contributor

Hi @peanball, thanks for working on this ❤️

By default this buffer fits 4k characters

Do you happen to have a reference to documentation for this? I remember debugging this at some point, but couldn't make heads or tails of it.

The change itself looks straigthforward enough 👍

@peanball
Copy link
Contributor Author

peanball commented Apr 8, 2023

Observation in the debugger. And 4K seemed reasonable as default set limit.

There is some docs on the the behaviour of reading data, which is limited to a buffer when reading from a stream, or until end of file when reading from a file. In this case the first applies. I can find the reference later if needed.

While working on the plug-in with streaming album set the image NSData kept ending up with just shy of 4K bytes, then I dug a bit deeper.

I've tested it now with streaming plugins that do more and less than 4K bytes per update and both work as expected.

Edit, found the bit of documentation I mention above: FileHandle.availableData:

Discussion
The data currently available through the receiver, up to the maximum size that can be represented by an NSData object.
If the receiver is a file, this method returns the data obtained by reading the file from the current file pointer to the end of the file. If the receiver is a communications channel, this method reads up to a buffer of data and returns it; if no data is available, the method blocks. Returns an empty data object if the end of file is reached. This method raises NSFileHandleOperationException if attempts to determine the file-handle type fail or if attempts to read from the file or channel fail.

The buffer size is not documented explicitly though.

@peanball
Copy link
Contributor Author

peanball commented Apr 8, 2023

Regarding the streamSeparator, you expect for ~~~ to show up, where the separator in the stream would actually be (^|\n)~~~(\n|$), i.e. should be in its own line. Right now this would match some string~~~here | sfimage=:cloud.fill: I think. But that's a different issue altogether.

@peanball peanball marked this pull request as draft April 8, 2023 19:18
@peanball
Copy link
Contributor Author

peanball commented Apr 8, 2023

I'm reviewing this again and I think adding it to the guard where I added it is not the right place. Debugging some more to get the right place. Will put it back to ready for review when I'm done.

It's fine after all. The guard acts when either there is only a single line in the next chunk read from the stream (what was there before), or there is no separator at all in the block.

The no separator in the block condition applies to leading and trailing separator equally and can thus be done before evaluating that condition.

.content is not set in this case on purpose, as there is potentially no complete separated set of items to display.

@peanball peanball marked this pull request as ready for review April 8, 2023 19:27
@melonamin
Copy link
Contributor

Can you please share the plugin you’re working on? I want to test it.

@peanball
Copy link
Contributor Author

peanball commented Apr 9, 2023

It's the one I mention above, https://github.com/peanball/music-swiftbar

And the swift script version of that, not the python version. I don't think I'll keep the python version around for much longer.

@melonamin melonamin merged commit 269a410 into swiftbar:main Apr 14, 2023
@peanball peanball deleted the streamable-allow-large-payloads branch April 15, 2023 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Streamable output refresh is size limited to 4k, won't allow | image=(base64data)
2 participants