lloydsargent / BlackRaccoon

IOS FTP Client Code
102 stars 37 forks source link

Added canceled flag #19

Closed colinhumber closed 11 years ago

colinhumber commented 11 years ago

Added a wasCanceled flag to let consumers check to see if a request was finished due to a cancel.

lloydsargent commented 11 years ago

If a user cancels a request, would they not know that? I'm not sure I see what the advantage is by adding the BOOL here.

colinhumber commented 11 years ago

When the user cancels a request, the stream info object just closes the request and tells the delegate that the request was completed, but that's not entirely accurate. Completed implies it finished, but in this case the request didn't fail, nor did it finish. In an app I'm working on I can have multiple requests. If a user cancels one request, it's convenient to just check the cancel flag on the request in the requestCompleted: callback than to keep track of what request was canceled. I think the consumer should be able to query the request for it's state than to have to track it externally.

On Sat, Jun 29, 2013 at 12:48 PM, Lloyd Sargent notifications@github.comwrote:

If a user cancels a request, would they not know that? I'm not sure I see what the advantage is by adding the BOOL here.

— Reply to this email directly or view it on GitHubhttps://github.com/lloydsargent/BlackRaccoon/pull/19#issuecomment-20235014 .

Colin

lloydsargent commented 11 years ago

I understand WHY you did it, however I'm not sure it is reason to push application information (that doesn't serve or assist Black Raccoon) into the class.

Let me suggest an alternative. A user dictionary. Then if you want to keep information per request (whatever it might be), then you can set that value. This would allow for a more "stateless" approach and people who don't want to use it don't have to use it. It also has the advantage that should people want to add MORE convenience variables, we don't end up with a slew of BOOLs, NSStrings, etc. that a majority of uses will never use.

I understand this is probably not the solution you were thinking of, but looking forward it is the only one that I see that will satisfy future requests AND it does follow a pattern Apple has established. It also compartmentalizes application data from class data.

colinhumber commented 11 years ago

I see the Black Raccoon classes as being very similar to NSOperations. Following Apple's design of the NSOperation class ( http://developer.apple.com/library/mac/#documentation/Cocoa/Reference/NSOperation_class/Reference/Reference.html), the current state of an operation is exposed as read-only properties which are directly affected by calling the start and cancel methods on an operaton. Since there are start and cancel methods on the BR objects, I would suggest exposing properties that show the current state of a request. This does follow Apple's guideline for operation-based classes. The current state of a request isn't application data...it is class data.

The userInfo property that Apple has on some of their classes are there for users to added app-specific logic to an object that doesn't have anything to do with the object itself. NSNotification is a prime example of that. A notification has it's own internal state and attributes, but allow devs to append their own custom state.

On Sat, Jun 29, 2013 at 2:46 PM, Lloyd Sargent notifications@github.comwrote:

I understand WHY you did it, however I'm not sure it is reason to push application information (that doesn't serve or assist Black Raccoon) into the class.

Let me suggest an alternative. A user dictionary. Then if you want to keep information per request (whatever it might be), then you can set that value. This would allow for a more "stateless" approach and people who don't want to use it don't have to use it. It also has the advantage that should people want to add MORE convenience variables, we don't end up with a slew of BOOLs, NSStrings, etc. that a majority of uses will never use.

I understand this is probably not the solution you were thinking of, but looking forward it is the only one that I see that will satisfy future requests AND it does follow a pattern Apple has established. It also compartmentalizes application data from class data.

— Reply to this email directly or view it on GitHubhttps://github.com/lloydsargent/BlackRaccoon/pull/19#issuecomment-20236891 .

Colin

lloydsargent commented 11 years ago

Again, I have to disagree. This is state information required by YOUR application: I can (and have) written BR code that doesn't require the cancel boolean. Nor is the NSOperation class displaying properties that are convenience operators - these are values required by NSOperation and made available to the user. That is very different than carrying along a value set by the application and then used by the application.

Your cancel flag, in your own words, exists for this sole reason:

In an app I'm working on I can have multiple requests. If a user cancels one request, 
it's convenient to just check the cancel flag on the request in the 
requestCompleted: callback than to keep track of what request was canceled.

Again, this falls back to the pattern that Apple includes in NSNotification - it is, again in your words, for application specific information. Exactly how you described it to me.

Examination of your change also shows that BR does not depend on this value for correct operation: the value can be YES or NO and it changes nothing. This differs from NSOperation where the values that are available are intrinsic to the correct operation of the threads.

An NSMutableDictionary is a good compromise, follows Apple's usage, and allows for future expansion.

colinhumber commented 11 years ago

Yes, not every application requires knowing the state of a request, the same way that not every application that uses NSOperations requires knowing if the operation is executing or has been canceled. Just because an application doesn't inspect isCanceled or isExecuting on an NSOperation doesn't mean they aren't valuable. That class is just exposing the internal state of an operation so consumers can use it based on the application needs.

The flag I was proposing isn't set by the application. It's set internally by the BRStreamInfo class when cancel is called. This is EXACTLY how NSOperations work. You call cancel on an NSOperation and the internal isCanceled flag is set to YES. The application doesn't set the flag. It's set internally by calling the cancel method. That way any consumer of the operation can check if the operation was canceled. That's what I'm proposing. Add a canceled flag to BRRequest that is set automatically by calling the cancel method that is already exposed on BRRequest. This mirrors what NSOperation does.

The reason I added this flag is because, unlike NSOperation, BRRequest does not tell me if a request was canceled. My application has to keep track of what requests were canceled. I should be able to query a request to see what it's state is. Again, exactly what NSOperation allows me to do.

The change I proposed isn't letting me set wasCanceled to YES/NO by a consumer. It allows me to view the current state of a request based on whether cancel, start, etc. was already called which does follow Apple's usage of such properties. How would adding a user dictionary help in this case? My application isn't going to tell the request it was canceled by saying:

request.userInfo = @{ "wasCanceled" : @YES };

It's the request's job to let me query that information for myself.

I think we are looking at this from two different perspectives. Can you send a code sample on how you envision the user dictionary working with the cancel flag? Keep in mind, I am not asking to have a property that I can set on a request (eg. request.wasCanceled = YES). I'm asking for the property to be read-only so I can just inspect it, not set it. That is already done by calling [request cancel];

On Sat, Jun 29, 2013 at 3:26 PM, Lloyd Sargent notifications@github.comwrote:

Again, I have to disagree. This is state information required by YOUR application: I can (and have) written BR code that doesn't require the cancel boolean. Nor is the NSOperation class displaying properties that are convenience operators - these are values required by NSOperation and made available to the user. That is very different than carrying along a value set by the application and then used by the application.

Your cancel flag, in your own words, exists for this sole reason:

In an app I'm working on I can have multiple requests. If a user cancels one request, it's convenient to just check the cancel flag on the request in the requestCompleted: callback than to keep track of what request was canceled.

Again, this falls back to the pattern that Apple includes in NSNotification - it is, again in your words, for application specific information. Exactly how you described it to me.

Examination of your change also shows that BR does not depend on this value for correct operation: the value can be YES or NO and it changes nothing. This differs from NSOperation where the values that are available are intrinsic to the correct operation of the threads.

An NSMutableDictionary is a good compromise, follows Apple's usage, and allows for future expansion.

— Reply to this email directly or view it on GitHubhttps://github.com/lloydsargent/BlackRaccoon/pull/19#issuecomment-20237485 .

Colin

colinhumber commented 11 years ago

By the way, I'm not saying a user dictionary isn't a good idea, just not in this instance. In fact, quite the opposite. I think there are a ton of classes that can benefit from having the ability to read/write arbitrary data attached to them without having to resort to associated properties.

On Sat, Jun 29, 2013 at 3:41 PM, Colin Humber colinhumber@gmail.com wrote:

Yes, not every application requires knowing the state of a request, the same way that not every application that uses NSOperations requires knowing if the operation is executing or has been canceled. Just because an application doesn't inspect isCanceled or isExecuting on an NSOperation doesn't mean they aren't valuable. That class is just exposing the internal state of an operation so consumers can use it based on the application needs.

The flag I was proposing isn't set by the application. It's set internally by the BRStreamInfo class when cancel is called. This is EXACTLY how NSOperations work. You call cancel on an NSOperation and the internal isCanceled flag is set to YES. The application doesn't set the flag. It's set internally by calling the cancel method. That way any consumer of the operation can check if the operation was canceled. That's what I'm proposing. Add a canceled flag to BRRequest that is set automatically by calling the cancel method that is already exposed on BRRequest. This mirrors what NSOperation does.

The reason I added this flag is because, unlike NSOperation, BRRequest does not tell me if a request was canceled. My application has to keep track of what requests were canceled. I should be able to query a request to see what it's state is. Again, exactly what NSOperation allows me to do.

The change I proposed isn't letting me set wasCanceled to YES/NO by a consumer. It allows me to view the current state of a request based on whether cancel, start, etc. was already called which does follow Apple's usage of such properties. How would adding a user dictionary help in this case? My application isn't going to tell the request it was canceled by saying: request.userInfo = @{ "wasCanceled" : @YES }; It's the request's job to let me query that information for myself.

I think we are looking at this from two different perspectives. Can you send a code sample on how you envision the user dictionary working with the cancel flag? Keep in mind, I am not asking to have a property that I can set on a request (eg. request.wasCanceled = YES). I'm asking for the property to be read-only so I can just inspect it, not set it. That is already done by calling [request cancel];

On Sat, Jun 29, 2013 at 3:26 PM, Lloyd Sargent notifications@github.comwrote:

Again, I have to disagree. This is state information required by YOUR application: I can (and have) written BR code that doesn't require the cancel boolean. Nor is the NSOperation class displaying properties that are convenience operators - these are values required by NSOperation and made available to the user. That is very different than carrying along a value set by the application and then used by the application.

Your cancel flag, in your own words, exists for this sole reason:

In an app I'm working on I can have multiple requests. If a user cancels one request, it's convenient to just check the cancel flag on the request in the requestCompleted: callback than to keep track of what request was canceled.

Again, this falls back to the pattern that Apple includes in NSNotification - it is, again in your words, for application specific information. Exactly how you described it to me.

Examination of your change also shows that BR does not depend on this value for correct operation: the value can be YES or NO and it changes nothing. This differs from NSOperation where the values that are available are intrinsic to the correct operation of the threads.

An NSMutableDictionary is a good compromise, follows Apple's usage, and allows for future expansion.

— Reply to this email directly or view it on GitHubhttps://github.com/lloydsargent/BlackRaccoon/pull/19#issuecomment-20237485 .

Colin

Colin

lloydsargent commented 11 years ago

Well, I had a long response that I just lost thanks to a crash in Safari. You are lucky :)

We are indeed looking at this from two different perspectives. I'm looking at this from a maintenance perspective - what is the least amount that has to change the effects the most options. I can easily see others arguing just as succinctly, just as passionately for their own convenience variables. Hence the user dictionary.

From your perspective, I'm sure it looks like I'm being an unreasonable - but the isCancelled is not merely a convenience value: it is a reflection of the thread state. It really isn't very similar at all to your variable which is an application convenience variable (it indicates that it was cancelled, but not that it IS cancelled - if I'm making myself clear).

The user dictionary is the only solution that makes sense in the long run - otherwise we will end up with a plethora of convenience variables.

I hope that you can agree to this compromise.

colinhumber commented 11 years ago

It was probably not named correctly, and should be isCanceled, not wasCanceled to reflect the fact that the response is, in fact, canceled.

What I still don't understand, though, is how a user dictionary would even work in this situation. I am not setting isCanceled from my application. It's set by the BRStreamInfo. Can you post some code as to how you see the user dictionary working in this case? From what I can see it doesn't make any sense to use if I wanted to see if the request was canceled, or to query any additional state about the request. The dictionary only makes sense to me if I wanted to, somewhere in my application, add some additional metadata to the request to identify it later, etc.

I just don't understand how the dictionary will work in this case.

lloydsargent commented 11 years ago

For example, assume the following code:

[uploadFile.userDictionary setObject: @"cancel" forKey: @"cancel"];
[uploadFile cancelRequest];

Then in the completion:

if ([uploadFile.userDictionary objectForKey: @"cancel"])
    // task was cancelled, deal with it

From my point of view, which is not your application, the "cancel" variable is YOUR variable. It is no better (or worse) than some BOOL you keep in your code. It IS meta-data to me as BR neither uses it nor depends on it.

colinhumber commented 11 years ago

Using a dictionary for that purpose means that anyone that wants to use BR needs to track the state of a request themselves, whether its started, is executing, has finished, is canceled. I just don't understand why the request shouldn't expose information about ITS state instead of requiring a user to track the state themselves.

The "cancel" variable is NOT my variable. It's the request exposing some state about itself. It's provided to make it convenient for people to use this library. Sure, it doesn't depend on it...it's the library exposing bits of its internal state to make the API easier to use.

You are already doing this with some properties in BRRequest. bytesSent and totalBytesSent aren't used anywhere in BRRequest, so BRRequest doesn't depend on or use those properties. They just pass along information from the BRStreamInfo, so by doing that you're allowing consumers to understand some of the internal state of the underlying system.

