Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add Pkpass parser #2038

Merged
merged 11 commits into from
Dec 7, 2024
Merged

Add Pkpass parser #2038

merged 11 commits into from
Dec 7, 2024

Conversation

TheLastProject
Copy link
Member

@TheLastProject TheLastProject commented Aug 11, 2024 •
edited
Loading

!!! Blocked by #2051 !!!

  • Parse pkpass files
  • Implement Stocard Zip bugfix
  • Load parsed images into edit activity
  • Basic tests
  • Consider adding more tests (we got more files from users)
  • Integrate with scan activity
  • Integrate with clicking pkpass file in file manager
  • Don't crash when pkpassparser throws an exception
  • Logo background colour takes priority over background colour defined in card (due to LoyaltyCardEditActivity setting headerColor on image thumbnail set)

New regression to fix:

  • Only load non-thumbnail images when actually needed
  • Rotating the edit activity without changes causes Catima to think you made a change

@TheLastProject TheLastProject linked an issue Aug 11, 2024 that may be closed by this pull request
Pkpass file support #48
Closed
@TheLastProject TheLastProject marked this pull request as draft August 11, 2024 11:49
@TheLastProject TheLastProject force-pushed the feature/pkpass2024 branch 4 times, most recently from 7124d28 to 657807c Compare September 25, 2024 19:42
@TheLastProject TheLastProject force-pushed the feature/pkpass2024 branch 7 times, most recently from 562d9d1 to 72fb68f Compare October 20, 2024 11:44
@TheLastProject TheLastProject force-pushed the feature/pkpass2024 branch 3 times, most recently from 1a9755e to 2150e2a Compare October 22, 2024 19:47
@TheLastProject TheLastProject force-pushed the feature/pkpass2024 branch 4 times, most recently from a78acad to 9362482 Compare November 11, 2024 18:01
@TheLastProject TheLastProject force-pushed the feature/pkpass2024 branch 7 times, most recently from 4eb7e68 to 936bbb8 Compare November 22, 2024 18:47
@TheLastProject TheLastProject force-pushed the feature/pkpass2024 branch 3 times, most recently from ffc82bd to 9130554 Compare December 3, 2024 17:10
@TheLastProject
Copy link
Member Author

I'm not sure if I want to spend too much time trying to fix "Rotating the edit activity without changes causes Catima to think you made a change", it's a very minor thing in the grand scheme of things. Maybe we should just get ready to put this live, it's been taking way too long already...

@TheLastProject TheLastProject force-pushed the feature/pkpass2024 branch 2 times, most recently from c4f640c to bf95447 Compare December 4, 2024 16:51
@TheLastProject
Copy link
Member Author

So, I've spend way too much time on this. This whole feature has been... challenging. Every little fix broke something else and I ended up having to change a huge amount of Catima to make this work (and future-proof) but I think we might be here. All of this to fix the lightbulb in the kitchen.

I would be great if people could do some testing to see if I missed anything. And not just of the pkpass part, just of... the whole app, because so many core things about card handling got changed.

If nobody reports any issues soon, I'll probably just merge this and give the translators some time to translate and then release :)

app-debug.zip

(Note: this is a debug build with a different package name, it will be installed as a second "Catima Debug" app, it won't override your main app)

@TheLastProject TheLastProject mentioned this pull request Dec 4, 2024
Closed
@TheLastProject TheLastProject marked this pull request as ready for review December 4, 2024 17:02
@TheLastProject
Copy link
Member Author

Oops, I forgot to handle the pkpass parser being unable to parse the file...

theimpulson
theimpulson reviewed Dec 5, 2024
View reviewed changes
build.gradle.kts Outdated Show resolved Hide resolved
app/src/main/java/protect/card_locker/ParseResultType.kt Outdated Show resolved Hide resolved
app/src/main/java/protect/card_locker/ParseResult.kt Outdated Show resolved Hide resolved
app/src/main/java/protect/card_locker/PkpassParser.kt
throw IOException(context.getString(R.string.errorReadingFile))
}

try {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider extracting this try-and-catch logic as a separate function to improve readability.

@TheLastProject TheLastProject force-pushed the feature/pkpass2024 branch 3 times, most recently from 6bf956a to 5efa933 Compare December 5, 2024 19:26
@TheLastProject
Copy link
Member Author

New debug build:

  • Fixes crashing on parsing failure
  • Fixes not parsing rgb(0,0,0) colours (though the thumbnail image colour will override it, this is kind of a bug)
  • Fixed some unnecesary double loading-from-storage issues
  • Updated dependencies and some other @theimpulson suggested cleanups (thanks)

app-debug.zip

@TheLastProject
Add Pkpass parser
ea456c6
@TheLastProject
Refactor ScanActivity result code (use ParseResult)
8eeff00
@TheLastProject
Add option to load pkpass from ScanActivity
711ca1e
@TheLastProject
Add option to share pkpass file to Catima
9358348
@TheLastProject
Support opening supported barcode files directly
f7697ce
@TheLastProject
Support for returning images from PkpassParser
1cb9dde
@TheLastProject
Delete old cache files on startup
96a9850
@TheLastProject
Use ViewModel to prevent hammering storage
83fca93 
When you turn a LoyaltyCard into a bundle, it writes the files to
storage as it can't otherwise fit in the limited storage size. This
means that, on rotation, you write all images to storage and load them
again. Using a ViewModel prevents that storage hit due to holding it in
memory (as a ViewModel has a longer lifecycle).
@TheLastProject
Move TaskHandler to ViewModel
7fe6796 
This should make it possible to properly cancel the running barcode
generation threads on rotation and prevent CPU rising on many rotations.
@TheLastProject
Load images on request
e078659 
This prevents loading the front and back images when scrolling through
the loyalty card list and should allow scaling to more images/files more
easily
@TheLastProject
Remove unnecessary image load from storage calls
8009bac 
The LoyaltyCard object itself loads the images itself
@TheLastProject
Copy link
Member Author

Okay, I've had multiple people tell me it works great now and all my testing seems to succeed to.

I guess it's time to hit the merge button :)

Thank you for testing, everyone! There's still some follow-up features that would be neat (like updating the pkpass card) but this PR has already taken much longer than I wanted so let's just call this done for now, give the translators a moment to translate, and do a new release.

If anyone still finds anything in the latest beta release after this message, still please do tell me :)

@TheLastProject TheLastProject merged commit f8a8a84 into main Dec 7, 2024
2 checks passed
@TheLastProject TheLastProject deleted the feature/pkpass2024 branch December 7, 2024 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theimpulson theimpulson theimpulson left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Pkpass file support
2 participants
@TheLastProject @theimpulson