Split setting screen into options and advanced options issue #704 #714

mhussien86 · 2016-11-28T11:21:39Z

This commit is solving the reorganizing of the setting screen into options and advanced options, change in the category name of Advanced settings maybe needed in the future.

…and clean up all calsses

ozbek · 2016-11-29T00:08:57Z

app/src/main/AndroidManifest.xml

@@ -52,6 +52,11 @@
        android:theme="@style/QuranToolBar"
        android:configChanges="keyboardHidden|orientation|screenSize"/>
    <activity
+        android:name=".AdvancedQuranPreferenceActivity"


IMHO, this should follow existing file naming convention. So, probably, should be named as QuranAdvancedPreferenceActivity.

ozbek · 2016-11-29T00:19:06Z

app/src/main/java/com/quran/labs/androidquran/AdvancedQuranPreferenceActivity.java

+    setContentView(R.layout.preferences);
+
+    final Toolbar toolbar = (Toolbar) findViewById(R.id.toolbar);
+    toolbar.setTitle(R.string.menu_settings);


Please use "Advanced settings" title.

Thiught about using the exsting string as we will need a localization String for any new String will do this.

use R.string.prefs_category_advanced for now

ozbek · 2016-11-29T00:23:07Z

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

+import rx.schedulers.Schedulers;
+import timber.log.Timber;
+
+public class QuranAdvancedSettingsFragment extends PreferenceFragment implements


If we are to use an additional fragment, is it possible to just have one original activity and let it utilize two fragments?

let's keep it as 2 and clean it up later since this is mostly a straight forward move of code - perhaps in another PR if it makes sense?

Agree for now, just to avoid more checks and conditions.

ozbek · 2016-11-29T00:26:23Z

app/src/main/res/xml/quran_preferences.xml

@@ -147,29 +147,16 @@
  </PreferenceCategory>

  <PreferenceCategory


Is it possible to handle PreferenceCategory clicks and launch advanced settings thereof, and get rid of additional Preference below?

in an ideal world, the patch would have been as simple as Mohamed's original patch where he'd just add PreferenceScreen around it - unfortunately though it doesn't work. maybe it would work with support-v7 fragments, but we can try that later?

Can you elaborate more ? do we need to change something here ?

i think @ozbek was saying, since PreferenceCategory extends Preference, instead of having the PreferenceCategory have a nested Preference for advanced settings, can you just make PreferenceCategory handle advanced settings without adding a child in xml?

i.e. instead of what you have now:

<PreferenceCategory android:key=""> <Preference android:key=""> <intent android:targetPackage="" /> </Preference> </PreferenceCategory>

instead, would it work if you move everything on the Preference up to the PreferenceCategory instead and remove the preference? i.e.

<PreferenceCategory android:key=""> <intent android:targetPackage="" /> </PreferenceCategory>

update - after more thought, i don't think that this will work the way we want, because the PreferenceCategory causes a group style ui to be drawn.

ozbek · 2016-11-29T00:28:35Z

Also, while you are at it, please consider implementing all the changes mentioned in #704.

ahmedre · 2016-11-29T07:16:24Z

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

