r2-studio / robotmon-scripts

Run Javascript on Android. Screenshot, Touch, ...
Apache License 2.0
78 stars 66 forks source link

Very big code refactorings #485

Closed mcs closed 10 months ago

mcs commented 10 months ago

This PR covers the following topics:

  1. Made settings more robust to version changes by switching from index-based settings to name-based ones.
  2. Split HTML and JS to make things clearer and IDEs happier.
  3. Introduced JS strict mode to make the code base safer.
  4. Added build date to settings.
  5. Extended and harmonized Windows and Linux build scripts.

Especially (1) felt absolutely mandatory for me. I wanted to add more bonus item settings to the config, but that would have broken existing settings. This would surely have happened with almost every feature extension where I would have added a new setting somewhere. As that is not very user friendly, I took the effort and rewrote big parts of the settings bindings, persistence and command passing.

The whole PR is big, so I tried to make small consistent and understandable commits. I hope that makes reviewing this a little bit less painful.

Edit: I intentionally did not update the version number, because this PR does not ship any "cool" features or bugfixes, so probably no user will currently care about these changes.

Sean-Hsueh commented 10 months ago

Hi Mcs,

As far as I can recall, spliting html and css might cause some issues on particular phone models, causing the config page unable to render.

Since this is a huge refactoring, may we suggest one of the following:

  1. merge the HTML and CSS back to the same file, and we will proceed merging this PR
  2. creating a new folder (say, scripts/com.r2studio.TsumMCS) with these changes. We will followup user amount through Google analytics. If a certain amount of players does not have any issue, we will merge it back to main code.

This code was first built roughtly 4 years ago. There were some legacy issues causing the JS hard to maintain. Recently we build a framework, the Rerouter, intending to help developers write Robotmon code easier: https://github.com/Hellomon/rerouter

Should you wish, perhaps we can provide a Rerouter hello world demo project for your reference?

mcs commented 10 months ago

Hi @Sean-Hsueh,

Thanks a lot for your feedback 👍

As far as I do not inevitably have to put the JS back into the HTML, I am absolutely fine with re-joining the HTML and CSS if that improves stability on certain legacy devices. So I take option 1 because realistically, I don't think that enough players will switch to a new plugin "TsumMCS" or however it is named, if the old one "runs fine". And as I want to start develop features sooner than later, I stick to a combined HTML/CSS file and hope getting early feedback if the other changes were fine (they should, but who knows without a thorough test suite...).

I just reverted the CSS extraction, corrected the build files rebased the changed into the existing commits and rebuilt the index.zip in the HEAD commit. "Real" changed occured on 1c6ee8c, 0010297 and 2dca469. The rest just got new hashes.

(Edit: Just found out that Github even has a diff view especially for "force pushes" where one can see just the effective changes. Quite neat, that feature 👍)


In general, TypeScript probably would be a massively better programming language, especially because I almost never worked with JavaScript, but usually Java and Kotlin. So TS would be great on the one hand. On the other, that brings even more refactorings I guess 🥹

mcs commented 10 months ago

Regarding Rerouter: I probably didn't really understand about how I might use it. Did you mention it as a reference TypeScript project in case I want to migrate the existing TsumBeta JS files to TS? Or am I expected to somehow include the Rerouter into the TsumBeta code instead of the "baked in" functions? I am not good with the modern JS ecosystem and TS, and I also do not completely understand how things work together within Robotmon and the r2-studio...

Sean-Hsueh commented 10 months ago

Hi Mcs,

Thanks for the feedback. To be more specifically (sorry for not mentioned in the last reply), we should only have two files, index.html and index.js, to ensure properly render the setting page for most of the devices. Therefore we need to merge the setting.js into index.js as well.

We do understand the current html is hard to maintain, and we totally agree Typescript would be a great alternative. We will most likely do so if we draft a new script for another game. Since we are not planning to add new features to Tsum in near future, we kind of just leave it until next time we need it. So we are very much appreciate your effort in help refactoring it :)

Hi @Sean-Hsueh,

Thanks a lot for your feedback 👍

As far as I do not inevitably have to put the JS back into the HTML, I am absolutely fine with re-joining the HTML and CSS if that improves stability on certain legacy devices. So I take option 1 because realistically, I don't think that enough players will switch to a new plugin "TsumMCS" or however it is named, if the old one "runs fine". And as I want to start develop features sooner than later, I stick to a combined HTML/CSS file and hope getting early feedback if the other changes were fine (they should, but who knows without a thorough test suite...).

I just reverted the CSS extraction, corrected the build files rebased the changed into the existing commits and rebuilt the index.zip in the HEAD commit. "Real" changed occured on 1c6ee8c, 0010297 and 2dca469. The rest just got new hashes.

(Edit: Just found out that Github even has a diff view especially for "force pushes" where one can see just the effective changes. Quite neat, that feature 👍)

In general, TypeScript probably would be a massively better programming language, especially because I almost never worked with JavaScript, but usually Java and Kotlin. So TS would be great on the one hand. On the other, that brings even more refactorings I guess 🥹

Sean-Hsueh commented 10 months ago

More about rerouter: after building some more automation scripts in the past few years, we learned that a huge piece of logic can be generalized into a framework. For example, matching pixel with specific color in a screen, repeatly doing something for X times, or until Y minutes are reached, should be included in a lib for developers to reference.

In another words, Rerouter is built (and we are still working on it) to help developers focus on automation logic, comparing to scripts/com.r2studio.TsumBeta/index.js, where we need to handle doing job X for Y times until Z fulfilled.

However refacting current TsumBeta/index.js into Rerouter would be pretty complicated so is not recommended at the moment. We just share our latest progress in case you want to build some other script :)

Regarding Rerouter: I probably didn't really understand about how I might use it. Did you mention it as a reference TypeScript project in case I want to migrate the existing TsumBeta JS files to TS? Or am I expected to somehow include the Rerouter into the TsumBeta code instead of the "baked in" functions? I am not good with the modern JS ecosystem and TS, and I also do not completely understand how things work together within Robotmon and the r2-studio...

mcs commented 10 months ago

Okay, so introducing something like https://www.npmjs.com/package/html-inline-external would fix the problem, right?

Sean-Hsueh commented 10 months ago

Looks promising! Can you please put that into build.sh so we can apply the same in case we need to update the index.html as well?

Okay, so introducing something like https://www.npmjs.com/package/html-inline-external would fix the problem, right?

mcs commented 10 months ago

I close this as the latest change (inlining of settings.js) breaked the settings page. I'll reopen when this finally works.