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.

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

Use NumberPicker for Range and Verse Repeat Selection #1519

Closed
wants to merge 2 commits into from

Conversation

benomaire
Copy link
Contributor

@benomaire benomaire commented Dec 30, 2020

This is a replacement of #1220 Thanks to @fnzainal for starting this.
Should fix #985
Now we have 99 options for repeating. And yes, we lost the suffix 'time' or 'times'.
For Arabic, I used a string array to flip the visual appearance of the horizontal number picker.

Copy link
Contributor

@ahmedre ahmedre left a comment

Choose a reason for hiding this comment

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

haven't tested this yet, but some initial comments

// Using Eastern Arabic Numerals
isArabicNames = QuranSettings.getInstance(context).isArabicNames();
if (isArabicNames){
String[] arNums = {"٩٩","٩٨","٩٧","٩٦","٩٥","٩٤","٩٣","٩٢","٩١","٩٠","٨٩","٨٨","٨٧","٨٦","٨٥","٨٤","٨٣","٨٢","٨١","٨٠","٧٩","٧٨","٧٧","٧٦","٧٥","٧٤","٧٣","٧٢","٧١","٧٠","٦٩","٦٨","٦٧","٦٦","٦٥","٦٤","٦٣","٦٢","٦١","٦٠","٥٩","٥٨","٥٧","٥٦","٥٥","٥٤","٥٣","٥٢","٥١","٥٠","٤٩","٤٨","٤٧","٤٦","٤٥","٤٤","٤٣","٤٢","٤١","٤٠","٣٩","٣٨","٣٧","٣٦","٣٥","٣٤","٣٣","٣٢","٣١","٣٠","٢٩","٢٨","٢٧","٢٦","٢٥","٢٤","٢٣","٢٢","٢١","٢٠","١٩","١٨","١٧","١٦","١٥","١٤","١٣","١٢","١١","١٠","٩","٨","٧","٦","٥","٤","٣","٢","١"};
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of this, can we just do:

val numberFormatter = NumberFormat.getInstance(Locale.ARABIC);
numberFormatter.format(number)

instead of having this array of numbers? as a matter of fact, guessing if we don't pass the locale, it gets it from the default locale, which we should be overriding to Arabic already.

if (isArabicNames){
String[] arNums = {"٩٩","٩٨","٩٧","٩٦","٩٥","٩٤","٩٣","٩٢","٩١","٩٠","٨٩","٨٨","٨٧","٨٦","٨٥","٨٤","٨٣","٨٢","٨١","٨٠","٧٩","٧٨","٧٧","٧٦","٧٥","٧٤","٧٣","٧٢","٧١","٧٠","٦٩","٦٨","٦٧","٦٦","٦٥","٦٤","٦٣","٦٢","٦١","٦٠","٥٩","٥٨","٥٧","٥٦","٥٥","٥٤","٥٣","٥٢","٥١","٥٠","٤٩","٤٨","٤٧","٤٦","٤٥","٤٤","٤٣","٤٢","٤١","٤٠","٣٩","٣٨","٣٧","٣٦","٣٥","٣٤","٣٣","٣٢","٣١","٣٠","٢٩","٢٨","٢٧","٢٦","٢٥","٢٤","٢٣","٢٢","٢١","٢٠","١٩","١٨","١٧","١٦","١٥","١٤","١٣","١٢","١١","١٠","٩","٨","٧","٦","٥","٤","٣","٢","١"};
numOfOptions = arNums.length;
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.KITKAT) {
Copy link
Contributor

Choose a reason for hiding this comment

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

probably don't need a version check here since these are just numbers - only restricting the tafseer text because the tashkeel renders strange on older versions, numbers should be ok though

@@ -96,17 +96,30 @@
android:text="@string/play_verses_range"
android:textAppearance="@style/PanelLabel"
/>
<com.quran.labs.androidquran.view.QuranSpinner
android:id="@+id/repeat_range_spinner"
<com.shawnlin.numberpicker.NumberPicker
Copy link
Contributor

Choose a reason for hiding this comment

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

what do you think about using he default Android NumberPicker? the only downside is we can't style it on api levels before 24, but i think that is ok?

@ahmedre
Copy link
Contributor

ahmedre commented Jan 6, 2021

also jazakumAllah khairan for pushing this forward!

@benomaire
Copy link
Contributor Author

Replying to all your very resonable comments:

  1. I want a horizontal number picker, hopefully looking the same on all devices. This answer looks discouraging for the default NumberPicker. The used NumberPicker is easily customizable and works on API 15.
  2. I agree the string array is very ugly. But the main reason is I flip the numbers. So the NumberPicker looks to be from right to left (for Arabic users). I actually did not notice setOrder, and that is mainly why I did that ugly solution.

So the only thing remaining is whether we keep the external NumberPicker.

Copy link
Contributor

@ahmedre ahmedre left a comment

Choose a reason for hiding this comment

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

جزاكم الله خيراً
really sorry for the very late review - just tried this and it looks good, barak Allah feekum - one minor thing - may we make the font size smaller in English? please see:

Screenshot_1612044041

also - maybe let's make the max 25? 99 seems a bit excessive?
جزاكم الله خيراً

@ahmedre
Copy link
Contributor

ahmedre commented Jan 30, 2021

i made a small change here - combined all these layouts into 2 layouts - one for play_each_verse and one for play_set_of_verses so we can change one instead of changing all of them

@ahmedre
Copy link
Contributor

ahmedre commented Jan 30, 2021

merged at b8ebffe and some other commits.
please do see if you can fix the font size in English though - maybe a dimen for the text size that's different for Arabic and English - jazakumAllah khairan

@benomaire
Copy link
Contributor Author

also - maybe let's make the max 25? 99 seems a bit excessive?

But you had the infinite loop before :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Making a custom entries in the number of ayah looping
2 participants