+  }
+
+  @Override
+  public void onSharedPreferenceChanged(SharedPreferences sharedPreferences, String s) {


remove this

ahmedre · 2016-11-29T07:16:51Z

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

+import timber.log.Timber;
+
+public class QuranAdvancedSettingsFragment extends PreferenceFragment implements
+    SharedPreferences.OnSharedPreferenceChangeListener {


you don't need the implements SharedPreferences.OnSharedPreferenceChangeListener here.

ahmedre · 2016-11-29T07:16:54Z

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

+import rx.schedulers.Schedulers;
+import timber.log.Timber;
+
+public class QuranAdvancedSettingsFragment extends PreferenceFragment implements


let's keep it as 2 and clean it up later since this is mostly a straight forward move of code - perhaps in another PR if it makes sense?

ahmedre · 2016-11-29T07:22:52Z

app/src/main/res/xml/quran_preferences.xml

@@ -147,29 +147,16 @@
  </PreferenceCategory>

  <PreferenceCategory


in an ideal world, the patch would have been as simple as Mohamed's original patch where he'd just add PreferenceScreen around it - unfortunately though it doesn't work. maybe it would work with support-v7 fragments, but we can try that later?

ahmedre · 2016-11-29T07:23:08Z

app/src/main/AndroidManifest.xml

@@ -52,6 +52,11 @@
        android:theme="@style/QuranToolBar"
        android:configChanges="keyboardHidden|orientation|screenSize"/>
    <activity
+        android:name=".AdvancedQuranPreferenceActivity"


ahmedre · 2016-11-29T07:24:11Z

re #704 agree, but split them into separate patches - let's get this one merged first and then if you're cool with continuing, would love more PRs about this insha'Allah?

ahmedre · 2016-11-29T08:17:52Z

jazakAllah khairan - +1 from me, but will wait for @ozbek's +1 before merging.

ozbek

So, we can't have a single activity with two fragments?

ozbek · 2016-11-29T13:08:59Z

app/src/main/res/xml/quran_advanced_preferences.xml

+  </PreferenceCategory>
+</PreferenceScreen>
+
+


Unnecessary empty line

ozbek · 2016-11-29T13:10:13Z

app/src/main/res/xml/quran_preferences.xml

+    <intent
+        android:targetPackage="com.quran.labs.androidquran"
+        android:targetClass="com.quran.labs.androidquran.QuranAdvancedPreferenceActivity"
+        android:action="com.quran.labs.androidquran.PREFS_ONE" />


Why do we need this action?

Yes you are right unneeded action will remove it. also about the one activity with two fragments won't it be a one level of navigation what do you think i prefer advanced options to be in a second level of navigation.

ozbek · 2016-11-29T15:06:55Z

also about the one activity with two fragments won't it be a one level of navigation what do you think i prefer advanced options to be in a second level of navigation.

That's basically what I am asking about :)

Is it possible to have your desired behaviour without adding QuranAdvancedPreferenceActivity?

mhussien86 · 2016-11-29T16:30:51Z

Yes possible but not cleaner it will be with some hacks, like sending string value from setting fragment and restart setting activity with string value to replace setting fragment with advanced settings fragment ! , the current we have is more separated and better.

ozbek · 2016-11-29T22:45:47Z

OK, please cleanup the unused code. Thank you :)

ahmedre · 2016-11-30T04:02:39Z

i'll go ahead and merge and then please send another PR removing whatever should be removed. jazakAllah khairan for doing this @mhussien86 !

Split setting screen into options and advanced options issue #704

mohamed added 3 commits November 27, 2016 17:09

Split the setting advanced options into a new preferences screen

45c3f56

Split advanced setting options into a new activity with new fragment …

11a6213

…and clean up all calsses

Merge branch 'master' of https://github.com/quran/quran_android

3ed03af

ozbek suggested changes Nov 29, 2016

View reviewed changes

ahmedre reviewed Nov 29, 2016

View reviewed changes

Do all of the comments on the the previous pull request

be1f836

ozbek suggested changes Nov 29, 2016

View reviewed changes

ahmedre merged commit cb566e1 into quran:master Nov 30, 2016

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

Merge pull request #714 from mhussien86/master

da4838e

Split setting screen into options and advanced options issue #704

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split setting screen into options and advanced options issue #704 #714

Split setting screen into options and advanced options issue #704 #714

mhussien86 commented Nov 28, 2016

ozbek Nov 29, 2016

ahmedre Nov 29, 2016

ozbek Nov 29, 2016

mhussien86 Nov 29, 2016

ahmedre Nov 29, 2016

ozbek Nov 29, 2016

ahmedre Nov 29, 2016

mhussien86 Nov 29, 2016 •

edited

Loading

ozbek Nov 29, 2016

ahmedre Nov 29, 2016

mhussien86 Nov 29, 2016

ahmedre Nov 29, 2016 •

edited

Loading

ozbek commented Nov 29, 2016

ahmedre Nov 29, 2016

ahmedre Nov 29, 2016

ahmedre Nov 29, 2016

ahmedre Nov 29, 2016

ahmedre Nov 29, 2016

ahmedre commented Nov 29, 2016

ahmedre commented Nov 29, 2016

ozbek left a comment

ozbek Nov 29, 2016

ozbek Nov 29, 2016

mhussien86 Nov 29, 2016

ozbek commented Nov 29, 2016

mhussien86 commented Nov 29, 2016

ozbek commented Nov 29, 2016

ahmedre commented Nov 30, 2016

		@@ -147,29 +147,16 @@
		</PreferenceCategory>

		<PreferenceCategory

		</PreferenceCategory>
		</PreferenceScreen>

Split setting screen into options and advanced options issue #704 #714

Split setting screen into options and advanced options issue #704 #714

Conversation

mhussien86 commented Nov 28, 2016

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

mhussien86 Nov 29, 2016 • edited Loading

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 Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

ozbek commented Nov 29, 2016

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

ahmedre commented Nov 29, 2016

ahmedre commented Nov 29, 2016

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

ozbek commented Nov 29, 2016

mhussien86 commented Nov 29, 2016

ozbek commented Nov 29, 2016

ahmedre commented Nov 30, 2016

mhussien86 Nov 29, 2016 •

edited

Loading

ahmedre Nov 29, 2016 •

edited

Loading