Adjust width of barcode #1109

mn-0000 · 2022-10-24T19:52:03Z

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!

…is now functional

…ztec 100% zoomHeight

…s as stated in the failed Android CI workflow run

Altonss · 2022-10-25T09:51:17Z

It fails to build because of some error in the strings translation it seems...

mn-0000 · 2022-10-25T09:57:55Z

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

TheLastProject

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.

TheLastProject · 2022-10-25T16:45:30Z

app/src/main/java/protect/card_locker/DBHelper.java

+            db.execSQL("ALTER TABLE " + LoyaltyCardDbIds.TABLE
+                    + " ADD COLUMN " + LoyaltyCardDbIds.ZOOM_WIDTH + " INTEGER DEFAULT '100' ");


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.

16 shouldn't be above 15, please keep consistent ordering.

app/src/main/java/protect/card_locker/LoyaltyCard.java

TheLastProject · 2022-10-25T16:48:55Z

app/src/main/java/protect/card_locker/LoyaltyCardEditActivity.java

@@ -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,


Should have the same note as above

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.

The line that lacks the comment needs a comment so it's clear what the last 100 value is for.

app/src/test/java/protect/card_locker/DatabaseTest.java

app/src/main/java/protect/card_locker/LoyaltyCardViewActivity.java

app/src/main/res/values/strings.xml

app/src/main/java/protect/card_locker/LoyaltyCardViewActivity.java

app/src/main/res/layout/loyalty_card_view_layout.xml

mn-0000 · 2022-10-26T19:03:10Z

Thank you for these initial feedback! I'll get to them as soon as I can :)

mn-0000 · 2022-12-26T10:58:28Z

Hello! Sorry for the delay. I've integrated the fixes into my new commits. Please take a look at them :)

TheLastProject

I don't have time to very deeply look at everything, but I'm seeing a fair bit of issues from a quick glance.

TheLastProject · 2022-12-26T13:26:54Z

app/src/main/java/protect/card_locker/DBHelper.java

+            db.execSQL("ALTER TABLE " + LoyaltyCardDbIds.TABLE
+                    + " ADD COLUMN " + LoyaltyCardDbIds.ZOOM_WIDTH + " INTEGER DEFAULT '100' ");


16 shouldn't be above 15, please keep consistent ordering.

TheLastProject · 2022-12-26T13:28:31Z

app/src/main/java/protect/card_locker/DBHelper.java

+    public static int getColumnCount(SQLiteDatabase db) {
+        Cursor cursor = db.rawQuery("SELECT * FROM " + LoyaltyCardDbIds.TABLE + ";", null, null);
+        return cursor.getColumnCount();
+    }


This seems completely unused.

TheLastProject · 2022-12-26T13:29:24Z

app/src/main/java/protect/card_locker/LoyaltyCard.java


+    // Another constructor, with zoomWidth as an extra parameter


This comment makes no sense, this is the only constructor.

TheLastProject · 2022-12-26T13:40:59Z

app/src/main/java/protect/card_locker/LoyaltyCardViewActivity.java

+    SeekBar barcodeHeightScaler;
+    SeekBar barcodeWidthScaler;
+    TextView zoomHeightText;
+    TextView zoomWidthText;
+    LinearLayout widthScalerLayout;
+    LinearLayout heightScalerLayout;


Please be consistent in order.

Now it is:
Height
Width
Height
Width
Width
Height

TheLastProject · 2022-12-26T13:42:26Z

app/src/main/java/protect/card_locker/LoyaltyCardViewActivity.java

+        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);


Again inconsistent ordering:

Width
Height
Height
Width

And only the width has a comment and the height doesn't

Also the extra barcodeXScaler = binding.barcodeXScaler seems unnecessary and zoomWidthText seems completely unused.

TheLastProject · 2022-12-26T13:44:10Z

app/src/main/java/protect/card_locker/LoyaltyCardViewActivity.java

+        barcodeHeightScaler.setProgressTintList(ColorStateList.valueOf(darkenedColor));
+        barcodeHeightScaler.setThumbTintList(ColorStateList.valueOf(darkenedColor));
+        barcodeWidthScaler.setProgressTintList(ColorStateList.valueOf(darkenedColor));
+        barcodeWidthScaler.setThumbTintList(ColorStateList.valueOf(darkenedColor));


And now it's suddenly

Height
Height
Width
Width

These inconsistencies make things really hard to maintain and prone to future bugs.

TheLastProject · 2022-12-26T13:45:02Z

app/src/main/java/protect/card_locker/LoyaltyCardViewActivity.java

+            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);


Again inconsistent ordering:

Height
Width
Width
Height
Height
Width
Width
Height
Width
Height

TheLastProject · 2022-12-26T13:45:16Z

app/src/main/java/protect/card_locker/LoyaltyCardViewActivity.java

+            widthScalerLayout.setVisibility(View.GONE);
+            heightScalerLayout.setVisibility(View.GONE);
+            barcodeHeightScaler.setVisibility(View.GONE);
+            barcodeWidthScaler.setVisibility(View.GONE);
+            zoomWidthText.setVisibility(View.GONE);
+            zoomHeightText.setVisibility(View.GONE);


Again inconsistent ordering

TheLastProject · 2022-12-26T13:48:01Z

app/src/main/java/protect/card_locker/LoyaltyCardViewActivity.java

+            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();
+            }


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.

mn-0000 added 4 commits October 23, 2022 15:30

Added zoomWidth to database, and a semi-working version of zoomWidth …

0558280

…is now functional

Removed extra code

b1ab929

Added some comments and preparation for slider labeling

1e9879a

Added labels to sliders and moved sliders down a bit to accommodate A…

3b509b8

…ztec 100% zoomHeight

mn-0000 closed this Oct 24, 2022

mn-0000 reopened this Oct 24, 2022

mn-0000 changed the title ~~Fixes issue 788 (Adjust width of barcode)~~ Adjust width of barcode Oct 24, 2022

mn-0000 added 3 commits October 24, 2022 13:18

Add back mistakenly removed method

674c24c

Merge branch 'master' into branch_minh

b6a5c83

Reverted Gradle plugin upgrade as it seems to give localization error…

edb9fb5

…s as stated in the failed Android CI workflow run

Reverted one unintended change, and fixed unintended barcode displayi…

becfb68

…ng behavior after exiting maximize mode

TheLastProject requested changes Oct 25, 2022

View reviewed changes

Minor improvements

74e3eec

TheLastProject marked this pull request as draft November 22, 2022 17:26

Renamed certain objects for better code readability

31427f3

mn-0000 marked this pull request as ready for review December 26, 2022 10:58

TheLastProject requested changes Dec 26, 2022

View reviewed changes

TheLastProject marked this pull request as draft January 18, 2023 18:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjust width of barcode #1109

Adjust width of barcode #1109

mn-0000 commented Oct 24, 2022 •

edited

Loading

Altonss commented Oct 25, 2022

mn-0000 commented Oct 25, 2022

TheLastProject left a comment

TheLastProject Oct 25, 2022

TheLastProject Dec 26, 2022

TheLastProject Oct 25, 2022

mn-0000 Dec 26, 2022

TheLastProject Dec 26, 2022

mn-0000 commented Oct 26, 2022

mn-0000 commented Dec 26, 2022

TheLastProject left a comment

TheLastProject Dec 26, 2022

TheLastProject Dec 26, 2022

TheLastProject Dec 26, 2022

TheLastProject Dec 26, 2022

TheLastProject Dec 26, 2022

TheLastProject Dec 26, 2022

TheLastProject Dec 26, 2022

TheLastProject Dec 26, 2022

TheLastProject Dec 26, 2022

TheLastProject Dec 26, 2022

		db.execSQL("ALTER TABLE " + LoyaltyCardDbIds.TABLE
		+ " ADD COLUMN " + LoyaltyCardDbIds.ZOOM_WIDTH + " INTEGER DEFAULT '100' ");


		// Another constructor, with zoomWidth as an extra parameter

Adjust width of barcode #1109

Are you sure you want to change the base?

Adjust width of barcode #1109

Conversation

mn-0000 commented Oct 24, 2022 • edited Loading

Altonss commented Oct 25, 2022

mn-0000 commented Oct 25, 2022

TheLastProject left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mn-0000 commented Oct 26, 2022

mn-0000 commented Dec 26, 2022

TheLastProject left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mn-0000 commented Oct 24, 2022 •

edited

Loading