v6 settings reorganization

bellerbrock commented 5 months ago

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)
[x] ENHANCEMENT (non-breaking change which improves existing functionality)
[ ] NONE (if none of the other choices apply. Example, tooling, build system, CI, docs, etc.)

Checklist:

[x] 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 commented 4 months ago

BrAPI should be removed as a default import/export source if it is not enabled in the settings
I think the "Verify name" is unnecessary for every verification interval option
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.)
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 commented 4 months ago

@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 commented 3 months ago

[x] Resolve conflicts
[x] 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)
[x] Upgrade SearchPreference to newest
[x] Replace "Advance" with "Navigate" for volume keys related prefs

bellerbrock commented 3 months ago

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 commented 3 months ago

[ ] 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
[x] 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)
[x] Pair device rename to Location Provider
[x] Location Collection Level should be Off by default
[x] 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
[x] Observation collect level needs warning about battery usage
[x] Geonav trapezoid parameters can be moved out of their own section to under the search method
[x] Exclude base pref fragment from search results

bellerbrock commented 3 months ago

[x] 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

[x] 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)

[x] Pair device rename to Location Provider

[x] Location Collection Level should be Off by default

[x] 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

[x] Observation collect level needs warning about battery usage

[x] Geonav trapezoid parameters can be moved out of their own section to under the search method

[x] 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 commented 2 months ago

[x] As discussed last week, remove the require person setting, add a Never option to Time between verification, and set 24h as default
[x] 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 commented 1 week ago

@bellerbrock once you're able to resolve the conflicts this can be merged in. It looks really good and is a great update to get everything organized!

PhenoApps / Field-Book