Regroup preferences #1635

agrajaghh · 2014-06-23T23:35:53Z

I regrouped the preferences like discussed in the mailing list. I put notifications, old message deletion and passphrase into their own category pages. This is saving us ten items in the preference list. Its looking like this now:

But I dont know how to add the "back arrow" in the red square to the new PreferenceScreen's. Do you know how to put it there?

And I'm not sure about the new category for the PreferenceScreen's. I put them in "Miscellaneous" now, but perhaps you have a better idea?

agrajaghh · 2014-06-23T23:37:18Z

src/org/thoughtcrime/securesms/ApplicationPreferencesActivity.java

+    Context context = ApplicationPreferencesActivity.this;
+
+    SpannableString spanOn  = new SpannableString(getString(R.string.ApplicationPreferencesActivity_on) + " ");
+    SpannableString spanOff = new SpannableString(getString(R.string.ApplicationPreferencesActivity_off) + " ");


I had to put an extra space there, otherwise the last "f" from the Italic "Off" would have been cropped. Do you have a better solution?

mcginty · 2014-06-24T08:19:51Z

@agrajaghh had a bit of work on this already, and continued a bit further. I added a workaround from SO for the home up navigation in a subscreen. Also to keep the discussion on how to regroup going, I did a bit more radical reorganization, let me know what you think about it. The summary generation code isn't in there, it's hardcoded in my branch so if you want to include it, that still needs to be done.

https://github.com/mcginty/TextSecure/tree/group_settings

mcginty · 2014-06-24T08:47:29Z

In general, my opinion is that we should choose to either have PreferenceCategories (current) or PreferenceScreens (this) but not both, as it looks kind of strange hierarchically-speaking.

As a mock, I also removed "automatic key exchange" as I think we can, at this point, just not have automated key exchange (or change the dialog for exchange to have "don't ask again"). We'll have to remove all the checks for that if people agree that's a fair thing to remove.

agrajaghh · 2014-06-24T08:52:28Z

Your regrouping looks good to me!

But there is one issue with how I set the summary. The problem is that it's not refreshing immediately. You have to scroll away from the summary and scroll back in to display it. Which is not possible anymore in your branch for me, because there are not enough settings :)
When I put the summary in italic it was refreshing, but you're change looks better. I prefer the grey summary. Do you have an idea how to display the changed summarys immediately?

Regarding the key exchange: I think if we put a "don't ask again" checkbox in the dialog, we need an option in the preferences to ask again...

mcginty · 2014-06-24T08:59:01Z

I feel it's not a task that's repeated often enough to justify the need to have a preference automating it. Especially with data messages, it's no longer as common a task for secure messages.

If we do want that preference to still exist, then yeah we might want an option that's a reset for "remember me" dialogs prompts.

mcginty · 2014-06-24T08:59:15Z

Will look into the summary refresh issues :)

agrajaghh · 2014-06-24T09:05:19Z

I would be fine with removing the key exchange option aswell, but perhaps we should start another issue for this discussion?

mcginty · 2014-06-25T17:09:42Z

Sounds fair to me 😄 Feel free to take whatever you want from my branch and integrate as you see fit

agrajaghh · 2014-06-25T18:03:12Z

Okay, I took the changes from your branch and put the key exchange option back. But I still didn't find a solution to the summary refresh problem :( You have an idea?

agrajaghh · 2014-06-26T10:03:34Z

The refresh problem is solved. I think I'm done. Any suggestions?

agrajaghh · 2014-06-26T10:06:09Z

src/org/thoughtcrime/securesms/ApplicationPreferencesActivity.java

    return false;
  }

+  /** Sets up the action bar for an {@link PreferenceScreen} */


@mcginty: this is from http://stackoverflow.com/questions/16374820/action-bar-home-button-not-functional-with-nested-preferencescreen no?
Are we allowed to put it in here?

mcginty · 2014-06-26T16:14:14Z

@agrajaghh SO is cc-by-sa - if I forgot to put a comment attribution in that draft branch, we should do that as a comment above the method but since we're open source it shouldn't be a problem to include it (but I'm not a lawyer), especially since this is a "snippet" or "quote" sized piece of code which'd qualify under fair use.

agrajaghh · 2014-06-26T16:51:33Z

I added a comment above the mothod, is it okay like that?

edit: changed it, so it follows: http://blog.stackoverflow.com/2009/06/attribution-required/

mcginty · 2014-06-26T20:03:56Z

That looks like fine attribution to me :). Will check out your change, thanks for taking this feature on! What an improvement it will be.

mcginty · 2014-06-28T00:28:50Z

Just threw it on a device and ran through it, a couple comments on the general preferences flow:

