added PendingRequestListener::onRequestAttached

[mignick]

Hi!

In our project we need an ability to attach to a pending request after device configuration changed. For example, a user wants to cancel a long running request or it's necessary to show a dialog to a user if there is a pending request still. I saw the discussion about this in #335 but no activity on the feature. I'm not very experienced in android development, so please be patient about my PR :) If I did it wrong, please advice how to improve it.

Thanks, Nick

[softwaremaverick]

Quick review seems OK except there's a null check before an instanceof check which effectively makes the null check not required.

However, I'll check out the bits that call it when I get home.

On 25 November 2014 17:46:20 GMT+00:00, mignick notifications@github.com wrote:

Hi!

In our project we need an ability to attach to a pending request after device configuration changed. For example, a user wants to cancel a long running request or it's necessary to show a dialog to a user if there is a pending request still. I saw the discussion about this in #335 but no activity on the feature. I'm not very experienced in android development, so please be patient about my PR :) If I did it wrong, please advice how to improve it.

Thanks, Nick You can merge this Pull Request by running:

git pull https://github.com/mignick/robospice onRequestAttached

Or you can view, comment on it, or merge it online at:

https://github.com/stephanenicolas/robospice/pull/383

-- Commit Summary --

• added PendingRequestListener::onRequestAttached

-- File Changes --

M robospice-core-parent/robospice-core-test/src/main/java/com/octo/android/robospice/stub/PendingRequestListenerWithProgressStub.java (13) M robospice-core-parent/robospice/src/main/java/com/octo/android/robospice/request/listener/PendingRequestListener.java (4) M robospice-core-parent/robospice/src/main/java/com/octo/android/robospice/request/notifier/DefaultRequestListenerNotifier.java (35)

-- Patch Links --

https://github.com/stephanenicolas/robospice/pull/383.patch https://github.com/stephanenicolas/robospice/pull/383.diff

---

Reply to this email directly or view it on GitHub: https://github.com/stephanenicolas/robospice/pull/383

Sent from my Android device with K-9 Mail. Please excuse my brevity.

[softwaremaverick]

Looking through the rest of the code your changes seem to be aligned with the existing code in that region so it's good with me.

Whilst the change means an API change on PendingRequestListener which would result in compilation errors when users use the new version, I personally welcome that error. Users wishing to utilise the new feature will then atleast be notified of it's existence.

As for the null check on instanceof's I can see other instances of them in the DefaultRequestListenerNotifier class. I personally would welcome you to remove the combination of null check and instanceof as the instanceof returns false on a null object.

Eg.

if (listener != null && listener instanceof PendingRequestListener) {

could be

if (listener instanceof PendingRequestListener) {

On a wider discussion I question the requirement for a null check on listeners in a collection., ie. if (listener != null)...

Is there at any point where a null listener would be added? If one is added I would welcome a NPE indicating such an error.

That would be as mentioned part of a wider discussion.

Otherwise thanks for the changes I'm sure many users would look forward to utilising it.

Andrew

On 25/11/2014 18:37, Andrew Clark wrote:

Quick review seems OK except there's a null check before an instanceof check which effectively makes the null check not required.

However, I'll check out the bits that call it when I get home.

On 25 November 2014 17:46:20 GMT+00:00, mignick notifications@github.com wrote:

Hi!

In our project we need an ability to attach to a pending request
after device configuration changed. For example, a user wants to
cancel a long running request or it's necessary to show a dialog
to a user if there is a pending request still.
I saw the discussion about this in #335
<https://github.com/stephanenicolas/robospice/issues/335> but no
activity on the feature. I'm not very experienced in android
development, so please be patient about my PR :) If I did it
wrong, please advice how to improve it.

Thanks,
Nick

------------------------------------------------------------------------

        You can merge this Pull Request by running

   git pull https://github.com/mignick/robospice onRequestAttached

Or view, comment on, or merge it at:

https://github.com/stephanenicolas/robospice/pull/383

        Commit Summary

  * added PendingRequestListener::onRequestAttached

        File Changes

  * *M*
    robospice-core-parent/robospice-core-test/src/main/java/com/octo/android/robospice/stub/PendingRequestListenerWithProgressStub.java
    <https://github.com/stephanenicolas/robospice/pull/383/files#diff-0>
    (13)
  * *M*
    robospice-core-parent/robospice/src/main/java/com/octo/android/robospice/request/listener/PendingRequestListener.java
    <https://github.com/stephanenicolas/robospice/pull/383/files#diff-1>
    (4)
  * *M*
    robospice-core-parent/robospice/src/main/java/com/octo/android/robospice/request/notifier/DefaultRequestListenerNotifier.java
    <https://github.com/stephanenicolas/robospice/pull/383/files#diff-2>
    (35)

        Patch Links:

  * https://github.com/stephanenicolas/robospice/pull/383.patch
  * https://github.com/stephanenicolas/robospice/pull/383.diff

—
Reply to this email directly or view it on GitHub
<https://github.com/stephanenicolas/robospice/pull/383>.

Sent from my Android device with K-9 Mail. Please excuse my brevity.

[mignick]

Thank you @softwaremaverick for the review. If it would necessary to remove the null check, I could do it for all cases.

As I understand now we are waiting for @stephanenicolas's PR review and comments about null check?

[softwaremaverick]

Yes that's correct. Maybe Stephan will know of a scenario why it was added. If it's for safety reasons then I'd prefer the code that adds listeners to prevent nulls from being added to the list.

On 26 November 2014 09:16:19 GMT+00:00, mignick notifications@github.com wrote:

Thank you @softwaremaverick for the review. If it would necessary to remove the null check, I could do it for all cases.

As I understand now we are waiting for @stephanenicolas's PR review and comments about null check?

---

Reply to this email directly or view it on GitHub: https://github.com/stephanenicolas/robospice/pull/383#issuecomment-64534636

Sent from my Android device with K-9 Mail. Please excuse my brevity.

[mignick]

@stephanenicolas, any chance that you will have time to review this PR in near future?

[softwaremaverick]

@mignick I was just glancing through these changes again as I noticed they've not yet been merged in for some reason.

However, looking at PendingRequestListenerWithProgressStub I noticed that the request itself is being stored in the listener which I don't know why it should be.

I'm not actually sure what PendingRequestListenerWithProgressStub is used for but I'd have imagine the onRequestAttached would work like onRequestNotFound and just be used as an indicator to show it's actually active.

Is there any reason why the request itself should be passed to the listener?

This kind of sounds problematic if potentially the request itself started to know about all the listeners and the listener wanted to hold onto the request which I don't think it does right now but would cause a memory leak if it became the case.

[mignick]

@softwaremaverick PendingRequestListenerWithProgressStub is for testing purpose only, I guess. Your concerns about retain cycles are understandable. The reason was just a convenient way to get to the request to cancel it. Ok, it should be enough to have just an indicator about active request and cancel it another way.

As a workaround against retain cycles the onRequestAttached could return a weak reference to the request.

Thank you.