Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 17 commits into from
Oct 4, 2016
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
95f65cc
Add files via upload
Sep 22, 2016
1d9f8cf
Add files via upload
Sep 22, 2016
16afeda
Add files via upload
Sep 22, 2016
a179690
Add files via upload
Sep 23, 2016
efb02e6
Add files via upload
Sep 23, 2016
95505a4
Add files via upload
Sep 23, 2016
251c19d
Update QuranDisplayHelper.java
Sep 23, 2016
35ef9a1
Update HighlightingImageView.java
Sep 23, 2016
e22a589
Update HighlightingImageView.java
Sep 23, 2016
de92385
Update QuranPageFragment.java
Sep 23, 2016
cf7a95f
Update HighlightingImageView.java
Sep 23, 2016
84b8054
Update TabletFragment.java
Sep 27, 2016
dc50e55
Update QuranDisplayHelper.java
Sep 27, 2016
67f7c2c
Update HighlightingImageView.java
Sep 27, 2016
008cf28
Update HighlightingImageView.java
Sep 27, 2016
a73abc3
Update TabletFragment.java
Sep 29, 2016
ae6ed35
Update HighlightingImageView.java
Sep 29, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion app/src/main/java/com/quran/labs/androidquran/ui/fragment/QuranPageFragment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import com.quran.labs.androidquran.ui.helpers.AyahSelectedListener;
import com.quran.labs.androidquran.ui.helpers.AyahTracker;
import com.quran.labs.androidquran.ui.helpers.HighlightType;
import com.quran.labs.androidquran.ui.helpers.QuranDisplayHelper;
import com.quran.labs.androidquran.ui.helpers.QuranPageWorker;
import com.quran.labs.androidquran.ui.util.ImageAyahUtils;
import com.quran.labs.androidquran.ui.util.PageController;
Expand Down Expand Up @@ -301,7 +302,8 @@ protected void onPostExecute(RectF[] rect) {
String suraText = QuranInfo.getSuraNameFromPage(context, page, true);
String juzText = QuranInfo.getJuzString(context, page);
String pageText = QuranUtils.getLocalizedNumber(context, page);
mImageView.setOverlayText(suraText, juzText, pageText);
String rub3Text = QuranDisplayHelper.displayRub3(context,page);
mImageView.setOverlayText(suraText, juzText, pageText, rub3Text);
}
}
}
Expand Down
11 changes: 7 additions & 4 deletions app/src/main/java/com/quran/labs/androidquran/ui/fragment/TabletFragment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import com.quran.labs.androidquran.ui.helpers.AyahSelectedListener;
import com.quran.labs.androidquran.ui.helpers.AyahTracker;
import com.quran.labs.androidquran.ui.helpers.HighlightType;
import com.quran.labs.androidquran.ui.helpers.QuranDisplayHelper;
import com.quran.labs.androidquran.ui.helpers.QuranPageWorker;
import com.quran.labs.androidquran.ui.util.ImageAyahUtils;
import com.quran.labs.androidquran.ui.util.PageController;
Expand Down Expand Up @@ -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 && isAdded()) {
Copy link
Contributor

@ozbek ozbek Sep 23, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+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 :(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Context context = getContext();
String suraText = QuranInfo.getSuraNameFromPage(context, mPageNumber - 1, true);
String juzText = QuranInfo.getJuzString(context, mPageNumber - 1);
String pageText = QuranUtils.getLocalizedNumber(context, mPageNumber - 1);
mRightImageView.setOverlayText(suraText, juzText, pageText);
String rub3Text =QuranDisplayHelper.displayRub3(context,mPageNumber - 1);
mRightImageView.setOverlayText(suraText, juzText, pageText, rub3Text);
suraText = QuranInfo.getSuraNameFromPage(context, mPageNumber, true);
juzText = QuranInfo.getJuzString(context, mPageNumber);
pageText = QuranUtils.getLocalizedNumber(context, mPageNumber);
mLeftImageView.setOverlayText(suraText, juzText, pageText);
rub3Text = QuranDisplayHelper.displayRub3(context,mPageNumber);
mLeftImageView.setOverlayText(suraText, juzText, pageText, rub3Text);
}
}
}
Expand Down
23 changes: 23 additions & 0 deletions app/src/main/java/com/quran/labs/androidquran/ui/helpers/QuranDisplayHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,29 @@ public static long displayMarkerPopup(Context context, int page,
return System.currentTimeMillis();
}

