Niedzielski has uploaded a new change for review.
https://gerrit.wikimedia.org/r/258638
Change subject: Hygiene: reorder methods to match interface
......................................................................
Hygiene: reorder methods to match interface
Light refactoring of JsonPageLoadStrategy and PageLoadStrategy:
- Reorder implementation methods to match ordering in interface.
- Consolidate @VisibleForTesting API below overridden and public
methods.
- Replace Log and TAG with L.
- Make JsonPageLoadStrategy.loadSavedPage and STATE_* private.
- Move Javadocs from implementation to interface.
No other functional changes intended.
Change-Id: I8c5abc3e855c33d55d592ae572b538e56539c6a5
---
M app/src/main/java/org/wikipedia/page/JsonPageLoadStrategy.java
M app/src/main/java/org/wikipedia/page/PageLoadStrategy.java
2 files changed, 206 insertions(+), 212 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/apps/android/wikipedia
refs/changes/38/258638/1
diff --git a/app/src/main/java/org/wikipedia/page/JsonPageLoadStrategy.java
b/app/src/main/java/org/wikipedia/page/JsonPageLoadStrategy.java
index 070a806..86c8d64 100644
--- a/app/src/main/java/org/wikipedia/page/JsonPageLoadStrategy.java
+++ b/app/src/main/java/org/wikipedia/page/JsonPageLoadStrategy.java
@@ -41,7 +41,6 @@
import android.support.annotation.NonNull;
import android.support.annotation.VisibleForTesting;
import android.text.TextUtils;
-import android.util.Log;
import android.util.SparseArray;
import android.view.View;
import android.view.ViewGroup;
@@ -66,12 +65,15 @@
* - and many handlers.
*/
public class JsonPageLoadStrategy implements PageLoadStrategy {
- private static final String TAG = "JsonPageLoad";
+ private interface ErrorCallback {
+ void call(@Nullable Throwable error);
+ }
+
private static final String BRIDGE_PAYLOAD_SAVED_PAGE = "savedPage";
- public static final int STATE_NO_FETCH = 1;
- public static final int STATE_INITIAL_FETCH = 2;
- public static final int STATE_COMPLETE_FETCH = 3;
+ private static final int STATE_NO_FETCH = 1;
+ private static final int STATE_INITIAL_FETCH = 2;
+ private static final int STATE_COMPLETE_FETCH = 3;
private int state = STATE_NO_FETCH;
@@ -97,10 +99,6 @@
*/
private boolean cacheOnComplete = true;
- private interface ErrorCallback {
- void call(@Nullable Throwable error);
- }
-
private ErrorCallback networkErrorCallback;
// copied fields
@@ -117,10 +115,6 @@
private BottomContentInterface bottomContentHandler;
- @Override
- public void setBackStack(@NonNull List<PageBackStackItem> backStack) {
- this.backStack = backStack;
- }
@Override
public void setup(PageViewModel model,
@@ -151,6 +145,191 @@
// from being stored in the activity's fragment backstack), then load
the topmost page
// on the backstack.
loadPageFromBackStack();
+ }
+
+ @Override
+ public void backFromEditing(Intent data) {
+ //Retrieve section ID from intent, and find correct section, so where
know where to scroll to
+ sectionTargetFromIntent =
data.getIntExtra(EditSectionActivity.EXTRA_SECTION_ID, 0);
+ //reset our scroll offset, since we have a section scroll target
+ stagedScrollY = 0;
+ }
+
+ @Override
+ public void onDisplayNewPage(boolean pushBackStack, Cache cachePreference,
int stagedScrollY) {
+ fragment.updatePageInfo(null);
+ leadImagesHandler.updateBookmark(false);
+ leadImagesHandler.updateNavigate(null);
+
+ if (pushBackStack) {
+ // update the topmost entry in the backstack, before we start
overwriting things.
+ updateCurrentBackStackItem();
+ pushBackStack();
+ }
+
+ state = STATE_NO_FETCH;
+
+ // increment our sequence number, so that any async tasks that depend
on the sequence
+ // will invalidate themselves upon completion.
+ sequenceNumber.increase();
+
+ if (cachePreference == Cache.NONE) {
+ // If this is a refresh, don't clear the webview contents
+ this.stagedScrollY = stagedScrollY;
+ loadPageOnWebViewReady(cachePreference);
+ } else {
+ // kick off an event to the WebView that will cause it to clear
its contents,
+ // and then report back to us when the clearing is complete, so
that we can synchronize
+ // the transitions of our native components to the new page
content.
+ // The callback event from the WebView will then call the
loadPageOnWebViewReady()
+ // function, which will continue the loading process.
+ leadImagesHandler.hide();
+ bottomContentHandler.hide();
+ activity.getSearchBarHideHandler().setFadeEnabled(false);
+ try {
+ JSONObject wrapper = new JSONObject();
+ // whatever we pass to this event will be passed back to us by
the WebView!
+ wrapper.put("sequence", sequenceNumber.get());
+ wrapper.put("cachePreference", cachePreference.name());
+ wrapper.put("stagedScrollY", stagedScrollY);
+ bridge.sendMessage("beginNewPage", wrapper);
+ } catch (JSONException e) {
+ L.logRemoteErrorIfProd(e);
+ }
+ }
+ }
+
+ @Override
+ public boolean isLoading() {
+ return state != STATE_COMPLETE_FETCH;
+ }
+
+ @Override
+ public void onHidePageContent() {
+ bottomContentHandler.hide();
+ }
+
+ @Override
+ public boolean onBackPressed() {
+ popBackStack();
+ if (!backStack.isEmpty()) {
+ loadPageFromBackStack();
+ return true;
+ }
+ return false;
+ }
+
+ @Override
+ public void setEditHandler(EditHandler editHandler) {
+ this.editHandler = editHandler;
+ }
+
+ @Override
+ public void setBackStack(@NonNull List<PageBackStackItem> backStack) {
+ this.backStack = backStack;
+ }
+
+ @Override
+ public void updateCurrentBackStackItem() {
+ if (backStack.isEmpty()) {
+ return;
+ }
+ PageBackStackItem item = backStack.get(backStack.size() - 1);
+ item.setScrollY(webView.getScrollY());
+ }
+
+ @Override
+ public void loadPageFromBackStack() {
+ if (backStack.isEmpty()) {
+ return;
+ }
+ PageBackStackItem item = backStack.get(backStack.size() - 1);
+ // display the page based on the backstack item, stage the scrollY
position based on
+ // the backstack item.
+ fragment.displayNewPage(item.getTitle(), item.getHistoryEntry(),
Cache.PREFERRED, false,
+ item.getScrollY());
+ L.d("Loaded page " + item.getTitle().getDisplayText() + " from
backstack");
+ }
+
+ @Override
+ public void layoutLeadImage() {
+ leadImagesHandler.beginLayout(new
LeadImagesHandler.OnLeadImageLayoutListener() {
+ @Override
+ public void onLayoutComplete(int sequence) {
+ if (fragment.isAdded()) {
+
searchBarHideHandler.setFadeEnabled(leadImagesHandler.isLeadImageEnabled());
+ }
+ }
+ }, sequenceNumber.get());
+ }
+
+ @VisibleForTesting
+ protected void loadLeadSection(final int startSequenceNum) {
+ app.getSessionFunnel().leadSectionFetchStart();
+ PageLoadUtil.getApiService(model.getTitle().getSite()).pageLead(
+ model.getTitle().getPrefixedText(),
+ calculateLeadImageWidth(),
+ !app.isImageDownloadEnabled(),
+ new PageLead.Callback() {
+ @Override
+ public void success(PageLead pageLead, Response response) {
+ L.v(response.getUrl());
+ app.getSessionFunnel().leadSectionFetchEnd();
+ onLeadSectionLoaded(pageLead, startSequenceNum);
+ }
+
+ @Override
+ public void failure(RetrofitError error) {
+ L.e("PageLead error: ", error);
+ commonSectionFetchOnCatch(error, startSequenceNum);
+ }
+ });
+ }
+
+ @VisibleForTesting
+ protected void commonSectionFetchOnCatch(Throwable caught, int
startSequenceNum) {
+ ErrorCallback callback = networkErrorCallback;
+ networkErrorCallback = null;
+ cacheOnComplete = false;
+ state = STATE_COMPLETE_FETCH;
+ activity.supportInvalidateOptionsMenu();
+ if (!sequenceNumber.inSync(startSequenceNum)) {
+ return;
+ }
+ if (callback != null) {
+ callback.call(caught);
+ }
+ }
+
+ private void loadSavedPage(final ErrorCallback errorCallback) {
+ new LoadSavedPageTask(model.getTitle(), sequenceNumber.get()) {
+ @Override
+ public void onFinish(Page result) {
+ if (!fragment.isAdded() ||
!sequenceNumber.inSync(getSequence())) {
+ return;
+ }
+
+ // Save history entry and page image url
+ new SaveHistoryTask(model.getCurEntry(), app).execute();
+
+ model.setPage(result);
+ editHandler.setPage(model.getPage());
+ layoutLeadImage(new Runnable() {
+ @Override
+ public void run() {
+ displayNonLeadSectionForSavedPage(1);
+ leadImagesHandler.updateBookmark(true);
+
+ setState(STATE_COMPLETE_FETCH);
+ }
+ });
+ }
+
+ @Override
+ public void onCatch(Throwable caught) {
+ errorCallback.call(caught);
+ }
+ }.execute();
}
private void setupSpecificMessageHandlers() {
@@ -210,58 +389,6 @@
(ViewGroup)
fragment.getView().findViewById(R.id.bottom_content_container));
}
- @Override
- public void backFromEditing(Intent data) {
- //Retrieve section ID from intent, and find correct section, so where
know where to scroll to
- sectionTargetFromIntent =
data.getIntExtra(EditSectionActivity.EXTRA_SECTION_ID, 0);
- //reset our scroll offset, since we have a section scroll target
- stagedScrollY = 0;
- }
-
- @Override
- public void onDisplayNewPage(boolean pushBackStack, Cache cachePreference,
int stagedScrollY) {
- fragment.updatePageInfo(null);
- leadImagesHandler.updateBookmark(false);
- leadImagesHandler.updateNavigate(null);
-
- if (pushBackStack) {
- // update the topmost entry in the backstack, before we start
overwriting things.
- updateCurrentBackStackItem();
- pushBackStack();
- }
-
- state = STATE_NO_FETCH;
-
- // increment our sequence number, so that any async tasks that depend
on the sequence
- // will invalidate themselves upon completion.
- sequenceNumber.increase();
-
- if (cachePreference == Cache.NONE) {
- // If this is a refresh, don't clear the webview contents
- this.stagedScrollY = stagedScrollY;
- loadPageOnWebViewReady(cachePreference);
- } else {
- // kick off an event to the WebView that will cause it to clear
its contents,
- // and then report back to us when the clearing is complete, so
that we can synchronize
- // the transitions of our native components to the new page
content.
- // The callback event from the WebView will then call the
loadPageOnWebViewReady()
- // function, which will continue the loading process.
- leadImagesHandler.hide();
- bottomContentHandler.hide();
- activity.getSearchBarHideHandler().setFadeEnabled(false);
- try {
- JSONObject wrapper = new JSONObject();
- // whatever we pass to this event will be passed back to us by
the WebView!
- wrapper.put("sequence", sequenceNumber.get());
- wrapper.put("cachePreference", cachePreference.name());
- wrapper.put("stagedScrollY", stagedScrollY);
- bridge.sendMessage("beginNewPage", wrapper);
- } catch (JSONException e) {
- L.logRemoteErrorIfProd(e);
- }
- }
- }
-
private void performActionForState(int forState) {
if (!fragment.isAdded()) {
return;
@@ -284,7 +411,6 @@
});
break;
default:
- // This should never happen
throw new RuntimeException("Unknown state encountered " +
state);
}
}
@@ -310,16 +436,11 @@
@Override
public void onPutError(Throwable e) {
- Log.e(TAG, "Failed to add page to cache.", e);
+ L.e("Failed to add page to cache.", e);
}
});
}
}
- }
-
- @Override
- public boolean isLoading() {
- return state != STATE_COMPLETE_FETCH;
}
private void loadPageOnWebViewReady(Cache cachePreference) {
@@ -340,7 +461,7 @@
If anything bad happens during loading of a saved page,
then simply bounce it
back to the online version of the page, and re-save the
page contents locally when it's done.
*/
- Log.d("LoadSavedPageTask", "Error loading saved page: " +
error.getMessage());
+ L.d("Error loading saved page: ", error);
error.printStackTrace();
fragment.refreshPage(true);
}
@@ -406,8 +527,7 @@
return;
}
if (page != null) {
- Log.d(TAG, "Using page from cache: "
- +
model.getTitleOriginal().getDisplayText());
+ L.d("Using page from cache: " +
model.getTitleOriginal().getDisplayText());
model.setPage(page);
model.setTitle(page.getTitle());
// Update our history entry, in case the Title was
changed (i.e. normalized)
@@ -433,7 +553,7 @@
@Override
public void onGetError(Throwable e, int sequence) {
- Log.e(TAG, "Failed to get page from cache.", e);
+ L.e("Failed to get page from cache.", e);
if (!sequenceNumber.inSync(sequence)) {
return;
}
@@ -448,37 +568,6 @@
cacheOnComplete = true;
setState(STATE_NO_FETCH);
performActionForState(state);
- }
-
- public void loadSavedPage(final ErrorCallback errorCallback) {
- new LoadSavedPageTask(model.getTitle(), sequenceNumber.get()) {
- @Override
- public void onFinish(Page result) {
- if (!fragment.isAdded() ||
!sequenceNumber.inSync(getSequence())) {
- return;
- }
-
- // Save history entry and page image url
- new SaveHistoryTask(model.getCurEntry(), app).execute();
-
- model.setPage(result);
- editHandler.setPage(model.getPage());
- layoutLeadImage(new Runnable() {
- @Override
- public void run() {
- displayNonLeadSectionForSavedPage(1);
- leadImagesHandler.updateBookmark(true);
-
- setState(STATE_COMPLETE_FETCH);
- }
- });
- }
-
- @Override
- public void onCatch(Throwable caught) {
- errorCallback.call(caught);
- }
- }.execute();
}
private void updateThumbnail(String thumbUrl) {
@@ -508,45 +597,6 @@
private void pushBackStack() {
PageBackStackItem item = new
PageBackStackItem(model.getTitleOriginal(), model.getCurEntry());
backStack.add(item);
- }
-
- /**
- * Update the current topmost backstack item, based on the currently
displayed page.
- * (Things like the last y-offset position should be updated here)
- * Should be done right before loading a new page.
- */
- @Override
- public void updateCurrentBackStackItem() {
- if (backStack.isEmpty()) {
- return;
- }
- PageBackStackItem item = backStack.get(backStack.size() - 1);
- item.setScrollY(webView.getScrollY());
- }
-
- @Override
- public void loadPageFromBackStack() {
- if (backStack.isEmpty()) {
- return;
- }
- PageBackStackItem item = backStack.get(backStack.size() - 1);
- // display the page based on the backstack item, stage the scrollY
position based on
- // the backstack item.
- fragment.displayNewPage(item.getTitle(), item.getHistoryEntry(),
Cache.PREFERRED, false,
- item.getScrollY());
- Log.d(TAG, "Loaded page " + item.getTitle().getDisplayText() + " from
backstack");
- }
-
- @Override
- public void layoutLeadImage() {
- leadImagesHandler.beginLayout(new
LeadImagesHandler.OnLeadImageLayoutListener() {
- @Override
- public void onLayoutComplete(int sequence) {
- if (fragment.isAdded()) {
-
searchBarHideHandler.setFadeEnabled(leadImagesHandler.isLeadImageEnabled());
- }
- }
- }, sequenceNumber.get());
}
private void layoutLeadImage(@Nullable Runnable runnable) {
@@ -590,7 +640,7 @@
private void sendLeadSectionPayload(Page page) {
JSONObject leadSectionPayload = leadSectionPayload(page);
bridge.sendMessage("displayLeadSection", leadSectionPayload);
- Log.d(TAG, "Sent message 'displayLeadSection' for page: " +
page.getDisplayTitle());
+ L.d("Sent message 'displayLeadSection' for page: " +
page.getDisplayTitle());
}
private JSONObject leadSectionPayload(Page page) {
@@ -660,8 +710,7 @@
private void displayNonLeadSection(int index, boolean savedPage) {
activity.updateProgressBar(true, false,
- PageActivity.PROGRESS_BAR_MAX_VALUE / model.getPage()
- .getSections().size() * index);
+ PageActivity.PROGRESS_BAR_MAX_VALUE /
model.getPage().getSections().size() * index);
if (index > model.getPage().getSections().size()) {
// TODO: Remove this check if we find that it no longer happens,
or if we fix it.
String errorText = "Section index mismatch!";
@@ -709,29 +758,6 @@
} catch (JSONException e) {
L.logRemoteErrorIfProd(e);
}
- }
-
- @VisibleForTesting
- protected void loadLeadSection(final int startSequenceNum) {
- app.getSessionFunnel().leadSectionFetchStart();
- PageLoadUtil.getApiService(model.getTitle().getSite()).pageLead(
- model.getTitle().getPrefixedText(),
- calculateLeadImageWidth(),
- !app.isImageDownloadEnabled(),
- new PageLead.Callback() {
- @Override
- public void success(PageLead pageLead, Response response) {
- Log.v(TAG, response.getUrl());
- app.getSessionFunnel().leadSectionFetchEnd();
- onLeadSectionLoaded(pageLead, startSequenceNum);
- }
-
- @Override
- public void failure(RetrofitError error) {
- Log.e(TAG, "PageLead error: " + error);
- commonSectionFetchOnCatch(error, startSequenceNum);
- }
- });
}
private void onLeadSectionLoaded(PageLead pageLead, int startSequenceNum) {
@@ -800,14 +826,14 @@
new PageRemaining.Callback() {
@Override
public void success(PageRemaining pageRemaining, @NonNull
Response response) {
- Log.v(TAG, response.getUrl());
+ L.v(response.getUrl());
app.getSessionFunnel().restSectionsFetchEnd();
onRemainingSectionsLoaded(pageRemaining,
startSequenceNum);
}
@Override
public void failure(RetrofitError error) {
- Log.e(TAG, "PageRemaining error: " + error);
+ L.e("PageRemaining error: ", error);
commonSectionFetchOnCatch(error, startSequenceNum);
}
});
@@ -825,44 +851,6 @@
setState(STATE_COMPLETE_FETCH);
fragment.onPageLoadComplete();
- }
-
- @VisibleForTesting
- protected void commonSectionFetchOnCatch(Throwable caught, int
startSequenceNum) {
- ErrorCallback callback = networkErrorCallback;
- networkErrorCallback = null;
- cacheOnComplete = false;
- state = STATE_COMPLETE_FETCH;
- activity.supportInvalidateOptionsMenu();
- if (!sequenceNumber.inSync(startSequenceNum)) {
- return;
- }
- if (callback != null) {
- callback.call(caught);
- }
- }
-
- /**
- * Convenience method for hiding all the content of a page.
- */
- @Override
- public void onHidePageContent() {
- bottomContentHandler.hide();
- }
-
- @Override
- public boolean onBackPressed() {
- popBackStack();
- if (!backStack.isEmpty()) {
- loadPageFromBackStack();
- return true;
- }
- return false;
- }
-
- @Override
- public void setEditHandler(EditHandler editHandler) {
- this.editHandler = editHandler;
}
private float getDimension(@DimenRes int id) {
diff --git a/app/src/main/java/org/wikipedia/page/PageLoadStrategy.java
b/app/src/main/java/org/wikipedia/page/PageLoadStrategy.java
index 8baa389..ea2bbad 100644
--- a/app/src/main/java/org/wikipedia/page/PageLoadStrategy.java
+++ b/app/src/main/java/org/wikipedia/page/PageLoadStrategy.java
@@ -51,6 +51,7 @@
boolean isLoading();
+ /** Convenience method for hiding all the content of a page. */
void onHidePageContent();
boolean onBackPressed();
@@ -59,6 +60,11 @@
void setBackStack(@NonNull List<PageBackStackItem> backStack);
+ /**
+ * Update the current topmost backstack item, based on the currently
displayed page.
+ * (Things like the last y-offset position should be updated here)
+ * Should be done right before loading a new page.
+ */
void updateCurrentBackStackItem();
void loadPageFromBackStack();
--
To view, visit https://gerrit.wikimedia.org/r/258638
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8c5abc3e855c33d55d592ae572b538e56539c6a5
Gerrit-PatchSet: 1
Gerrit-Project: apps/android/wikipedia
Gerrit-Branch: master
Gerrit-Owner: Niedzielski <sniedziel...@wikimedia.org>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
