0x0ece / yopa-native

0 stars 2 forks source link

Settings framework #69

Closed 0x0ece closed 6 years ago

0x0ece commented 6 years ago

I'm adding a little framework to manage settings.

My fear in the long term is that settings will become a throw in of all random features that we don't want in the main app, and the code will get super messy.

My proposal is to split settings in 2 parts. One declarative that describes the layout. One that implements the specs and render the screens & navigation.

This is first iteration of the concept. The layout is currently a static object/tree, but will be dynamic as soon as we implement editing groups (for each group, there will be an entire branch of the tree). The renderer implements a couple of functions (including the import that we had already) and a basic recursive navigation.

Next steps:

Feedback welcome!

kalamaico commented 6 years ago

Uhm, I don't know exactly what you have in mind here, but I would think first what features we want to add, to avoid having tons of features that have little added value.

My 2 cents: keep the app simple, let it do well a few things.

0x0ece commented 6 years ago

Respecting all the best practices, I've added fingerprint to this PR because it touches a lot of files :) Please test it especially on real devices.

@kalamaico yes, the settings framework is exactly for that. Because we'll iterate a lot (adding/removing things in the spirit of simplifying) I fear that the code will become a mess. With the framework, you have a big object that describes how the settings will look like, so it's easier to keep it under control (const settings = ...): https://github.com/0x0ece/yopa-native/blob/70dfefffd8c68a945f58e4f1be1d031fb2d15859/src/screens/SettingsScreen.js#L11

Currently I added a bunch of features that I think are for a v2, but we'll strip it in the next commits. I'll create a separated doc/task for discussing the features long term.

0x0ece commented 6 years ago

Added a quick fix for export, see comment in #29.

Added also the "edit security level" functionality, currently only for the master password. I'm pretty sure there are some bugs. I'll add some tests to the spreadsheet to clarify the expected behavior.

jacksilv commented 6 years ago

I've checked out your branch and run it on Mempa on my iPhone. By clicking on Master Password in the Settings, I got this error image 2017-10-30 09 31 02

0x0ece commented 6 years ago

Epic failure - fixed now. I had to change that to a static method, but I forgot to update the calls. :/

jacksilv commented 6 years ago

Tested again in production mode, enable fingerprint, created one secrets, unlocked the groups, initialized "Important" to use fingerprint added 5 secrets to "Important", closed Expo, reopened in and got this error stack:

Invariant Violation: Tried to get frame for out of range index 1

This error is located at:
    in VirtualizedList
    in FlatList
    in RCTView
    in SecretList
    in Connect(SecretList)
    in HomeScreen
    in Connect(HomeScreen)
    in SceneView
    in RCTView
    in AnimatedComponent
    in Card
    in Container
    in RCTView
    in RCTView
    in CardStack
    in RCTView
    in Transitioner
    in CardStackTransitioner
    in Unknown
    in Navigator
    in NavigationContainer
    in SceneView
    in RCTView
    in RCTView
    in RCTView
    in AnimatedComponent
    in Card
    in Container
    in RCTView
    in RCTView
    in CardStack
    in RCTView
    in Transitioner
    in CardStackTransitioner
    in Unknown
    in Navigator
    in NavigationContainer
    in Provider
    in App
    in RCTView
    in AwakeInDevApp
    in RootErrorBoundary
    in ExpoRootComponent
    in RCTView
    in RCTView
    in AppContainer

_getFrameMetrics
    hashAssetFiles:46697:14
_getFrameMetricsApprox
    hashAssetFiles:46675:40
render
    hashAssetFiles:46229:57
finishClassComponent
    hashAssetFiles:31656:46
performUnitOfWork
    hashAssetFiles:32593:29
workLoop
    hashAssetFiles:32611:138
performWork
    hashAssetFiles:32648:21
scheduleUpdateImpl
    hashAssetFiles:32773:101
enqueueSetState
    hashAssetFiles:31496:136
setState
    hashAssetFiles:13603:103
<unknown>
    hashAssetFiles:87121:33
dispatch
    hashAssetFiles:88141:15
<unknown>
    hashAssetFiles:87161:31
tryCallOne
    hashAssetFiles:16549:14
<unknown>
    hashAssetFiles:16635:25
_callTimer
    hashAssetFiles:17248:15
_callImmediatesPass
    hashAssetFiles:17284:17
callImmediates
    hashAssetFiles:17488:31
__callImmediates
    hashAssetFiles:6639:30
<unknown>
    hashAssetFiles:6495:31
__guard
    hashAssetFiles:6625:11
flushedQueue
    hashAssetFiles:6494:20

The error occurs before I can take any action, at startup. The only change I made to the code was enabling production mode

diff --git a/src/Config.js b/src/Config.js
index 668b56e..350ab9e 100644
--- a/src/Config.js
+++ b/src/Config.js
@@ -2,7 +2,7 @@ import { Platform } from 'react-native';

 const Config = {

-  PRODUCTION: false,
+  PRODUCTION: true,

   AMPLITUDE_API_KEY: 'cbafe8abda90290beea53eb177ab25f2',
jacksilv commented 6 years ago

After further testing I found out that the issue is not related to this PR, so it means the master is broken :(. To reproduce the issue just create one secret in the default group, and then add other secrets in the other groups to reach at least 5 secrets (search is enabled). I created a specific issue for this #77