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

View actions #27

Merged
merged 4 commits into from
Oct 19, 2014
Merged

View actions #27

merged 4 commits into from
Oct 19, 2014

Conversation

ronshapiro
Copy link
Contributor

Addresses #26

This was originally written for when RxAndroid was part of the same repo as RxJava: ReactiveX/RxJava#1584
@ronshapiro
Copy link
Contributor Author

There was some discussion about making the Action1s hold WeakReference<View> to avoid leaking memory by retaining Contexts/Activitys - @dpsm @mttkay, do you still want this?

@dpsm
Copy link
Contributor

dpsm commented Oct 13, 2014

Yep :) @mttkay ?

private final View view;

public ViewActionSetActivated(View view) {
this.view = view;
Copy link
Contributor

Choose a reason for hiding this comment

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

You might want to consider creating a ViewAction1 abstract class to contain the reference to the WeakReference and avoid having a Field for the view in all your Action1 implementations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea that's what I was thinking. Probably also have a call method that only executed if the WeakReference.get() != null?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep something like: void call(View view, T t) {..}

The view is safely passed in the method above only IF it's "still around" :)

@ronshapiro
Copy link
Contributor Author

@dpsm lmk what you think about the ViewAction1 in 602deb7

@mttkay
Copy link
Collaborator

mttkay commented Oct 15, 2014

I'm 👍

waiting for @dpsm to 👍 then I'll merge

general question btw: should we add Netflix copyright notices to all files from here on, even though Netflix isn't working on this project anymore? I'm not good with the legal stuff, but whatever we do I guess should be consistent

@ronshapiro
Copy link
Contributor Author

Probably a good q for @benjchristensen. I had those on the files from the old repo and didn't think much about it when I brought them here.

@benjchristensen
Copy link
Member

I've asked the lawyers. The license header should stay the same but the copyright portion can either be skipped entirely or you can add your own name to the copyright if you work on a file.

@ronshapiro
Copy link
Contributor Author

In that case, I think the copyright should be just skipped. Many people could work on one file, and it will make diffs unnecessarily confusing to add your name there.

@benjchristensen
Copy link
Member

Yup, that's my perspective. Copyrights are odd things for code. Every conversation I've had with lawyers on the topic has become more ambiguous than the last on how to handle copyright in source code.

@headinthebox is ultimately the one who will make the final determination as we formalization the ReactiveX org now that all the RxJava code is out of Netflix. Until that point though skip it or put what you want.

The minimum to have is this:

/**
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 * 
 * http://www.apache.org/licenses/LICENSE-2.0
 * 
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

@mttkay
Copy link
Collaborator

mttkay commented Oct 17, 2014

👍

@mttkay
Copy link
Collaborator

mttkay commented Oct 17, 2014

@ronshapiro do you wanna make this change real quick? then I'll land this. I will go through all other files and add the license notice where required

@ronshapiro
Copy link
Contributor Author

@mttkay done :)

mttkay added a commit that referenced this pull request Oct 19, 2014
@mttkay mttkay merged commit 67e3630 into ReactiveX:0.x Oct 19, 2014
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.

4 participants