Closed timothyasp closed 6 years ago
amandaradakovich
commented
7 years ago @timothyasp You can strike Magnoliamedical from the custom app list, they are no longer a customer. Let me know how I can help with testing. Thanks for the comprehensive update!
marczych
commented
7 years ago I made it through res and now I'm working through the java code and will continue another day. Seeing all of the refactoring is kind of cathartic!
Reviewed 514 of 693 files at r1, 2178 of 2219 files at r2. Review status: 2691 of 2796 files reviewed at latest revision, 10 unresolved discussions.
a discussion (no related file): Not a big deal, but there are a lot of files without a trailing newline.
.gitignore, line 47 at r2 (raw file):
# App Store Resources PlayStoreResources
This directory already exists - you want to ignore any new content in it?
App/build.gradle, line 162 at r2 (raw file):
} } }
Removed the trailing newline?
App/res/values-it/strings.xml, line 266 at r2 (raw file):
<string name="media_manager_title">Media Manager</string> <string name="media_storage_exception_toast_message">We\'re having problems accessing your storage, try again.</string> <string name="all">All</string>
Is it best to add the English versions to the other languages for them to translate rather than omitting them?
Also, the indentation is a bit off. Same goes for the other strings files.
App/src/com/dozuki/ifixit/model/dozuki/Site.java, line 269 at r2 (raw file):
site.mPublicRegistration = false; site.mObjectNamePlural = res.getString(R.string.categories); site.mObjectNameSingular = res.getString(R.string.category);
We still haven't refactored this out yet? =(
App/src/com/dozuki/ifixit/model/search/GuideSearchResult.java, line 65 at r2 (raw file):
public void onClick(View v) { Intent intent = new Intent(v.getContext(), GuideViewActivity.class); intent.putExtra(GuideViewActivity.GUIDEID, mGuideInfo.mGuideid);
If I remember correctly, you can do something like GuideViewActivity.viewGuide(mGuideInfo.mGuideid) to get an Intent.
App/src/com/dozuki/ifixit/model/search/TopicSearchResult.java, line 96 at r2 (raw file):
result = 31 * result + (mUrl != null ? mUrl.hashCode() : 0); result = 31 * result + (mText != null ? mText.hashCode() : 0); result = 31 * result + (mImage != null ? mImage.hashCode() : 0);
Might be worth using an annotation to generate the equals and hash code methods. I don't know how much work it would be to switch to something like auto value: https://github.com/google/auto/tree/master/value.
App/src/com/dozuki/ifixit/ui/AnswersWebViewActivity.java, line 30 at r2 (raw file):
mWebView = new WebViewFragment(); Bundle args = new Bundle(); args.putString(WebViewFragment.URL_KEY, "https://" + App.get().getSite().mDomain + "/Answers");
Is the site necessarily https? I suppose it will redirect if not?
App/src/com/dozuki/ifixit/ui/BaseActivity.java, line 46 at r2 (raw file):
private static final int LOGGED_OUT_USERID = -1; protected Toolbar mToolbar;
I don't think that this is used anywhere. In this file at least.
App/src/com/dozuki/ifixit/ui/BaseMenuDrawerActivity.java, line 224 at r2 (raw file):
*/ protected enum NavigationItem { SITE_LIST(
I remember making this crazy enum class to define all of the menu options. I'm glad that it's getting replaced. =)
Comments from Reviewable
timothyasp
commented
7 years ago You're a legend @marczych for tackling this CR - i'm trying to figure out how to use Reviewable, so comments incoming.
marczych
commented
7 years ago Reviewable is pretty nice because it keeps track of files that still need review and outstanding discussions that need to be resolved. It's a little painful for large diffs like this one so don't expect it to be blazing fast.
marczych
commented
7 years ago Reviewed 70 of 693 files at r1, 41 of 2219 files at r2. Review status: all files reviewed at latest revision, 31 unresolved discussions.
a discussion (no related file): Also, there are some debug logs sprinkled throughout that you might want to clean up.
a discussion (no related file): I'm not very familiar with OkHttp's cache policies but I suppose it isn't any worse than storing manually storing the files in the cache directory which can get wiped at anytime? As far as data security is concerned anyway.
App/AndroidManifest.xml, line 15 at r2 (raw file):
<uses-permission android:name="android.permission.READ_EXTERNAL_STORAGE"/> <uses-permission android:name="android.permission.ACCESS_NETWORK_STATE"/> <uses-permission android:name="android.permission.CAMERA"/>
Can some of these permissions be removed? I saw what I thought was the new runtime permissions code but I'm not sure how it works.
App/build.gradle, line 20 at r2 (raw file):
} maven { url 'https://dl.bintray.com/alexeydanilov/maven'
What is this one used for?
App/build.gradle, line 59 at r2 (raw file):
defaultConfig { versionCode 61 versionName "3.0.0"
Major version, eh?
App/src/com/dozuki/ifixit/ui/BaseSearchMenuDrawerActivity.java, line 74 at r2 (raw file):
} }*/
No more barcode scanner? Or is it handled in a not so crappy way now?
App/src/com/dozuki/ifixit/ui/GuideListRecyclerAdapter.java, line 119 at r2 (raw file):
Intent intent = new Intent(context, GuideViewActivity.class); intent.putExtra(GuideViewActivity.GUIDEID, mGuide.mGuideid); context.startActivity(intent);
This should use the Intent factory that I commented about before.
App/src/com/dozuki/ifixit/ui/LoadingFragment.java, line 21 at r2 (raw file):
text = args.getString(TEXT_KEY); } else { text = "Loading";
Should this be a proper translatable string?
App/src/com/dozuki/ifixit/ui/auth/OpenIDActivity.java, line 80 at r2 (raw file):
public void onPageStarted(WebView view, String url, Bitmap favicon) { //setTitle(url); }
We should probably delete this whole method if it's commented out, right?x.
App/src/com/dozuki/ifixit/ui/guide/create/GuideCreateActivity.java, line 79 at r2 (raw file):
mRecyclerListView.setAdapter(mGuideRecyclerListAdapter); FloatingActionButton fab = (FloatingActionButton) findViewById(R.id.add_guide_fab);
Yay, new Android design stuff!
App/src/com/dozuki/ifixit/ui/guide/create/GuideCreateActivity.java, line 96 at r2 (raw file):
@Override public void onRefresh() { Api.call(GuideCreateActivity.this, ApiCall.userGuides());
I think we can leave off GuideCreateActivity. here.
App/src/com/dozuki/ifixit/ui/guide/create/GuideCreateActivity.java, line 150 at r2 (raw file):
mUserGuideList.clear(); mUserGuideList.addAll(event.getResult()); Log.d("GuideCreateActivity", "Size: " + mUserGuideList.size());
Debug log snuck in.
App/src/com/dozuki/ifixit/ui/guide/create/GuideCreateActivity.java, line 254 at r2 (raw file):
Intent intent = new Intent(this, StepsActivity.class); intent.putExtra(StepsActivity.GUIDE_ID_KEY, guide.mGuideid); intent.putExtra(StepsActivity.GUIDE_PUBLIC_KEY, guide.mPublic);
Intent factory pattern here too.
App/src/com/dozuki/ifixit/ui/guide/create/GuideCreateActivity.java, line 269 at r2 (raw file):
guide.mIsPublishing = true; Log.d("Api", "Guide Revisionid before: " + guide.mRevisionid);
Another debug log.
App/src/com/dozuki/ifixit/ui/guide/create/GuideCreateActivity.java, line 284 at r2 (raw file):
Intent intent = new Intent(this, GuideViewActivity.class); intent.putExtra(GuideViewActivity.GUIDEID, guide.mGuideid); intent.putExtra(GuideViewActivity.CURRENT_PAGE, 0);
Intent factory here too.
App/src/com/dozuki/ifixit/ui/guide/create/GuideListItemHolder.java, line 111 at r2 (raw file):
public boolean onLongClick(View v) { AlertDialog.Builder builder = new AlertDialog.Builder(v.getContext()); builder.setItems(App.get().getSite().isIfixit() ? R.array.guide_list_item_options
This should probably be moved into a new method e.g. Site.allowDelete().
App/src/com/dozuki/ifixit/ui/guide/view/GuideIntroViewFragment.java, line 106 at r2 (raw file):
if (previousState == TOUCH_RELEASED) previousState = TOUCH_TOUCHED; else previousState = TOUCH_UNDEFINED; break;
I find these conditionals without braces hard to read. The switch isn't helper either.
App/src/com/dozuki/ifixit/ui/guide/view/GuideStepViewFragment.java, line 42 at r2 (raw file):
StepVideoFragment mVideoFrag; StepEmbedFragment mEmbedFrag; StepLinesFragment mLinesFrag;
The m prefix should be removed from these variables because they aren't member variables anymore.
App/src/com/dozuki/ifixit/ui/search/SearchActivity.java, line 126 at r2 (raw file):
@Override public void onLoadMore(int page, int totalItemsCount, RecyclerView view) { if (totalItemsCount >= LIMIT - 3) { // We have to offset by 3 because some responses should have 20 results, but they often have less
This is a failing of the pagination in the REST API? Probably caused by filtering results after querying?
App/src/com/dozuki/ifixit/ui/topic/TopicViewFragment.java, line 34 at r2 (raw file):
private TopicNode mTopicNode; private TopicLeaf mTopicLeaf; private Site mSite;
This isn't used anywhere.
App/src/com/dozuki/ifixit/util/api/Api.java, line 389 at r2 (raw file):
*/ request = HttpRequest.post(url); request.header("X-HTTP-Method-Override", apiCall.mEndpoint.mMethod);
It's good to see this hack and the Responder interface finally deleted.
App/src/com/dozuki/ifixit/util/api/GuideMediaProgress.java, line 75 at r2 (raw file):
String url = cachedUrls.next(); if (guideMedia.contains(url)) { guideMedia.remove(url);
It looks like HashSet.remove() merely returns false if the item isn't in the set so you're probably better off just removing it without checking contains first.
App/src/com/dozuki/ifixit/views/HelperTextInputLayout.java, line 21 at r2 (raw file):
public class HelperTextInputLayout extends TextInputLayout { private static final Interpolator FAST_OUT_SLOW_IN_INTERPOLATOR = new FastOutSlowInInterpolator();;
Double semicolon.
Comments from Reviewable
timothyasp
commented
7 years ago My browser keeps crashing on reviewable.. bummer - i'll try a different computer.
timothyasp
commented
7 years ago Review status: all files reviewed at latest revision, 32 unresolved discussions.
a discussion (no related file):
I'm not very familiar with OkHttp's cache policies but I suppose it isn't any worse than storing manually storing the files in the cache directory which can get wiped at anytime? As far as data security is concerned anyway.
That's what I was thinking - the added benefit is that the offline cache is slowly built up while using the app, in addition to the offline syncer. The biggest downside is that there isn't a guarantee that OkHttp won't evict images or responses that are necessary for offline guides once the cache is filled. On large phones, this is very unlikely to happen since it will use up to 10% of the available space on the phone, but on smaller, devices, this could be a problem.
One solution could be to intercept the eviction requests and check to make sure it's not an offline guide or request.
The other solution is to look at this from a different perspective, and store GuideInfo, topic and sites in sqlite rather than depending on volatile file disk caches. A separate issue I ran into is targeting v25 started crashing the app because 7.1 introduced strict policies on passing around objects > 512kb in savedInstanceStates and through Otto. I realized the solution would be a massive overhaul of all our saveState code, and our serializeable objects. Todays best practices seem to be storing the objects locally in a database and passing around IDs when handling state transitions.
The other option is that I can remove the cache unification and keep it how you had it (with some minor modifications to allow it to work with OkHttp3 requests)
I'm not sure about the right solution.
Thoughts?
.gitignore, line 47 at r2 (raw file):
This directory already exists - you want to ignore any new content in it?
Yup - eventually we'll figure out a better way to deal with the play store assets, but for now I just wanted to clean up my local git status
App/AndroidManifest.xml, line 15 at r2 (raw file):
Can some of these permissions be removed? I saw what I thought was the new runtime permissions code but I'm not sure how it works.
From what I understand of the new permission model, Marshmallow and up ignores these permissions and checks them on-demand. Anything below marshmallow still requires the manifest permissions.
App/build.gradle, line 20 at r2 (raw file):
What is this one used for?
The latest version of ViewPagerIndicator - unfortunatly there's a Lint error with 2.4.1 with some math library not existing in newer versions of android. That repo has an updated version with that fixed.
App/build.gradle, line 59 at r2 (raw file):
Major version, eh?
Figured 10k commits warrented it ;)
Another more practical reason - android now allows multiple APKs to be released targeting different versions. Since iFixit is all about reusing old hardware, i thought it would be worthwhile to release this new version in addition to keeping the old version around and providing security and basic maintenance to the older version since it's fairly stable. A major version bump allows this.
App/build.gradle, line 162 at r2 (raw file):
Removed the trailing newline?
IDE's... man
App/res/values-it/strings.xml, line 266 at r2 (raw file):
Is it best to add the English versions to the other languages for them to translate rather than omitting them? Also, the indentation is a bit off. Same goes for the other strings files.
I'm not sure about this.. I probably should have left them out and resubmitted them for translation... i'll address this.
App/src/com/dozuki/ifixit/model/dozuki/Site.java, line 269 at r2 (raw file):
We still haven't refactored this out yet? =(
Yea - couldn't knock everything out.
App/src/com/dozuki/ifixit/model/search/GuideSearchResult.java, line 65 at r2 (raw file):
If I remember correctly, you can do something like `GuideViewActivity.viewGuide(mGuideInfo.mGuideid)` to get an `Intent`.
Good memory - fixing
App/src/com/dozuki/ifixit/model/search/TopicSearchResult.java, line 96 at r2 (raw file):
Might be worth using an annotation to generate the equals and hash code methods. I don't know how much work it would be to switch to something like auto value: https://github.com/google/auto/tree/master/value.
This was generated by the IDE for the search comparator - I'm 100% open to a better way to do it
App/src/com/dozuki/ifixit/ui/AnswersWebViewActivity.java, line 30 at r2 (raw file):
Is the site necessarily https? I suppose it will redirect if not?
From what I remember, everything works as https, but it's not always required for all sites. It's not going to break anything to have it default to https.
App/src/com/dozuki/ifixit/ui/BaseActivity.java, line 46 at r2 (raw file):
I don't think that this is used anywhere. In this file at least.
Yea, it's used in BaseMenuDrawerActivity and TopicViewActivity - with the way the new drawerlayout works and the coordinatorlayouts, there are a couple of base layouts now that include the toolbar. Because of that, I uncluded the toolbar member var in the base class.
App/src/com/dozuki/ifixit/ui/BaseMenuDrawerActivity.java, line 224 at r2 (raw file):
I remember making this crazy enum class to define all of the menu options. I'm glad that it's getting replaced. =)
It did it's job well - luckily androids new menu library is wonderful to use and didn't need clever enum classes to make it sane :)
App/src/com/dozuki/ifixit/ui/BaseSearchMenuDrawerActivity.java, line 74 at r2 (raw file):
No more barcode scanner? Or is it handled in a not so crappy way now?
It's broken - and I'm working on fixing it... lower priority
App/src/com/dozuki/ifixit/ui/guide/view/GuideStepViewFragment.java, line 42 at r2 (raw file):
The `m` prefix should be removed from these variables because they aren't member variables anymore.
True - auto refactoring for the loss
App/src/com/dozuki/ifixit/ui/search/SearchActivity.java, line 126 at r2 (raw file):
This is a failing of the pagination in the REST API? Probably caused by filtering results after querying?
Yup - I told the ifixit team about it and they're working on a fix.
App/src/com/dozuki/ifixit/util/api/Api.java, line 389 at r2 (raw file):
It's good to see this hack and the `Responder` interface finally deleted.
the new okhttp is pretty great. - if we were to start over, I would couple it with their Retrofit library. http://square.github.io/retrofit/ could clean up the JSONHelper and other static libs we have quite a bit. But thats a big task, with little added benefit
Comments from Reviewable
marczych
commented
7 years ago My browser is having a hard time with reviewable so I ended up doing the review in single file mode which was fairly snappy but there doesn't appear to be an overall publish button so I'm hoping that this will do it.
And by the way, you can get to single file mode at a URL like this: https://reviewable.io/reviews/iFixit/iFixitAndroid/265/App/res/values-it/strings.xml
Then you can use . to go to the next item that needs your attention.
Comments from Reviewable
marczych
commented
7 years ago Reviewed 10 of 10 files at r3. Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.
App/build.gradle, line 59 at r2 (raw file):
Figured 10k commits warrented it ;) Another more practical reason - android now allows multiple APKs to be released targeting different versions. Since iFixit is all about reusing old hardware, i thought it would be worthwhile to release this new version in addition to keeping the old version around and providing security and basic maintenance to the older version since it's fairly stable. A major version bump allows this.
That's really cool! They also have some neat staged rollout features now, right?
App/src/com/dozuki/ifixit/ui/BaseSearchMenuDrawerActivity.java, line 74 at r2 (raw file):
It's broken - and I'm working on fixing it... lower priority
Okay, I'll leave this as a blocking discussion so we don't forget to fix it before merging.
App/src/com/dozuki/ifixit/util/api/Api.java, line 389 at r2 (raw file):
the new okhttp is pretty great. - if we were to start over, I would couple it with their Retrofit library. http://square.github.io/retrofit/ could clean up the JSONHelper and other static libs we have quite a bit. But thats a big task, with little added benefit
Yeah, I've had my eye on Retrofit for a while now but the homegrown solution we had in place accomplished much of the same thing so it's probably not worth switching over.
Comments from Reviewable
marczych
commented
7 years ago Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.
a discussion (no related file):
That's what I was thinking - the added benefit is that the offline cache is slowly built up while using the app, in addition to the offline syncer. The biggest downside is that there isn't a guarantee that OkHttp won't evict images or responses that are necessary for offline guides once the cache is filled. On large phones, this is very unlikely to happen since it will use up to 10% of the available space on the phone, but on smaller, devices, this could be a problem. One solution could be to intercept the eviction requests and check to make sure it's not an offline guide or request. The other solution is to look at this from a different perspective, and store GuideInfo, topic and sites in sqlite rather than depending on volatile file disk caches. A separate issue I ran into is targeting v25 started crashing the app because 7.1 introduced strict policies on passing around objects > 512kb in savedInstanceStates and through Otto. I realized the solution would be a massive overhaul of all our saveState code, and our serializeable objects. Todays best practices seem to be storing the objects locally in a database and passing around IDs when handling state transitions. The other option is that I can remove the cache unification and keep it how you had it (with some minor modifications to allow it to work with OkHttp3 requests) I'm not sure about the right solution. Thoughts?
I'm most worried about it not getting adequate testing and a few weeks or months down the road we realize that media is getting evicted from the cache which is hard to test because it takes a while to create the right conditions. I guess it would fix itself pretty shortly after it gets evicted because the sync adapter runs at least once a day, right?
Also, will releasing this cause users to download all of the offline content from scratch?
Comments from Reviewable
marczych
commented
7 years ago Reviewed 26 of 26 files at r4. Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.
App/src/com/dozuki/ifixit/ui/auth/LoginFragment.java, line 9 at r4 (raw file):
import android.content.IntentSender; import android.os.AsyncTask; import android.os.Build;
This import was added but it isn't used as far as I can tell.
App/src/com/dozuki/ifixit/ui/guide/create/GuideCreateRecyclerListAdapter.java, line 44 at r4 (raw file):
userGuide.mPublic = guide.isPublic(); userGuide.mIsPublishing = false; mGuides.set(i, userGuide);
Is this necessary? userGuide is modified in place so it might be fine to remove this line.
Comments from Reviewable
marczych
commented
7 years ago Reviewed 30 of 30 files at r5. Review status: all files reviewed at latest revision, 12 unresolved discussions.
App/src/com/dozuki/ifixit/ui/auth/RegisterFragment.java, line 159 at r5 (raw file):
} else { if (email.length() <= 0) { mEmail.setError("Email is required to register.");
No translations on these?
App/src/com/dozuki/ifixit/util/api/ApiCall.java, line 127 at r5 (raw file):
requestBody.put("password", password); requestBody.put("username", username); requestBody.put("unique_username", username);
Oops, how long has this been required in the API?
Comments from Reviewable
marczych
commented
7 years ago This PR is getting pretty big and it seems like new features are getting added. Are you planning on merging this soon?
Reviewed 4 of 4 files at r6, 37 of 64 files at r7. Review status: 2828 of 2855 files reviewed at latest revision, 12 unresolved discussions.
Comments from Reviewable
timothyasp
commented
7 years ago Yea, I'm planning on it soon. I've just had other bugs and small feature requests come in based on the new app, so I add, release and merge into here. Not ideal.
On Jun 25, 2017 7:57 PM, "Marc Zych" notifications@github.com wrote:
This PR is getting pretty big and it seems like new features are getting added. Are you planning on merging this soon?
Reviewed 4 of 4 files at r6, 37 of 64 files at r7. Review status: 2828 of 2855 files reviewed at latest revision, 12 unresolved discussions.
Comments from Reviewable https://reviewable.io:443/reviews/ifixit/ifixitandroid/265#-:-KnXK3jn66KAyybatFI3:b-wy8y36
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/iFixit/iFixitAndroid/pull/265#issuecomment-310952361, or mute the thread https://github.com/notifications/unsubscribe-auth/AArMc0vKbaeqMjlBntKCP85Al6HeqyFyks5sHx4EgaJpZM4MfuOJ .
timothyasp
commented
6 years ago Merging because this has been live for a while and we need to get back to a proper release management cycle.
Overview
The changes in this pull are pretty massive, so warning about that, but the changes are so sweeping because it involved removing dependencies on old (5+ years) design back-compat libraries such as ActionBarSherlock, MenuDrawer and HttpRequest which were designed to bring modern UX back to old version of android.
The android ecosystem in the last 3 years has grown tremendously to support back-compat with the AppCompat and Android Design libraries, so I wanted to drag our app into the modern day of android development and rip out those old libraries and replace them with that new-new.
Summary of changes
attr/colorPrimaryand other android Design Library color constants. This was magical because it resulted in removing hundreds of drawables.Bugfixes
To-do
This change is