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

Index update #22

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Index update #22

wants to merge 5 commits into from

Conversation

lcmaqueda
Copy link
Collaborator

Fix the problem of replicated entries when updating items in the password store. With this PR, we perform the minimum operation required per type of update, i.e.:

  • Add a single entry when a new password is added to the store
  • Remove a single entry when a password is removed from the store
  • Re-index everything when a password is moved
  • Do nothing when when a password is updated

In addition, this PR also tries to make the UI updates displayed in the lower left corner more consistent and durable (before, most of them where overwritten with an empty message right after being published, so they were not visible).

@lcmaqueda
Copy link
Collaborator Author

lcmaqueda commented Nov 11, 2016

I think this PR is an improvement in general, but I'd like to know your opinion about commits 3e76375 and ee9897b in particular

@cortex
Copy link
Owner

cortex commented Nov 16, 2016

Thanks a lot for writing this, and sorry for taking so long to review!

I was initially a bit put off by just using string/string as types for name and path respectively, maybe it would be good to use a special type to distinguish then, but then again maybe it would just be confusing. But this solution is much simpler, so I think it is better. Maybe I'll add in some types or similar to make it clear. In any case, this is much better than before, and it solves an important bug. Nicely done!

Since it is using a map, maybe it needs a mutex to protect it, did you verify that this is not needed?

In the future, I think it would be better to do different things in different PRs, it is easier to review that way!

@lcmaqueda
Copy link
Collaborator Author

You're right, I let myself get carried away and ended up adding too much stuff to this PR... I also considered defining specific types for path and name, I think it would make things more clear. I would be happy to add those types for path and name and a mutex-controlled access to that map. I could do it either as part of this PR or as a separate one.

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.

2 participants