Viewing new rub3 permenantly instead of just poping it up #687

Ahmed9914 · 2016-09-23T00:20:24Z

Asalamu 3alaikum,

The current situation:
When the user reaches a new rub3, a toast will pop up showing this rub3 number. If the user took some time reading the current page and wanted to know in which rub3 he was, he has to swipe left and then swipe right to show the toast again and I think that this is not user-friendly.

The suggested change:
There is a space in the top middle of the page, so why we don't show the current rub3 in this space upon reaching it.

Wa jazakum allaho khairan.

ozbek · 2016-09-23T06:21:36Z

app/src/main/java/com/quran/labs/androidquran/widgets/HighlightingImageView.java

  }

-  public void setOverlayText(String suraText, String juzText, String pageText) {
+  public void setOverlayText(String suraText, String juzText, String pageText,String rub3Text) {


ozbek

Please check the code style and adhere to the rules used in existing code.

ozbek · 2016-09-23T06:21:49Z

app/src/main/java/com/quran/labs/androidquran/widgets/HighlightingImageView.java

@@ -178,6 +181,7 @@ public void setOverlayText(String suraText, String juzText, String pageText) {
    mOverlayParams.suraText = suraText;
    mOverlayParams.juzText = juzText;
    mOverlayParams.pageText = pageText;
+    mOverlayParams.rub3Text=rub3Text;


ozbek · 2016-09-23T06:23:15Z

app/src/main/java/com/quran/labs/androidquran/widgets/HighlightingImageView.java

+          canvas.drawText(mOverlayParams.rub3Text,
+                  getWidth() / 2.0f, mOverlayParams.topBaseline,
+                  mOverlayParams.paint);
+      }


space: does not seem to align with other lines

ozbek · 2016-09-23T06:24:11Z

app/src/main/java/com/quran/labs/androidquran/ui/helpers/QuranDisplayHelper.java

@@ -39,6 +39,29 @@ public static Response getQuranPage(OkHttpClient okHttpClient,
    return response;
  }

+  // same logic used in displayMarkerPopup method
+  public static String displayRub3(Context context, int page)
+  {


Please use the same line as method declaration for opening bracket

ozbek · 2016-09-23T06:25:15Z

app/src/main/java/com/quran/labs/androidquran/ui/helpers/QuranDisplayHelper.java

@@ -39,6 +39,29 @@ public static Response getQuranPage(OkHttpClient okHttpClient,
    return response;
  }

+  // same logic used in displayMarkerPopup method


I think, for this comment to make sense, displayRub3() needs to come below/after displayMarkerPopup()

ozbek · 2016-09-23T06:27:27Z

app/src/main/java/com/quran/labs/androidquran/ui/fragment/TabletFragment.java

@@ -319,16 +320,18 @@ protected void onPostExecute(RectF[] rect) {
          if (mRightImageView != null && mLeftImageView != null) {
            mRightImageView.setPageBounds(rect[0]);
            mLeftImageView.setPageBounds(rect[1]);
-            Context context = getContext();
-            if (mOverlayText && context != null) {


+1!
There is no point in checking if context is null. If it is null, then something is seriously broken beyond repair.

i intentionally added that null check because we were having crashes due to this, so please re-add this back.

because this is in onPostExecute, we can easily have a case where the fragment is killed before the AsyncTask finishes - when the AsyncTask is done, there is no context because the Fragment is gone, so we should just bail - by not doing this check, we'll crash in the next few lines where we try to use the context to get sura names, etc.

I see.

I think, for that scenario, checking if fragment is being removed (isRemoving()) or if it is visible (isVisible()) would fit better.

good call, i agree. in QuranPageFragment, we're checking isAdded() and that seems to be working fine (no crash there), so am good with wrapping the entire block with an isAdded() check.

ahmedre · 2016-09-23T07:05:06Z

i am not against this patch conceptually, but have a few suggestions (aside from addressing the comments) - i think this will look strange in the top middle. my suggestion instead is either:
a. merge it with the juz' text at the top right or
b. move the page number to the left (or right, depending on the side of the page the page is on), and on the opposite side, put the hizb info.

ahmedre · 2016-09-23T07:05:33Z

(oh, and most importantly, jazakumAllah khairan for doing this!)

Ahmed9914 · 2016-09-23T10:45:17Z

First, I'm sorry about not following some styling rules.
Second, @ahmedre I think option "a" is better and I'm working on it.
JazkomoAllah Khairan.

Ahmed9914

changes done

Ahmed9914

spacing change done

Ahmed9914

Removed unnecessary duplicate and clarified more on the comment

ozbek · 2016-09-27T08:52:39Z

app/src/main/java/com/quran/labs/androidquran/ui/fragment/TabletFragment.java

@@ -319,16 +320,18 @@ protected void onPostExecute(RectF[] rect) {
          if (mRightImageView != null && mLeftImageView != null) {
            mRightImageView.setPageBounds(rect[0]);
            mLeftImageView.setPageBounds(rect[1]);
-            Context context = getContext();
-            if (mOverlayText && context != null) {
+            if (mOverlayText) {


Please add isAdded() check.

oh, sorry I totally forgot that.
now added.

ozbek · 2016-09-27T08:54:27Z

app/src/main/java/com/quran/labs/androidquran/ui/helpers/QuranDisplayHelper.java

+    int hizb = (rub3 / 4) + 1;
+    StringBuilder sb = new StringBuilder();
+    sb.append(" , ");
+    if (rub3 == -1) {


Can we do this check earlier (right after line 78)?

ozbek · 2016-09-27T09:00:32Z

app/src/main/java/com/quran/labs/androidquran/ui/helpers/QuranDisplayHelper.java

+    if (rub3 == -1) {
+      return "";
+    }
+    int remainder = rub3 % 4;


For the sake of readability (just a suggestion):

switch(rub3%4) { case 1: sb.append(context.getString(R.string.quran_rob3)).append(' '); break; case 2: sb.append(context.getString(R.string.quran_nos)).append(' '); break; case 3: sb.append(context.getString(R.string.quran_talt_arb3)).append(' '); break; default: // case 0 // do nothing }

No need for this check, as it's already checked in 'QuranDisplayHelper.displayRub3' method

Ahmed9914

edited

hakimrie · 2016-09-28T00:00:49Z

app/src/main/java/com/quran/labs/androidquran/ui/helpers/QuranDisplayHelper.java

+      sb.append(context.getString(R.string.quran_nos)).append(' ');
+    } else if (remainder == 3) {
+      sb.append(context.getString(R.string.quran_talt_arb3)).append(' ');
+    }


I think it has too many magic number here, it will be better and more readable if we define 1, 2, 3 as constant variable.

ozbek · 2016-09-28T09:05:30Z

app/src/main/java/com/quran/labs/androidquran/ui/fragment/TabletFragment.java

@@ -320,15 +321,17 @@ protected void onPostExecute(RectF[] rect) {
            mRightImageView.setPageBounds(rect[0]);
            mLeftImageView.setPageBounds(rect[1]);
            Context context = getContext();
-            if (mOverlayText && context != null) {
+            if (mOverlayText && isAdded()) {


+1! Now, since context is not used outside, you could move Context context = getContext(); to inside the brackets (your original change). Sorry for sounding like nitpicking :(

May Allah rewards you for your great effort in improving this app.

ozbek · 2016-09-28T09:06:10Z

app/src/main/java/com/quran/labs/androidquran/widgets/HighlightingImageView.java

@@ -178,6 +181,7 @@ public void setOverlayText(String suraText, String juzText, String pageText) {
    mOverlayParams.suraText = suraText;
    mOverlayParams.juzText = juzText;
    mOverlayParams.pageText = pageText;
+    mOverlayParams.rub3Text=rub3Text;


spacing is not fixed yet

ahmedre · 2016-10-04T05:10:55Z

merging to unblock, jazakAllah khairan.
if you get a chance, please also do @hakimrie's suggestion (in a new patch).

hulk9914 added 6 commits September 23, 2016 02:11

Add files via upload

95f65cc

Add files via upload

1d9f8cf

Add files via upload

16afeda

Add files via upload

a179690

Add files via upload

efb02e6

Add files via upload

95505a4

ozbek reviewed Sep 23, 2016

View reviewed changes

ozbek suggested changes Sep 23, 2016

View reviewed changes

Ahmed9914 closed this Sep 23, 2016

Ahmed9914 reopened this Sep 23, 2016

Ahmed Abdelaal added 3 commits September 23, 2016 13:47

Update QuranDisplayHelper.java

251c19d

Update HighlightingImageView.java

35ef9a1

Update HighlightingImageView.java

e22a589

Ahmed9914 commented Sep 23, 2016

View reviewed changes

Update QuranPageFragment.java

de92385

Ahmed9914 commented Sep 23, 2016

View reviewed changes

Update HighlightingImageView.java

cf7a95f

Ahmed9914 commented Sep 23, 2016

View reviewed changes

ozbek suggested changes Sep 27, 2016

View reviewed changes

Ahmed Abdelaal added 4 commits September 27, 2016 19:32

Update TabletFragment.java

84b8054

Update QuranDisplayHelper.java

dc50e55

Update HighlightingImageView.java

67f7c2c

No need for this check, as it's already checked in 'QuranDisplayHelper.displayRub3' method

Update HighlightingImageView.java

008cf28

Ahmed9914 commented Sep 27, 2016

View reviewed changes

hakimrie reviewed Sep 28, 2016

View reviewed changes

ozbek reviewed Sep 28, 2016

View reviewed changes

Update TabletFragment.java

a73abc3

Update HighlightingImageView.java

ae6ed35

ahmedre merged commit 415569b into quran:master Oct 4, 2016

ahmedre pushed a commit that referenced this pull request Oct 14, 2017

Viewing new rub3 permenantly instead of just poping it up (#687)

2163a43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Viewing new rub3 permenantly instead of just poping it up #687

Viewing new rub3 permenantly instead of just poping it up #687

Ahmed9914 commented Sep 23, 2016 •

edited

ozbek Sep 23, 2016

ozbek left a comment

ozbek Sep 23, 2016

ozbek Sep 23, 2016

ozbek Sep 23, 2016

ozbek Sep 23, 2016

ozbek Sep 23, 2016 •

edited

ahmedre Sep 23, 2016

ozbek Sep 23, 2016

ahmedre Sep 23, 2016

ahmedre commented Sep 23, 2016

ahmedre commented Sep 23, 2016

Ahmed9914 commented Sep 23, 2016

Ahmed9914 left a comment

Ahmed9914 left a comment

Ahmed9914 left a comment

ozbek Sep 27, 2016

Ahmed9914 Sep 27, 2016

ozbek Sep 27, 2016

Ahmed9914 Sep 27, 2016

ozbek Sep 27, 2016

Ahmed9914 left a comment

hakimrie Sep 28, 2016 •

edited

ozbek Sep 28, 2016

Ahmed9914 Sep 29, 2016

ozbek Sep 28, 2016 •

edited

Ahmed9914 Sep 29, 2016

ahmedre commented Oct 4, 2016

Viewing new rub3 permenantly instead of just poping it up #687

Viewing new rub3 permenantly instead of just poping it up #687

Conversation

Ahmed9914 commented Sep 23, 2016 • edited

Choose a reason for hiding this comment

ozbek 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

ozbek Sep 23, 2016 • edited

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

ahmedre commented Sep 23, 2016

ahmedre commented Sep 23, 2016

Ahmed9914 commented Sep 23, 2016

Ahmed9914 left a comment

Choose a reason for hiding this comment

Ahmed9914 left a comment

Choose a reason for hiding this comment

Ahmed9914 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

Ahmed9914 left a comment

Choose a reason for hiding this comment

hakimrie Sep 28, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ozbek Sep 28, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahmedre commented Oct 4, 2016

Ahmed9914 commented Sep 23, 2016 •

edited

ozbek Sep 23, 2016 •

edited

hakimrie Sep 28, 2016 •

edited

ozbek Sep 28, 2016 •

edited