// same logic used in displayMarkerPopup method
public static String displayRub3(Context context, int page){
int rub3 = QuranInfo.getRub3FromPage(page);
if (rub3 == -1) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

return "";
}
int hizb = (rub3 / 4) + 1;
StringBuilder sb = new StringBuilder();
sb.append(" , ");
int remainder = rub3 % 4;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

if (remainder == 1) {
sb.append(context.getString(R.string.quran_rob3)).append(' ');
} else if (remainder == 2) {
sb.append(context.getString(R.string.quran_nos)).append(' ');
} else if (remainder == 3) {
sb.append(context.getString(R.string.quran_talt_arb3)).append(' ');
}
Copy link
Contributor

@hakimrie hakimrie Sep 28, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

sb.append(context.getString(R.string.quran_hizb)).append(' ')
.append(QuranUtils.getLocalizedNumber(context, hizb));

return sb.toString();
}

public static PaintDrawable getPaintDrawable(int startX, int endX) {
PaintDrawable drawable = new PaintDrawable();
drawable.setShape(new RectShape());
Expand Down
15 changes: 10 additions & 5 deletions app/src/main/java/com/quran/labs/androidquran/widgets/HighlightingImageView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@
import com.quran.labs.androidquran.R;
import com.quran.labs.androidquran.common.AyahBounds;
import com.quran.labs.androidquran.data.Constants;
import com.quran.labs.androidquran.data.QuranInfo;
import com.quran.labs.androidquran.ui.helpers.HighlightType;
import com.quran.labs.androidquran.util.QuranUtils;

import java.util.HashSet;
import java.util.List;
Expand Down Expand Up @@ -166,9 +168,10 @@ private static class OverlayParams {
String suraText = null;
String juzText = null;
String pageText = null;
String rub3Text = null;
}

public void setOverlayText(String suraText, String juzText, String pageText) {
public void setOverlayText(String suraText, String juzText, String pageText, String rub3Text) {
// Calculate page bounding rect from ayahinfo db
if (mPageBounds == null) {
return;
Expand All @@ -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;
mOverlayParams.paint = new Paint(Paint.ANTI_ALIAS_FLAG | Paint.DEV_KERN_TEXT_FLAG);
mOverlayParams.paint.setTextSize(fontSize);

Expand Down Expand Up @@ -237,14 +241,15 @@ private void overlayText(Canvas canvas, Matrix matrix) {
canvas.drawText(mOverlayParams.suraText,
mOverlayParams.offsetX, mOverlayParams.topBaseline,
mOverlayParams.paint);
mOverlayParams.paint.setTextAlign(Align.RIGHT);
canvas.drawText(mOverlayParams.juzText,
getWidth() - mOverlayParams.offsetX, mOverlayParams.topBaseline,
mOverlayParams.paint);
mOverlayParams.paint.setTextAlign(Align.CENTER);
canvas.drawText(mOverlayParams.pageText,
getWidth() / 2.0f, mOverlayParams.bottomBaseline,
mOverlayParams.paint);
// Merge the current rub3 text with the juz' text
mOverlayParams.paint.setTextAlign(Align.RIGHT);
canvas.drawText(mOverlayParams.juzText + mOverlayParams.rub3Text,
getWidth() - mOverlayParams.offsetX, mOverlayParams.topBaseline,
mOverlayParams.paint);
mDidDraw = true;
}

Expand Down