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

Detect background changes to database file. #12

Open
wants to merge 1 commit into
from

Conversation

Projects
None yet
@Typz

Typz commented Apr 30, 2013

This gives the option to reload the database.
A setting is available to let the program automatically reload the
database if there is no change, always ignore, or always ask what to do.

@airdrummingfool

This comment has been minimized.

Show comment
Hide comment
@airdrummingfool

airdrummingfool Jul 24, 2013

+1 this would be great to have in the next version.

+1 this would be great to have in the next version.

@pdenya

This comment has been minimized.

Show comment
Hide comment

pdenya commented Jul 25, 2013

+1

@wstrucke

This comment has been minimized.

Show comment
Hide comment
@wstrucke

wstrucke Sep 24, 2013

Please add this feature.

Please add this feature.

@airdrummingfool

This comment has been minimized.

Show comment
Hide comment
@airdrummingfool

airdrummingfool Oct 10, 2013

I just compiled this PR against current master branch (b64276c) and aside from a couple easily-resolved header import conflicts it merged in fine.

The reload function works as expected for both "always ask" and "reload unmodified" settings, until a change is made and saved locally in KeePassX. After that, the user is not alerted if the database is modified in the background and the db is nor reloaded. The app must be restarted to get the reload functionality back.

@Typz I looked through the code but couldn't find anything off. C++ isn't exactly my forte though, so that doesn't mean much ;) Thanks for this PR!

I just compiled this PR against current master branch (b64276c) and aside from a couple easily-resolved header import conflicts it merged in fine.

The reload function works as expected for both "always ask" and "reload unmodified" settings, until a change is made and saved locally in KeePassX. After that, the user is not alerted if the database is modified in the background and the db is nor reloaded. The app must be restarted to get the reload functionality back.

@Typz I looked through the code but couldn't find anything off. C++ isn't exactly my forte though, so that doesn't mean much ;) Thanks for this PR!

Detect background changes to database file.
This gives the option to reload the database.
A setting is available to let the program automatically reload the
database if there is no change, always ignore, or always ask what to do.
@Typz

This comment has been minimized.

Show comment
Hide comment
@Typz

Typz Oct 31, 2013

I believe this happens because when saving the database, a new (temp) file is created then moved to replace the actual database file. Somehow, this causes the file system watcher to fail, and was fixed by simply stopping and restarting watching the database when it is written.

Typz commented Oct 31, 2013

I believe this happens because when saving the database, a new (temp) file is created then moved to replace the actual database file. Somehow, this causes the file system watcher to fail, and was fixed by simply stopping and restarting watching the database when it is written.

@airdrummingfool

This comment has been minimized.

Show comment
Hide comment
@airdrummingfool

airdrummingfool Nov 1, 2013

I tested your updated code and it appears to fix the problems. Thanks!

@debfx can this be merged?

I tested your updated code and it appears to fix the problems. Thanks!

@debfx can this be merged?

@airdrummingfool

This comment has been minimized.

Show comment
Hide comment
@airdrummingfool

airdrummingfool Apr 15, 2014

I tested this against Alpha 6 and it is still working great! There was one merge conflict, but the fix was easy.

I tested this against Alpha 6 and it is still working great! There was one merge conflict, but the fix was easy.

@daniel-ro

This comment has been minimized.

Show comment
Hide comment

+1

@derekaug

This comment has been minimized.

Show comment
Hide comment
@derekaug

derekaug Apr 30, 2014

Really wish this would get rolled in, love to use KeePassX only and this is the only thing keeping me from using it on Windows.

Really wish this would get rolled in, love to use KeePassX only and this is the only thing keeping me from using it on Windows.

@anders-larsson

This comment has been minimized.

Show comment
Hide comment
@anders-larsson

anders-larsson May 1, 2014

Would be lovely if background changes was detected. +1.

Would be lovely if background changes was detected. +1.

@testcocoon

This comment has been minimized.

Show comment
Hide comment
@testcocoon

testcocoon Jun 21, 2014

I have review your commit and the implementation is nicer as mine (#56).
But you should consider use the FileSystemWatcher that I have used since that I'm not sure that QFileSystemWatcher works correctly with tools that are deleting and renaming files after a transfer.
In my mind rsync and unison are doing so.

I have review your commit and the implementation is nicer as mine (#56).
But you should consider use the FileSystemWatcher that I have used since that I'm not sure that QFileSystemWatcher works correctly with tools that are deleting and renaming files after a transfer.
In my mind rsync and unison are doing so.

@testcocoon

This comment has been minimized.

Show comment
Hide comment
@testcocoon

testcocoon Aug 11, 2014

@Typz as explained in #56 , your implementation is nice, but I know that QFileSystemWatcher does not work when the file get deleted+replaced. My own wrapper of QFileSystemWatcher handle it. I think we should use it instead of QFileSystemWatcher.
To you have some concern if I'm doing this modification?

@Typz as explained in #56 , your implementation is nice, but I know that QFileSystemWatcher does not work when the file get deleted+replaced. My own wrapper of QFileSystemWatcher handle it. I think we should use it instead of QFileSystemWatcher.
To you have some concern if I'm doing this modification?

@Typz

This comment has been minimized.

Show comment
Hide comment
@Typz

Typz Aug 11, 2014

@testcocoon No problem with me, please help yourself and submit a patch [against my clone] I'll gladly integrate it into this merge request.

I just looked at your FileSystemWatcher class, and I would suggest you make some slight changes before submitting, though:

  • Do not inherit from QFileSystemWatcher, but wrap it; the base class API cannot really be used (and should not be!) so there is no point using inheritance. Plus it makes the code harder to read, with the same signal name in QFileSystemWatcher and its subclass FileSystemWatcher.
  • use the same API as QFileSystemWatcher: addPath()/removePath(), to keep source compatibility. Makes it easier to swap one for the other, possibly when this is handled directly upstream (in Qt implementation).
  • some cases are not handled: watching multiple files, parent directly removed, file 'simply' removed (no notification at all!)
  • please add unit tests for your file system watcher class

Typz commented Aug 11, 2014

@testcocoon No problem with me, please help yourself and submit a patch [against my clone] I'll gladly integrate it into this merge request.

I just looked at your FileSystemWatcher class, and I would suggest you make some slight changes before submitting, though:

  • Do not inherit from QFileSystemWatcher, but wrap it; the base class API cannot really be used (and should not be!) so there is no point using inheritance. Plus it makes the code harder to read, with the same signal name in QFileSystemWatcher and its subclass FileSystemWatcher.
  • use the same API as QFileSystemWatcher: addPath()/removePath(), to keep source compatibility. Makes it easier to swap one for the other, possibly when this is handled directly upstream (in Qt implementation).
  • some cases are not handled: watching multiple files, parent directly removed, file 'simply' removed (no notification at all!)
  • please add unit tests for your file system watcher class
@airdrummingfool

This comment has been minimized.

Show comment
Hide comment
@airdrummingfool

airdrummingfool Oct 28, 2014

For anyone still watching this, I rebased @Typz's work onto master as of this morning (f1aa6ac). I had to make a few small adjustments, but I've tested it and it seems to be working fine. My work is here: https://github.com/airdrummingfool/keepassx/tree/update-typz-autoreload .

@Typz: if this is useful and you want a PR, just let me know. Thanks again for your work on this!

For anyone still watching this, I rebased @Typz's work onto master as of this morning (f1aa6ac). I had to make a few small adjustments, but I've tested it and it seems to be working fine. My work is here: https://github.com/airdrummingfool/keepassx/tree/update-typz-autoreload .

@Typz: if this is useful and you want a PR, just let me know. Thanks again for your work on this!

@eternaltyro

This comment has been minimized.

Show comment
Hide comment
@eternaltyro

eternaltyro Sep 22, 2015

+1 Long time pending?

+1 Long time pending?

@Tklaversma

This comment has been minimized.

Show comment
Hide comment
@Tklaversma

Tklaversma Oct 7, 2015

Does anyone know the current status of this feature, also known as issue #73? Can't wait for it to be merged into KeePass..

Does anyone know the current status of this feature, also known as issue #73? Can't wait for it to be merged into KeePass..

@daniellandau

This comment has been minimized.

Show comment
Hide comment
@daniellandau

daniellandau Aug 3, 2016

I rebased unto current master (see https://github.com/daniellandau/keepassx/tree/autoreload) and it seems to still work. This is extremely useful for people who sync the database with other devices and never close their computer or KeePassX.

I rebased unto current master (see https://github.com/daniellandau/keepassx/tree/autoreload) and it seems to still work. This is extremely useful for people who sync the database with other devices and never close their computer or KeePassX.

@Tklaversma

This comment has been minimized.

Show comment
Hide comment
@Tklaversma

Tklaversma Aug 3, 2016

So your saying you got a working example? If so, could you post the executable for Mac?

So your saying you got a working example? If so, could you post the executable for Mac?

@daniellandau

This comment has been minimized.

Show comment
Hide comment
@daniellandau

daniellandau Aug 3, 2016

I don't regularly use a Mac (I use Linux), but I might be able to find one in a week or two. If anyone else can produce an executable (or you yourself, it's not terribly hard!) that's preferrable.

And yes, it does work for me.

I don't regularly use a Mac (I use Linux), but I might be able to find one in a week or two. If anyone else can produce an executable (or you yourself, it's not terribly hard!) that's preferrable.

And yes, it does work for me.

@Tklaversma

This comment has been minimized.

Show comment
Hide comment
@Tklaversma

Tklaversma Aug 4, 2016

Since I do not have the knowledge, I would appreciate it very much if you or someone else can compile it for me (and a lot of others I guess...).

Thanks in advance!

Since I do not have the knowledge, I would appreciate it very much if you or someone else can compile it for me (and a lot of others I guess...).

Thanks in advance!

@daniellandau

This comment has been minimized.

Show comment
Hide comment
@daniellandau

daniellandau Aug 22, 2016

I couldn't find a suitable Mac, but you should really go ahead and build it yourself. You can open an issue in my fork and I'll help you with the steps. Start with https://github.com/keepassx/keepassx#from-source

I couldn't find a suitable Mac, but you should really go ahead and build it yourself. You can open an issue in my fork and I'll help you with the steps. Start with https://github.com/keepassx/keepassx#from-source

@daniellandau daniellandau referenced this pull request Oct 2, 2016

Closed

Autoreload #9

@phoerious

This comment has been minimized.

Show comment
Hide comment
@phoerious

phoerious Jan 14, 2017

This is implemented in the upcoming KeePassXC version 2.1.0.

This is implemented in the upcoming KeePassXC version 2.1.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment