Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Meta] Code celanup ideas #303

Closed
25 of 29 tasks
Leptopoda opened this issue Oct 8, 2021 · 18 comments
Closed
25 of 29 tasks

[Meta] Code celanup ideas #303

Leptopoda opened this issue Oct 8, 2021 · 18 comments
Assignees

Comments

@Leptopoda
Copy link
Collaborator

Leptopoda commented Oct 8, 2021

So we don't forget our discussed TODOs from #284:

  • JsonSerializable for the config/preference/settings saving

  • flutter gen_l10n this is an arb parser built into flutter

  • use showLicensePage for the licence page

  • remove need for flutter-init.sh script (the above two will do most of it)

  • migrate theeming into the ThemeDatacalss and access it via Theme.of(context)

  • reactive programming. Use Provider or ValueLIsteners instead of static fields (especially for config/settings/preference) this will allow us to access those just like theme via Settings.of(context) listeners should be added where needed to update the UI (this might be needed for JsonSerializable aswell)

  • go through null safety (the current builds are null safe but there are a lot of values we could make better null aware)

  • Build types/settings (following stuff shouldn't be needed in the production build)

    • developer mode
    • advanced debuging utilities (as we log quite a bit we could get performance problems)
  • migrate to dart_ffi and ffi_gen for the platform interfce. This will enable easier builds as the dart build system could compile the C code and we wouldn't need to add many extra settings for iOS, Linux etc.

EDIT:

  • restructure used Icons. I don't know how FluentIconsis implemented but adding custom icons usually means another font that's added to the app. This leads to larger apk sizes. We could use material icons or segmentize the used ones
  • refractor the splash screen. As said by the compiler the current splash screen implementation is deprecated.
  • refractor storage backend. Maybe something like hive?
  • rework the android flutter binding (the gradle linter says that enable-R8 is deprecated as R8 is enabled by default. Also some kotlin stuff.) related to the splashscreen thingy
  • refractor the drawer. We currently maintain our own drawer implementation. as seen in Tap menu item will get an exception when AI is thinking #316 this can lead to some problems. We should either rewrite it or use a lib for this.
  • use IndexedStack for the home screen view. This might need a bit more resources but as the screens are allready build switching between (using the drawer) should be more performant althought the switching is more performat the resource usage is to high
  • remove the duplicate rules classes
  • use directional propperties so we don't need to check rtl/ltr manually
  • delete unused overrides on the game page
  • rework privacy page
  • remove dead code and unused parameters (make things private so the linter will find them
  • better utilize arb's localization features (variables, pluralization, etc)
  • better utilize our compile time configs. (disable developer mode in production ...)#
  • extends our enums dart doesn't let us add methods to enums but we can still extend them :) (just stumbled over this in java and I quite miss it now xP)
  • unify string notation ("" or '') the first is prefered as we don't need to escape ' in that case
  • remove uneeded files (in l10n and other dirs)
  • Reactive font scaling Reactive font scaling #400

Performance

  • Use repaint boundary
  • use should repaint
  • let the gamePage get the engine type from the gameInstance so it doesn't need it as a parameter

Anything I missed?

@calcitem
Copy link
Owner

calcitem commented Oct 8, 2021

These ideas are very cool! Many points are beyond my imagination!

The developer mode can be retained in the release version, there is no problem, because the user need to continuously click on the Mill title to activate the menu, so hard to enable it. In the production version, we also need to perform some tests.

I also thought of one that allows users to export configuration files and send them to developers via email to facilitate the diagnosis of whether the abnormality is related to the configuration. For example, some users report that AI has cheated, and it is necessary to know whether the user has enabled the flying rule.

Change most of the logic in the mill folder to call C++ code through ffi. Currently, the code modification of this folder is relatively small, which is acceptable.

I want to add some test codes related to widgets for regression testing, but I don't know how to add basic code quickly.

At present, there are at least 3 users hope me to add 3 and 6 Men's Morris but the current code flexibility is not enough to support rapid development of this function, and the front-end and back-end parts need to be refactored for this feature.

I don't know if there is a way to successfully sign Github Action without the need for developers to make signature files src/ui/flutter_app/android/key.properties to compile locally. I have not yet found a good way. However, this issue is not of high priority.

These ideas may not belong to the category of meta, but these requirements may need to be considered when refactoring.

Thinking of these for the time being.

Thanks!

@calcitem
Copy link
Owner

calcitem commented Oct 8, 2021

The modification of the Shell script may affect the compilation of Github Action and F-Droid, so please pay attention.

F-Droid:
https://gitlab.com/fdroid/fdroiddata/-/blob/master/metadata/com.calcitem.sanmill.yml

@calcitem
Copy link
Owner

calcitem commented Oct 11, 2021

restructure used Icons. I don't know how FluentIconsis implemented but adding custom icons usually means another font that's added to the app. This leads to larger apk sizes. We could use material icons or segmentize the used ones

FluentIcons was used in the early days. Because there was no suitable icon in FluentIcon for this game, the most typical problem is that we can't find any robot icon. So the icon was replaced by an external iconfont file based on the suggestion of a UI designer https://github.com/iJahangard from Iran. There is no significant increase in file size.

I don't know if it is possible to extract only the used icons into a condensed font. If possible, this is an optimization direction.

refractor storage backend. Maybe something like hive?

I want to know what the database is used to store? Is it configuration? Will the refactoring cause the user to lose the configuration after the upgrade?

@Leptopoda
Copy link
Collaborator Author

Do we need all the logging in the config stuff?

now that #318 is going to provide the storage backend we should probably remove a lot of the logging. The new database service is very robust so we should be able to eliminate it (too much logging => less performance).

For debugging reasons we could let catcher log the current configs when a crash happens.

What do you think @calcitem ??

@calcitem
Copy link
Owner

calcitem commented Oct 17, 2021

Sometimes when an crash occurring in a monkey test or a manual test, it is necessary to know what has been tapped before. Therefore, there needs to be a way to record the history of taps or configuration changes. If there is no log, other alternatives are needed.

Tip: No matter how to refactor and add new functions, I hope to maintain the characteristics of this app that does not require any special permissions. Our guideline is to keep this app as a reliable and safe app.

@calcitem
Copy link
Owner

calcitem commented Oct 18, 2021

The Git atomization of these commits on the current dev branch is doing well! Awesome!

It's incredible to be able to make tens of thousands of lines of changes in a short time!

I have finished the review, and all the questions have been posted. Thanks!

For branch dev, there are only two questions left at the moment:

  1. Swiss German and Traditional Chinese cannot take effect;
  2. Main toolbar background color config.

To merge from the dev branch to the master branch, is it better to use merge or rebase? In other words, is it better to maintain a tree structure or a linear history?

@calcitem
Copy link
Owner

rework privacy page -- means?

My expectation is that this app do not need to apply for any special permissions, including networking/storage/camera permissions, etc., just keep it clean. Although some features are very tempting, I had wanted to increase the function of identifying the board through the camera, and I have wanted to report the Catcher information to third-party websites so that many people view, but give up. User trust is more important.

@Leptopoda
Copy link
Collaborator Author

rework privacy page -- means?

just that the current code that launches it does not look that great :(

I only want to rewrite the current implementation. Keep in mind that the issue and these ideas are code cleanup ideas that shouldn't alter the program/UX at all.

@calcitem
Copy link
Owner

The branch dev has been merged to master:

615047f

@Leptopoda
Copy link
Collaborator Author

why did you merge dev into master??
we clearly still have some bugs.

Can I just revert the merge and we can hold off until the bugs are fixed??

@calcitem
Copy link
Owner

It doesn't matter. At this time, only you are involved in the project, and everyone else is a foreign language translator.

@Leptopoda
Copy link
Collaborator Author

Leptopoda commented Oct 30, 2021

So I don't forget:

  static const String environmentVariablesFilename =
      "assets/files/environment_variables.txt";
  static const String gplLicenseFilename = "assets/licenses/GPL-3.0.txt";

the paths should also be generated by flutter_gen

EDIT: done

@Leptopoda
Copy link
Collaborator Author

Another thing to remember: I want to cleanly implement the feedback localization thingy

@Leptopoda
Copy link
Collaborator Author

@calcitem some other cleanup not directly related to the code would be to delete all unnecessary branches. We have a lot of stale branches cluttering the repo. I think we should delete them.

@calcitem
Copy link
Owner

calcitem commented Nov 1, 2021

Not long after you joined the project, I have deleted a lot of branches, and I don't think there is any need to delete further now. The branches left on the above are all WIP, and we need to refer to the implementation of the original branch when developing on the new code base.

@calcitem
Copy link
Owner

calcitem commented Nov 1, 2021

In fact, most of branches are modifying the AI ​​part, which should not affect the Flutter front end.

At present, the most important thing is the support of new rules and the implementation of the MCTS algorithm introduced to reduce the difficulty of AI. The difficulty is too high and the support of rules is incomplete. These two parts have caused the most negative reviews.

In the front-end part, the main negative comment comes from the lack of guidance/widzard. Some people think that the AI ​​is cheating. Users who believe that AI is cheating often do not have mailboxes, they cannot receive e-mails explaining the rules of the game, and they often uninstall directly after making negative reviews. These users are often too lazy to look at the configuration items. There are too many configuration items now. Some people are impatient to read them, so we may need to let the user choose whether to enable the advanced mode in the wizard. In the wizard mode, we need to add some animations and images to make it easy for users to understand the rules.

In addition, some users think that the pieces move too fast and lack animation.

When the app was first released, the average score was 4.9. As time went by, it dropped all the way to 4.1. In the past, when the number of downloads was not high, it was often downloaded by professional users through searching keyword. After the ranking rose, some players who did not understand mill also downloaded it to experienced, so all kinds of weird bad reviews have come out, so this program should not only take care of the needs of professional and serious players, but also need to start considering novice players.

@calcitem
Copy link
Owner

My point is that there is no need to go too far to reduce the size of the application. NNUE training files will be added in the future, and the size will increase by 30MB!

@calcitem calcitem unpinned this issue Feb 2, 2022
@calcitem
Copy link
Owner

calcitem commented Feb 2, 2022

Thank you for your hard work, the remaining work has been migrated to #530 for recording, so this issue is closed.

@calcitem calcitem closed this as completed Feb 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants