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 requested review from fmeum, msfjarvis and Skrilltrax as code owners Aug 19, 2020
@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

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

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@msfjarvis msfjarvis left a comment

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

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/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)

This comment has been minimized.

@fmeum

fmeum Aug 19, 2020
Member

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

This comment has been minimized.

@Dioxo

Dioxo Aug 19, 2020
Author Contributor

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

Dioxo and others added 7 commits Aug 19, 2020
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>
Missed it
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
@msfjarvis msfjarvis added this to the 1.12.0 milestone Aug 20, 2020
@msfjarvis msfjarvis added the feature label Aug 20, 2020
@msfjarvis msfjarvis merged commit cfb42f0 into android-password-store:develop Aug 20, 2020
5 checks passed
5 checks passed
test-pr (23, freeRelease)
Details
test-pr (23, nonFreeRelease)
Details
test-pr (29, freeRelease)
Details
test-pr (29, nonFreeRelease)
Details
WIP Ready for review
Details
Copy link
Contributor Author

@Dioxo Dioxo left a comment

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

@msfjarvis msfjarvis mentioned this pull request Aug 20, 2020
4 of 8 tasks complete
@Dioxo Dioxo deleted the Dioxo:feature/sortByRecentlyUsedv2 branch Aug 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can鈥檛 perform that action at this time.