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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consolidate password list refresh #887

Merged
merged 4 commits into from Jun 28, 2020
Merged

Consolidate password list refresh #887

merged 4 commits into from Jun 28, 2020

Conversation

fmeum
Copy link
Member

@fmeum fmeum commented Jun 26, 2020

馃摙 Type of change

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

馃摐 Description

Replace refreshPasswordList and resetPasswordList with a single function that resets navigation only if necessary, namely if the
current directory no longer exists.
This function is now also called mostly from PasswordStore, whereas previously refresh and reset calls were made from many different places in the code base, sometimes twice, sometimes ineffectively.

@msfjarvis There is a remaining issue to fix: The sync operation does not return with the correct request code (105), but with 65535. As a result, just as before, no refresh/reset happens after a sync operation while everything works as expected after a pull operation.

馃挕 Motivation and Context

Password list refresh and reset is currently handled all over the place in our code base and not handled correctly in all cases.

馃挌 How did you test it?

I verified that the correct kind of refresh is performed after moves, password creations, push and pull operations, but this needs more testing.

馃摑 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

Come up with a good changelog entry once we know whether we can fix the sync issue.

馃摳 Screenshots / GIFs

Replace refreshPasswordList and resetPasswordList with a single
function that resets navigation only if necessary, namely if the
current directory no longer exists.
This function is now also called mostly from PasswordStore, whereas
previously refresh and reset calls were made from many different
places in the code base, sometimes twice, sometimes ineffectively.

There is a remaining issue to fix: The sync operation does not return
with the correct request code (105), but with 65535. As a result, as
before, no refresh/reset happens after a sync operation while
everything works as expected after a pull operation.
@fmeum fmeum added this to the 1.10.0 milestone Jun 26, 2020
@msfjarvis
Copy link
Member

@msfjarvis There is a remaining issue to fix: The sync operation does not return with the correct request code (105), but with 65535. As a result, just as before, no refresh/reset happens after a sync operation while everything works as expected after a pull operation.

Hm I think I encountered this while refactoring PasswordStore to use ActivityResultContracts, I'll push a fix to this PR when I get to my PC for testing this.

@msfjarvis msfjarvis self-assigned this Jun 26, 2020
* develop:
  Update Public Suffix List data (#888)
@msfjarvis
Copy link
Member

20 minutes of frustrating println debugging later I got nothing :/

@fmeum
Copy link
Member Author

fmeum commented Jun 27, 2020

20 minutes of frustrating println debugging later I got nothing :/

It is really weird that the behavior differs between sync and pull. I don't see any activity-related difference between the two operations.

If we really can't figure this out, how about adding a fixme comment and calling reset in an else branch? It's ugly, but I'm out of other ideas.

@msfjarvis
Copy link
Member

20 minutes of frustrating println debugging later I got nothing :/

It is really weird that the behavior differs between sync and pull. I don't see any activity-related difference between the two operations.

If we really can't figure this out, how about adding a fixme comment and calling reset in an else branch? It's ugly, but I'm out of other ideas.

Yeah I think that's gonna do for now, hopefully ActivityResultContracts will eliminate this when I finish the migration.

@fmeum
Copy link
Member Author

fmeum commented Jun 28, 2020

The refresh is also run when an unexpected request code is encountered (with an appropriate FIXME comment).

@msfjarvis msfjarvis merged commit 535ad1d into develop Jun 28, 2020
@msfjarvis msfjarvis deleted the enhancement/refresh branch June 28, 2020 07:59
msfjarvis added a commit that referenced this pull request Jun 29, 2020
* develop:
  Offer TOTP Autofill for OTP fields (#899)
  Merge SshKeyGenFragment into its activity (#897)
  Reintroduce TOTP support (#890)
  Sync with release branch (#896)
  Rework GitHub Actions (#893)
  Consolidate password list refresh (#887)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants