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

Add new event handler to allow tracking of progress of extraction progress for individual entry #226

Merged
merged 6 commits into from
Apr 25, 2017

Conversation

gardebring
Copy link

Add new event handler to allow tracking of progress of extraction progress for individual entry. This allows for showing or logging progress of the extraction process, especially useful for large files that might take a long time to extract.

This allows us to assign an event to the reader as such:
reader.EntryExtractionProgress += Reader_EntryExtractionProgress;

And then for example use it as such:
`
private int lastPercentage = -1;
//---//

private void Reader_EntryExtractionProgress(object sender, ReaderExtractionEventArgs e)
{
var progress = (SharpCompress.Readers.ReaderProgress)e.ParamList[0];
if (progress.PercentageRead % 10 == 0 && lastPercentage != progress.PercentageRead)
{
// Do something for every 10 percent of the extraction
}
}
`

Anders Gardebring added 3 commits April 20, 2017 11:45
…rocess. This allows for showing or logging progress of the extraction process, especially useful for large files that might take a long time to extract.
…action process. This allows for showing or logging progress of the extraction process, especially useful for large files that might take a long time to extract."

This reverts commit 467fc2d.
…l entry when extracting an archive. This allows for showing or logging progress of the extraction process, especially useful for large files that might take a long time to extract.
@adamhathcock
Copy link
Owner

Why did you close the other PR to recreate it? My review comments are still the same: #225

@gardebring
Copy link
Author

Hi. The code is completely rewritten so I thought it made sense to close the existing one and create a new instead. Sorry if it was confusing, was not my intention. The review comments in #225 is not really valid anymore since the code is now rewritten using an event handler instead.

@adamhathcock
Copy link
Owner

It's the same code on the same branch. Did you mean to make this pull request from a different branch other than gardebring:master ?

@gardebring
Copy link
Author

gardebring commented Apr 25, 2017

Sorry but you lost me now.
The old code used an action as a parameter passed along. The new code use an event handler, so it is not the same code.
However, the PR do contain the old commits that were reverted.
So, first commit is old code, second is revert, third is the new code.
I can create a completely new PR with only the final change instead if you want. It will not have a functional difference, but the commit history will look nicer.

@adamhathcock
Copy link
Owner

So the code is correct as you only have your master branch. Pull Requests work per branch so it was updated when you pushed the latest changes.

My review from the other Pull Request is the same as this Pull Request as it's the same branch with the same code.

Ill just move over my comments.

}

public T Item { get; private set; }
public object[] ParamList { get; private set; }
Copy link
Owner

Choose a reason for hiding this comment

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

Make this strongly typed. I don't see a need for this as is.

Copy link
Author

Choose a reason for hiding this comment

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

The idea here was to generalize what could be passed, but am I correct in assuming that you want ReaderProgress here instead of object[] ?

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah. We'll worry about other options later. No need to over genericize at the moment I think.

{
using (Stream s = OpenEntryStream())
{
s.TransferTo(writeStream);
s.TransferTo(writeStream, (sizeTransferred, iterations) => streamListener.FireEntryExtractionProgress(Entry, sizeTransferred, iterations));
Copy link
Owner

Choose a reason for hiding this comment

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

I guess this is okay. I wouldn't have a problem just duplicating the transfer code to have listening and not listening.

Kind of doesn't make sense to have a lambda just to call the listener when the listener can just be passed in.

@@ -8,6 +8,7 @@ public interface IReader : IDisposable
{
event EventHandler<ReaderExtractionEventArgs<IEntry>> EntryExtractionBegin;
Copy link
Owner

Choose a reason for hiding this comment

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

I would dump the Begin and End and only have Progress. They can check iterations or something to see if it's started.

Copy link
Author

Choose a reason for hiding this comment

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

I sort of agree, but this would break current implementations using those event handlers. Would this be ok?

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, I break things all the time :)

I keep this library "beta" just because I haven't given things enough thought nor have the time to worry about API breaking on updates. Users should expect to at least recompile to find API breakages.

public int Iterations { get; private set; }

public int PercentageRead => (int)Math.Round(PercentageReadExact);
public double PercentageReadExact => (float)BytesTransferred / _entry.Size * 100;
Copy link
Owner

Choose a reason for hiding this comment

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

I like the way this was done

@@ -229,15 +229,18 @@ public static DateTime DosDateToDateTime(Int32 iTime)
return DosDateToDateTime((UInt32)iTime);
}

public static long TransferTo(this Stream source, Stream destination)
public static long TransferTo(this Stream source, Stream destination, Action<long, int> action = null)
Copy link
Owner

Choose a reason for hiding this comment

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

Again, I would just pass IStreamListener as an optional value like this.

Copy link
Author

Choose a reason for hiding this comment

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

As I stated, my thought process was to not introduce a new dependency into utility, but if you are fine with this I will change to that behavior.

Copy link
Owner

Choose a reason for hiding this comment

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

This Utility won't be reused and it's just for the library. I'm fine with internal dependencies.

@gardebring
Copy link
Author

Hi again and thanks for feedback. For some strange reason I could not see your code comments previously, which probably explains the confusion. I will look into them and get back to you.

@gardebring
Copy link
Author

Alright, so I believe the requested changes now has been implemented. Let me know if there is anything else :)

@adamhathcock adamhathcock merged commit bf55595 into adamhathcock:master Apr 25, 2017
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.

2 participants