Break down PGP Activity into focused sections

[msfjarvis]

📢 Type of change

• Bugfix
• New feature
• Enhancement
• Refactoring

📜 Description

This PR is a concentrated effort to break PgpActivity down to more logical parts that will allow this to be more maintainable and logical going forward. Also opens up the possibility to finally get rid of kotlin-android-extensions and replace it with ViewBinding.

💡 Motivation and Context

PgpActivity is a blackbox of arcane logic and bad design choices that stump contributors and maintainers alike and reducing development friction by bringing this up to standards is an important goal of mine.

💚 How did you test it?

Generating a password both from the FAB as well as the Autofill action works as expected.

📝 Checklist

• I formatted the code with the IDE's reformat action (Ctrl + Shift + L/Cmd + Shift + L)
• I reviewed submitted code

🔮 Next steps

📸 Screenshots / GIFs

[msfjarvis]

It is insane to me just how much "magic" this monolith of madness encapsulates...

[msfjarvis]

@FabianHenneke I'm not very happy with how the DelayShow cruft in PgpActivity works. Would replacing the foreground service for clearing clipboard with WorkManager be a better option?

[fmeum]

@FabianHenneke I'm not very happy with how the DelayShow cruft in PgpActivity works. Would replacing the foreground service for clearing clipboard with WorkManager be a better option?

Based on my limited experience with WorkManager, it is pretty good at ensuring minimal delays, but I don't know how to set reliable maximal delays. Even though it's effort this way, I think that a foreground service is the way to go for such a sensitive operation.

[msfjarvis]

@FabianHenneke I'm not very happy with how the DelayShow cruft in PgpActivity works. Would replacing the foreground service for clearing clipboard with WorkManager be a better option?

Based on my limited experience with WorkManager, it is pretty good at ensuring minimal delays, but I don't know how to set reliable maximal delays. Even though it's effort this way, I think that a foreground service is the way to go for such a sensitive operation.

That's a fair point... I'll keep that around and get rid of DelayShow anyway.

[msfjarvis]

Looks like pressing the Save button in PasswordCreationActivity does nothing until I press it again, but the Save-and-Copy button works properly. Not sure what's up...

I think that this happens because the else branch in bindToOpenKeychain does not trigger the registerForActivityResult "listener", only the if branch does. I still haven't wrapped my head around the new flow or I would propose a fix, but maybe there is a way to trigger it synchronously?

I'll take a look, thanks for the insight!

[msfjarvis]

Can't test this right now but I think it'll work, will be able to confirm in about 8 to 10 hours.

[msfjarvis]

Yep this works. @FabianHenneke we should be good to go for a final review and merge 🎉

[fmeum]

Well done! I have left some comments, but nothing serious. I will do another test round after they have been resolved.

[msfjarvis]

Well done! I have left some comments, but nothing serious. I will do another test round after they have been resolved.

Awesome, thanks a lot for the review. I'll have it sorted in a couple hours when I take a break off work.

[msfjarvis]

Okay I think all review comments are addressed now.

[msfjarvis]

Had to revert 392edb7 because it didn't behave correctly. What was /website before became ../../../../../website, presumably because calling relative path with the full store path (/data/data/dev.msfjarvis.aps/files/store) made it traverse that entire chain and create a relative path from that as opposed to just stripping $fullPath.

[fmeum]

I tested this more and found only the following two remaining issues:

1. When arriving at the PasswordCreationActivity via the "Edit" action on a decrypted entry, the title should probably become "Edit password" instead of "New password".
2. When pressing/tapping back while editing password or using the "Save" action, the decrypt activity is shown again and immediately redecrypts the password. This can cause OpenKeychain prompts to appear, for example when the key is on a hardware token. This is particularly unexpected for the "Save" action.
   How about the following: We keep this behavior on a back press/tap, but finish the underlying password activity after "Save" has been executed. Who wants to see the entry they just edited anyway? ;-)
   The alternative would be to somewhow inject the result of the edit into the password decrypt activity without going through a decrypt, but that sounds like a lot of additional logic.

[msfjarvis]

I tested this more and found only the following two remaining issues:

1. When arriving at the `PasswordCreationActivity` via the "Edit" action on a decrypted entry, the title should probably become "Edit password" instead of "New password".

Yeah that makes sense.

2. When pressing/tapping back while editing password or using the "Save" action, the decrypt activity is shown again and immediately redecrypts the password. This can cause OpenKeychain prompts to appear, for example when the key is on a hardware token. This is particularly unexpected for the "Save" action.
   How about the following: We keep this behaviour on a back press/tap, but finish the underlying password activity after "Save" has been executed. Who wants to see the entry they just edited anyway? ;-)

I think this can be done and would probably make sense too.

   The alternative would be to somehow inject the result of the edit into the password decrypt activity without going through a decrypt, but that sounds like a lot of additional logic.

Yeah this is gonna be too much work for not enough gain :/

[fmeum]

Confirmed fixed.
Thank you very much, this was a heroic effort!