Migration to Material Design 3 (fixes #1833, fixes #1418)

[Bnyro]

The PR contains the following:

• Monochrome icon (closes Add support for Material You themed icon #1833)
• Migration to Material Design 3, including dynamic colors if available

also closes #1418

It might be possible that I forgot to migrate some more hidden parts of the app, feel free to let me know if that's the case so that it can be migrated too!

[imsodin]

Thanks a lot for your contribution!

In the spirit of transparancy disclaimer up-front: This app is basically on life support. I am even struggling to find the time/energy to keep it building with new android/syncthing/go versions (currently build with go 1.20 is broken). On a very high level you changes look clean, and luckily we have a few motivated test users - so if everything looks and behaves fine we can move forward but I likely won't be able to give much assistance with any problems. Though it looks like you are quite proficient with android apps, so I hope you'll be just fine there.

CI is failing due to this - you can check yourself, you don't need to create an account there (there's some guest access link on the login page):

49
/opt/tcagent/syncthing-3-work/19a8ed19ae4d1032/app/src/main/java/com/nutomic/syncthingandroid/activities/QRScannerActivity.java:62: Error: Overriding method should call super.onRequestPermissionsResult [MissingSuperCall]

19:11:49
public void onRequestPermissionsResult(int requestCode, @nonnull String[] permissions, @nonnull int[] grantResults) {

Though I would have expected it failing due to bumping of the compile/target SDK: That both requires some changes to the build setup and I'd expect also some changes to the code to adapt to SDK changes. Is this a requirement for the material migration? If not I'd suggest to just roll that back.

[Bnyro]

Thanks for the feedback!

CI is failing due to this - you can check yourself, you don't need to create an account there (there's some guest access link on the login page):

Alright, I've attempted to fix that now!

Though I would have expected it failing due to bumping of the compile/target SDK: That both requires some changes to the build setup and I'd expect also some changes to the code to adapt to SDK changes. Is this a requirement for the material migration? If not I'd suggest to just roll that back.

Unfortunately we need to use at least SDK 32 to be able to use the newer version of the Material Design library (which is used for Material Design 3), so I've now changed it from 33 to 32 with the hope that this will make it easier for us for now.

I've also just noted that the app introduction screens are not yet migrated to Material Design 3 yet, so I'll give that a shot too now :)

[Bnyro]

Seems like the checks have passed! 🎉

[imsodin]

The built setup changes don't automatically apply, a new builder docker image needs to be deployed first. Still a good sign it already builds on the old image. Build tools should likely be bumped to match the new target (32) as well. To make testing/deploying with the new setup simple, would you mind posting separate PRs for the changes, starting with the SDK bump only? Resp. I can take care of that myself, but maybe not in the next two weeks. I'll try to get it done before the next release cycle (1.23.4 resp 1.24.0, syncthing RC will be released on second Tuesday of April and android app usually too).

Also looks like 32 is just a "minor" upgrade, still android 12 with stuff for larger screens. From a quick look into the docs, there shouldn't be any behaviour changes that materially (haha) change behaviour for this app - maybe stuff breaks/improves for large devices, which honestly I don't care about - so that should be good.

The material UI changes: Is this mainly a dependency bump/migration without big changes or does this actually change the UI and maybe even behaviour? If it does, could you provide some sample screenshots and in case of behaviour changes a quick summary of what changes please.

Thanks again for your work here!

[Bnyro]

Indeed that also brings in some design changes as per the guidelines of Material Design 3. However, these are not that significant and there are no changes to the behavior of the app. Below some screenshots built from the latest commit of the PR:

[Bnyro]

The built setup changes don't automatically apply, a new builder docker image needs to be deployed first. Still a good sign it already builds on the old image. Build tools should likely be bumped to match the new target (32) as well. To make testing/deploying with the new setup simple, would you mind posting separate PRs for the changes, starting with the SDK bump only? Resp. I can take care of that myself, but maybe not in the next two weeks. I'll try to get it done before the next release cycle (1.23.4 resp 1.24.0, syncthing RC will be released on second Tuesday of April and android app usually too).

You mean just creating a separate PR that bumps the SDK and build tools to version 32 (and also fixes that error that caused the builds to fail previously)? I could do that, however I don't really see the benefit of it since there's no advantage in getting in work within a different PR?

[imsodin]

I just tested, merged and pushed the sdk bump to 32 - no fixes were necessary, that's likely related to the other changes in this PR. Also re-ran CI on this PR with the new builder image -> green. You can rebase on latest main and drop the sdk related build changes in this PR.

[Bnyro]

I'm sorry for being annoying, but is the thing currently blocking here that some builds don't work properly yet? The CI on GitHub seems to pass, but I'm not familiar with how the Syncthing CI integration is working actually. Though no trouble if it's just because it requires some testers before, I don't mind that, I just want to make sure I didn't do anything wrong that stops us from continuing and you'd be waiting for me all the time :)

[imsodin]

Nothing wrong on your side - I am the bottleneck. Started to look now, I'll try to finish this week. Roughly my plan is: Ask you a bunch of question, that you'll hopefully indulge so I feel ok rubberstamping this (not much doubt/uncertainty here), then get the RC out with this and ask a few (esp. one) of our estemeed tester to try to poke holes into them. Ah yeah, and would be nice if you'd stick around for fixes if any found with this :)
Does that sound ok?

[Bnyro]

Yes, of course, I'm waiting patiently for some questions I can answer and suggestions I can work on :)

[imsodin]

Looks good. Not going to pretend I understand the purpose of all changes, but changes look sensible (and move away from compat/deprecated stuff, so anyway good), screenshot look nice and anything else will come up in testing (resp. hopefully wont).

Added a few comments. For the "why" questions there's no need to go in depth or anything, if there's a simple answer I'd appreciate it, otherwise for android "because that's how it's done" goes a long way.

[imsodin]

@Bnyro Did you see the review/questions? I am now more or less just waiting for you to give me a bit of reassurance on the few questions above and then I'll ship this. And the next syncthing RC just dropped, so it's either more or less now or another month until that gets release (a month isn't an issue at all of course, plus I am not complaining - especially not considering how long it took me to react).

[Bnyro]

Oh, I just saw it, I'm really sorry!
I somehow didn't get notified (or just missed it because I had so many other unimportant notifications), otherwise I would already have reacted earlier :/
I'm be a bit busy currently but I think I'll find time for this in the next days, so we can finally get this done :)

[Bnyro]

The latest commit should have addressed all your comments.
I've now also added a top bar to the settings activity, because it would probably feel inconsistent to not have one in the settings and the layout would feel very empty without (in my opinion).

[imsodin]

Awesome - thank you!