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

Add direct write save option #6594

Merged
merged 2 commits into from
Oct 9, 2021
Merged

Conversation

droidmonkey
Copy link
Member

@droidmonkey droidmonkey commented Jun 6, 2021

Also fixes infinite save bug when saving fails:

Screenshots

image

Testing strategy

Tested all three options (Atomic, Non-Atomic Temp File, and Non-Atomic Direct Write) on Windows and Linux
Tested against GVFS on Gnome 3

Type of change

  • ✅ Bug fix (non-breaking change that fixes an issue)
  • ✅ New feature (change that adds functionality)

src/core/Database.cpp Outdated Show resolved Hide resolved
@michaelk83
Copy link

michaelk83 commented Jul 14, 2021

@droidmonkey Would if help the gvfs situation if KPXC always used a local copy to open and save? IOW: copy to local disk → open from local disk → make changes → save to local disk → copy to origin? If this is what "Temporary file moved into place" does, I wonder why that still fails? Is the problem only when saving, or could there also be an issue when opening from gvfs?

A related idea is use a local temporary file but with atomic move. Database.cpp lines 348-353 are not atomic.
edit: Or is the problem with gvfs at the move operation?

src/core/Database.cpp Outdated Show resolved Hide resolved
src/core/Database.cpp Outdated Show resolved Hide resolved
src/core/Database.h Outdated Show resolved Hide resolved
@droidmonkey droidmonkey force-pushed the feature/direct-write-saves branch from 5f5a403 to 987a238 Compare October 1, 2021 21:21
@droidmonkey droidmonkey requested a review from phoerious October 1, 2021 21:23
Copy link
Member

@phoerious phoerious left a comment

Choose a reason for hiding this comment

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

Ready to go, just one nitpick.

src/core/Database.h Outdated Show resolved Hide resolved
* Introduced in #6438, modified signal is not blocked at the Database level when emitting is blocked. This causes infinite saving to occur when Always Save After Every Change is enabled.
* Closes #6335
* Modify application settings presentation to  allow for alternative saving strategies
* Transition Database::save calls to using flags to control saving behavior. Reduces boolean flags on function call.
* Made direct write save option a local setting to prevent unintentional carry over between platforms.
@droidmonkey droidmonkey force-pushed the feature/direct-write-saves branch from 987a238 to c153b22 Compare October 8, 2021 20:51
@codecov-commenter
Copy link

codecov-commenter commented Oct 8, 2021

Codecov Report

Attention: Patch coverage is 73.91304% with 12 lines in your changes missing coverage. Please review.

Project coverage is 63.67%. Comparing base (bd744d1) to head (c153b22).
Report is 596 commits behind head on develop.

Files with missing lines Patch % Lines
src/core/Database.cpp 72.73% 6 Missing ⚠️
src/gui/DatabaseWidget.cpp 55.56% 4 Missing ⚠️
src/gui/ApplicationSettingsWidget.cpp 66.67% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #6594      +/-   ##
===========================================
- Coverage    63.68%   63.67%   -0.01%     
===========================================
  Files          330      330              
  Lines        41601    41617      +16     
===========================================
+ Hits         26491    26496       +5     
- Misses       15110    15121      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@droidmonkey droidmonkey merged commit f2aa32c into develop Oct 9, 2021
@droidmonkey droidmonkey deleted the feature/direct-write-saves branch October 9, 2021 15:12
@phoerious phoerious added pr: new feature Pull request that adds a new feature pr: bugfix Pull request that fixes a bug and removed new feature bug labels Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: bugfix Pull request that fixes a bug pr: new feature Pull request that adds a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a "Very Unsafe Save" option Ability to disable explicit delete before overwriting the database file
4 participants