Add a barcode home screen widget for displaying a selected loyalty card's barcode and with proper orientation handling.#3125
Conversation
JorySeverijnse
changed the title
Please drop that commit from your change. It turns a single-feature PR into a 3000 line change, way too much to review properly. |
|
I'm not opposed to introducing some style lint, but that should really be its own unique change. |
|
Alright, removed the linting. Also, do you have a suggestion as to what preview icon to use? We are now just using the same one that is being used for the Card list widget @drawable/widget_preview |
TheLastProject
left a comment
TheLastProject
left a comment
There was a problem hiding this comment.
Thanks for your contribution!
I can confirm it works, but I see some issues and I have some worries about the maintainability, especially given the code duplication. So this is a quick first review pass :)
|
|
||
| val barcodeFormatPreference = findPreference<Preference>(getString(R.string.settings_key_barcode_widget_show_format)) | ||
| barcodeFormatPreference?.setOnPreferenceChangeListener { _, newValue -> | ||
| activity?.let { activity -> | ||
| PreferenceManager.getDefaultSharedPreferences(activity) | ||
| .edit() | ||
| .putBoolean(activity.getString(R.string.settings_key_barcode_widget_show_format), newValue as Boolean) | ||
| .apply() | ||
| BarcodeWidget.triggerUpdate(activity) | ||
| } | ||
| true | ||
| } |
There was a problem hiding this comment.
I see no use for this feature. I cannot imagine anyone possibly needing the barcode type displayed on their homescreen below it. So let's just remove it to lower maintenance burden.
There was a problem hiding this comment.
This seems to be a direct copy of the card_shortcut_configure_activity. Wouldn't it be better to just reuse the existing screen? After all, the flow is exactly the same when choosing a barcode, regardless on if you want a single shortcut or the barcode/qr code displayed itself.
| <string name="settings_use_volume_keys_navigation">Switch cards using volume buttons</string> | ||
| <string name="settings_use_volume_keys_navigation_summary">Use the volume buttons to change which card is displayed</string> | ||
| <string name="settings_key_use_volume_keys_navigation" translatable="false">pref_use_volume_keys_navigation</string> | ||
| <string name="settings_key_barcode_widget_show_format" translatable="false">pref_barcode_widget_show_format</string> |
There was a problem hiding this comment.
As stated before let's remove this
| <string name="settings_barcode_widget_show_format">Show barcode type on widget</string> | ||
| <string name="settings_barcode_widget_show_format_summary">Display the barcode format name below the barcode on the home screen widget</string> |
There was a problem hiding this comment.
As stated before let's remove this
| android:minWidth="80dp" | ||
| android:minHeight="80dp" |
There was a problem hiding this comment.
I think these may be too high. On my Fairphone 6, I cannot make the widget any smaller than a 2 by 2 grid, which is quite a waste of space.
|
|
||
| <SwitchPreferenceCompat | ||
| android:widgetLayout="@layout/preference_material_switch" | ||
| android:defaultValue="false" | ||
| android:key="@string/settings_key_barcode_widget_show_format" | ||
| android:summary="@string/settings_barcode_widget_show_format_summary" | ||
| android:title="@string/settings_barcode_widget_show_format" | ||
| app:iconSpaceReserved="false" | ||
| app:singleLineTitle="false" /> |
There was a problem hiding this comment.
As stated before let's remove this
| <TextView | ||
| android:id="@+id/barcode_format" | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content" | ||
| android:textSize="11sp" | ||
| android:gravity="center" | ||
| android:visibility="gone" /> |
There was a problem hiding this comment.
As stated before let's remove this
| android:textStyle="bold" | ||
| android:gravity="center" | ||
| android:maxLines="1" | ||
| android:ellipsize="end" /> |
There was a problem hiding this comment.
This is unreadable on my phone due to it not having a background.
I also think it may be better to try to style this widget in general a bit? #737 shows an example design which I feel should work fine in Catima (we can hide the name if the barcode isn't wide enough). It may be good to look at the Utils.setIconOrTextWithBackground function and how it's called in other parts of Catima, or in the createShortcutBuilder code in the ShortCutHelper.java class (which uses Utils.generateIcon). (As a sidenote: setIconOrTextWithBackground and generateIcon already seem like duplication, maybe generateIcon can be phased out in the older widget too)
As a last question, is there a reason you opted to build this in the legacy Android widget system as opposed to using Glance? See #2594. I haven't worked with Glance myself yet but knowing how painful old Android XML can be, it may make stuff a lot easier?
There was a problem hiding this comment.
This seems to duplicate a lot of the code of the BarcodeImageWriterTask class. Is there any reason for this? Duplicated code is harder to maintain and increases the risk of introducing bugs (for example: forgetting to update the widget code when the general drawing code gets updated).
| android:initialLayout="@layout/barcode_widget" | ||
| android:minWidth="80dp" | ||
| android:minHeight="80dp" | ||
| android:previewImage="@drawable/widget_preview" |
There was a problem hiding this comment.
I agree we need to replace this image, but let's make an image for this widget when it looks as we want it to. It's still looking a bit... "alpha version" :)
2 participants
Full implementation of the #737 issue. Have tested it out on my android 16 (pixel 9a) for which it works perfectly
Add barcode widget for displaying loyalty card barcodes on home screen
Refactor barcode generation to use fixed dimensions and add optional format display setting
Fix widget resize handling and correct inverted format toggle behavior
Apply ktlint formatting to entire codebase