-
-
Notifications
You must be signed in to change notification settings - Fork 152
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
Adjust width of barcode #1109
base: main
Are you sure you want to change the base?
Adjust width of barcode #1109
Conversation
…s as stated in the failed Android CI workflow run
It fails to build because of some error in the strings translation it seems... |
I've noticed that as well, which is weird since I didn't work on any translations. It seems like it might have had something to do with the Gradle upgrade I did (it popped up in Android Studio and I thought "why not?"; this error might be the answer to that question...) I've reverted it back to the version Catima is currently using. Hopefully the next CI run will pass. |
…ng behavior after exiting maximize mode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some quick things I noticed, not a full review but I can take a deeper look after these are fixed.
Having a width option also doesn't always make sense, for example in QR codes. The CatimaBarcode
class should probably get some kind of function to check if the barcode type is "square" (not sure what the correct term is) like QR codes where the width resizes with the height.
db.execSQL("ALTER TABLE " + LoyaltyCardDbIds.TABLE | ||
+ " ADD COLUMN " + LoyaltyCardDbIds.ZOOM_WIDTH + " INTEGER DEFAULT '100' "); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to put the change in a new if statement, right now it will only create ZOOM_WIDTH when upgrading from database version 13 to 14. Users who are already at database version 14 won't be upgraded and the app will crash when trying to access the ZOOM_WIDTH field.
You should also update DATABASE_VERSION
at the top of the class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
16 shouldn't be above 15, please keep consistent ordering.
@@ -215,6 +215,7 @@ private static LoyaltyCard updateTempState(LoyaltyCard loyaltyCard, LoyaltyCardF | |||
(int) (fieldName == LoyaltyCardField.starStatus ? value : loyaltyCard.starStatus), | |||
0, // Unimportant, always set to null in doSave so the DB updates it to the current timestamp | |||
100, // Unimportant, not updated in doSave, defaults to 100 for new cards | |||
100, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should have the same note as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These comments were actually from someone else (and they don't seem to be commenting out code, but rather just a reminder of what those values are for?), but I can remove them if you think they no longer need to be there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The line that lacks the comment needs a comment so it's clear what the last 100 value is for.
app/src/main/java/protect/card_locker/LoyaltyCardViewActivity.java
Outdated
Show resolved
Hide resolved
app/src/main/java/protect/card_locker/LoyaltyCardViewActivity.java
Outdated
Show resolved
Hide resolved
Thank you for these initial feedback! I'll get to them as soon as I can :) |
Hello! Sorry for the delay. I've integrated the fixes into my new commits. Please take a look at them :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have time to very deeply look at everything, but I'm seeing a fair bit of issues from a quick glance.
db.execSQL("ALTER TABLE " + LoyaltyCardDbIds.TABLE | ||
+ " ADD COLUMN " + LoyaltyCardDbIds.ZOOM_WIDTH + " INTEGER DEFAULT '100' "); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
16 shouldn't be above 15, please keep consistent ordering.
public static int getColumnCount(SQLiteDatabase db) { | ||
Cursor cursor = db.rawQuery("SELECT * FROM " + LoyaltyCardDbIds.TABLE + ";", null, null); | ||
return cursor.getColumnCount(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems completely unused.
|
||
// Another constructor, with zoomWidth as an extra parameter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment makes no sense, this is the only constructor.
SeekBar barcodeHeightScaler; | ||
SeekBar barcodeWidthScaler; | ||
TextView zoomHeightText; | ||
TextView zoomWidthText; | ||
LinearLayout widthScalerLayout; | ||
LinearLayout heightScalerLayout; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please be consistent in order.
Now it is:
Height
Width
Height
Width
Width
Height
widthScalerLayout = binding.widthScalerLayout; | ||
heightScalerLayout = binding.heightScalerLayout; | ||
centerGuideline = binding.centerGuideline; | ||
barcodeScaler = binding.barcodeScaler; | ||
barcodeScaler.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 " + barcodeScaler.getMax()); | ||
float scale = (float) progress / (float) barcodeScaler.getMax(); | ||
Log.d(TAG, "Scaling to " + scale); | ||
barcodeHeightScaler = binding.barcodeHeightScaler; | ||
|
||
loyaltyCard.zoomLevel = progress; | ||
DBHelper.updateLoyaltyCardZoomLevel(database, loyaltyCardId, loyaltyCard.zoomLevel); | ||
SeekBarListener heightScalerListener = new SeekBarListener(barcodeHeightScaler); | ||
barcodeHeightScaler.setOnSeekBarChangeListener(heightScalerListener); | ||
|
||
setCenterGuideline(loyaltyCard.zoomLevel); | ||
|
||
drawMainImage(mainImageIndex, true, isFullscreen); | ||
} | ||
|
||
@Override | ||
public void onStartTrackingTouch(SeekBar seekBar) { | ||
|
||
} | ||
|
||
@Override | ||
public void onStopTrackingTouch(SeekBar seekBar) { | ||
|
||
} | ||
}); | ||
// set zoom width of barcode | ||
barcodeWidthScaler = binding.barcodeWidthScaler; | ||
zoomWidthText = binding.zoomWidthText; | ||
SeekBarListener widthScalerListener = new SeekBarListener(barcodeWidthScaler); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again inconsistent ordering:
Width
Height
Height
Width
And only the width has a comment and the height doesn't
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also the extra barcodeXScaler = binding.barcodeXScaler
seems unnecessary and zoomWidthText
seems completely unused.
barcodeHeightScaler.setProgressTintList(ColorStateList.valueOf(darkenedColor)); | ||
barcodeHeightScaler.setThumbTintList(ColorStateList.valueOf(darkenedColor)); | ||
barcodeWidthScaler.setProgressTintList(ColorStateList.valueOf(darkenedColor)); | ||
barcodeWidthScaler.setThumbTintList(ColorStateList.valueOf(darkenedColor)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And now it's suddenly
Height
Height
Width
Width
These inconsistencies make things really hard to maintain and prone to future bugs.
barcodeHeightScaler.setProgress(loyaltyCard.zoomLevel); | ||
barcodeWidthScaler.setProgress(loyaltyCard.zoomWidth); | ||
setCenterGuideline(loyaltyCard.zoomLevel); | ||
|
||
// Hide maximize and show minimize button and scaler | ||
widthScalerLayout.setVisibility(View.VISIBLE); | ||
heightScalerLayout.setVisibility(View.VISIBLE); | ||
maximizeButton.setVisibility(View.GONE); | ||
minimizeButton.setVisibility(View.VISIBLE); | ||
barcodeScaler.setVisibility(View.VISIBLE); | ||
barcodeHeightScaler.setVisibility(View.VISIBLE); | ||
barcodeWidthScaler.setVisibility(View.VISIBLE); | ||
zoomWidthText.setText("Width"); | ||
zoomHeightText.setText("Height"); | ||
zoomWidthText.setVisibility(View.VISIBLE); | ||
zoomHeightText.setVisibility(View.VISIBLE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again inconsistent ordering:
Height
Width
Width
Height
Height
Width
Width
Height
Width
Height
widthScalerLayout.setVisibility(View.GONE); | ||
heightScalerLayout.setVisibility(View.GONE); | ||
barcodeHeightScaler.setVisibility(View.GONE); | ||
barcodeWidthScaler.setVisibility(View.GONE); | ||
zoomWidthText.setVisibility(View.GONE); | ||
zoomHeightText.setVisibility(View.GONE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again inconsistent ordering
if (seekBar == barcodeHeightScaler) { | ||
loyaltyCard.zoomLevel = progress; | ||
DBHelper.updateLoyaltyCardZoomLevel(database, loyaltyCardId, loyaltyCard.zoomLevel); | ||
setCenterGuideline(loyaltyCard.zoomLevel); | ||
} else if (seekBar == barcodeWidthScaler) { | ||
loyaltyCard.zoomWidth = progress; | ||
DBHelper.updateLoyaltyCardZoomWidth(database, loyaltyCardId, loyaltyCard.zoomWidth); | ||
mainImage.getLayoutParams().width = mainLayout.getWidth() * loyaltyCard.zoomWidth / 100; | ||
mainImage.requestLayout(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems better to me if this class gets told with a separate parameter if it's working on height and width so it doesn't break in unexpected ways if and variable name changes and so it doesn't depend on variable names outside of itself.
Hello!
This PR addresses #788 . Please check it out and see if my implementation is suitable for what you or the original issue opener had in mind.
Thanks!