search

markusfisch / ShaderEditor

Android app to create GLSL shaders and use them as live wallpaper
https://play.google.com/store/apps/details?id=de.markusfisch.android.shadereditor
MIT License
924 stars 137 forks source link

Errors #195

Closed AntonPieper closed 3 months ago

AntonPieper commented 3 months ago

Had to reopen the PR, because GitHub prohibited me from pushing. It was only resolved by completely deleting my fork all together.

See #194 for details

Closes #146

markusfisch commented 3 months ago

Great work! As always! 👍

I just really would have preferred separate PRs for the improved error handling and updating Material/AndroidX and migration to Material You. I know it's natural to change one with the other when you're at it, and these changes are very much intertwined. But since I like to squash these feature branches into one commit for a clean history, this would result in unrelated changes ending up in just one commit (what is already true for the very first commit of this PR). Which means the history of changes is less useful, and we can't work with certain changes only. For example, say a certain version of the Material library contains a bug. Then, it wouldn't be as easy to track that bug to a certain change.

Then, while I see the advantage and convenience of showing all the errors, and doing so in a bottom sheet, it's quite some change, and code, to make this work. For me, the original error toast was all I ever needed, so this feels a bit heavy for me 😉 But of course, there's demand for this improvement and I am very grateful you took this on! 👍

Now, because merging this also means abandoning API levels < 21, I've just published 2.34.3 as the last version for older phones.

AntonPieper commented 3 months ago

Thanks for the feedback😁 I'll try to commit my changes more atomically for the next PRs👍 I know this has been an issue with the error highlighting PR as well, so I definitely should keep that in mind more actively while working on features 😬

P.S.: I could also Backport the error handling to the older androidx and material support libraries if you prefer not to upgrade them or want an ad hoc in-between state :) I mainly updated the dependencies, because bottom sheet behavior looks nicer on Material You imo and Theme.Material3.DynamicColors.Night didn't have a NoActionBar version before the new update.