unsure that we should stick with "Access lock," it's kind of confusing. Any ideas for a better name? Device protection?
Enter key sends should probably have a dependency on Enable Enter key unless I'm missing something (not related to your change, that's in master right now).
super nitpick, the On and Off in SMS/MMS summary would look better lowercase.

mcginty · 2014-06-28T00:29:33Z

Also, in Access lock it might make more sense to have Disable passphrase actually turn into Enable passphrase, lest we have some weird inverted settings.

agrajaghh · 2014-06-28T11:32:16Z

I changed the "on" and "off" to lowercase for the SMS preference.

Regarding the "Access lock", I 'm not sure whats best. Somebody else got an idea?

Passphrase (old name)
Access lock
Device protection
Storage encryption (too technical?)
Storage protection

I think you are right, the inverted access lock setting doesn't really make sense. We should change that. But how do we migrate the old option? I think we have to change the settings key pref_disable_passphrase otherwise we don't know if we are reading the old or new value. But then we need to check for the old value everytime we check for the access lock. Should I put this check in TextSecurePreferences.isPasswordDisabled?

agrajaghh · 2014-06-28T11:33:54Z

I forgot the enter key preference: Should I leave it like it is, because you are changing the behaviour in #1656 anyway?

agrajaghh · 2014-06-28T18:13:44Z

Inverted the access lock preference now. But we still need the name...

generalmanager · 2014-06-28T19:48:36Z

How about something like "offline encryption". If we are already butchering the technical terms, we can at least try to make it easily understandable.

We could replace storage encryption with offline encryption and transport encryption with online encryption.
These terms should still be technically correct (encryption vs software lock) but much more user friendly. What do you think?

mcginty · 2014-06-28T20:31:13Z

What technical terms are being butchered? Nobody's arguing against using the word "encryption" for the passphrase, this is just a query as to what to call the general category of preferences that help secure your device as opposed to over-the-wire security. Screenshot security is also included in this category (although we can move it out if it turns it that doens't make as much sense).

generalmanager · 2014-06-28T20:34:57Z

I was talking about my own suggestion, because it "dumbs down" the technical term ;-)

If there are more non-encryption related subsettings (like the "screenshot blocker") I'd just call it offline security or device security.

agrajaghh · 2014-06-28T20:36:28Z

"Device security" sounds good to me!

generalmanager · 2014-06-28T20:39:00Z

Yeah it's my new preference too. If we should ever feel the need to refer to the transport, we can use online or message security for that.

mcginty · 2014-06-28T20:46:49Z

Device security sounds like the best choice so far to me too

agrajaghh · 2014-06-28T20:59:36Z

I changed it...

Screenshots:

generalmanager · 2014-06-28T21:01:36Z

Looks good to me!

agrajaghh · 2014-07-30T17:10:32Z

I just did a rebase. What do think @mcginty?

tinloaf · 2014-07-30T17:22:27Z

Just a question: The "complete key exchange" option is only relevant in the SMS world, since there is no key exchange at all with the axolotl-over-data protocol, right? (Well, there is, but it's only Server-Client, not Client-Client). Shouldn't it go into the SMS submenu?

mcginty · 2014-12-09T14:43:04Z

src/org/thoughtcrime/securesms/MmsPreferencesFragment.java

+
+  public static CharSequence getSummary(Context context) {
+    final int enabledResId   = R.string.MmsPreferencesFragment__enabled;
+    final int disabledResId  = R.string.MmsPreferencesFragment__disabled;


nitpick, extra space before the =

mcginty · 2014-12-09T14:59:55Z

src/org/thoughtcrime/securesms/preferences/PreferenceFragmentAdvanced.java

+    super.onActivityResult(reqCode, resultCode, data);
+
+    Log.w(TAG, "Got result: " + resultCode + " for req: " + reqCode);
+    if (resultCode == Activity.RESULT_OK && reqCode == ApplicationPreferencesActivity.PICK_IDENTITY_CONTACT) {


ApplicationPreferencesActivity.PICK_IDENTITY_CONTACT is now only used in PreferenceFragmentAdvanced, so I think it makes more sense to move that here instead of make it public in a class that doesn't care about it.

yep, you're totally right!

mcginty · 2014-12-09T15:24:55Z

src/org/thoughtcrime/securesms/preferences/PreferenceFragmentAdvanced.java

+import org.thoughtcrime.securesms.contacts.ContactIdentityManager;
+import org.thoughtcrime.securesms.util.TextSecurePreferences;
+
+public class PreferenceFragmentAdvanced extends PreferenceFragment {


Would prefer AdvancedPreferenceFragment over PreferenceFragmentAdvanced, same for all the other subsections.

done. Should I leave them in the preferences folder? Since they are not really a preference like PassphraseTimeoutPreference or LedBlinkPatternListPreference. But I guess it still makes sense..

mcginty · 2014-12-09T15:45:13Z

src/org/thoughtcrime/securesms/preferences/PreferenceFragmentNotifications.java

+        if (value.equals(asList.getEntryValues()[index])) {
+          break;
+        }
+      }


This is kind of gross. I'd either convert the findIndexOfValue to a generic method in Util, or my personal leaning would be to just use Arrays.asList(Object[]) and then call indexOf from that, since this isn't something that will be run constantly, so performance doesn't outweigh readability IMO.

mcginty · 2014-12-09T15:55:46Z

Ha! I didn't notice you were updating the whole time I was giving comments. Incredible.

agrajaghh · 2014-12-09T16:00:18Z

Haha, yeah I just had time ;)

moxie0 · 2014-12-11T05:21:52Z

src/org/thoughtcrime/securesms/ApplicationPreferencesActivity.java

-    private class DisablePushMessagesTask extends AsyncTask<Void, Void, Integer> {
-      private ProgressDialog dialog;
-      private final Preference preference;
+      private class DisablePushMessagesTask extends AsyncTask<Void, Void, Integer> {


You can extend ProgressDialogAsyncTask

agrajaghh reviewed Jun 23, 2014
View reviewed changes

agrajaghh changed the title ~~regroup preferences~~ Regroup preferences Jun 24, 2014

agrajaghh reviewed Jun 26, 2014
View reviewed changes

fix "case" indent

0c43074

mcginty reviewed Dec 9, 2014
View reviewed changes

agrajaghh added 3 commits December 9, 2014 15:44

dont use getString if not necessary

4685e6e

DisablePushMessagesTask should only accept CheckBoxPreference

8031736

remove initializeEditTextSummary since its not used

28d9291

mcginty reviewed Dec 9, 2014
View reviewed changes

agrajaghh added 4 commits December 9, 2014 16:03

replace nested if's with &&

a665052

remove unnecessary spaces

43bf645

use getBooleanExtra

2f3fa2e

move PICK_IDENTITY_CONTACT into PreferenceFragmentAdvanced

2b10511

mcginty reviewed Dec 9, 2014
View reviewed changes

agrajaghh added 2 commits December 9, 2014 16:25

remove unused ENABLE_PASSPHRASE_ACTIVITY

b3f40cc

rename PreferenceFragment classes

eeea899

mcginty reviewed Dec 9, 2014
View reviewed changes

use Arrays.asList for "findIndexOfValue"

499e5b5

agrajaghh added 3 commits December 9, 2014 17:04

fix linebreak

7a8d775

use getString formatting options

e3dd24f

remove unnecessary spaces

dd6a5f6

moxie0 reviewed Dec 11, 2014
View reviewed changes

agrajaghh added 2 commits December 11, 2014 10:40

breaking up getSummary() into smaller methods

6e628f9

use ProgressDialogAsyncTask instead of AsyncTask

390d8d3

mcginty closed this Dec 11, 2014

agrajaghh deleted the group_settings branch March 6, 2015 14:06

Regroup preferences #1635

Regroup preferences #1635

Conversation

agrajaghh commented Jun 23, 2014

Choose a reason for hiding this comment

mcginty commented Jun 24, 2014

mcginty commented Jun 24, 2014

agrajaghh commented Jun 24, 2014

mcginty commented Jun 24, 2014

mcginty commented Jun 24, 2014

agrajaghh commented Jun 24, 2014

mcginty commented Jun 25, 2014

agrajaghh commented Jun 25, 2014

agrajaghh commented Jun 26, 2014

Choose a reason for hiding this comment

mcginty commented Jun 26, 2014

agrajaghh commented Jun 26, 2014

mcginty commented Jun 26, 2014

mcginty commented Jun 28, 2014

mcginty commented Jun 28, 2014

agrajaghh commented Jun 28, 2014

agrajaghh commented Jun 28, 2014

agrajaghh commented Jun 28, 2014

generalmanager commented Jun 28, 2014

mcginty commented Jun 28, 2014

generalmanager commented Jun 28, 2014

agrajaghh commented Jun 28, 2014

generalmanager commented Jun 28, 2014

mcginty commented Jun 28, 2014

agrajaghh commented Jun 28, 2014

generalmanager commented Jun 28, 2014

agrajaghh commented Jul 30, 2014

tinloaf commented Jul 30, 2014

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

mcginty commented Dec 9, 2014

agrajaghh commented Dec 9, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment