Navigation Menu

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

Sort by recently used #1031

Merged

Conversation

Dioxo
Copy link
Contributor

@Dioxo Dioxo commented Aug 19, 2020

馃摙 Type of change

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

馃摐 Description

I added a new PasswordSortOrder for recently used passwords, saving the time when the password was used and comparing with others.

馃挕 Motivation and Context

#535

馃挌 How did you test it?

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

馃摳 Screenshots / GIFs

@Dioxo Dioxo changed the title Feature/sort by recently usedv2 Sort by recently used Aug 19, 2020
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.

Thanks for the v2, this approach looks pretty good to me. Just a few comments.

app/src/main/java/com/zeapo/pwdstore/PasswordFragment.kt Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@msfjarvis msfjarvis left a comment

Choose a reason for hiding this comment

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

Seems good to me, address Fabian's review comments and I'll merge it after testing. Great job!

CHANGELOG.md Outdated Show resolved Hide resolved
@Dioxo
Copy link
Contributor Author

Dioxo commented Aug 19, 2020

would it be better to save the file path or a hash?
and I also think that if the password/category is modified, then the previous name is saved and it is not modified. I'm going to fix that too.

@Dioxo
Copy link
Contributor Author

Dioxo commented Aug 19, 2020

I think i included all your reviews and manage the case of renaming category/password
tell me what you think now :) @msfjarvis @FabianHenneke

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.

Some more comments, but I'm overall quite happy. Thanks for the contribution!

CHANGELOG.md Outdated Show resolved Hide resolved
app/src/main/java/com/zeapo/pwdstore/PasswordFragment.kt Outdated Show resolved Hide resolved
app/src/main/java/com/zeapo/pwdstore/PasswordFragment.kt Outdated Show resolved Hide resolved
app/src/main/java/com/zeapo/pwdstore/PasswordStore.kt Outdated Show resolved Hide resolved
val timeP1 = recentHistory.getString(p1.file.absolutePath)
val timeP2 = recentHistory.getString(p2.file.absolutePath)
when {
timeP1 != null && timeP2 != null -> timeP2.compareTo(timeP1)
Copy link
Member

Choose a reason for hiding this comment

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

This could be simplified using compareBy and nullsLast, but I could also add that later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll leave it to you, I have no idea how to use those...

Dioxo and others added 7 commits August 19, 2020 20:00
Co-authored-by: Fabian Henneke <FabianHenneke@users.noreply.github.com>
Co-authored-by: Fabian Henneke <FabianHenneke@users.noreply.github.com>
Co-authored-by: Fabian Henneke <FabianHenneke@users.noreply.github.com>
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
@msfjarvis msfjarvis added this to the 1.12.0 milestone Aug 20, 2020
@msfjarvis msfjarvis merged commit cfb42f0 into android-password-store:develop Aug 20, 2020
Copy link
Contributor Author

@Dioxo Dioxo left a comment

Choose a reason for hiding this comment

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

Hey, @msfjarvis I think you missed one here too.

@msfjarvis msfjarvis mentioned this pull request Aug 20, 2020
8 tasks
@Dioxo Dioxo deleted the feature/sortByRecentlyUsedv2 branch August 20, 2020 14:50
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

3 participants