Add Translation activity to spinner #802

uzairmad · 2017-02-06T17:06:51Z

Added link to translations activity from spinner per #760, please review when you have a chance. thanks

ozbek · 2017-02-06T20:51:13Z

app/src/main/res/menu/home_menu.xml

@@ -50,6 +50,10 @@
      android:title="@string/menu_jump"
      app:showAsAction="never" />
  <item
+      android:id="@+id/translations"
+      android:title="Translations"


Please do not use hard-coded string literals, use one provided in strings.xml.

thanks can fix that, there are items in the home_menu which should be converted as well

Hmm... there should not be. Android Studio shows them as string literals, but actually they are not.

Overall, you will have to redo this patch anyway. Don't put the menu here. "More translations" option should go to translations spinner in translation screen/mode, the one you see at the top when you have more than one translation are installed.

ah ok, i misunderstood that ignore my bottom comment. I can add it in there. jzk

hey so ive looked at this a bit, focusing on TranslationsSpinnerAdapter and PagerActivity. I'm running into issues finding a straightforward solution - the translation spinner has a checkbox dropdown item, and implementing another view to the dropdown such as TextView is non-trivial unless both items are added to a parent view. Additionally, since the spinner has checkboxes, an event listener for OnItemSelectedListener isn't being picked up if implemented in PagerActivity. Please let me know if you have some guidance on this. Thank you

I suppose one will have to build a custom adapter that can hold multiple types of items (checkbox and text, in this case).

this is where the custom adapter is being made. since SpinnerAdapter is a BaseAdapter, you can just override getViewTypeCount to return 2, and getItemViewType to return 0 except for the last row (the custom row you will add), in which case you'll return 1.

then, in getView, if getItemViewType is 1, show a different xml or just a TextView with "get more translations"

hmm, it appears Spinners don't allow for setting this on the later APIs. "Spinner overrides getViewTypeCount() on the Adapter associated with this view. Calling getItemViewType(int) on the object returned from getAdapter() will always return 0. Calling getViewTypeCount() will always return 1. On API LOLLIPOP and above, attempting to set an adapter with more than one view type will throw an IllegalArgumentException." -https://developer.android.com/reference/android/widget/Spinner.html#setAdapter(android.widget.SpinnerAdapter) also https://code.google.com/p/android/issues/detail?id=17128. It looks like there can be a workaround using Tags however which I can look at

I actually have something working, will push later today - overrode getDropDownView as well

ozbek

This will add "Translations" menu, which is not what #760 describes.

ahmedre

jazakAllah khairan for doing this - some comments

ahmedre · 2017-02-28T04:41:16Z

app/src/main/java/com/quran/labs/androidquran/ui/util/TranslationsSpinnerAdapter.java

