Bump gradle version #79

coryroy · 2017-04-11T22:36:17Z

what

Originally the plan was simply to bump version of gradle to 2.3.1 and gradle wrapper to 3.3

This had some fall on effects as we were relying on:
fragmentManager.getFragments()

but this with the new gradle version this not shows correctly as hidden so we had to come up with another solution.

how

Keeping a static HashMap of the fragments by adapter position:

private static Map<Integer, DiscoveryFragment> fragmentMap;

seems to work very well. WeakReferences were tried but it seemed to cause many problems with fragments disappearing on config changes.

Some important things with the implementation:

/**
  * Don't pull directly from the map as it may be null
  */
  private @NonNull DiscoveryFragment safeGetFragment(final int position) {
    DiscoveryFragment fragment = fragmentMap.get(position);
    if (fragment == null) {
      fragment = getItem(position);
    }

    return fragment;
  }

and also in the constructor for the adapter:

if (fragmentMap == null) {
  fragmentMap = new HashMap<>();
}

If you don't do the null check the map will get wiped out on rotate etc which you don't want as the fragments themselves are still around.

mbrandonw · 2017-04-11T22:37:40Z

luoser

welcome 😎

luoser · 2017-04-11T22:53:59Z

oooh interesting, some lint failures with these gradle changes. we're gonna look into these tomorrow a.m. before merging

coryroy · 2017-04-12T13:30:33Z

I pushed a new commit to the branch. Now inheriting from AppCompatTextView in accordance with lint.

mbrandonw

Nice nice!

Regarding that unused import. I remember Android Studio having a setting to automatically strip unused imports (at least mine does). Worth turning that on, and wondering if we can share that setting across all native engineers.

coryroy · 2017-04-12T13:58:55Z

second lint issue is going to be a larger problem:
https://code.google.com/p/android/issues/detail?id=234673

Looks like this method is marked as hidden in the support api!

… find existing fragments

mbrandonw

just a few comments.

btw not sure if lisa has shown you this yet, but you can run all of the linting and checkstyle stuff locally by doing milkrun quality.

mbrandonw · 2017-04-13T13:49:26Z

app/src/main/java/com/kickstarter/ui/adapters/DiscoveryPagerAdapter.java

-    return DiscoveryFragment.newInstance(position);
+  public
+  @NonNull
+  Fragment getItem(final int position) {


we keep all of the modifiers in the same line of the function.

did something in our check style say to do this?

looks like something android studio does automatically when auto-completing override methods, cory and i are gonna pair this morning and i'll explain our code style!

mbrandonw · 2017-04-13T13:49:32Z

app/src/main/java/com/kickstarter/ui/adapters/DiscoveryPagerAdapter.java

-        return fragmentPosition == position;
-      })
-      .subscribe(frag -> frag.takeCategories(categories));
+    fragmentMap.get(position).takeCategories(categories);


sooooooooo much better!

mbrandonw · 2017-04-13T13:50:42Z

app/src/main/java/com/kickstarter/ui/adapters/DiscoveryPagerAdapter.java

+  public
+  @NonNull
+  Fragment getItem(final int position) {
+    DiscoveryFragment fragment = DiscoveryFragment.newInstance(position);


looks like check style wants this to be final. we like to final all the things

coryroy · 2017-04-13T22:09:49Z

This build is still failing during :app:lintExternalPre21Release, but the lint file isn't even created. I think it's crashing during lint!

mbrandonw · 2017-04-13T22:17:20Z

This build is still failing during :app:lintExternalPre21Release, but the lint file isn't even created. I think it's crashing during lint!

@coryroy according to the lint output it's pointing at this line:

pages.stream().forEach(page -> fragmentMap.get(page).clearPage());

I didn't catch this initially, but this isn't valid java 6 code because it's using java 8 features. we need to change this to the old school way.

christopherwright · 2017-04-14T16:41:29Z

app/src/main/java/com/kickstarter/ui/adapters/DiscoveryPagerAdapter.java


 public final class DiscoveryPagerAdapter extends FragmentPagerAdapter {
  private final Delegate delegate;
  private final FragmentManager fragmentManager;
  private List<String> pageTitles;
+  private Map<Integer, DiscoveryFragment> fragmentMap;


Main question I have here is whether there are any issues with holding onto a reference to these fragments – can it potentially cause any issues with GC? I couldn't see a clear explanation on how this works from a scan of the docs, so would just hope that we have confidence that this would not cause issues with destroying fragments.

@christopherwright Good point. Do you think putting weak references in the map would be a better idea.

luoser

just a few things

luoser · 2017-04-17T14:44:13Z

.gitignore

@@ -27,8 +27,14 @@ proguard/
 # Log Files
 *.log

-# OSX files
+# OS generated files #
+######################


think we should cut out this comment line, to match the rest of this file's style

luoser · 2017-04-17T14:44:45Z

.gitignore

 .DS_Store
+.DS_Store?


is there a reason why there's a ? appended?

luoser · 2017-04-17T14:47:34Z

app/src/main/java/com/kickstarter/ui/adapters/DiscoveryPagerAdapter.java

@@ -40,8 +38,11 @@ public void setPrimaryItem(final @NonNull ViewGroup container, final int positio
  }

  @Override
-  public @NonNull Fragment getItem(final int position) {
-    return DiscoveryFragment.newInstance(position);
+  @NonNull


in general, we put our nullability annotations right in front of the type which it is annotating, i.e.

public @NonNull DiscoveryFragment

i'll update our styleguide to include this, since it's something we discussed a while ago as a team but neglected to include in the styleguide!

luoser · 2017-04-17T14:47:55Z

app/src/main/java/com/kickstarter/ui/fragments/DiscoveryFragment.java

@@ -44,7 +43,8 @@

  public DiscoveryFragment() {}

-  public static @NonNull Fragment newInstance(final int position) {
+  @NonNull


same here with @NonNull

I think the auto-formatter messed this up as I didn't specifically change this. Is it possible to change the settings file so this doesn't happen on auto-format?

@luoser

Dismissing my review as I will defer to @luoser for her final approval!

… yet populated

merged in some related changes, feedback is now addressed

luoser · 2017-04-25T22:34:55Z

alrighty, we're ready to rock n roll. for anyone on an older version of Android Studio, be sure to upgrade to 2.3.1!

bump version of gradle to 2.3.1 and gradle wrapper to 3.3

a2c3b2e

coryroy requested a review from luoser April 11, 2017 22:36

luoser approved these changes Apr 11, 2017

View reviewed changes

fix lint issues by inheriting from AppCompatTextView

8985b0a

coryroy requested a review from mbrandonw April 12, 2017 13:31

remove unused import

4ae396e

mbrandonw approved these changes Apr 12, 2017

View reviewed changes

undeadly added 2 commits April 12, 2017 11:28

refactor pager adapter to use hashmap rather than fragment manager to…

6de4fe9

… find existing fragments

fix code-style issues

5e22f1e

mbrandonw previously requested changes Apr 13, 2017

View reviewed changes

fix modre code-style issues

a0f7499

undeadly added 2 commits April 14, 2017 04:42

change foreach style

666c38a

change foreach to old for style

9a4d917

christopherwright reviewed Apr 14, 2017

View reviewed changes

remove reference to unused fragment manager

2aa08c4

luoser previously requested changes Apr 17, 2017

View reviewed changes

undeadly added 2 commits April 18, 2017 09:38

remove unnecessary lines from gitignore

0ec9281

change position of nullable annotations

ff12e31

undeadly added 4 commits April 19, 2017 10:57

fix config change issues with adapter refactor

189c9ba

remove unused code in activity

d3ecb9a

fix some code-style issue. Thanks milkrun

c780b13

Add some more null checks to handle filter changes when viewmodel not…

5dd89e1

… yet populated

luoser mentioned this pull request Apr 25, 2017

Remove getFragments() usage #84

Merged

Merge remote-tracking branch 'oss/master' into upgrade_android_studio

6f3b5cc

luoser merged commit 74a7b34 into master Apr 25, 2017

luoser deleted the upgrade_android_studio branch April 25, 2017 22:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump gradle version #79

Bump gradle version #79

coryroy commented Apr 11, 2017 •

edited by luoser

mbrandonw commented Apr 11, 2017

luoser left a comment

luoser commented Apr 11, 2017

coryroy commented Apr 12, 2017

mbrandonw left a comment

coryroy commented Apr 12, 2017

mbrandonw left a comment

mbrandonw Apr 13, 2017

luoser Apr 13, 2017

mbrandonw Apr 13, 2017

mbrandonw Apr 13, 2017

coryroy commented Apr 13, 2017

mbrandonw commented Apr 13, 2017

christopherwright Apr 14, 2017

coryroy Apr 14, 2017

luoser left a comment

luoser Apr 17, 2017

luoser Apr 17, 2017

luoser Apr 17, 2017

luoser Apr 17, 2017

coryroy Apr 18, 2017

luoser commented Apr 25, 2017

		.DS_Store
		.DS_Store?

Bump gradle version #79

Bump gradle version #79

Conversation

coryroy commented Apr 11, 2017 • edited by luoser

what

how

mbrandonw commented Apr 11, 2017

luoser left a comment

Choose a reason for hiding this comment

luoser commented Apr 11, 2017

coryroy commented Apr 12, 2017

mbrandonw left a comment

Choose a reason for hiding this comment

coryroy commented Apr 12, 2017

mbrandonw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coryroy commented Apr 13, 2017

mbrandonw commented Apr 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luoser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luoser commented Apr 25, 2017

coryroy commented Apr 11, 2017 •

edited by luoser