AsteroidOS / AsteroidOSSync

Android application to synchronize a phone with a watch running asteroid-btsyncd.
GNU General Public License v3.0
101 stars 37 forks source link

Some updated + material 3 icon #215

Open quantumde1 opened 7 months ago

quantumde1 commented 7 months ago

Material You icon, Android 5.0 backward compatability, Gradle update, dependencies update

FlorentRevest commented 6 months ago

The gradle and dependencies update look good to me but I don't understand the rationale behind changing the icon ? We should at least get an opinion from our designer @eLtMosen before doing such a change, and this should be thought through in a different context than a dependencies update.

Also, this PR bundles a bunch of precompiled binary files which shouldn't ship as part of our git repositories, like the app/release/ directory.

Also it would be good to explain why certain changes are done like those to app/src/main/java/org/asteroidos/sync/connectivity/ScreenshotService.java as part of the commit message.

eLtMosen commented 6 months ago

Indeed, a discussion to change the app icon should be out of scope of this PR due to its big impact. The orange and unicolor icon versions have been tailored to each display situation. And changing them would require testing all those again. In a PR with visual changes, it is advised to attach screenshots of the changes to speed up the review process and help more people understand the changes and voice an opinion on them.

quantumde1 commented 6 months ago

The gradle and dependencies update look good to me but I don't understand the rationale behind changing the icon ? We should at least get an opinion from our designer @eLtMosen before doing such a change, and this should be thought through in a different context than a dependencies update.

Also, this PR bundles a bunch of precompiled binary files which shouldn't ship as part of our git repositories, like the app/release/ directory.

Also it would be good to explain why certain changes are done like those to app/src/main/java/org/asteroidos/sync/connectivity/ScreenshotService.java as part of the commit message.

When updated Gradle and changed compatability i changed some code for work on Android 5.0+. Icon is changed because i think many android 12+ version users who is own watches on AsteroidOS want to get Material You UI(at least, many people who is using A12 and higher in environment doesnt like non-dynamic icons). And i think it will be nice to show users your attitude to them as developer, people will be happy! About binaries - im sorry, forget to remove binaries before push. I will remove them ASAP.

eLtMosen commented 5 months ago

Icon is changed because i think many android 12+ version users who is own watches on AsteroidOS want to get Material You UI(at least, many people who is using A12 and higher in environment doesnt like non-dynamic icons).

This sounds like an improvement for only a subset of users, with potential unknown regressions for others.

Discussing and improving design is a rather involved matter. While we appreciate the initiative, we need to ensure that the proposed icon changes do not negatively impact the user experience across different systems.

To be thorough, it is important to provide screenshots that compare the icons before and after the change for every different UI screen where the icon appears. This should be done for as many impacted systems as possible.

At a minimum, please show the before/after changes on the system and UI you use. It would be helpful to clarify what bothers you about the old icon and visually demonstrate how the new icon improves the experience in the given situations.

This way, reviewers can technically test and evaluate your changes on their systems to check for regressions. However, it is quite time-consuming for reviewers to do this without initial evidence. If you are serious about changing the icon and want to convince the community, it would be very helpful to conduct this initial work and check for regressions yourself.

That is the reason why @FlorentRevest asked to separate the icon changes from your much-welcome updates and fixes. These updates could quickly be merged, while the icon changes involve a community discussion and more preparation. This discussion is ideally done on our Matrix channel, which you could join if you haven't already. https://app.element.io/#/room/#Asteroid:matrix.org

quantumde1 commented 5 months ago

Screenshot_20240603-004243_Настройки Screenshot_20240603-004232_Trebuchet As you can see, now icon is white-orange while without material you, and system color when with. Okay, i'll cut commits for different branches, so you can merge gradle updates. I was tested my build on android 6.0, its working too, without any issues.