-
Notifications
You must be signed in to change notification settings - Fork 319
Add ability to enable Read Sharing when Writing to files #370
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
base: main
Are you sure you want to change the base?
Conversation
@dotnet-policy-service agree |
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.
Pull Request Overview
This PR adds a new ReadShareWhenWriting
property to the IFileAbstraction
interface that allows files to be read by other processes while being written to. The primary use case is enabling MP3 file editing while the file is being played by another application.
- Adds
ReadShareWhenWriting
boolean property toIFileAbstraction
interface - Implements the property in
LocalFileAbstraction
to controlFileShare.Read
behavior - Adds comprehensive unit tests to verify the functionality works as expected
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
src/TaglibSharp/File.cs | Core implementation adding the interface property and LocalFileAbstraction logic |
src/TaglibSharp/Tiff/Rw2/IFDReader.cs | Stub implementation throwing NotSupportedException |
src/TaglibSharp.Tests/Helpers.cs | Test helper stub implementation throwing NotSupportedException |
examples/ReadFromUri/ReadFromUri.cs | Example code stub implementation throwing NotSupportedException |
src/TaglibSharp.Tests/FileFormats/Id3BothFormatTest.cs | Unit test validating the new functionality |
Comments suppressed due to low confidence (1)
src/TaglibSharp/File.cs:196
- The field name
read_share_when_writing
uses snake_case which is inconsistent with C# naming conventions. It should bereadShareWhenWriting
or better yet, this appears to be unused since the property is implemented in LocalFileAbstraction.
public bool read_share_when_writing;
@@ -1621,7 +1627,15 @@ public LocalFileAbstraction (string path) | |||
/// </value> | |||
public Stream WriteStream => System.IO.File.Open (Name, | |||
FileMode.Open, | |||
FileAccess.ReadWrite); | |||
FileAccess.ReadWrite, | |||
ReadShareWhenWriting ? FileShare.Read : FileShare.None); |
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.
[nitpick] Consider using FileShare.None explicitly instead of the default parameter to make the intent clearer, or add a comment explaining why FileShare.None is the default behavior.
Copilot uses AI. Check for mistakes.
// keeping it open) until we tell it to stop playback and close the file. | ||
SemaphoreSlim playbackStarted = new SemaphoreSlim (0); | ||
SemaphoreSlim stopPlayback = new SemaphoreSlim (0); | ||
_ = Task.Run (async () => { |
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 discarded task should be awaited or stored in a variable to handle potential exceptions. Consider using var backgroundTask = Task.Run(...)
and awaiting it at the end of the test.
_ = Task.Run (async () => { | |
var playbackTask = Task.Run (async () => { |
Copilot uses AI. Check for mistakes.
SemaphoreSlim playbackStarted = new SemaphoreSlim (0); | ||
SemaphoreSlim stopPlayback = new SemaphoreSlim (0); | ||
_ = Task.Run (async () => { | ||
using (var fileToPlay = System.IO.File.Open (tmp_file, System.IO.FileMode.Open, System.IO.FileAccess.Read, System.IO.FileShare.ReadWrite)) { |
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.
[nitpick] Consider using FileMode.Open
constant instead of the fully qualified name for consistency with the rest of the codebase, and the variable name fileToPlay
could be more descriptive like readOnlyFileStream
.
using (var fileToPlay = System.IO.File.Open (tmp_file, System.IO.FileMode.Open, System.IO.FileAccess.Read, System.IO.FileShare.ReadWrite)) { | |
using (var readOnlyFileStream = System.IO.File.Open (tmp_file, FileMode.Open, System.IO.FileAccess.Read, System.IO.FileShare.ReadWrite)) { |
Copilot uses AI. Check for mistakes.
What
This change adds a new
LocalFileAbstraction.ReadShareWhenWriting
property. By default, this property isfalse
, resulting in no change in code flow. If a library user chooses to optionally enable it, then theWriteStream
created byLocalFileAbstraction
will useFileShare.Read
, meaning that other processes can continue to read the file while it is being written to.Currently, this change is only needed/useful when working with MP3 files. It will throw a
NotSupportedException()
if used anywhere else.Why
A common scenario when editing ID3 tags is that a user uses one application (something like Mp3tag) to edit tags, while they use another application (something like WinAmp) to simultaneously listen to the MP3 files for which tags are being edited. This scenario works perfectly fine with most application mixes like the ones mentioned above. The only "side-effect" is a slight audio glitch that occurs after the tag of a playing song gets updated.
Using the example applications mentioned above, if Mp3tag were to be implemented using
taglib-sharp
, the above scenario no longer becomes possible. A user plays a song to determine what Genre it should be. Then, while they continue to listen to the song, they attempt to set the Genre in its ID3 tag. But they can't, because the MP3 file is already in use.How Tested?