@@ -36,6 +37,9 @@ public TranslationsSpinnerAdapter(Context context,
    // and the String[] constructor makes a new (immutable) List with the items of the array.
    super(context, resource, new ArrayList<>());
    this.layoutInflater = LayoutInflater.from(context);
+    List<String> transList = new ArrayList<String>(Arrays.asList(translationNames));


List<String> transList = Arrays.asList(translationName);

I tried this, getting java.lang.UnsupportedOperationException on the next line however when trying to add to the list. Is there another way you intended to do this? jzk

ah, makes sense, because Arrays.asList generates an immutable list.

i'd then replace it with a loop - i.e.

List<String> transList = new ArrayList<>(); for (String translation : translationNames) { transList.add(translation); }

ahmedre · 2017-02-28T04:41:46Z

app/src/main/java/com/quran/labs/androidquran/ui/util/TranslationsSpinnerAdapter.java

@@ -36,6 +37,9 @@ public TranslationsSpinnerAdapter(Context context,
    // and the String[] constructor makes a new (immutable) List with the items of the array.
    super(context, resource, new ArrayList<>());
    this.layoutInflater = LayoutInflater.from(context);
+    List<String> transList = new ArrayList<String>(Arrays.asList(translationNames));
+    transList.add("More Translations");


add to strings.xml and use an R.string value

ahmedre · 2017-02-28T04:42:18Z

app/src/main/java/com/quran/labs/androidquran/ui/util/TranslationsSpinnerAdapter.java

@@ -36,6 +37,9 @@ public TranslationsSpinnerAdapter(Context context,
    // and the String[] constructor makes a new (immutable) List with the items of the array.
    super(context, resource, new ArrayList<>());
    this.layoutInflater = LayoutInflater.from(context);
+    List<String> transList = new ArrayList<String>(Arrays.asList(translationNames));
+    transList.add("More Translations");
+    translationNames = transList.toArray(new String[transList.size()]);


remove it, you don't need it

I did this initially as translationNames is also used in addAll(translationNames);, should "More translations" not be a part of the underlying array?

you have two options - one is to add it within the underlying array, and the other is to change the adapter to know that "count is always size + 1," and to know that "once you pass this element, the next element is the more option" - if it's cleaner to add it to the array (i.e. if the array is all Strings, as it seems to be), then it probably makes sense.

jzk, thanks for the clarification!

ahmedre · 2017-02-28T04:42:57Z

app/src/main/java/com/quran/labs/androidquran/ui/util/TranslationsSpinnerAdapter.java

@@ -87,24 +91,35 @@ public View getView(int position, View convertView, @NonNull ViewGroup parent) {
  @Override
  public View getDropDownView(int position, View convertView, ViewGroup parent) {
    CheckBoxHolder holder;
-    if (convertView == null) {
-      convertView = layoutInflater.inflate(
+    convertView = layoutInflater.inflate(


keep wrapped in null check, otherwise, we make a new one every time this is called

ahmedre · 2017-02-28T04:43:41Z

app/src/main/res/menu/home_menu.xml

@@ -50,6 +50,10 @@
      android:title="@string/menu_jump"
      app:showAsAction="never" />


undo changes to this file, not needed.

ahmedre · 2017-02-28T04:45:33Z

app/src/main/java/com/quran/labs/androidquran/ui/util/TranslationsSpinnerAdapter.java

  public void updateItems(String[] translationNames,
                          List<LocalTranslation> translations,
                          Set<String> selectedItems) {
    clear();
+    List<String> transList = new ArrayList<String>(Arrays.asList(translationNames));
+    transList.add("More Translations");
+    translationNames = transList.toArray(new String[transList.size()]);


remove, not needed

ahmedre · 2017-02-28T04:45:46Z

app/src/main/java/com/quran/labs/androidquran/ui/QuranActivity.java

@@ -204,6 +205,11 @@ public boolean onOptionsItemSelected(MenuItem item) {
        jumpToLastPage();
        return true;
      }
+      case R.id.translations: {


remove for now

ahmedre · 2017-02-28T04:46:42Z

app/src/main/java/com/quran/labs/androidquran/ui/PagerActivity.java

+        @Override
+        public View getDropDownView(int position, View convertView, ViewGroup parent) {
+
+          int type = super.getItemViewType(position);


i think you can probably remove this method altogether

ahmedre · 2017-02-28T04:46:50Z

app/src/main/java/com/quran/labs/androidquran/ui/PagerActivity.java

+          if (type == 1) {
+            LayoutInflater inflater = getLayoutInflater();
+            convertView = inflater.inflate(R.layout.translation_ab_spinner_selected, parent, false);
+          }else{


nit, spacing

ahmedre · 2017-02-28T04:47:22Z

app/src/main/java/com/quran/labs/androidquran/ui/PagerActivity.java

+            convertView = inflater.inflate(R.layout.translation_ab_spinner_selected, parent, false);
+          }else{
+            convertView = super.getView(position, convertView, parent);
+            SpinnerHolder holder = (SpinnerHolder) convertView.getTag();


make sure SpinnerHolder lets you get access to the checkbox and then hide/show it

thanks, could you clarify this, I didn't need to change anything here after implementing your other comments. Jzk

ozbek

Mostly cosmetic, but would love to see the changes.

ozbek · 2017-03-01T18:16:49Z

app/src/main/res/values/strings.xml

@@ -174,6 +174,7 @@
  <string name="prefs_highlight_bookmarks_summary">Highlight bookmarked ayahs while reading</string>
  <string name="prefs_translation_text_title">Translation text size</string>
  <string name="prefs_translations">Translations</string>
+  <string name="prefs_more_translations">More Translations</string>


Why do you need this string?

I was basing this off the pattern used for R.string.translations where it referenced <string name="translations">@string/prefs_translations</string>. Otherwise I would've just referenced the string directly. Please let me know if this isn't necessary

prefs_translations is used in the Settings, prefs_more_translations is not used and not needed. Please remove it.

will do thanks

ozbek · 2017-03-01T18:17:57Z

app/src/main/java/com/quran/labs/androidquran/ui/util/TranslationsSpinnerAdapter.java

+  }
+
+  public String[] updateTranslationNames(String[] translationNames) {
+


Unnecessary empty line?

jzk, yea this doesn't match the other methods, removing

ozbek · 2017-03-01T18:20:47Z

app/src/main/java/com/quran/labs/androidquran/ui/util/TranslationsSpinnerAdapter.java

+
+  public String[] updateTranslationNames(String[] translationNames) {
+
+    List<String> transList = new ArrayList<>();


Nitpick: I don't like the trans prefix. Can you make it translationsList or (since, it is a local variable) simply list would also work.

jzk, made into translationsList

ozbek · 2017-03-01T18:21:10Z

app/src/main/java/com/quran/labs/androidquran/ui/util/TranslationsSpinnerAdapter.java

+      holder.checkBox.setText(translationNames[position]);
+      holder.checkBox.setChecked(selectedItems.contains(translations.get(position).filename));
+      holder.checkBox.setOnClickListener(onCheckedChangeListener);
+


ozbek · 2017-03-01T18:21:29Z

app/src/main/java/com/quran/labs/androidquran/ui/util/TranslationsSpinnerAdapter.java

+      holder.checkBox.setButtonDrawable(transparentDrawable);
+      holder.checkBox.setText(R.string.more_translations);
+      holder.checkBox.setOnClickListener(this.textListener);
+


Unnecessary empty line.

ozbek · 2017-03-01T18:23:02Z

app/src/main/java/com/quran/labs/androidquran/ui/PagerActivity.java

+            String desc = QuranInfo.getPageSubtitle(PagerActivity.this, page);
+            holder.subtitle.setText(desc);
+            holder.subtitle.setVisibility(View.VISIBLE);
+


Unnecessary empty line.

ozbek

JazakAllah khair, looks good to me 👍

ozbek · 2017-03-01T19:19:02Z

app/src/main/java/com/quran/labs/androidquran/ui/PagerActivity.java

@@ -41,6 +41,8 @@
 import android.view.View;
 import android.view.ViewGroup;
 import android.view.WindowManager;
+import android.view.LayoutInflater;
+import android.widget.TextView;


Looks like this is not used.

ozbek · 2017-03-01T19:30:34Z

app/src/main/java/com/quran/labs/androidquran/ui/util/TranslationsSpinnerAdapter.java


 import com.quran.labs.androidquran.R;
 import com.quran.labs.androidquran.common.LocalTranslation;

 import java.util.ArrayList;
+import java.util.Arrays;


Unused import

ozbek · 2017-03-01T19:30:50Z

app/src/main/java/com/quran/labs/androidquran/ui/util/TranslationsSpinnerAdapter.java

+    this.textListener = onClickListener;
+  }
+
+  public String[] updateTranslationNames(String[] translationNames) {


Please make it private

ozbek · 2017-03-01T19:31:44Z

app/src/main/java/com/quran/labs/androidquran/ui/QuranActivity.java

@@ -35,6 +35,7 @@
 import com.quran.labs.androidquran.model.bookmark.RecentPageModel;
 import com.quran.labs.androidquran.presenter.translation.TranslationManagerPresenter;
 import com.quran.labs.androidquran.service.AudioService;
+import com.quran.labs.androidquran.ui.TranslationManagerActivity;


Unused import.

ozbek · 2017-03-01T19:37:58Z

app/src/main/java/com/quran/labs/androidquran/ui/util/TranslationsSpinnerAdapter.java

@@ -93,18 +99,26 @@ public View getDropDownView(int position, View convertView, ViewGroup parent) {
      convertView.setTag(new CheckBoxHolder(convertView));
    }
    holder = (CheckBoxHolder) convertView.getTag();
+    if (position == translationNames.length - 1) {
+      Drawable transparentDrawable = new ColorDrawable(Color.TRANSPARENT);
+      holder.checkBox.setButtonDrawable(transparentDrawable);


Please remove transparentDrawable and use setButtonDrawable(null) instead.

ozbek · 2017-03-01T19:47:56Z

app/src/main/java/com/quran/labs/androidquran/ui/util/TranslationsSpinnerAdapter.java

+
+  public String[] updateTranslationNames(String[] translationNames) {
+    List<String> translationsList = new ArrayList<>();
+    for (String translation : translationNames) {


Please remove this for loop and use List<String> translationsList = new ArrayList<>(Arrays.asList(translationNames)); instead.

jzk, I made the change to a loop based on @ahmedre earlier comment "i'd then replace it with a loop - i.e.

List transList = new ArrayList<>();
for (String translation : translationNames) {
transList.add(translation);
}" , Please let me know if the replacement is what we want as a final state. thanks

uzairmad · 2017-03-01T19:58:48Z

waeyyak. Thanks for all the feedback

ozbek · 2017-03-01T20:00:48Z

app/src/main/java/com/quran/labs/androidquran/ui/util/TranslationsSpinnerAdapter.java

+
+  private String[] updateTranslationNames(String[] translationNames) {
+    List<String> translationsList = new ArrayList<>();
+    for (String translation : translationNames) {


Please replaced the for loop with the code from previous comment.

ok sure, i had changed it based on Ahmeds comments but i can revert it to this

I see. Let's wait for him on this then. I wonder what's his reasoning for using a for loop.

ok sure thing, my only thought would be readability but ill leave the change out for now

ozbek · 2017-03-01T20:06:25Z

Oops, this has one major side-effect: note the first translation in the screenshot.

uzairmad · 2017-03-01T20:09:03Z

hmm, i didn't see that issue in my test, let me add some more translations and check

uzairmad · 2017-03-01T20:21:45Z

I didn't see it in my emulator but am seeing it on my phone, the previous commit from two days ago doesn't have the issue so it must be tied to how the views are being re-used in getDropDownView, checking now. UPDATE: i think this was a result of not setting the checkbox drawable back in the else block within getDropDownView, finding the correct drawable then commiting

ozbek · 2017-03-02T03:39:43Z

app/src/main/java/com/quran/labs/androidquran/ui/util/TranslationsSpinnerAdapter.java

+      holder.checkBox.setText(R.string.more_translations);
+      holder.checkBox.setOnClickListener(this.textListener);
+    } else {
+      holder.checkBox.setButtonDrawable(R.drawable.abc_btn_check_material);


Android Studio is complaining that 'The resource @drawable/abc_btn_check_material is marked as private in com.android.support:appcompat'. Can we change its visibility somehow (instead of setting to null or changing its transparency)?

hmm, im not sure how this would be done outside of using a drawable, is there a reason other than bloat to bring in the checkbox drawable resource into the project workspace?

That's something @ahmedre to decide :)

sounds good iA

ozbek · 2017-03-05T18:02:24Z

There still seems an issue with API level 14:

ozbek · 2017-03-05T18:05:22Z

@ahmedre, is #760 really needed? Looking at the changes required, this seems an overkill for a functionality with so little use...

There should be another (better) way of doing this though.

ahmedre · 2017-03-05T18:33:06Z

Do you have any suggestions? We get lots of emails asking things around how to add translations, and it seems a little unintuitive to force people out to settings to get a translation.

…

On Sun, Mar 5, 2017, 10:05 AM Shuhrat Dehkanov ***@***.***> wrote: @ahmedre <https://github.com/ahmedre>, is #760 <#760> really needed? Looking at the changes required, this seems an overkill for a functionality with so little use... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#802 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAC6m7HN0A-8pQOmAC8i6WwdSEzS73Qrks5rivligaJpZM4L4dk9> .

ozbek · 2017-03-05T20:34:03Z

Maybe something like this (remove .zip, apply on top of this PR): pr-802.patch.zip?

(Does GitHub allow changes on other people's PRs, something like Gerrit?)

ozbek · 2017-03-09T10:14:58Z

@uzairmad what do you think of the last bug and the patch?

uzairmad · 2017-03-09T16:06:41Z

sorry just checked, it looks good for me - I still need to test it on API 14 or if you have already, i can push it in this PR if thats ok. jzk

uzairmad · 2017-03-09T16:22:53Z

it looks good on api 14, jzk @ozbek , i can push it

ahmedre

i'll approve this, but if you get around to it, try to merge the two view types together.

ahmedre · 2017-03-19T01:35:01Z

app/src/main/java/com/quran/labs/androidquran/ui/PagerActivity.java

+          int type = super.getItemViewType(position);
+          if (type == 1) {
+            LayoutInflater inflater = getLayoutInflater();
+            convertView = inflater.inflate(R.layout.translation_ab_spinner_selected, parent, false);


i still wish we'd share the inflation instead of inflating a separate view

i can do this iA

ahmedre · 2017-03-19T01:36:59Z

jazakAllah khairan

uzairmad · 2017-03-20T03:21:09Z

waiyyakum jazakallahukhairan for the feedback

Add Translation activity to spinner

c469a87

ozbek reviewed Feb 6, 2017

View reviewed changes

ozbek suggested changes Feb 6, 2017

View reviewed changes

Add More Translations link to Translation Spinner

9c6c74c

ahmedre reviewed Feb 28, 2017

View reviewed changes

Update Translations Spinner with More translations item

909b9fb

ozbek reviewed Mar 1, 2017

View reviewed changes

Removed ws and style fixes for More Translations

d487e20

ozbek approved these changes Mar 1, 2017

View reviewed changes

ozbek suggested changes Mar 1, 2017

View reviewed changes

removed unused imports, small fixes for More translations

a760b41

ozbek reviewed Mar 1, 2017

View reviewed changes

enable checkbox in normal case for Translation Spinner

56b8b12

ozbek reviewed Mar 2, 2017

View reviewed changes

Add More Translations better handling for all API levels

dccbda9

ahmedre approved these changes Mar 19, 2017

View reviewed changes

ahmedre merged commit ac14e36 into quran:master Mar 19, 2017

uzairmad deleted the translation-to-spinner-760 branch March 20, 2017 03:40

uzairmad mentioned this pull request Mar 20, 2017

Add shared inflation for translation spinner #824

Merged

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

Add "More Translations" to spinner (#802)

8e4f606

		@@ -50,6 +50,10 @@
		android:title="@string/menu_jump"
		app:showAsAction="never" />

		}

		public String[] updateTranslationNames(String[] translationNames) {


		public String[] updateTranslationNames(String[] translationNames) {

		List<String> transList = new ArrayList<>();

Add Translation activity to spinner #802

Add Translation activity to spinner #802

Conversation

uzairmad commented Feb 6, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ozbek left a comment

Choose a reason for hiding this comment

ahmedre 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

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

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 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uzairmad commented Mar 1, 2017

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 commented Mar 1, 2017

uzairmad commented Mar 1, 2017

uzairmad commented Mar 1, 2017 • 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

ozbek commented Mar 5, 2017

ozbek commented Mar 5, 2017 • edited

ahmedre commented Mar 5, 2017 via email

ozbek commented Mar 5, 2017 • edited

ozbek commented Mar 9, 2017

uzairmad commented Mar 9, 2017

uzairmad commented Mar 9, 2017

ahmedre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahmedre commented Mar 19, 2017

uzairmad commented Mar 20, 2017

uzairmad commented Mar 1, 2017 •

edited

ozbek commented Mar 5, 2017 •

edited

ozbek commented Mar 5, 2017 •

edited