SchildiChat / SchildiChat-android

Matrix client / Element Android fork
https://schildi.chat/android/
Apache License 2.0
386 stars 49 forks source link

Adds in themable icon, with beta variant #165

Closed NathanC closed 1 year ago

NathanC commented 1 year ago

This PR adds in Android 13 themable icons, stemming from a discussion in https://github.com/SchildiChat/SchildiChat-android/issues/137

@SpiritCroc Your comment about my design looking like glasses was totally right, I couldn't stop seeing it after you suggested it.

As requested, here's the inverted version with the hollow shell: ![image](https://user-images.githubusercontent.com/6182419/205431287-30ceabbb-ad63-471c-9687-b7ed0a9b4486.png)

However, that feels a lot less robust/balanced to me, and I ended up modifying my original design from the other thread, but disconnected the feet and made them flatter and solid, to make them not look like glasses or eyes. I also tweaked the height and size a bit. Here are the main and beta themed variants I ended up with:

image

The .xmls in this commit were just created by directly importing the SVGs as a drawable. If this PR gets merged, I'll upload the SVGs I used to the schildichat meta repo. I initially had the beta character function as a mask in Inkscape, but Android Studios was not understanding it, so I turned the text into a path and diffed it with the shell section underneath it, cutting a whole in it. The feet are actually one path mirrored around the central axis, so both feet will be kept symmetrical if any changes are made to its path.

Thank you for all the work you've done, I really enjoy using this app!

NathanC commented 1 year ago

Wait to merge this, I went through a lot of iterations with the beta icon and I may have imported the SVG with the wrong stroke width, gotta make sure I have the source SVG properly saved (and will open a PR against schildichat-meta before this gets merged)

SpiritCroc commented 1 year ago

I agree, filled shell looks better. The new version looks good to me!

The icons in graphics are meant to be the single source of truth, so in case you want to change icons in the app, you only need to change the svgs in there, and then you can run graphics/icon_gen.sh. It's not really necessary for restoring after changing to beta, since we have version control with git ;) But looks like you did it correctly either way :)

Thanks for your dedication to update the build process, I didn't really expect that :)

SpiritCroc commented 1 year ago

Hm, it would be nicer though to not have an xml, but an svg in graphics, and then generate the xml from icon_gen.sh - if it's not difficult to implement.

NathanC commented 1 year ago

Glad you like it! Also, disregard my above comment, this PR is ready to go as is. I opened a PR at https://github.com/SchildiChat/schildichat-meta/pull/4 and wanted to double check that the final version of those SVGs were correctly imported as the drawables in this PR, and they are.

And no problem about updating the build process. I'm working on a personal android project, and this was very educational seeing how a much larger project is built.

NathanC commented 1 year ago

Hm, it would be nicer though to not have an xml, but an svg in graphics, and then generate the xml from icon_gen.sh - if it's not difficult to implement.

