Fix comments typo, rename param, refine obtainMessage #671
Conversation
zxw1962
commented
Aug 18, 2014
- fix typo of omit;
- change param name from "value" to a meaningful "sync";
- seems obtainMessage can be refined as one line;
| msg = Message.obtain(handler, responseMessageId, responseMessageData); | ||
| } | ||
| return msg; | ||
| return Message.obtain(handler, responseMessageId, responseMessageData); |
There was a problem hiding this comment.
@zxw1962
This is wrong as handler could be null. The alternative when that happens is meant to emulate a Message.obtain(), so the entire method has to stay as-is.
There was a problem hiding this comment.
@zxw1962 yes, sincerely this is invalid change, we have to support different scenarios, and this is key component that was written like this for that purpose.
There was a problem hiding this comment.
This is correct now, but what happens in the future if the underlying implementation changes and a check for null is introduced to Message.obtain()? (by the Android team, or any other manufacturer -- Nokia X, Sony, Samsund, htc, etc. -- who goes in and changes the source code to their satisfaction.)
Potentially, this will make all apps relying on AHC to fail if the device is upgraded to run a newer Android (with a different implementation for Message.obtain()).
We have to consider that this library is used by thousands of apps and we have to rely on backward-compatibility even if leaving some useless (currently) code around.
There was a problem hiding this comment.
@fineswap future changes to this API is unlikely, as this is stable behavior since API v1
@zxw1962 I'm sorry, I've gone too quickly through implementation of Message API, this is correct change, as the [Message.obtain(Handler,int,Object)](https://developer.android.com/reference/android/os/Message.html#obtain%28android.os.Handler, int, java.lang.Object%29) just sets passed values into internal fields of Message. I'm pulling back my previous complaints.
There was a problem hiding this comment.
so what should I do, revert or just leave it as it is?
There was a problem hiding this comment.
Leave it, I approve the change, it's correct, thanks
There was a problem hiding this comment.
Got it, thanks, will update others soon...
|
Otherwise, this is a honorable pull request and I'll be happy to merge it once these 2 points are taken care of, @zxw1962. |
|
@smarek should i merge these to upstream or you guys will take care of that? |
|
nice, good to know. |
|
Good, merging it then. |
Fix comments typo, rename param, refine obtainMessage
|
@zxw1962 thanks for the contribution and patience :-) |
|
@zxw1962 |
|
@zxw1962 there are two options
Second option is more uncertain, as you can't be sure what will and won't be merged, so if I were you, I'd stick with first option. |
|
@smarek big thanks, really appreciate. |