v6 settings reorganization

[bellerbrock]

Description

Provide a summary of your changes including motivation and context.
If these changes fix a bug or resolves a feature request, be sure to link to that issue.

Reorganizes settings according to #850 checklist
Closes #850

Type of change

What type of changes does your code introduce? Put an x in boxes that apply.

• CHANGE (fix or feature that would cause existing functionality to not work as expected)
• FEATURE (non-breaking change which adds functionality)
• BUGFIX (non-breaking change which fixes an issue)
• ENHANCEMENT (non-breaking change which improves existing functionality)
• NONE (if none of the other choices apply. Example, tooling, build system, CI, docs, etc.)

Checklist:

• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• I have updated relevant documentation

Changelog entry

Please add a one-line changelog entry below. This will be copied to the changelog file during the release process.

settings standardized and reorganized

[chaneylc]

1. BrAPI should be removed as a default import/export source if it is not enabled in the settings
2. I think the "Verify name" is unnecessary for every verification interval option
3. Something is broken with appearances (works on main), selecting a different theme switches the text size to small, then choosing small text crashes the app, and a style-based runtime crash keeps it from opening. (I'm guessing this is due to your overall refactoring of xml array positions starting at 0 instead of 1, these were meant to be positions not memory locations which is why they are starting from 1. It doesn't matter to me that we change this, but now all relevant code references will need to be updated, preferably an enum should be defined for these positions and used instead of static strings.)
4. Maybe connected to (3), when changing the theme the next entry and barcode toolbar buttons are shown in collect, but they don't do anything. (this does not happen on main)

[bellerbrock]

@chaneylc Should all be addressed now, ready for re-review or merge.

Most of the xml arrays were already starting at 0, I moved the rest to 0 for consistency, and because 0 as default/disabled seemed more intuitive as well. But yeah unfortunately missed some hardcoded config and theme activity references, thanks for catching the resulting bugs.

[trife]

• Resolve conflicts
• Collection format is hidden when selecting Obs collection level. Is that because it's being stored as JSON? Regardless, we probably want to save coordinates as labeled JSON (similar to categorical data) so we can manipulate as/when needed (might be a new issue)
• Upgrade SearchPreference to newest
• Replace "Advance" with "Navigate" for volume keys related prefs

[bellerbrock]

Collection format is hidden when selecting Obs collection level. Is that because it's being stored as JSON?

The visibility problem was because I'd changed the order/indexes of the collection level options, but not realized they were also hardcoded in the companion object

[trife]

• There's a disconnect with name and require person. Just because it's not required doesn't mean we want to hide it from being entered. Required is a separate thing that actually forces (with reminders) users to enter in a name
• Change Appearance > Collect Toolbar > Default Buttons to Appearance > Collect Toolbar > Toolbar features
• When using dialogs to convey actions (e.g. paired sound effects), the positive button should be the affirmative action (e.g. "Enable") and if the main point is/can be conveyed in the title the body doesn't need to be populated (like with cycling traits)
• Pair device rename to Location Provider
• Location Collection Level should be Off by default
• Is collection format only used for these coordinates or also used for location and gnss traits? If the latter, then it needs to be always visible
• Observation collect level needs warning about battery usage
• Geonav trapezoid parameters can be moved out of their own section to under the search method
• Exclude base pref fragment from search results

[chaneylc]

We shouldn't refactor preference keys, this will cause existing preference choices to be erased after the update.

[bellerbrock]

Hmm, yeah it seems best to avoid erasing settings if possible. But I'm not sure how best to reconcile that with last instruction in the ticket to Swap disable sharing for enable file sharing

Seems either we don't swap, or we do and the pref key has to change as well, or we have to treat the Enable File Sharing checkbox value as a dummy value and then manually set the DisableShare pref key to the opposite of the checkbox value in the pref code.

I'm not a fan of the third solution, and if we still want the swap I don't think having the share pref reset to enabled for whatever users had disabled it isn't too bad of an update side effect. @trife any thoughts?

[chaneylc]

I know I've done this similarly in the past but I think it would be more descriptive to use enum classes here and in the future instead. This is relevant for any array resources with integer values.

