-
Notifications
You must be signed in to change notification settings - Fork 17
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
Feature/init sample #2
base: master
Are you sure you want to change the base?
Conversation
compileSdkVersion 25 | ||
buildToolsVersion "25.0.0" | ||
defaultConfig { | ||
applicationId "sk.inloop.support" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are using "eu.inloop....."
|
||
import sk.inloop.support.sample.adapter.CorrectFragmentAdapter; | ||
|
||
public class FixedMainActivity extends AppCompatActivity { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should separate these "test" activities into a separate module - so that it's not part of the library .aar.
|
||
@Override | ||
public Object instantiateItem(ViewGroup container, int position) { | ||
Fragment result = tryGetExistingFragment(position); //performance, if fragment has been initialized before, just return it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should search for fragments based on the position. The fragments can change their position in the list (after updating the dataset). We should search based on an ID or tag (see FragmentPagerAdapter getItemId() implementation).
@Override | ||
public void destroyItem(ViewGroup container, int position, Object object) { | ||
Fragment fragment = (Fragment) object; | ||
if (DEBUG) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DEBUG is always false - we probably need a static setter for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, I forgot this while refactoring (this is from original FragmentState.. impl). Is Log.isloggable() accepted?
* Return not null and empty {@link List} for storing of initialized {@link Fragment}s. Depend on adapter's purpose (only-read, only-insert, insert-delete), developer have to decide which implementation if list should be used. Default is {@link ArrayList}. | ||
*/ | ||
@NonNull | ||
protected List<Fragment> buildFragmentsList() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this a protected method? Is there any reason why you would override it? (I don't think so)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if developer knows, that fragments will be removed and added too often (ie some 'news' list), LinkedList could have better performance. If there will be 100+ fragments with 'some' search feature, TreeList will be helpfull. ALSO, developer could use Guava libraries and so on. As abstract impl shouldn't decide, which List implementation is optimal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think this is a micro-optimalisation and the performance impact is really minimal :-). The disadvantage is that it makes the code a little more complex and less easier to understand. The developer can misunderstand, override and return something we are not expecting. There will never be more than tens or maybe a hundred of items - for this case we are safe with an ArrayList. No one will override this method in real use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I have less exp so you convinced me 👍
* Return not null and empty {@link List} for storing of {@link android.support.v4.app.Fragment.SavedState}s. Depend on adapter's purpose (only-read, only-insert, insert-delete), developer have to decide which implementation if list should be used. Default is {@link ArrayList}. | ||
*/ | ||
@NonNull | ||
protected List<Fragment.SavedState> buildFragmentStatesList() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an internal functionality, I wouldn't make this protected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same reason for '#buildFragmentsList'
private static final String TAG = FragmentStatePagerAdapter.class.getSimpleName(); | ||
private static final boolean DEBUG = false; | ||
|
||
private final FragmentManager mFragmentManager; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Project is missing settings.gradle file (with ":app" content) |
.gitignore is missing in commit. |
State restoration is not working - add an Edittext to the fragment, enter some value, swipe to third fragment and return back - edittext is empty. |
966c9a6
to
8d90238
Compare
No description provided.