If anything, calling cancelRequest should either set an internal cancel flag that is exposed through the API so that when requestCompleted: is called, developers can inspect to see if the request was actually completed or was canceled (see https://github.com/pokeb/asi-http-request/blob/master/Classes/ASIHTTPRequest.h), do what NSURLConnection does and not call any delegate methods at all since a canceled request != a completed request, or have a requestCanceled: callback.

lloydsargent commented 11 years ago

Perhaps I'm confused, but isn't "bytesSent" uses bytesThisIteration which, in fact, is used by BR. Likewise, totalBytesSent comes from the internal variable bytesTotal which is used to generate the percentage download (a feature requested by White Raccoon and not implemented when I did my port). These are not values that are pushed from the user application into the class. However, you disagree. That's okay.

Let me clarify once and for all: it is the user who defines the cancel request. For the your purposes you wish the request to carry this information. In no shape way or form does BR use this value. It is therefore user metadata. Nor is it state information: it is a flag set by the user executing the cancelRequest, but does not represent any internal state.

THAT SAID: If you wish to implement your boolean, please name it correctly (I suggest userCanceled or userCancelRequest). Likewise implement the user dictionary for future convenience variables. We're not going to clutter the code up with everyone's suggested convenience variables.

Future convenience variables will go in the user dictionary.

colinhumber commented 11 years ago

The point I was making was that properties bytesSent and totalBytesSent, which was exposed in BRRequest.h are never used inside the library itself. They are exposed so consumers can get access to the internal state of a request. The internal state of the stream info is used inside BRRequest. Obviously they are not pushed from the user application to the class. They are there to give the user the ability to determine the state from their own application which is what I wanted to see in BRRequest, the ability to see if a request had been canceled.

We are on the same page that the user defines the cancel request. But in the same way that BR exposes to the user certain information about the current state of a request (ie. bytesSent and totalBytesSent, which are NOT used by BR but just expose information from the BRStreamInfo class (bytesThisIteration and bytesTotal)), a user should be able to determine if a request has been canceled. This is not user metadata, but a depiction of the internal state. How else are you supposed to determine if, when requestCompleted: is called now, the request ACTUALLY completed or was canceled? There is no way to do it!

On NSOperation, if a user called [operation cancel]; does that mean there should be a userCanceled property? Of course not. The class provides a property allowing the consumer to check on the CURRENT state of the operation, so you can [operation isCanceled]. That's good API design. If you are concerned with the cluttering of convenience variables, I would suggest dropping bytesSent and totalBytes sent which are NOT used by BR and just leave the bytesTotal and bytesThisIteration properties on BRStreamInfo which ARE used cause those are definitely convenience variables.

Obviously we are not going to agree on this. I will change the name of my variable to isCanceled on my own fork, as that is a more appropriate name but I won't bother submitting a pull request.

Colin

On Sunday, June 30, 2013 at 5:35 PM, Lloyd Sargent wrote:

Perhaps I'm confused, but isn't "bytesSent" using bytesThisIteration which, in fact, is used by BR. Likewise, totalBytesSent comes from the internal variable bytesTotal which is used to generate the percentage download (a feature requested by White Raccoon and not implemented when I did my port). These are not values that are pushed from the user application into the class. However, you disagree. That's okay. Let me clarify once and for all: it is the user who defines the cancel request. For the your purposes you wish the request to carry this information. In no shape way or form does BR use this value. It is therefore user metadata. Nor is it state information: it is a flag set by the user executing the cancelRequest, but does not represent any internal state. THAT SAID: If you wish to implement your boolean, please name it correctly (I suggest userCanceled or userCancelRequest). Likewise implement the user dictionary for future convenience variables. We're not going to clutter the code up with everyone's suggested convenience variables. Future convenience variables will go in the user dictionary.

— Reply to this email directly or view it on GitHub (https://github.com/lloydsargent/BlackRaccoon/pull/19#issuecomment-20257804).

lloydsargent commented 11 years ago

Yes, I AM concerned about it. I spent a lot of time and effort in turning White Raccoon into Black Raccoon. I'm also very concerned about maintenance of the code and assuring that I future-proof it.

Backing up to your original description: "In an app I'm working on I can have multiple requests. If a user cancels one request, it's convenient to just check the cancel flag on the request in the requestCompleted: callback than to keep track of what request was canceled." It seems more than reasonable to implement a method that allows the request to carry with it information such as userCancelRequest.

Whether you wish to carry on with BR is, of course, up to you.

On Jul 1, 2013, at 12:50 , Colin Humber notifications@github.com wrote:

The point I was making was that properties bytesSent and totalBytesSent, which was exposed in BRRequest.h are never used inside the library itself. They are exposed so consumers can get access to the internal state of a request. The internal state of the stream info is used inside BRRequest. Obviously they are not pushed from the user application to the class. They are there to give the user the ability to determine the state from their own application which is what I wanted to see in BRRequest, the ability to see if a request had been canceled.

We are on the same page that the user defines the cancel request. But in the same way that BR exposes to the user certain information about the current state of a request (ie. bytesSent and totalBytesSent, which are NOT used by BR but just expose information from the BRStreamInfo class (bytesThisIteration and bytesTotal)), a user should be able to determine if a request has been canceled. This is not user metadata, but a depiction of the internal state. How else are you supposed to determine if, when requestCompleted: is called now, the request ACTUALLY completed or was canceled? There is no way to do it!

On NSOperation, if a user called [operation cancel]; does that mean there should be a userCanceled property? Of course not. The class provides a property allowing the consumer to check on the CURRENT state of the operation, so you can [operation isCanceled]. That's good API design. If you are concerned with the cluttering of convenience variables, I would suggest dropping bytesSent and totalBytes sent which are NOT used by BR and just leave the bytesTotal and bytesThisIteration properties on BRStreamInfo which ARE used cause those are definitely convenience variables.

Obviously we are not going to agree on this. I will change the name of my variable to isCanceled on my own fork, as that is a more appropriate name but I won't bother submitting a pull request.

Colin

On Sunday, June 30, 2013 at 5:35 PM, Lloyd Sargent wrote:

Perhaps I'm confused, but isn't "bytesSent" using bytesThisIteration which, in fact, is used by BR. Likewise, totalBytesSent comes from the internal variable bytesTotal which is used to generate the percentage download (a feature requested by White Raccoon and not implemented when I did my port). These are not values that are pushed from the user application into the class. However, you disagree. That's okay. Let me clarify once and for all: it is the user who defines the cancel request. For the your purposes you wish the request to carry this information. In no shape way or form does BR use this value. It is therefore user metadata. Nor is it state information: it is a flag set by the user executing the cancelRequest, but does not represent any internal state. THAT SAID: If you wish to implement your boolean, please name it correctly (I suggest userCanceled or userCancelRequest). Likewise implement the user dictionary for future convenience variables. We're not going to clutter the code up with everyone's suggested convenience variables. Future convenience variables will go in the user dictionary.

— Reply to this email directly or view it on GitHub (https://github.com/lloydsargent/BlackRaccoon/pull/19#issuecomment-20257804).

— Reply to this email directly or view it on GitHub.

colinhumber commented 11 years ago

The point I was trying to make is that it shouldn't be up to the developer to determine what the state of a request is. The request itself should be queryable to determine what the current state is. All the information about the current state is already in BRRequest, so why not expose it to make it easier for developers? Having to tell the request that userCancelRequest = YES is incredibly error prone since a developer would have to remember to set that when they call [request cancelRequest]. Like NSOperation, you should be able to just ask the BRRequest what it's state is which is what the original request was all about. As a consumer of the library, I have to do more work to get information that should be readily available from the request classes.

colinhumber commented 11 years ago

As a side note, your work on BR has saved me a ton of time, so please don't take this debate as anything but an intellectual argument. I do apologize if it came across as overly critical.

lloydsargent commented 11 years ago

Two points you have failed to consider. I keep bringing them up but I'm not sure you properly appreciate them. The state in NSOperation is an INTERNAL state machine required for proper operation of threads (actually, tasks in RTOS's have similar states, something I'm very familiar). What NSOperation is expose the internal state machine. This is no way similar to BR in any shape, way or form.

BR takes a cancel request from the user and then sets a flag. This flag is used to "gum the works" as it were - it doesn't truly represent so much as a state so much as a "bailing out". It would be far more similar to this:

while (!userCancelFlag)
{
    // perform thread operations
}

than a reflection of the internal state (which is opaque for the most part). In other words, you are projecting a state where none exists (the user THINKS there is a state, but in reality, not so much). It is an illusory state at best. The fact that it can be pushed back (and out) of BR means it is not truly a state of BR, but a state of the application (or a state the application THINK's BR is in).

This is where we fundamentally disagree. But it is because you are looking at it as if it truly is a state when, in fact, it really isn't. There is no NSStreamEventCancelled and that is where it all falls apart. It is something that I forced into the gears of the system to abort the transfer. Fortunately, NSStream doesn't care :)

What I plan on implementing is the following: a category that users can modify. In this category will be a new method [request cancelRequestWithFlag] ... rename as you wish. To query it will be request.userCancelFlag. Rename as you wish.

This will give you what you want (whilst adding another flavor of cancel). This is, I think, a good compromise.

It also will allow users to have the request carry along with it as much (or little) information as required.

On Jul 1, 2013, at 16:04 , Colin Humber notifications@github.com wrote:

The point I was trying to make is that it shouldn't be up to the developer to determine what the state of a request is. The request itself should be queryable to determine what the current state is. All the information about the current state is already in BRRequest, so why not expose it to make it easier for developers? Having to tell the request that userCancelRequest = YES is incredibly error prone since a developer would have to remember to set that when they call [request cancelRequest]. Like NSOperation, you should be able to just ask the BRRequest what it's state is which is what the original request was all about. As a consumer of the library, I have to do more work to get information that should be readily available from the request classes.

— Reply to this email directly or view it on GitHub.

lloydsargent commented 11 years ago

Yes, it took a ton of time to turn White Raccoon into Black Raccoon :) NSStream class is not without its darker sides (like the "hack" to fix the directories).

I'm glad that this project has been helpful. My goal is to keep it usable, which means I have to question every request: where does this fit in the big picture? For example, one request was to have it save to files; I realized that this created a problem and thus made it more the users responsibility. This actually makes for a far more flexible system.

The new category system will also enhance things: you will be able to cancel and it will carry that information along. But it also allows for future convenience variables without having to hack the core of it every time.

On Jul 1, 2013, at 16:08 , Colin Humber notifications@github.com wrote:

As a side note, your work on BR has saved me a ton of time, so please don't take this debate as anything but an intellectual argument. I do apologize if it came across as overly critical.

— Reply to this email directly or view it on GitHub.

colinhumber commented 11 years ago

I understand the complexity of NSOperation. I was mostly just using it as an example of objects exposing state information (eg. running, canceled, finished, etc.) I spent a bunch of time looking through how the BRStreamInfo operates in regards to cancelation before adding the flags. All that I needed was the ability to determine why the request was completed since a canceled request is not the same as a completed request. Since there were already a requestComplete: and requestFailed:, it is quite possible to have a system where a request could be canceled by something other than direct user input.

Ultimately, this is your library and that's something I definitely want to respect. I've had my fair share of debates with some of my own projects, and if nothing else, it's interesting to get differing perspectives.

On Mon, Jul 1, 2013 at 5:10 PM, Lloyd Sargent notifications@github.comwrote:

Two points you have failed to consider. I keep bringing them up but I'm not sure you properly appreciate them. The state in NSOperation is an INTERNAL state machine required for proper operation of threads (actually, tasks in RTOS's have similar states, something I'm very familiar). What NSOperation is expose the internal state machine. This is no way similar to BR in any shape, way or form.

BR takes a cancel request from the user and then sets a flag. This flag is used to "gum the works" as it were - it doesn't truly represent so much as a state so much as a "bailing out". It would be far more similar to this:

while (!userCancelFlag) { // perform thread operations }

than a reflection of the internal state (which is opaque for the most part). In other words, you are projecting a state where none exists (the user THINKS there is a state, but in reality, not so much). It is an illusory state at best. The fact that it can be pushed back (and out) of BR means it is not truly a state of BR, but a state of the application (or a state the application THINK's BR is in).

This is where we fundamentally disagree. But it is because you are looking at it as if it truly is a state when, in fact, it really isn't. There is no NSStreamEventCancelled and that is where it all falls apart. It is something that I forced into the gears of the system to abort the transfer. Fortunately, NSStream doesn't care :)

What I plan on implementing is the following: a category that users can modify. In this category will be a new method [request cancelRequestWithFlag] ... rename as you wish. To query it will be request.userCancelFlag. Rename as you wish.

This will give you what you want (whilst adding another flavor of cancel). This is, I think, a good compromise.

It also will allow users to have the request carry along with it as much (or little) information as required.

On Jul 1, 2013, at 16:04 , Colin Humber notifications@github.com wrote:

The point I was trying to make is that it shouldn't be up to the developer to determine what the state of a request is. The request itself should be queryable to determine what the current state is. All the information about the current state is already in BRRequest, so why not expose it to make it easier for developers? Having to tell the request that userCancelRequest = YES is incredibly error prone since a developer would have to remember to set that when they call [request cancelRequest]. Like NSOperation, you should be able to just ask the BRRequest what it's state is which is what the original request was all about. As a consumer of the library, I have to do more work to get information that should be readily available from the request classes.

— Reply to this email directly or view it on GitHub.

— Reply to this email directly or view it on GitHubhttps://github.com/lloydsargent/BlackRaccoon/pull/19#issuecomment-20316963 .

Colin

colinhumber commented 11 years ago

Agreed, NSStream is a real pain at times. Cheers for creating one of the few FTP libraries with iOS support!

On Mon, Jul 1, 2013 at 5:17 PM, Lloyd Sargent notifications@github.comwrote:

Yes, it took a ton of time to turn White Raccoon into Black Raccoon :) NSStream class is not without its darker sides (like the "hack" to fix the directories).

I'm glad that this project has been helpful. My goal is to keep it usable, which means I have to question every request: where does this fit in the big picture? For example, one request was to have it save to files; I realized that this created a problem and thus made it more the users responsibility. This actually makes for a far more flexible system.

The new category system will also enhance things: you will be able to cancel and it will carry that information along. But it also allows for future convenience variables without having to hack the core of it every time.

On Jul 1, 2013, at 16:08 , Colin Humber notifications@github.com wrote:

As a side note, your work on BR has saved me a ton of time, so please don't take this debate as anything but an intellectual argument. I do apologize if it came across as overly critical.

— Reply to this email directly or view it on GitHub.

— Reply to this email directly or view it on GitHubhttps://github.com/lloydsargent/BlackRaccoon/pull/19#issuecomment-20317246 .

Colin

lloydsargent commented 11 years ago

Thanks!

Okay, here is what I'm doing. Creating a category called BRRequest+_UserData which will look like the following:

.h @interface BRRequest (_UserData) @property BOOL userCancelRequest; @property NSString *uuid; @end

.m

- (void) setUserCancelRequest: (BOOL) value
{
    if (value)
        [self.userDictionary setObject: @"cancel" forKey: @"cancel"];
    else
        [self.userDictionary removeObjectForKey: @"cancel"];
}

- (BOOL) userCancelRequest
{
    return [self.userDictionary objectForKey: @"cancel"] != nil;
}

- (void) cancelRequestWithFlag
{
    self.userCancelRequest = TRUE;
    [self cancelRequest];
}

- (void) setUuid: (NSString *) value
{
    [self.userDictionary setObject: value forKey: @"uuid"];
}

- (NSString *) uuid
{
    return [self.userDictionary objectForKey: @"uuid"];
}

This means that you can now do exactly the same thing as before:

[request cancelRequestWithFlag];

In the complete you can query request.userCancelRequest.

Since this will be the file that will be COMPLETELY user configurable, it means that you can add as many things you want to carry along with the request without it turning into a maintenance nightmare for me. :)

Also, by doing it this way, you can LOOK like you're adding variables to the BRRequest when in fact you're just adding them to the NSMutableDictionary.

Let me know if this is workable.

colinhumber commented 11 years ago

Cool, that looks just fine!

On Mon, Jul 1, 2013 at 6:08 PM, Lloyd Sargent notifications@github.comwrote:

Thanks!

Okay, here is what I'm doing. Creating a category called BRRequest+_UserData which will look like the following:

.h @interface https://github.com/interface BRRequest (_UserData) @property https://github.com/property BOOL userCancelRequest; @property https://github.com/property NSString *uuid; @end https://github.com/end

.m

  • (void) setUserCancelRequest: (BOOL) value { if (value) [self.userDictionary setObject: @"cancel" forKey: @"cancel"]; else [self.userDictionary removeObjectForKey: @"cancel"]; }
  • (BOOL) userCancelRequest { return [self.userDictionary objectForKey: @"cancel"] != nil; }
  • (void) cancelRequestWithFlag { self.userCancelRequest = TRUE; [self cancelRequest]; }
  • (void) setUuid: (NSString *) value { [self.userDictionary setObject: value forKey: @"uuid"]; }
  • (NSString *) uuid { return [self.userDictionary objectForKey: @"uuid"]; }

This means that you can now do exactly the same thing as before:

[request cancelRequestWithFlag];

In the complete you can query request.userCancelRequest.

Since this will be the file that will be COMPLETELY user configurable, it means that you can add as many things you want to carry along with the request without it turning into a maintenance nightmare for me. :)

Also, by doing it this way, you can LOOK like you're adding variables to the BRRequest when in fact you're just adding them to the NSMutableDictionary.

Let me know if this is workable.

— Reply to this email directly or view it on GitHubhttps://github.com/lloydsargent/BlackRaccoon/pull/19#issuecomment-20318979 .

Colin

lloydsargent commented 11 years ago

Glad you like it. Actually, I was kind of pleased it turned out so nicely. I need to test it (I will have to get to it tonight) and then I'll push it up to the main build. Probably ought to add some comments ;)