[bellerbrock]

Gotcha. I copied those over from the old generalpreferences fragment when merging location prefs into the new location preferences fragment. But I see what you/re saying. Not sure if it makes sense to add the enum conversions to the scope of this PR though, maybe it'd be better to make it a separate ticket?

And if we want to convert all array resources with integer vlaues, what would that look like? Something like this?

package com.fieldbook.tracker.enums

import com.fieldbook.tracker.R

enum class LocationCollectionMode(val value: Int, val labelResId: Int) {
    OFF(0, R.string.location_level_off),
    STUDY(1, R.string.location_level_study),
    OBS_UNIT(2, R.string.location_level_observation_unit),
    OBS(3, R.string.location_level_observation);

    companion object {
        fun fromValue(value: Int): LocationCollectionMode? = values().find { it.value == value }
    }
}

Then the enums could used to populate preferences programatically like this (in addition to checking preference values in other parts of the app)

val locationModePreference = findPreference<ListPreference>("com.fieldbook.tracker.GENERAL_LOCATION_COLLECTION")
val locationModes = LocationCollectionMode.values()

locationModePreference?.entries = locationModes.map { getString(it.labelResId) }.toTypedArray()
locationModePreference?.entryValues = locationModes.map { it.value.toString() }.toTypedArray()

[bellerbrock]

• There's a disconnect with name and require person. Just because it's not required doesn't mean we want to hide it from being entered. Required is a separate thing that actually forces (with reminders) users to enter in a name
• Change Appearance > Collect Toolbar > Default Buttons to Appearance > Collect Toolbar > Toolbar features
• When using dialogs to convey actions (e.g. paired sound effects), the positive button should be the affirmative action (e.g. "Enable") and if the main point is/can be conveyed in the title the body doesn't need to be populated (like with cycling traits)
• Pair device rename to Location Provider
• Location Collection Level should be Off by default
• Is collection format only used for these coordinates or also used for location and gnss traits? If the latter, then it needs to be always visible
• Observation collect level needs warning about battery usage
• Geonav trapezoid parameters can be moved out of their own section to under the search method
• Exclude base pref fragment from search results

Addressed most of these in the latest commits. For the location collection format question, yes, from what I see it applies just to what gets stored in the observation table's geoCoordinates column for each observation, not to the observation values for location or gnss traits.

For the two unchecked items:

1- Whats the ideal interaction of the person prefs? The overlap between the 3 prefs is a bit confusing to me. What about reducing to two prefs? Just the person name entry, and if a name is entered also show the verification interval with no verificiation, every time app opens, 12 hr, and 24hr as the options (default to no verification?). Require seems uncessary since once person is set it does not get unset, the main issue is to ensure it switches when the actual operator switches, which the verification interval pref handles well.

Some other thoughts that are probably out of scope here but I think would be useful enhancements

• make person name a requirement regardless of settings (it's nearly always helpful, is it ever unhelpful?)
• show the Person name in config screen and/or collect screen (bottom toolbar? paired with collected values in summary tool?) for better awareness of who the app thinks is operating it, and who has collected existing data. Also show the names of data collectors in data details on field detail pages
• append each distinct person name entered in the app to a list in preferences, and offer the previously used names as options when changing person name.

2- Not sure what needs addressing here, aren't all the boolean settings like that (sounds for example) handled with checkboxes, not dialogs? Is the idea that the sound checkbox preferences don't need summaries, just titles?

[trife]

• As discussed last week, remove the require person setting, add a Never option to Time between verification, and set 24h as default
• I do like the option of having a list of previous people and selecting from this list, but I think it's outside the scope of this PR. Make an issue for this.

[trife]

• Move directory definition and database settings to a Storage section below BrAPI and use this icon
• Add new section Features above Appearance
• Move Datagrid, Next Entry, Move to Unique ID, Tutorial to Features
• Merge Collect Toolbar into Collect Screen section
• Move crashlytics to System
• Change location provider icon
• Rename Toolbar Features to Toolbar Actions
• Replace swap navigation icon
• Update dialogs to use action words for positive buttons (e.g. Enable vs OK)
• Move System to below BrAPI, above Storage