Do you know if there's a way to programmatically invoke the same logic that android studios uses to import drawables? Drawables can be derived from SVGs, but aren't exactly the same (e.g., masks don't seem to be supported), they seem to essentially be a subset. I'd prefer not generating all the png mipmaps here, but I could if you think it's worthwhile.

.. eh, PNGs may be a more faithful representation of the SVG though, which could be good, and it would be nice not to have the indirection of a manually imported resource. I'll give it a whirl.

SpiritCroc commented 1 year ago

I haven't used a CLI for that before, don't know.

Do we actually get an advantage when doing xmls over pngs? At least back in the day, you wanted pngs for less expensive device computations, but not sure, if vector xmls are actually better-looking on user devices, or just more easy to work with for developers.

NathanC commented 1 year ago

Do we actually get an advantage when doing xmls over pngs?

I'm honestly not sure, especially for themable icons. My intuition is that SVGs would just be sharper/have correct DPI on everything, but Drawables aren't really SVGs. It would save some space and make a smaller APK, but it's not a huge difference.

Testing the process with PNGs now, we'll see how it looks :)

SpiritCroc commented 1 year ago

btw, launcher icon pngs go into mipmap instead of drawable, just in case you didn't know - apparently makes a difference in how they are treated based on dpi and icon size

NathanC commented 1 year ago

Running icon_gen (to produce the initial batch of mipmaps for monochrome) shows all the mipmap pngs as being changed (e.g., Riot splash), because svg -> png transformation is presumably not deterministic. I can just comment out all the other exports within icon_gen, but that feels a bit hackish—how do you usually deal with that? I believe fdroid enforces reproducible builds?

NathanC commented 1 year ago

(It's possible that the deltas are because of your computer vs mine, maybe different graphics cards or Inkscape versions. The PNGs look identical, but git marks them as changed)

SpiritCroc commented 1 year ago

Last time I tried, it changed all as well, maybe a newer inkscape version or something. I've been thinking a bit about the Material You theming, and I think we better stick to vector graphics, since I'd expect that should allow for more accurate post-processing. Having the xmls in the graphics dir is ok to me as long as we have the svg somewhere (I know it's possible to get a svg from an xml, but I remember it's a little cumbersome).

SpiritCroc commented 1 year ago

We don't have reproducible builds right now. I think at some point in the past it didn't change all icons, but not sure. My approach if necessary would be to just git reset the changes.

NathanC commented 1 year ago

I don't think it's because of Inkscape version tbh, I think the conversion process is likely just not fully deterministic. See here for some interesting discussion around it.

NathanC commented 1 year ago

Are you sure you're OK with sticking with the xml approach for now? I see how the png generation is done, and can definitely do it if you'd prefer. It's possible it'll look slightly different than the screenshot I gave though.

The SVGs that lead to the XMLs in this PR can be found in my PR to the meta repo. We could include them in this PR even if we don't leverage them directly, but in that case I'd recommend sticking with just the meta repo so it acts as more of a source of truth. Up to you.

SpiritCroc commented 1 year ago

If I run the script twice, it doesn't change again. So to me looks like inkscape is at least deterministic for a fixed setup. Actually, if you look at the meta repo's readme, it says that the downstream repos are the source of truth and the meta repo just collects the icons (I forgot that myself for a moment, or I would've told you earlier) :)

SpiritCroc commented 1 year ago

Currently reading into https://www.androiddesignpatterns.com/2018/11/android-studio-svg-to-vector-cli.html - sounds like you can use AndroidStudios conversion, but only if you build it yourself :thinking: Will look a bit more

SpiritCroc commented 1 year ago

I think let's just have both xml and svg in graphics, and maybe some readme stating you have to convert it manually :shrug:

Btw, I'll delay merging this a bit until I have time to test it, don't know when

NathanC commented 1 year ago

heh, I just pushed a commit doing exactly that! I think it's a good middle ground for now.

And sounds good. I'm eager to get this on my main device, but there's no actual rush. Thanks for all the help with this, and being open to the change!

NathanC commented 1 year ago

btw when you're testing this, you need android 13, and you'll need to make sure that you've gone into "Wallpaper & Style" and enabled "Themed icons". Let me know if you run into any issues.

SpiritCroc commented 1 year ago

btw when you're testing this, you need android 13, and you'll need to make sure that you've gone into wallpaper and style and enabled "Themed icons". Let me know if you run into any issues.

Yeah, figured that out by now :) Sure, will do.

First, time to fight WYSIWYG merge conflicts in the latest upstream merge though...

SpiritCroc commented 1 year ago

This should be included in the latest beta. Github apparently didn't notice I merged it into the sc_beta branch, but I guess the issue will get closed automatically once I push it to sc.

NathanC commented 1 year ago

Probably because this was opened against sc so it got confused :woman_shrugging: Anyhow, thanks! It works in the newest beta for me. This rounds out my homescreen, everything is themed now.

I just realized, this monochrome icon might work instead of the material design turtle for the android notification icon, though it may look a little too small. Alternatively, just the horn section could be stylized and used for the notification icon, which would connect it more with the main icon and kind of works thematically. Thoughts?

SpiritCroc commented 1 year ago

Probably because this was opened against sc so it got confused woman_shrugging

yeah, probably. But opening against sc is the way to go either way. I just have a personal workflow that doesn't match github's expectations. It makes sense to still have it open until I push to sc anyways, I would have expected github to at least link it here though, but not important.

I just realized, this monochrome icon might work instead of the material design turtle for the android notification icon, though it may look a little too small. Alternatively, just the horn section could be stylized and used for the notification icon, which would connect it more with the main icon and kind of works thematically. Thoughts?

Hm, I prefer the current notification icon.

NathanC commented 1 year ago

Mind giving a tl;dr; of the workflow? I'm curious how changes normally flow into sc.

Hm, I prefer the current notification icon.

:+1: the current one is good too, I rather like it. However, most chat apps (and apps in general) have the same/similar notification and app icon, which makes it a little unintitive when getting the SchildiChat notification for me. If I test the monochrome icon out and it looks good, would you be opposed to an off-by-default preference in the Notifications section to use the monochrome app icon instead? Not a big deal at all if you're opposed to that, just floating the option.

SpiritCroc commented 1 year ago

Mind giving a tl;dr; of the workflow? I'm curious how changes normally flow into sc.

I'm using a personal gerrit instance (hence the Change-Id things in my commit messages) which mirrors to github, so I never push directly to github (you might've noticed @Kirchmaus doing the pushing). In an ideal world, everyone would be using that gerrit instead of creating github-PRs, but I currently don't want to open my instance to the public and it's a learning curve that would likely defer many contributors, so I just use it for myself out of convenience. For beta builds for which I'm not sure if I'll want to change or revert the commits and for upstream merges that haven't been tested much, I force-push to sc_beta instead of sc, so weblate and downstream forks doesn't get confused about force-pushing. My fdroid beta release script automatically uses either sc or sc_beta, depending on which is more up-to-date.

+1 the current one is good too, I rather like it. However, most chat apps (and apps in general) have the same notification and app icon though, which makes it a little unintitive when getting the SchildiChat notification for me. If I test the monochrome icon out and it looks good, would you be opposed to an off-by-default preference in the Notifications section that used the monochrome one instead? Since it's a Drawable it should be easy to implement.

If it looks ok-ish and the preference is clean (i.e. is unlikely to create much merge conflicts), why not.

NathanC commented 1 year ago

I've never heard of gerrit before. It looks like opinions are split on it, why would everyone use it in your ideal world? If I end up making more meaningful contributions I'd be happy to learn/use it if it helped with your flow.

i.e. is unlikely to create much merge conflicts

Maintain a fork of this scale seems like a huge headache. Do most of the conflicts end up being in vector, or in matrix-sdk-android? Not sure how much of your forked functionality involves sdk changes verses changes in the application layer. I was toying around with using jetpack compose to make a material 3 UI around the sdk as a personal experiment to learn more, and I'm curious to get your take on how coupled the app and sdk are.

SpiritCroc commented 1 year ago

I've never heard of gerrit before. It looks like opinions are split on it, why would everyone use it in your ideal world? If I end up making more meaningful contributions I'd be happy to learn/use it if it helped with your flow.

Well, ideal world is obviously exaggerated. I like gerrit for having revisions of the same change. In traditional git, when you amend a commit during development, it's a completely new commit without any link to the old one. If you instead do new commits all the time, your commit history gets messy. It's just personal taste. And no, I won't open gerrit to public contributors for now, that's not worth the effort. I'm used to gerrit from the custom ROM community, and new contributors not used to it regularly have issues using it. Just not worth it forcing it on people for single repos. And doing both github PRs and gerrit won't help either way. I'll just keep picking PRs manually, which I would need to do either way for testing, so doesn't hurt.

Maintain a fork of this scale seems like a huge headache. Do most of the conflicts end up being in vector, or in matrix-sdk-android? Not sure how much of your forked functionality involves sdk changes verses changes in the application layer. I was toying around with using jetpack compose to make a material 3 UI around the sdk as a personal experiment to learn more, and I'm curious to get your take on how coupled the app and sdk are.

Vector tends to break more, but I do have changes and thus conflicts in the sdk a fair bit too. I keep the conflicting files in the merge commit messages, so you can check the conflicting files there for each merge. But that doesn't include how severe the conflicts are of course.