Skip to content

Fix of #1753 causes white lines for incorrectly cropped cards, deal with this more gracefully (somehow) #2248

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
solokot opened this issue Dec 23, 2024 · 20 comments · Fixed by #2471
Labels
common: occasional Affects or can be seen by some users regularly or most users rarely severity: major Severely degrades major functionality or product features, with no satisfactory workaround type: bug Something isn't working

Comments

@solokot
Copy link
Contributor

solokot commented Dec 23, 2024

After upgrading from version 2.33 to 2.34.1, the thumbnails have vertical white stripes (F-Droid builds were used, as they were a few years earlier).
OS: Android 11 MIUI 12.5
Strip 3
Perhaps this is due to #2199

Attached are a couple of thumbnail samples from the backup file and their original images used when adding maps to the database.

card_1_icon
card_1_original
card_17_icon
card_17_original

@TheLastProject TheLastProject added state: invalid This doesn't seem right state: consensus-needed A consensus needs to be reached before this can be implemented labels Dec 23, 2024
@TheLastProject
Copy link
Member

The display isn't broken, these thumbnails weren't cropped correctly and thus don't exactly fit.

Catima's old behaviour was to use the dominant colour as background, which caused issues like #1753 and #1972. A lot of pkpass files contain transparent images, meaning the old behaviour would make the logo unreadable for the vast majority of pkpass files, which is why I changed it in #2201.

Sure, the new behaviour isn't perfect either, but these images were always wrong, it just was less obvious because Catima added a small green/blue/orange line instead, which was less visible than a white or black line.

I don't think there is a perfect solution here sadly, not sure what I could do here without breaking something else :/

@TheLastProject
Copy link
Member

According to #2249 (comment) this was fixed by #2253 too. Not sure how, but can always reopen if it comes back :)

@solokot
Copy link
Contributor Author

solokot commented Dec 26, 2024

It's fixed
Strip 2

@solokot
Copy link
Contributor Author

solokot commented Dec 27, 2024

Oh, the thumbnail display has not been corrected in the release https://github.com/CatimaLoyalty/Android/releases/tag/v2.34.2, need to open the PR back.
Screenshot_2024-12-27-14-13-49-318_me hackerchick catima

@TheLastProject TheLastProject reopened this Jan 4, 2025
@TheLastProject TheLastProject removed the state: invalid This doesn't seem right label Jan 4, 2025
@TheLastProject
Copy link
Member

I'm still quite confused that this "suddenly changed", it really shouldn't because the aspect ratio of a card is hardcoded and hasn't changed ever (maybe I made a mistake in margin/padding?). Did this change between some release?

Someone else suggested outside of GitHub to stretch the images to make them fit if they're too small, but I worry that might break pkpass-created images, as those aren't necessarily the right size and may look bad stretched out (and so may other images).

Not quite sure what would be the right solution here, also not quite sure what's even going wrong in the first place.

@solokot
Copy link
Contributor Author

solokot commented Jan 5, 2025

Did this change between some release?

The problem with the light vertical stripes in the thumbnails appeared in 2.34, it did not exist in 2.33 & any previous version of Catima (three years for sure).
In a couple of days, will I be able to check the behavior on a couple of other devices (including non-MIUI), I'll post the results.

@TheLastProject
Copy link
Member

Okay, so, if it broke in 2.34.0 I still think the issue lied with your images in that they weren't cropped correctly then. Because 2.34 changed the images to no longer use a background based from the dominant colour, but just a white/black background, as described in #2248 (comment).

So I don't think this is really a Catima bug, it's more a feature request to deal more nicely with images which weren't cropped to the correct size. In which case the discussion is: what should Catima do if the image doesn't fit?

@TheLastProject TheLastProject changed the title Display of thumbnails in 2.34.x is broken Fix of #1753 causes white lines for incorrectly cropped cards, deal with this more gracefully (somehow) Jan 5, 2025
@SwcIt
Copy link

SwcIt commented Feb 18, 2025

Hi, I also noticed this 'white line' issue (on a dark background) and my conclusion is that this issue has always been there but not visible in version 2.33 and before. I made crops of images to use for the card icons in 2.33 and in 2.34.x, when using the 2.34.x version, images cropped in either 2.33 or 2.34 have this issue. I think the very specific aspect ratio of the cards used in Catima is causing the crop to sometimes of by a pixel. Maybe this also has something to do with the card overview being rendered anti-aliased and therefor showing a faint line of the bandgroundcolor to bleed through. In 2.33 this was not visible because of the background (and so the line) used the dominant color.

I downloaded the latest sourcecode of Catima and was able to fix this issue. I changed the scaleType to "cropCenter" of the ImageView "@+id/thumbnail" in file loyalty_card_layout.xml. This fixes the issue by scaling / zooming in just a tiny bit to make all card icon images fit perfectly. Works also in viewing the cards in a grid with more or less cards on a single row.

This change is only done in the card overview screen and this should not cause any (new) issues for anyone. I don't think zooming in a little (or a bit more if de aspect ratio is way off), will cause any type of trouble using the application, since the card detail screen is not changed at all, only the icon overview. If I'm wrong, please let me know what I missed :o)

What do you think?

@TheLastProject
Copy link
Member

@SwcIt Did you check if it also behaves correctly when someone in the cropper purposefully chooses another aspect ratio like square? I'm fairly sure I looked into cropCenter and concluded it "broke" for example square icons (by cutting it off instead of keeping the square)

@SwcIt
Copy link

SwcIt commented Feb 18, 2025

Hi Sylvia, I did not check square crops. But does that really matter, I wonder? You have specified a aspect ratio which corresponds to normal psysical cards like credit cards. If someone crops a square, then it will zoom in indeed, filling the 'credit card' format of the overview, but does that really matter? Zooming in would make all the icons exactly the same size, whereas keeping squares (or other ratio's) would make the overview of cards look all over the place having different sizes. I think soon enough people will stop cropping squares, haha.

Maybe you could implement an option to choose whether the cards will be shown as it is now, or filling the whole available card icon space (and use cropCenter). I would be a fan of that!

@solokot
Copy link
Contributor Author

solokot commented Feb 19, 2025

Ok, I'm not a developer and I'm coming at this from a different angle: in the Catima backup file, all logo images are 512x313 px (and have no white bars on the sides).
I've tried in the editor to provide the original image at that resolution, selecting the options “crop” and “scale”, “size by card” and “original”, and still end up with white bars on the card list screen.
And then a question to the developer: what size should be the original image of the card logo, so that it would not be considered incorrectly cropped and would be accepted by the program and displayed “as is”?

@SwcIt
Copy link

SwcIt commented Feb 19, 2025

@solokot As I understand it, there is not a resolution of a cropped icon image that always works in every orientation of the card overview. This is because of the chosen aspect ration a card, being faily specific at 85.6 : 53.98. This means that you probably never will have an image cropped at exactly this aspect ratio. Because of that, there will almost always never be an exact fit of the image into the 'rectangle' (imageview) reserved in the card overview. Taken into account that the app also uses a background color of white or black for transparent parts of the icon and also uses anti-aliassing, this results in the lines we see at the sides or top and bottom.

The suggestion I did will 'zoom' in just a little bit to alwas have the 'rectangle' always filled, even if the aspect ratio of the image is different. This means that a part of the image 'could' fall of, but if this is a normal sized card, this will be limited to only a single line of pixels probably.

The way the app works now, is that it also has support for square images for example. These images will be show completely inside the rectagle, filling the left and right of the 'rectangle' with either white or black, depending of the dominant color of the squared image.

I wonder though, how many people use square images (or other aspec ratios) for an app that is technically build for only credit card sized loyalty cards, at least, that is how the card overview is build up. I have a fair selection of cards and all of the images are credit card sized. I would not mind losing the support for square images, having the square images being zoomed in to make them fir into the 'rectangle', this will give the card overview a more uniform look. Having this change and using a square image does not change the card detail view, this will stil show up correctly squared (i think).

As an alternative to my suggestion of having the scale mode as an option, as I mentioned yesterday, maybe there is a possibility to alter the cropped image. If an image is cropped and is does not have a correct aspect ratio, the image could be altered and filled with transparent pixels on the left and right, to make it credit card sized, before saving the image file. This way all icon images are of the same aspect ratio (roughly) and the 'cropCenter' could be used as a default.

@TheLastProject
Copy link
Member

Thanks for looking so deeply into this @SwcIt!

This is because of the chosen aspect ration a card, being faily specific at 85.6 : 53.98. This means that you probably never will have an image cropped at exactly this aspect ratio. Because of that, there will almost always never be an exact fit of the image into the 'rectangle' (imageview) reserved in the card overview. Taken into account that the app also uses a background color of white or black for transparent parts of the icon and also uses anti-aliassing, this results in the lines we see at the sides or top and bottom.

That would explain the issue very well and that sounds very logical.

I do agree most users will most likely not explicitly change cropping mode, but Catima did kind of suggest it would work and "be supported" by allowing changing it. So if we go this route, we should probably block people from using other cropping modes in the cropper too so the supported size is very explicit.

My second concern would be pkpass files. There is no real official "standard" for a pkpass file logo, so there is no guarantee that that logo would ever be in correct shape nor is there any way to "enforce" that shape (although this wouldn't be necessary with your resize code idea, which would also help towards reducing the pain of #1374).

I'm semi-okay (although I would definitely prefer to avoid it) with breaking manually square-cropped images as I expect this to be a very very small group, but I do think that the pkpass issue will make breaking "random images" a bit too common even for images the user didn't manually crop like this.

As an alternative to my suggestion of having the scale mode as an option, as I mentioned yesterday, maybe there is a possibility to alter the cropped image. If an image is cropped and is does not have a correct aspect ratio, the image could be altered and filled with transparent pixels on the left and right, to make it credit card sized, before saving the image file. This way all icon images are of the same aspect ratio (roughly) and the 'cropCenter' could be used as a default.

That could definitely be a possibility. However, there are of course already saved pictures in user libraries and there may be cards imported with other sizes from other apps or manually-edited exports (although this last one is definitely not a supported use case) so perhaps it would be simpler to apply this resize on load time?

Doing it during runtime definitely wouldn't be the nicest thing performance-wise, of course, but it may be worth trying to see how "intense" the performance hit would be and to make the change simpler to test. If the performance hit is too intense, we could call the exact same code at another moment to enforce the size as part of card saving (so it also affects all importers) and make it part of a "version upgrade" (like a database schema update) to force this for existing cards.

So, to summarize: I think your change of cropCenter is logical, but we probably do need resizing code to deal with at very least pkpass files (although it would also allow us to keep "supporting" square images which would be a nice bonus).

(While we're at this, we should probably also fix the edit screen icon to use the same aspect ratio as other parts of the app: #2318)

@adripo
Copy link

adripo commented Apr 19, 2025

Hi @TheLastProject, regarding the custom 85.6:53.98 ratio in loyalty_card_layout.xml that @SwcIt mentioned. I’m curious what was the original reason for choosing those exact numbers?

If there isn’t a specific constraint driving it, would you be open to trying a more common 16:9 ratio instead? It could make things a bit simpler to show and crop.
Thanks for any insight!

@TheLastProject
Copy link
Member

85.6:53.98 is the same ratio as a credit card: https://en.wikipedia.org/wiki/Credit_card#Technical_specifications

I'd rather not change the ratio if it can be prevented, because it would mean breaking all existing images :( I also think the credit card size does feel logical for loyalty cards, as many physical loyalty cards are that exact size. We probably just need to replace the cropper, it's not actively maintained anymore anyway.

@adripo
Copy link

adripo commented Apr 19, 2025

Thanks for the clarification!
Since we’re already hard‑coding

static final int BITMAP_SIZE_SMALL = 512;
static final int BITMAP_SIZE_BIG   = 2048;

applying the 85.6:53.98 ratio gives approximately 322.87 px, which rounds to 323 px.
As a workaround, would it make sense to update the ratio to 512:323?

<!-- temporary integer‑only ratio to avoid fractional pixels -->
app:layout_constraintDimensionRatio="512f:323f"

This would keep us very close to the true credit‑card shape and eliminate any fractional‑pixel issues.
Once the library is updated, we can revert to the exact 85.6 : 53.98 ratio.

@TheLastProject
Copy link
Member

Oh, good catch, that does explain the issue quite well.

I don't think we can fix any of the existing images then (ouch...), but we could probably extend the resizeBitmap feature to take both a max width and a max height value and to pick values that would not create a fractional value. Or just ensure that the resizeBitmap function never picks a fractional value, as that would cause some data loss (of like a pixel).

static public Bitmap resizeBitmap(Bitmap bitmap, double maxSize) {
if (bitmap == null) {
return null;
}
double width = bitmap.getWidth();
double height = bitmap.getHeight();
if (height > width) {
double scale = height / maxSize;
height = maxSize;
width = width / scale;
} else if (width > height) {
double scale = width / maxSize;
width = maxSize;
height = height / scale;
} else {
height = maxSize;
width = maxSize;
}
return Bitmap.createScaledBitmap(bitmap, (int) Math.round(width), (int) Math.round(height), true);
}

@TheLastProject TheLastProject added type: bug Something isn't working severity: major Severely degrades major functionality or product features, with no satisfactory workaround common: occasional Affects or can be seen by some users regularly or most users rarely and removed state: consensus-needed A consensus needs to be reached before this can be implemented labels Apr 21, 2025
@adripo
Copy link

adripo commented Apr 23, 2025

That makes sense, and yes, existing images can't really be adjusted retroactively without introducing some inconsistency.

Extending resizeBitmap() to accept both a max width and max height sounds like a good approach. That way it could calculate the largest integer size within those bounds that still respects the aspect ratio. Even just ensuring it avoids fractional results (e.g. rounding intelligently or adjusting the base ratio slightly) could help prevent subtle issues like pixel loss or visual artifacts.

In the meantime I created a simple webapp to generate colorful PNG images from transparent logos, that I use to update the cards in Catima. Feel free to check it. It would be a nice idea to be integrated directly in Catima with the new cropper library.

https://adripo.github.io/loyalty-card-generator/

@TheLastProject
Copy link
Member

After some testing it became clear to me there is indeed no reasonable way to reach the 85.6 : 53.98 ratio. It seems the smallest picture that can fullfil that is something like 4280x2699.

I had quite some trouble reproducing this, I guess my main phone just happens to have a resolution where it's not visible. But I looked on another phone and when staring really closely could barely see the line.

I tried changing resizeBitmap to instead Math.ceil, but the resulting image still got the one line missing, so it seems to be Android's scaling (as suggested by @SwcIt in #2248 (comment)).

Thinking a bit more, with all the info we have now (thanks everyone!), I decided to try to allow cropping the image only if the aspect ratio is "close enough". I've tried that change on #2471 and it seems to work well on my local device, but I feel I don't have the eyesight for such perfect viewing, so it'd be great if someone who could tell this issue more easily can double check the debug build I've attached there :)

@TheLastProject
Copy link
Member

Btw, @adripo, that's a really cool project you wrote! I'm thinking we should maybe add it to https://catima.app/faq/, to talk about how you can best generate an thumbnail image for your cards? May also be helpful for #89 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
common: occasional Affects or can be seen by some users regularly or most users rarely severity: major Severely degrades major functionality or product features, with no satisfactory workaround type: bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants