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.

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

Remove GitAsyncTask and replace with non-blocking coroutines #865

Merged
merged 25 commits into from Aug 5, 2020
Merged

Remove GitAsyncTask and replace with non-blocking coroutines #865

merged 25 commits into from Aug 5, 2020

Conversation

msfjarvis
Copy link
Member

@msfjarvis msfjarvis commented Jun 20, 2020 •
edited

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring
  • Dependency updates

📜 Description

Refactors the git command execution to be less obtrusive to the users, offer better error messages and use coroutines in place of the aging AsyncTask.

💡 Motivation and Context

GitAsyncTask is next on our list of ugly monoliths that need to go. Like GitActivity and PgpActivity, it encapsulates a bit too much logic that is better suited in different places of our execution flow.

💚 How did you test it?

Completely untested, might not even compile.

📝 Checklist

  • I formatted the code with the IDE's reformat action (Ctrl + Shift + L/Cmd + Shift + L)
  • I reviewed submitted code
  • I added a CHANGELOG entry if applicable

🔮 Next steps

moar coroutines!

@msfjarvis msfjarvis added this to the 1.10.0 milestone Jun 20, 2020
@msfjarvis msfjarvis self-assigned this Jun 20, 2020
This was linked to issues Jun 20, 2020
Handle the blocking actions in another thread #160
Closed
Provide contextual error messages, not “Internal exception occurred” #219
Closed
@msfjarvis
Copy link
Member Author

@FabianHenneke any particular reason for AutofillSaveActivity not extending AppCompatActivity? Would be
a bit easier to make things suspend if I don't have to wire up lifecycleScope myself 😬

@fmeum
Copy link
Member

fmeum commented Jun 20, 2020

@FabianHenneke any particular reason for AutofillSaveActivity not extending AppCompatActivity? Would be
a bit easier to make things suspend if I don't have to wire up lifecycleScope myself 😬

I don't think there was, please go ahead and change it.

@msfjarvis
Copy link
Member Author

@FabianHenneke any particular reason for AutofillSaveActivity not extending AppCompatActivity? Would be
a bit easier to make things suspend if I don't have to wire up lifecycleScope myself grimacing

I don't think there was, please go ahead and change it.

Cool, thanks for quick response! Now get back on with your weekend :P

@msfjarvis
Copy link
Member Author

It seems I broke Android with my extreme refactoring prowess... https://bin.msfjarvis.dev/~5eee41b25d7115001813f9b5

@msfjarvis msfjarvis changed the title Begin refactoring GitAsyncTask for removal Remove GitAsyncTask and replace everything with non-blocking coroutines Jun 20, 2020
@msfjarvis
Copy link
Member Author

Seems like I ran into a bug: Kotlin/kotlinx.coroutines#2041. 1.4-M2 seems to fix it, trying that out.

@msfjarvis
Copy link
Member Author

Alright that fixed the crash and everything's broken in the correct ways now. Will come back to this when I need to mellow out.

@msfjarvis
Copy link
Member Author

@FabianHenneke how do we feel about eradicating PasswordFragment and merging it into PasswordStore

@fmeum
Copy link
Member

fmeum commented Jun 21, 2020

@FabianHenneke how do we feel about eradicating PasswordFragment and merging it into PasswordStore

They are highly coupled anyway and we are not really ever swapping out fragments anyway, so I would say that's fine if it simplifies things elsewhere.

@msfjarvis
Copy link
Member Author

@FabianHenneke how do we feel about eradicating PasswordFragment and merging it into PasswordStore

They are highly coupled anyway and we are not really ever swapping out fragments anyway, so I would say that's fine if it simplifies things elsewhere.

Hm but we still have the ToCloneOrNot fragment that's shown on first run, might honestly just make better sense to make it a separate activity.

@msfjarvis
Copy link
Member Author

Giving up on that effort for the moment, this is gonna entail a larger refactor involving Autofill and SelectFolderFragment as well which is a distraction I do not want right now. Gonne see if GitOperationActivity can somehow be remove since it is basically a shell.

@msfjarvis msfjarvis closed this Jun 24, 2020
@msfjarvis msfjarvis reopened this Jun 24, 2020
@msfjarvis msfjarvis changed the base branch from master to develop June 24, 2020 19:01
@msfjarvis msfjarvis removed this from the 1.10.0 milestone Jul 16, 2020
@msfjarvis msfjarvis mentioned this pull request Jul 31, 2020
Closed
@msfjarvis
Bring in updated exec tooling
1c3a364 
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
@msfjarvis
Refactor git operations
f79fee1 
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
@msfjarvis
Fixup ErrorMessages
c38627c 
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
@msfjarvis
Make git operations suspend and switch to GitCommandExecutor
d35a9c5 
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
@msfjarvis
Clear leaks in GitCommandExecutor
d3133c4 
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
@msfjarvis
Formatting spree
79bf76c 
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
@msfjarvis
Copy link
Member Author

msfjarvis commented Jul 31, 2020 •
edited

TODO:

  • Add back some semblance of progress UI
  • Figure out how I broke CloneOperation
  • Find and fix everything else destroyed by me

@msfjarvis
Fix hard reset operation when branch exists
49a931d 
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
@msfjarvis
Copy link
Member Author

Think this is about ready now

@msfjarvis
Move remoteBranch to GitOperation and add missing copyright header
c9d806d 
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
@msfjarvis
GitOperation: log error message in default onError implementation
13c5e89 
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
@msfjarvis
Copy link
Member Author

msfjarvis commented Aug 2, 2020 •
edited

commitChange seems to be broken, files are being staged but the commit never finishes. Fixed with https://msfjarvis.dev/aps/bbf711824722

@msfjarvis
Remove thread hint annotations
e53e39b 
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
@msfjarvis
Make GitException a sealed class
8262099 
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
@msfjarvis
Commonize error dialog in git operation
a31f084 
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
@msfjarvis
Log original exception in error flow
dda010a 
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
@msfjarvis
Remove useless variable
e97fac1 
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
@msfjarvis
Fix committing
bbf7118 
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
@fmeum
Copy link
Member

fmeum commented Aug 3, 2020

When I create a new password, the commit seems to be created correctly, but Password Store is finished when the commit is done. Maybe there is an issue related to finishActivityOnEnd here?

@msfjarvis
Copy link
Member Author

When I create a new password, the commit seems to be created correctly, but Password Store is finished when the commit is done. Maybe there is an issue related to finishActivityOnEnd here?

I'll check after work.

@msfjarvis
Bring back finishActivityOnEnd
9ce8da3 
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
@msfjarvis
Copy link
Member Author

When I create a new password, the commit seems to be created correctly, but Password Store is finished when the commit is done. Maybe there is an issue related to finishActivityOnEnd here?

Should be fixed now along with all the other git related tasks.

@msfjarvis msfjarvis mentioned this pull request Aug 4, 2020
Closed
@msfjarvis
Replace some AppCompatActivity references with FragmentActivity
1647ec0 
This lets us use these directly with requireActivity() without having to cast to AppCompatActivity

Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
@fmeum
Copy link
Member

fmeum commented Aug 4, 2020

Will look into it tomorrow morning. Sorry for the late response, I got caught up in the natural follow-up PR that allows us to finally drop Jsch (coming soon).

fmeum
fmeum approved these changes Aug 5, 2020
View reviewed changes
Copy link
Member

@fmeum fmeum left a comment

Choose a reason for hiding this comment

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

Looks good to me and worked in simple use cases, great work! Let's send it off to the snapshot build.

@msfjarvis msfjarvis changed the title Remove GitAsyncTask and replace everything with non-blocking coroutines Remove GitAsyncTask and replace with non-blocking coroutines Aug 5, 2020
@msfjarvis msfjarvis merged commit 14c44bf into android-password-store:develop Aug 5, 2020
@msfjarvis msfjarvis deleted the refactor/unblock-ui branch August 5, 2020 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmeum fmeum fmeum approved these changes

@Skrilltrax Skrilltrax Awaiting requested review from Skrilltrax

Assignees

@msfjarvis msfjarvis

Labels
None yet
Projects
None yet
Milestone
1.11.0
3 participants
@msfjarvis @fmeum @Skrilltrax