Skip to content

Commit

Permalink
Throw an exception if the index file can't be successully writter
Browse files Browse the repository at this point in the history
  • Loading branch information
@myrsloik
myrsloik committed Jul 22, 2024
1 parent 55fb7fd commit cf1fa44
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 4 deletions.
6 changes: 4 additions & 2 deletions src/audiosource.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -400,8 +400,10 @@ BestAudioSource::BestAudioSource(const std::filesystem::path &SourceFile, int Tr
if (!IndexTrack(Progress))
throw BestSourceException("Indexing of '" + Source.u8string() + "' track #" + std::to_string(AudioTrack) + " failed");

if (CacheMode == bcmAlwaysWrite || (CacheMode == bcmAuto && TrackIndex.Frames.size() >= 100))
WriteAudioTrackIndex(CachePath);
if (CacheMode == bcmAlwaysWrite || (CacheMode == bcmAuto && TrackIndex.Frames.size() >= 100)) {
if (!WriteAudioTrackIndex(CachePath))
throw BestSourceException("Failed to write index to '" + CachePath.u8string() + "' for track #" + std::to_string(AudioTrack));
}
}

AP.NumFrames = TrackIndex.Frames.size();
Expand Down
6 changes: 4 additions & 2 deletions src/videosource.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -914,8 +914,10 @@ BestVideoSource::BestVideoSource(const std::filesystem::path &SourceFile, const
if (!IndexTrack(Progress))
throw BestSourceException("Indexing of '" + Source.u8string() + "' track #" + std::to_string(VideoTrack) + " failed");

if (CacheMode == bcmAlwaysWrite || (CacheMode == bcmAuto && TrackIndex.Frames.size() >= 100))
WriteVideoTrackIndex(CachePath);
if (CacheMode == bcmAlwaysWrite || (CacheMode == bcmAuto && TrackIndex.Frames.size() >= 100)) {
if (!WriteVideoTrackIndex(CachePath))
throw BestSourceException("Failed to write index to '" + CachePath.u8string() + "' for track #" + std::to_string(VideoTrack));
}
}

if (TrackIndex.Frames[0].RepeatPict < 0)
Expand Down

1 comment on commit cf1fa44

@BoatsMcGee
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a polite notice asking you to revert this commit. Instead, submit the changes collaboratively via Draft Pull Request #60.

PR #60 includes similar code, albeit slightly earlier in execution from that of this commit. Although this commit is not a verbatim copy of my changes, it achieves a veery similar effect. Additionally, PR #60 is a draft to which I explicitly invited collaboration and review as well as stated the following:

Before completing this PR, please consider the following:
There are temporary logging statements to help demonstrate the issue and how it is resolved should you wish to debug it. These should be removed or modified to fit the logging practices of the project before merging.
...
Cache Path is not ensured to be a writable path. We should discuss where and how this could be handled. I suggest that if it fails to write to the disk then we should fallback to the default behavior. This should also be reflected in the documentation.

I also provided a summary in your Discord server stating "There is commented code to instead return the path as-is should you not wish to implement a fallback. Other concerns and improvement opportunities have been raised in the PR." This can also be seen along with other context in screenshots below:

However, instead of discussing with me on how to make the Pull Request suitable for your requirements you instead closed it without discussion. You have also halted discussion with me on your private Discord server as shown below:

PR Closed 0
PR Closed 1
PR Closed 2
PR Closed 3
Uhhh

I will reiterate/rephrase from my last message: I am not insisting that all changes in PR #60 be accepted wholesale. You are free to define and require the necessary modifications to make it befitting of the project.

The Pull Request is a Draft Pull Request to facilitate collaboration and requires agreement from both parties before merging. Disagreement with only 1 of many parts of the PR does not justify denying all of it and committing similar changes separately without proper attribution. In other words, if you did not like one of the changes introduced in PR #60, then ask for it to be removed/modified - do not take one of the other changes and submit it as if it were entirely your own.

As this is a polite notice, I will give you benefit of the doubt and assume this commit was made inadvertently without remembering where it originated. Please revert this commit and open PR #60. I will in turn allow you to make the necessary changes.

Please sign in to comment.