Skip to content

Conversation

@perepujal
Copy link
Contributor

Hi,
Reason, one of my cards is not able to be readed unless I shrink it to the half of the screen.
Maybe caused by a combination of old reader devices in the shop and own preference for landscape screen.
I see there is yet another MR from a couple of years ago, but it was hard to cope with Catima changes in the last 2 years, so I started fresh.
Thanks for considering.
Pere

Copy link
Member

@TheLastProject TheLastProject left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this contribution, it's nice to see someone pick up the feature originally created in #1109. I get requests for this occasionally but it's just not really been a high priority for me with all the other issues.

This already seems to be in a much better state than #1109 was when development on that stalled, mostly some minor issues in this quick review and some options to simplify the code for easier future maintenance :)

Comment on lines 536 to 538
public static boolean updateLoyaltyCardZoomLevelWidth(SQLiteDatabase database, int loyaltyCardId, int zoomLevelWidth) {
ContentValues contentValues = new ContentValues();
contentValues.put(LoyaltyCardDbIds.ZOOM_LEVEL_WIDTH, zoomLevelWidth);
Log.d("updateLoyaltyCardZoomLW", "Card Id = " + loyaltyCardId + " Zoom level width= " + zoomLevelWidth);
int rowsUpdated = database.update(LoyaltyCardDbIds.TABLE, contentValues,
whereAttrs(LoyaltyCardDbIds.ID),
withArgs(loyaltyCardId));
Log.d("updateLoyaltyCardZoomLW", "Rows changed = " + rowsUpdated);
return (rowsUpdated == 1);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm already not a fan of how many functions the DBHelper class has. I think we could maybe just merge height and width into one function that updates both tables?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unified in f19b597

Comment on lines 320 to 349
binding.barcodeWidthscaler.setOnSeekBarChangeListener(new SeekBar.OnSeekBarChangeListener() {
@Override
public void onProgressChanged(SeekBar seekBar, int progress, boolean fromUser) {
if (!fromUser) {
Log.d(TAG, "non user triggered onProgressChanged, ignoring, progress is " + progress);
return;
}
Log.d(TAG, "Progress is " + progress);
Log.d(TAG, "Max is " + binding.barcodeScaler.getMax());
float scale = (float) progress / (float) binding.barcodeScaler.getMax();
Log.d(TAG, "Scaling to " + scale);

loyaltyCard.zoomLevelWidth = progress;
DBHelper.updateLoyaltyCardZoomLevelWidth(database, loyaltyCardId, loyaltyCard.zoomLevelWidth);

setScalerWidthGuideline(loyaltyCard.zoomLevelWidth);

drawMainImage(mainImageIndex, true, isFullscreen);
}

@Override
public void onStartTrackingTouch(SeekBar seekBar) {

}

@Override
public void onStopTrackingTouch(SeekBar seekBar) {

}
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the onProgressChanged callback seems to be a direct copy-paste of the one of the height scaler, it seems this could be put into a single function shared by both the height and the width scalers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

b621e80 also unifies the 2 functions, please test

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to work, but more by luck than correctness (because both seekbars have the same max). I'm seeing a few issues with the new code:

Notably the following lines:

                Log.d(TAG, "Max is " + binding.barcodeScaler.getMax());
                float scale = (float) progress / (float) binding.barcodeScaler.getMax();

This always refers to the height scaler (because binding.barcodeScaler is the barcode_scaler object in the XML), this probably needs to be seekBar instead of binding.barcodeScaler.

                if (sbar.getId() == binding.barcodeScaler.getId()) {

Given seekBar is given in the onProgressChanged, this would probably be cleaner as seekBar.getId(). Because then you can also get rid of the sbar variable in the setOnSeekBarChangeListenerUnifiedFunction function and change

        SeekBar.OnSeekBarChangeListener sb;
        sb = new SeekBar.OnSeekBarChangeListener() { ... }
        return (sb);

to simply:

    return new SeekBar.OnSeekBarChangeListener() { ... }

But otherwise looks fine to me :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in e0a8cc3
Also in that commit there are some tweaks to be able to center the barcode.

Comment on lines 160 to 161
app:layout_constraintLeft_toLeftOf="parent"
app:layout_constraintRight_toLeftOf="@+id/scaler_widthguideline"/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

constraintLeft and constraintRight are rarely the correct choices, as they force things to the left/right side of the screen, even in cultures which use RTL instead of LTR. constrainedStart and constrainedEnd would make more sense here I think, it would use the left of the screen as the "start" in LTR cultures, while using the right of the screen as the start in RTL cultures, which I would guess is more like what people would consider "normal" (human culture is complex :))

I'm also wondering: is there any specific reason you've chosen to force it to a side of the screen instead of keeping it centered?

(Sidenote, the white background not staying in the whole area looks odd, but that was kind of already an issue before but became much more noticeable)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could say that the barcode where following the seekbar, but the real reason is that I was too tired of trying to make the barcode to show at all that i went with the first solution that half worked.
Now I've managed to show the barcode centered, and also put it on a white background.
In e0a8cc3

Also I added a android:min="10" to the width seekbar, so the barcode don't disappears when the seekbar is at the minimum.
Should android:min="10" be added also to the height seekbar?

Thanks
Pere

Comment on lines 224 to 238
<SeekBar
android:id="@+id/barcode_widthscaler"
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:layout_gravity="center"
android:contentDescription="@string/setBarcodeWidth"
android:max="100" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seekbar doesn't seem to use the theming colour like the Height bar does:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also corrected in e0a8cc3

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if squared barcodes desserves two seekbars, will explore to not show the Width one and let the height alone...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In b9bdeb3 setWidthLayout hides when the barcode is a square, please test
Thanks
Pere

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

7b3519b fixes also don't show it when turning the phone.

Comment on lines 231 to 232
</LinearLayout>
</LinearLayout>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please ensure correct indentation to keep the (already complex) XML file as easy to scan as possible. These tags should not both close at the same indent, the first one should be indented further inwards.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also in e0a8cc3
I removed a useless LinearLayout that I added previously

public static final int DATABASE_VERSION = 17;

// NB: changing this value requires a migration
// NB: changing thoose values requires a migration
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small grammar thing :)

Suggested change
// NB: changing thoose values requires a migration
// NB: changing these values requires a migration

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also in f19b597

Copy link
Member

@TheLastProject TheLastProject left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking almost ready to merge. I had a look and spotted some issues. When these are resolved I can probably take one last thorough look :)

Comment on lines 356 to 391
private SeekBar.OnSeekBarChangeListener setOnSeekBarChangeListenerUnifiedFunction(SeekBar sbar) {
return new SeekBar.OnSeekBarChangeListener() {
@Override
public void onProgressChanged(SeekBar seekBar, int progress, boolean fromUser) {
if (!fromUser) {
Log.d(TAG, "non user triggered onProgressChanged, ignoring, progress is " + progress);
return;
}

Log.d(TAG, "Progress is " + progress);
if (sbar.getId() == binding.barcodeScaler.getId()) {
Log.d(TAG, "Max is " + binding.barcodeScaler.getMax());
float scale = (float) progress / (float) binding.barcodeScaler.getMax();
Log.d(TAG, "Scaling to " + scale);
}
else {
Log.d(TAG, "Max is " + binding.barcodeWidthscaler.getMax());
float scale = (float) progress / (float) binding.barcodeWidthscaler.getMax();
Log.d(TAG, "Scaling to " + scale);
}

DBHelper.updateLoyaltyCardZoomLevel(database, loyaltyCardId, loyaltyCard.zoomLevel, loyaltyCard.zoomLevelWidth);
if (sbar.getId() == binding.barcodeScaler.getId()) {
loyaltyCard.zoomLevel = progress;

setScalerGuideline(loyaltyCard.zoomLevel);
}
else {
loyaltyCard.zoomLevelWidth = progress;

setScalerWidthGuideline(loyaltyCard.zoomLevelWidth);
}
drawMainImage(mainImageIndex, true, isFullscreen);
}

@Override
public void onStartTrackingTouch(SeekBar seekBar) {

}

@Override
public void onStopTrackingTouch(SeekBar seekBar) {

}
};
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need the sbar variable at all anymore? Or am I mistaking here?

Suggested change
private SeekBar.OnSeekBarChangeListener setOnSeekBarChangeListenerUnifiedFunction(SeekBar sbar) {
return new SeekBar.OnSeekBarChangeListener() {
@Override
public void onProgressChanged(SeekBar seekBar, int progress, boolean fromUser) {
if (!fromUser) {
Log.d(TAG, "non user triggered onProgressChanged, ignoring, progress is " + progress);
return;
}
Log.d(TAG, "Progress is " + progress);
if (sbar.getId() == binding.barcodeScaler.getId()) {
Log.d(TAG, "Max is " + binding.barcodeScaler.getMax());
float scale = (float) progress / (float) binding.barcodeScaler.getMax();
Log.d(TAG, "Scaling to " + scale);
}
else {
Log.d(TAG, "Max is " + binding.barcodeWidthscaler.getMax());
float scale = (float) progress / (float) binding.barcodeWidthscaler.getMax();
Log.d(TAG, "Scaling to " + scale);
}
DBHelper.updateLoyaltyCardZoomLevel(database, loyaltyCardId, loyaltyCard.zoomLevel, loyaltyCard.zoomLevelWidth);
if (sbar.getId() == binding.barcodeScaler.getId()) {
loyaltyCard.zoomLevel = progress;
setScalerGuideline(loyaltyCard.zoomLevel);
}
else {
loyaltyCard.zoomLevelWidth = progress;
setScalerWidthGuideline(loyaltyCard.zoomLevelWidth);
}
drawMainImage(mainImageIndex, true, isFullscreen);
}
@Override
public void onStartTrackingTouch(SeekBar seekBar) {
}
@Override
public void onStopTrackingTouch(SeekBar seekBar) {
}
};
}
private SeekBar.OnSeekBarChangeListener setOnSeekBarChangeListenerUnifiedFunction() {
return new SeekBar.OnSeekBarChangeListener() {
@Override
public void onProgressChanged(SeekBar seekBar, int progress, boolean fromUser) {
if (!fromUser) {
Log.d(TAG, "non user triggered onProgressChanged, ignoring, progress is " + progress);
return;
}
Log.d(TAG, "Progress is " + progress);
if (seekBar.getId() == binding.barcodeScaler.getId()) {
Log.d(TAG, "Max is " + binding.barcodeScaler.getMax());
float scale = (float) progress / (float) binding.barcodeScaler.getMax();
Log.d(TAG, "Scaling to " + scale);
}
else {
Log.d(TAG, "Max is " + binding.barcodeWidthscaler.getMax());
float scale = (float) progress / (float) binding.barcodeWidthscaler.getMax();
Log.d(TAG, "Scaling to " + scale);
}
DBHelper.updateLoyaltyCardZoomLevel(database, loyaltyCardId, loyaltyCard.zoomLevel, loyaltyCard.zoomLevelWidth);
if (seekBar.getId() == binding.barcodeScaler.getId()) {
loyaltyCard.zoomLevel = progress;
setScalerGuideline(loyaltyCard.zoomLevel);
}
else {
loyaltyCard.zoomLevelWidth = progress;
setScalerWidthGuideline(loyaltyCard.zoomLevelWidth);
}
drawMainImage(mainImageIndex, true, isFullscreen);
}
@Override
public void onStartTrackingTouch(SeekBar seekBar) {
}
@Override
public void onStopTrackingTouch(SeekBar seekBar) {
}
};
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In 2226e3c

Comment on lines 232 to 233
private void setScalerWidthGuideline(int zoomLevelWidth) {
float halfscale = zoomLevelWidth / 200f;

binding.scalerEndwidthguideline.setGuidelinePercent(0.5f + halfscale);
binding.scalerStartwidthguideline.setGuidelinePercent(0.5f - halfscale);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think something may be going wrong here (or somewhere else)? When dragging the width slider it seems to stay at a consistent width for a long time and then suddenly jump. Did you see this behaviour on your side when testing?

screen-20250111-115606.mp4

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An approach to improve it is in 2031921
This one toke me some time to understand what was happening
Seems the datamatrix barcode generator generates some pre-defined sizes and pads the rest with white space.

I found two approaches to overcome this:
The first one was to force it to always generate an oversized barcode, then let the scaling system already in place to resize to the final size, worked fine until I found some corner cases where it failed to generate the barcode.

The second one is the one in this commit, it crops the extra white space before being scaled so the final image barcode follows the seekbar smoother.

Thanks for reviewing
Pere

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, I did not know that was going on. I think we need to be very careful trying to override the padding ourselves, especially when reducing the padding. In the past, we've ran into multiple cases where users couldn't scan barcodes because there wasn't enough white space on the edges (see #833 for when we had to fix a similar thing). It should be noted this also changes the padding in non-fullscreen mode. In general, I think we should trust that zxing generates the correct padding as per the spec as they have a lot of unit tests for all their different barcode types and are very careful to merge in changes there.

I've been testing this and I did run into issues scanning in fullscreen due to the decreased padding that I didn't before, but only in dark mode. My guess would be that the reduced padding together with the general lower contrast of the screen confuses some barcode scanners into trying to treat the screen edge (which is bigger on my FP3 due to the bumper) as a black line and thus "part of the barcode".

For example, here is a "code 93" barcode with value "1234567890" that no longer easily scans for me in default fullscreen mode:

Catima stable This MR
image image

And here is a "codabar" barcode, also with value "1234567890" that no longer scans for me in default fullscreen mode:

Catima stable This MR
image image

Similar issues occur with ean 8 and ean 13 at least (after that I stopped testing for now as I've been spending a lot of time already testing all the different barcode types)

I'm not sure what the best thing to do here is. I basically see 3 options:

  1. Always add some minimum padding, to reduce this risk
  2. Instead of managing the padding, make the width change bar less precise (bigger increments)
  3. Instead of managing the padding, put a bit of text explaining (not sure what to explain)
  4. Ignore the problem given fullscreen mode is rarely used.

It should be noted that the datamatrix code always fails to scan for me in dark mode, even on the latest stable (so before your changes), which implies that the built-in padding may not always be sufficient. So perhaps the most correct fix is some minimum padding. Pff, such difficulty barcodes can be at times 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, Thanks for reviewing,

Well, let me undo this commit that touches the paddings and try with the approach of generating an oversized barcode.
With 2031921 undone, just removing the

else if (imageView.getWidth() < MAX_WIDTH) {
            imageHeight = imageViewHeight;
            imageWidth = imageViewWidth;
        }

block in BarcodeImageWriterTask.java, then the generated barcode (before scaling) is WIDTH_MAX size, the paddings are respected and the final barcode follows smooth the seekbar, but soon one founds some cases where the barcode fails to generate, will try to debug this approach...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,
Well, I hope this is fixed, I have no more errors displaying the cards with the 7dd3969 commit.
Many of the problems where about too much memory usage, hence limiting MAX_WIDTH to the width of the screen, but I can only test on my FP4, not sure about low end phones or tablets, and can't test to scan the barcodes
I increased too the minimum value for the width seeker, that also gave failures drawing the barcode when it was at the minimum, and added datamatrix as a type to hide the width seeker.

Thanks for testing
Pere

Comment on lines 1176 to 1167

binding.barcodeWidthscaler.setProgress(loyaltyCard.zoomLevelWidth);
setScalerWidthGuideline(loyaltyCard.zoomLevelWidth);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Misindented:

Suggested change
binding.barcodeWidthscaler.setProgress(loyaltyCard.zoomLevelWidth);
setScalerWidthGuideline(loyaltyCard.zoomLevelWidth);
binding.barcodeWidthscaler.setProgress(loyaltyCard.zoomLevelWidth);
setScalerWidthGuideline(loyaltyCard.zoomLevelWidth);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also in 2226e3c

Comment on lines 142 to 147
if (format.isSquare()) {
binding.setWidthLayout.setVisibility(View.GONE);
}
else {
binding.setWidthLayout.setVisibility(View.VISIBLE);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're doing this in two places which shouldn't be necessary. If anywhere, I'd think inside the setFullscreen function is probably cleaner.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also in 2226e3c

Comment on lines 1165 to 1170
if (format.isSquare()) {
binding.setWidthLayout.setVisibility(View.GONE);
}
else {
binding.setWidthLayout.setVisibility(View.VISIBLE);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can make this a lot smaller :)

Suggested change
if (format.isSquare()) {
binding.setWidthLayout.setVisibility(View.GONE);
}
else {
binding.setWidthLayout.setVisibility(View.VISIBLE);
}
// Only show width slider if the barcode isn't square (square barcodes will resize height and width together)
binding.setWidthLayout.setVisibility(format.isSquare() ? View.GONE : View.VISIBLE);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And also in 2226e3c
Many thanks for reviewing :)

@TheLastProject
Copy link
Member

Edit 2: Nevermind my nevermind, the issue seems to also occur on commit 7dd3969

Seems to be a major regression in the display of barcodes, huge increase in padding.

Tested with this card: https://catima.app/share#store%3D128%2Blong%26note%3D%26balance%3D0%26cardid%3D12345678901234567890123456789012345678901234567890%26barcodetype%3DCODE_128%26headercolor%3D-416706

Catima stable (2.34.4) Your MR (commit 7dd3969)
image image

@perepujal
Copy link
Contributor Author

Sorry for taking so long to take a look.

Don't worry, everybody has a life :)
Thanks for testing

In 399f79c I restricted the oversized barcode approach to the fullscreen view so regular views should be not affected.
Thanks
Pere

@TheLastProject
Copy link
Member

I promise I haven't forgotten this PR :) I want to first make sure we target Android 15, so Google won't complain, but after I'm done with #2378 I want to take another look at this, hopefully final look :)

@TheLastProject
Copy link
Member

Okay, the Android 15 change is merged. Could you rebase this MR when you find the time? Android 15 changes a lot with how screen layouts work so I'd like to make sure it didn't break anything in this MR :)

@perepujal
Copy link
Contributor Author

Should be rebased now, do you also want all resize width commits joined in only one?
Thanks

Copy link
Member

@TheLastProject TheLastProject left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel kinda bad for taking so long to take a look at this again. Life has been very chaotic lately, so I didn't really find the time/energy to look at more than very simple changes. Thank you for all your patience so far.

I think I found 2 small bugs remaining, but I really think these are the last ones. I totally understand if you're tired of all the reviews though, so tell me if you want me to apply these changes myself 😅

Comment on lines 93 to 96
// Scale down the image to reduce the memory needed to produce it
imageWidth = MAX_WIDTH;
imageWidth = Math.min(MAX_WIDTH, this.mContext.getResources().getDisplayMetrics().widthPixels);
double ratio = (double) MAX_WIDTH / (double) imageViewWidth;
imageHeight = (int) (imageViewHeight * ratio);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand this change but I think using MAX_WIDTH in ratio is wrong? If widthPixels would be less than MAX_WIDTH, the ratio would be calculated with the wrong value I think?

I think this needs to be:

            imageWidth = Math.min(MAX_WIDTH, this.mContext.getResources().getDisplayMetrics().widthPixels);
            double ratio = (double) imageWidth / (double) imageViewWidth;

Comment on lines 527 to 538
public static boolean updateLoyaltyCardZoomLevel(SQLiteDatabase database, int loyaltyCardId, int zoomLevel, int zoomLevelWidth) {
ContentValues contentValues = new ContentValues();
contentValues.put(LoyaltyCardDbIds.ZOOM_LEVEL, zoomLevel);
contentValues.put(LoyaltyCardDbIds.ZOOM_LEVEL_WIDTH, zoomLevelWidth);
Log.d("updateLoyaltyCardZLevel", "Card Id = " + loyaltyCardId + " Zoom level= " + zoomLevel);
Log.d("updateLoyaltyCardZoomLW", "Card Id = " + loyaltyCardId + " Zoom level width= " + zoomLevelWidth);
int rowsUpdated = database.update(LoyaltyCardDbIds.TABLE, contentValues,
whereAttrs(LoyaltyCardDbIds.ID),
withArgs(loyaltyCardId));
Log.d("updateLoyaltyCardZLevel", "Rows changed = " + rowsUpdated);
return (rowsUpdated == 1);
Log.d("updateLoyaltyCardZoomLW", "Rows changed = " + rowsUpdated);
return (rowsUpdated == 2);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's kinda unclear from the but if I look at https://developer.android.com/reference/android/database/sqlite/SQLiteDatabase#update(java.lang.String,%20android.content.ContentValues,%20java.lang.String,%20java.lang.String[]) I'd think that, if only the width would change, this would return "false" as only one row changed?

Testing this a bit, it appears that way:

2025-04-16 18:55:53.166 21832-21832 updateLoyaltyCardZLevel me.hackerchick.catima.debug          D  Rows changed = 1

So I think we want this return:

return (rowsUpdated >= 1)

I also think the logging can be cleaned up a bit, the first one can become

        Log.d("updateLoyaltyCardZLevel", "Card Id = " + loyaltyCardId + " Zoom level = " + zoomLevel + " Zoom level width = " + zoomLevelWidth);

and for the second double-log-level the "updateLoyaltyCardZoomLW" can just be removed.

@perepujal
Copy link
Contributor Author

Don't apologize :) I know by experience how difficult is to compaginate open source development with other aspects of life.
I've put your findings in 8a7ef8f , I've also rebased to current git, so it should be easy to apply
Thanks

Copy link
Member

@TheLastProject TheLastProject left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really think this MR is fine. I think every possible flaw must be found by now. Just have this one last little bug I found, after that I think I'll (finally!) hit merge :) Thank you for all your patience!

Comment on lines 372 to 382
DBHelper.updateLoyaltyCardZoomLevel(database, loyaltyCardId, loyaltyCard.zoomLevel, loyaltyCard.zoomLevelWidth);
if (seekBar.getId() == binding.barcodeScaler.getId()) {
loyaltyCard.zoomLevel = progress;
setScalerGuideline(loyaltyCard.zoomLevel);
}
else {
loyaltyCard.zoomLevelWidth = progress;
setScalerWidthGuideline(loyaltyCard.zoomLevelWidth);
}
drawMainImage(mainImageIndex, true, isFullscreen);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this logic is the wrong way around and it just happens to work "well enough" because we tend to drag the bar for more than one value (but the saved value will always be one off).

(For safety, let's do the DB update last. If we somehow set a value that causes a crash it won't be saved to the DB)

Suggested change
DBHelper.updateLoyaltyCardZoomLevel(database, loyaltyCardId, loyaltyCard.zoomLevel, loyaltyCard.zoomLevelWidth);
if (seekBar.getId() == binding.barcodeScaler.getId()) {
loyaltyCard.zoomLevel = progress;
setScalerGuideline(loyaltyCard.zoomLevel);
}
else {
loyaltyCard.zoomLevelWidth = progress;
setScalerWidthGuideline(loyaltyCard.zoomLevelWidth);
}
drawMainImage(mainImageIndex, true, isFullscreen);
if (seekBar.getId() == binding.barcodeScaler.getId()) {
loyaltyCard.zoomLevel = progress;
setScalerGuideline(loyaltyCard.zoomLevel);
}
else {
loyaltyCard.zoomLevelWidth = progress;
setScalerWidthGuideline(loyaltyCard.zoomLevelWidth);
}
drawMainImage(mainImageIndex, true, isFullscreen);
DBHelper.updateLoyaltyCardZoomLevel(database, loyaltyCardId, loyaltyCard.zoomLevel, loyaltyCard.zoomLevelWidth);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all your work, and specially for your reviews, I hope I learned a lot from you :)
I've just commited your suggestion in 1ed2b01 and rebased again.
HTH
Pere

…card_view_layout.xml: center barcode, white background for it, indent.
…ing recovered the space left by setWidthLayout.
…small, cropping it and scaling to the destination size.
…size is small, cropping it and scaling to the destination size."

Reverted because there were too many errors with the small padding left.
This reverts commit 2031921.
…led down. Also increasing the minimum value for the width seeker.
…st. If we somehow set a value that causes a crash it won't be saved to the DB'
@TheLastProject TheLastProject merged commit 4b77700 into CatimaLoyalty:main Apr 21, 2025
2 checks passed
@TheLastProject
Copy link
Member

Thanks again for all your patience and work! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants