Config rework #318

Leptopoda · 2021-10-16T19:48:37Z

DO NOT MERGE!
this is the first proposed version for the new storage backend.

You can audit and test the new code.
Currently the migration does not seem to work so I'll debug this tomorrow.

Also sorry for the PR in the other repo. GH auto selected it as we are still marked as a fork. As this project seems to independent we should probably remove the fork dependency all together.

- removed `shared/common/` - implemented new data models for the settings - implemented a new `LocalDatabaseService` Why: The new code will allow use to better utilize flutter's ractivenes. `HiveDB` (our new database backend) will give us `ValueListenable`, `Streams` and real time storage access. The sriliazation for the models is generated by `build_runner` and even the old seriliazation for the storage migration is generated (json_serializable). TODO: - implement reactivenes. We currently don't utilize this functionality to it's full potential (`Consumer`) :) - implement migration. The migration service for the old database service isn't there yet.

calcitem · 2021-10-17T03:06:08Z

I've been in touch with the author of NineChess from the upstream repository, and he knows that Sanmill has evolved into a mobile app. He says he no longer maintains the NineChess project and is happy to see Sanmill continue to grow, and expects the community to continue to make the Mill app great.

Now I found a way to unfork: https://stackoverflow.com/questions/18390249/github-make-fork-an-own-project and create a ticket: https://support.github.com/ticket/personal/0/1362284

Leptopoda · 2021-10-17T08:49:44Z

ok never mind the migration works just fine.
Can you conduct further testing??

also when this is merged we can also nearly merge #306

calcitem · 2021-10-17T09:02:50Z

OK I will do a test.

calcitem · 2021-10-17T09:24:26Z

I ran ./flutter-init.sh and open Android Studio to build but failed:

$ flutter build apk --debug

Running Gradle task 'assembleDebug'...
lib/models/color.dart:26:6: Error: Error when reading 'lib/models/color.g.dart': No such file or directory
part 'color.g.dart';
     ^

lib/services/storage/storage.dart:79:41: Error: Method not found: 'ColorSettingsAdapter'.
    Hive.registerAdapter<ColorSettings>(ColorSettingsAdapter());
                                        ^^^^^^^^^^^^^^^^^^^^

calcitem · 2021-10-17T10:40:05Z

Do I need to rebase the dev branch first? But there are a lot of conflicts between the two branches.

calcitem · 2021-10-17T11:11:13Z

Can build after merging the branch dev and is now testing.

calcitem · 2021-10-17T11:18:04Z

After launching the app, after about a few seconds, an exception appears without clicking on the screen, as follows.

exception = {LateError} LateInitializationError: Field '_drawSoundId@1008306828' has already been initialized.
 _stackTrace = null
 _message = "Field '_drawSoundId@1008306828' has already been initialized."
fieldName = "_drawSoundId@1008306828"

LateError._throwFieldAlreadyInitialized (internal_patch.dart:211)
Audios._drawSoundId (audios.dart:47)
Audios.loadSounds (audios.dart:65)
<asynchronous gap>

This problem happened by chance and has occurred 2 times so far.

See #326

calcitem · 2021-10-17T11:34:21Z

When choosing a different algorithm, the value stored in LocalDatabase doesn't feel right?

Since testing whether the algorithm works is a bit tricky, I didn't try the actual validity of the different algorithms for now but just saw from the log that it doesn't seem to work as expected.

I/flutter (29811): [game_settings_page] algorithm = 0
I/flutter (29811): [game_settings_page] LocalDatabaseService.preferences.algorithm: 2
I/flutter (29811): [game_settings_page] algorithm = 1
I/flutter (29811): [game_settings_page] LocalDatabaseService.preferences.algorithm: 0
I/flutter (29811): [game_settings_page] algorithm = 2
I/flutter (29811): [game_settings_page] LocalDatabaseService.preferences.algorithm: 1
I/flutter (29811): [game_settings_page] algorithm = 0
I/flutter (29811): [game_settings_page] LocalDatabaseService.preferences.algorithm: 2
I/flutter (29811): [game_settings_page] algorithm = 2
I/flutter (29811): [game_settings_page] LocalDatabaseService.preferences.algorithm: 0

From the [uci] input: setoption name Algorithm value N command, it seems that the configuration sent to the backend engine is working, so only the log is just incorrect?

Status: Fixed

Leptopoda · 2021-10-17T11:40:59Z

I know about the problem. But that is something that got introduced by #284. I'll fix that but in a separate commit.

And yes this commit depends on build_runner but as this branch was still based on master and not dev the build_runner wasn't activated in the build script.

Did the migration work?

Leptopoda · 2021-10-17T11:45:06Z

When choosing a different algorithm, the value stored in LocalDatabase doesn't feel right?

Since testing whether the algorithm works is a bit tricky, I didn't try the actual validity of the different algorithms for now but just saw from the log that it doesn't seem to work as expected.
I/flutter (29811): [game_settings_page] algorithm = 0
I/flutter (29811): [game_settings_page] LocalDatabaseService.preferences.algorithm: 2
I/flutter (29811): [game_settings_page] algorithm = 1
I/flutter (29811): [game_settings_page] LocalDatabaseService.preferences.algorithm: 0
I/flutter (29811): [game_settings_page] algorithm = 2
I/flutter (29811): [game_settings_page] LocalDatabaseService.preferences.algorithm: 1
I/flutter (29811): [game_settings_page] algorithm = 0
I/flutter (29811): [game_settings_page] LocalDatabaseService.preferences.algorithm: 2
I/flutter (29811): [game_settings_page] algorithm = 2
I/flutter (29811): [game_settings_page] LocalDatabaseService.preferences.algorithm: 0
From the [uci] input: setoption name Algorithm value N command, it seems that the configuration sent to the backend engine is working, so only the log is just incorrect?

I'll have a look

src/ui/flutter_app/lib/screens/game_settings/game_settings_page.dart

calcitem · 2021-10-17T11:58:31Z

Restoring the default settings now doesn't require quitting the app, which is fantastic! It would be nice to add a confirmation dialog, though. This can be left to be refined later.

Status: Invalid

Dynamic switching of languages does not work correctly now.

Status: Pending

calcitem · 2021-10-17T12:00:06Z

Did the migration work?

Yes.

calcitem · 2021-10-17T12:33:19Z

The setting of mainToolbarBackgroundColor does not take effect.

Leptopoda · 2021-10-17T12:45:41Z

Restoring the default settings now doesn't require quitting the app, which is fantastic! It would be nice to add a confirmation dialog, though. This can be left to be refined later.

Dynamic switching of languages does not work correctly now.

This is going to be done in the gen-l10n branch (#306) I already have it working over there

Leptopoda · 2021-10-17T12:46:23Z

The setting of mainToolbarBackgroundColor does not take effect.

I had a typo there. Might still be related to that. I'll have a look.

Leptopoda · 2021-10-17T12:53:17Z

Restoring the default settings now doesn't require quitting the app, which is fantastic! It would be nice to add a confirmation dialog, though. This can be left to be refined later.

Dynamic switching of languages does not work correctly now.

There already is a confirmation dialog.
🤷‍♀️

calcitem · 2021-10-17T12:55:23Z

Dynamic switching of languages does not work correctly now.

There already is a confirmation dialog. 🤷‍♀️

Sorry, this is a false alarm. There is indeed a dialog box.

calcitem · 2021-10-17T13:12:45Z

The passive option is not in effect, and when the passive switch is tapped, the log prints out:

I/flutter ( 6035): [game_settings_page] aiMovesFirst: false
I/flutter ( 6035): [game_settings_page] aiMovesFirst: true

calcitem · 2021-10-17T13:20:40Z

Every commit should work to be expected, so it is unnecessary to ensure that every branch is based on the master branch, and I suggest that this branch be rebased to the dev branch later.

calcitem · 2021-10-17T13:25:43Z

The piecesCount option is not in effect, and when the piecesCount setting is changed, the log prints out:

I/flutter ( 7948): [config] piecesCount = 9
I/flutter ( 7948): [config] rule.piecesCount: 9
I/flutter ( 7948): [config] piecesCount = 10
I/flutter ( 7948): [config] rule.piecesCount: 9
I/flutter ( 7948): [config] piecesCount = 11
I/flutter ( 7948): [config] rule.piecesCount: 9
I/flutter ( 7948): [config] piecesCount = 12
I/flutter ( 7948): [config] rule.piecesCount: 9

From the verification results of the function, it is also true that it does not take effect.

Status: Fixed

isWhiteLoseButNotDrawWhenBoardFull cannot test until this bug is fixed.

Leptopoda · 2021-10-17T13:42:45Z

mainToolbarBackgroundColor

works for me

The passive option is not in effect, and when the passive switch is tapped, the log prints out:

also works for me

The piecesCount option is not in effect, and when the piecesCount setting is changed, the log prints out:

I see what happened there. The Ruleobject (wich updates the engine) is not updated after Rules get changed. I'll fix that.

EDIT:
I'll also fix all the other multiple logging things

calcitem · 2021-10-17T13:45:50Z

The nMoveRule is 100. But in the following move list, less than ten moves and the game is over, which means there is also a problem with this configuration. The reason may be the same as the one you just mentioned.

 1.     d6    b4
 2.     f4    d2
 3.     f6    b6
 4.     f2xb4    b2
 5.     b4    d3
 6.     d1    d7
 7.     g4    a7
 8.     e4xd7    a4
 9.     a1    c4
10.    g4-g1xb6    a7-d7
11.    b4-b6xc4    d7-a7
12.    g1-g4xd3    a7-d7
13.    e4-e3    d7-a7
14.    d6-d7    a4-b4
15.    e3-d3    b4-a4
16.    d3-c3    a4-b4

Status: Fixed

I've paused the test on the Rules -> Generic for now.

calcitem · 2021-10-17T14:03:01Z

Many entries in the Open source licenses are followed by null. Maybe an earlier modification has introduced this.

See #324

calcitem · 2021-10-17T14:12:53Z

When developer mode is enabled, the original behavior of restoring default settings is disabled and is now in effect.

The reason why the function of restoring default settings needs to be disabled is that in the Monkey test, it is necessary to prevent Monkey from randomly clicking on some links that lead to jumping to the web browser and cannot return to the app GUI. If the function of restoring default settings is not disabled, the default settings may be restored randomly in the Monkey test and it will turn off the developer mode, resulting in the following Monkey test appears abnormal.

Status: Fixed

calcitem · 2021-10-17T14:18:54Z

Double-clicking on the big word Mill should only show the configuration items for the developer options instead of turning on developer mode by default. As stated in a post above, the reason for this is that when developer mode is enabled, some features will not respond to user taps. And the user is likely to double-click on Mill by accident, and everything that follows may make the user think that an exception has occurred.

Status: Fixed

calcitem · 2021-10-17T14:24:21Z

This round of testing is complete, pending regression testing after the following code update.

Hopefully, the next code round can rebase multiple branches to one line, so I don't need to merge again locally. Thanks a lot!

calcitem · 2021-10-17T15:01:33Z

For file naming, toolbar is an English word and does not need to be written as tool_bar.

See #325

Leptopoda · 2021-10-17T15:05:15Z

For file naming, toolbar is an English word and does not need to be written as tool_bar.

ups. My bad.
would you please track this, the licence thingy and the audio late init error from above in #303 as regressions. All of them where introduces in #284 and we should probably do a followup to that seperately.

calcitem · 2021-10-17T15:06:04Z

I am not sure if these two lines can be removed from .gitignore.

# Sanmill Settings
sanmill_settings.json

See: #327

calcitem · 2021-10-17T15:09:59Z

For file naming, the toolbar is an English word and does not need to be written as a tool_bar.

ups. My bad. Would you please track this, the license thingy, and the audio late init error from above in #303 as regressions? All of them were introduces in #284, and we should probably do a follow-up to that separately.

Okay, I will create a few issues that are not related to this change.

Leptopoda · 2021-10-17T15:17:51Z

ok I've done a few things:

cleaned up the code base
fixed some rules not being applied correctly
removed duplicate logging (hopefully everywhere)

TODO:

remove some unused functions (seperate commit)
fix the developer mode stuff (clear storage and how it's enabled)

Every commit should work to be expected, so it is unnecessary to ensure that every branch is based on the master branch, and I suggest that this branch be rebased to the dev branch later.

Yes I just started the branch before dev was even a thing. I'll rebase that for the merge. for now you can just run
flutter pub run build_runner build --delete-conflicting-outputs as this should generate the needed code.
EDIT:
It would also be better if you would use the dev branch for future commits :)

I am not sure if these two lines can be removed from .gitignore.

I don't know either. I don't even know what the heck they exclude in the first place 🙃

Okay, I will create a few issues that are not related to this change.

just add them in a list to #303 or assign them to me so I'll find them easier

Also for future code testings:
What about doing a single comment with all findings (in a list)?
I'ts way easier to go through them that way.

calcitem · 2021-10-17T15:33:26Z

It would also be better if you would use the dev branch for future commits :)

You can continue to use the current dev branch.
I can also migrate to this branch and continue committing so that we don't have to do a merge.

What about doing a single comment with all findings (in a list)?
It's way easier to go through them that way.

OK, I'll put it all together, I posted multiple posts before because it was like getting you a quick email alert.

Leptopoda · 2021-10-17T16:20:58Z

the developer mode stuff should be fixed

(this should be everything ...)

calcitem · 2021-10-17T16:42:35Z

334367b Buglist:

Unsloved:

The passive option is not in effect
The setting of mainToolbarBackgroundColor does not take effect.

New:

In the personalization settings, all configurations are not available in the log output. (I forget if this was originally a problem.)

calcitem · 2021-10-17T17:06:05Z

You've worked hard!

I have regression tested the problems that appeared in the previous tests and have found two unresolved issues and one new issue as above. Please take a look. Thanks!

Leptopoda · 2021-10-17T17:22:20Z

The passive option is not in effect

another one where the engine isn't informed about changes in the DB (we should probably fund a new method of this later). Fix is coming...

The setting of mainToolbarBackgroundColor does not take effect.

seems to be there even before the commit (can't find a reference to this color anywhere in the code) probably something for a separate issue

In the personalization settings, all configurations are not available in the log output. (I forget if this was originally a problem.)

it was like this before. I don't think that much logging is needed anyways.

P.s. you don't need to ping me with more comments. I'll see it sooner or later. It just floods my inbox xD

calcitem · 2021-10-17T17:28:36Z

seems to be there even before the commit (can't find a reference to this color anywhere in the code

Confirm that this problem does not exist in git tag v1.1.38.

apk: https://github.com/calcitem/Sanmill/actions/runs/1304450821

P.s. you don't need to ping me with more comments. I'll see it sooner or later. It just floods my inbox xD

OK!

Leptopoda · 2021-10-17T17:55:25Z

Confirm that this problem does not exist in git tag v1.1.38.

an issue should be opened for that.

the other bug should be resolved.

- make new storage reactive - add Database Migrator change how the developerMode reacts

Leptopoda added 2 commits October 14, 2021 16:29

make constants constant :)

16a4e3d

restyled-io bot mentioned this pull request Oct 16, 2021

Restyle Config rework #319

Closed

Leptopoda force-pushed the config_rework branch from 47213ad to 98f2362 Compare October 17, 2021 08:48

Leptopoda commented Oct 17, 2021

View reviewed changes

src/ui/flutter_app/lib/screens/game_settings/game_settings_page.dart Outdated Show resolved Hide resolved

Leptopoda mentioned this pull request Oct 17, 2021

WIP: poc L10n #306

Merged

Leptopoda force-pushed the config_rework branch from 98f2362 to 6a0e6bd Compare October 17, 2021 15:19

Leptopoda force-pushed the config_rework branch from 6a0e6bd to 334367b Compare October 17, 2021 16:20

Leptopoda force-pushed the config_rework branch from 334367b to 67fc96c Compare October 17, 2021 17:48

Leptopoda mentioned this pull request Oct 17, 2021

[Meta] Code celanup ideas #303

Closed

29 tasks

Leptopoda force-pushed the config_rework branch from 67fc96c to fcfaa08 Compare October 17, 2021 20:05

Leptopoda added 2 commits October 18, 2021 11:28

reactive storage and db migration

e93f62b

- make new storage reactive - add Database Migrator change how the developerMode reacts

remove unused code

dc19874

Leptopoda force-pushed the config_rework branch from fcfaa08 to dc19874 Compare October 18, 2021 09:29

Leptopoda merged commit 150fd72 into dev Oct 18, 2021

Leptopoda deleted the config_rework branch October 18, 2021 19:06

Config rework #318

Config rework #318

Conversation

Leptopoda commented Oct 16, 2021

calcitem commented Oct 17, 2021 • edited Loading

Leptopoda commented Oct 17, 2021

calcitem commented Oct 17, 2021

calcitem commented Oct 17, 2021

calcitem commented Oct 17, 2021

calcitem commented Oct 17, 2021

calcitem commented Oct 17, 2021 • edited Loading

calcitem commented Oct 17, 2021 • edited Loading

Leptopoda commented Oct 17, 2021 • edited Loading

Leptopoda commented Oct 17, 2021

calcitem commented Oct 17, 2021 • edited Loading

calcitem commented Oct 17, 2021

calcitem commented Oct 17, 2021

Leptopoda commented Oct 17, 2021 • edited Loading

Leptopoda commented Oct 17, 2021

Leptopoda commented Oct 17, 2021

calcitem commented Oct 17, 2021

calcitem commented Oct 17, 2021

calcitem commented Oct 17, 2021

calcitem commented Oct 17, 2021 • edited Loading

Leptopoda commented Oct 17, 2021 • edited Loading

calcitem commented Oct 17, 2021 • edited Loading

calcitem commented Oct 17, 2021 • edited Loading

calcitem commented Oct 17, 2021 • edited Loading

calcitem commented Oct 17, 2021 • edited Loading

calcitem commented Oct 17, 2021

calcitem commented Oct 17, 2021 • edited Loading

Leptopoda commented Oct 17, 2021

calcitem commented Oct 17, 2021 • edited Loading

calcitem commented Oct 17, 2021

Leptopoda commented Oct 17, 2021 • edited Loading

calcitem commented Oct 17, 2021

Leptopoda commented Oct 17, 2021 • edited Loading

calcitem commented Oct 17, 2021 • edited Loading

calcitem commented Oct 17, 2021 • edited Loading

Leptopoda commented Oct 17, 2021 • edited Loading

calcitem commented Oct 17, 2021 • edited Loading

Leptopoda commented Oct 17, 2021

calcitem commented Oct 17, 2021 •

edited

Loading

calcitem commented Oct 17, 2021 •

edited

Loading

calcitem commented Oct 17, 2021 •

edited

Loading

Leptopoda commented Oct 17, 2021 •

edited

Loading

calcitem commented Oct 17, 2021 •

edited

Loading

Leptopoda commented Oct 17, 2021 •

edited

Loading

calcitem commented Oct 17, 2021 •

edited

Loading

Leptopoda commented Oct 17, 2021 •

edited

Loading

calcitem commented Oct 17, 2021 •

edited

Loading

calcitem commented Oct 17, 2021 •

edited

Loading

calcitem commented Oct 17, 2021 •

edited

Loading

calcitem commented Oct 17, 2021 •

edited

Loading

calcitem commented Oct 17, 2021 •

edited

Loading

calcitem commented Oct 17, 2021 •

edited

Loading

Leptopoda commented Oct 17, 2021 •

edited

Loading

Leptopoda commented Oct 17, 2021 •

edited

Loading

calcitem commented Oct 17, 2021 •

edited

Loading

calcitem commented Oct 17, 2021 •

edited

Loading

Leptopoda commented Oct 17, 2021 •

edited

Loading

calcitem commented Oct 17, 2021 •

edited

Loading