fix(editLocally): evaluate lock-record DB call outside Q_ASSERT#9973
Conversation
greatjourney589
requested review from
camilasan,
i2h3,
mgallien and
nilsding
as code owners
greatjourney589
force-pushed
the
fix/editlocally-qassert-side-effect
branch
from
a16ee94 to
4d6ce40
Compare
| if (!recordFetched || !rec.isValid() || !rec._lockstate._locked) { | ||
| qCWarning(lcEditLocallyJob()) << "File reported as already locked but journal record is missing or stale, showing notification without remaining time" | ||
| << _relPath << "recordFetched:" << recordFetched << "valid:" << rec.isValid() << "locked:" << rec._lockstate._locked; | ||
| fileLockProcedureComplete(tr("File %1 already locked.").arg(_fileName), | ||
| tr("You can unlock this file manually once you are finished editing."), | ||
| true); | ||
| return; | ||
| } |
There was a problem hiding this comment.
How did you manage this notification to show up? while testing the PR I still got the other message:
the only caller of this method (EditLocallyJob::lockFile) already queries the lock state from the sync journal via Account::fileLockStatus
EditLocallyJob::fileAlreadyLocked would then not be called at all in case the file is already unlocked or not present in the sync journal.
There was a problem hiding this comment.
That is exactly correct. fileAlreadyLocked() is called from within lockFile() only after Account::fileLockStatus() has already executed getFileRecord() on the same journal and confirmed that _locked == true. Therefore, the fallback branch code I had added was, in reality, an unreachable path. I have force-pushed the corrected version. The PR now contains only the minimal fix addressing the Q_ASSERT side effect (+2 −1).
greatjourney589
force-pushed
the
fix/editlocally-qassert-side-effect
branch
2 times, most recently
from
7b8bbe9 to
3db2948
Compare
Signed-off-by: greatjourney589 <wangkyle589@gmail.com>
nilsding
force-pushed
the
fix/editlocally-qassert-side-effect
branch
from
3db2948 to
cfad288
Compare
|
/backport to stable-33.0 |
|
/backport to stable-4.0 |
|
Artifact containing the AppImage: nextcloud-appimage-pr-9973.zip Digest: To test this change/fix you can download the above artifact file, unzip it, and run it. Please make sure to quit your existing Nextcloud app and backup your data. |
2 participants
Resolves
#9972
Summary
In
EditLocallyJob::fileAlreadyLocked(), the call that fetches the lock state fromthe journal DB was wrapped directly in
Q_ASSERT:Q_ASSERT(x)expands to((void)0)whenQT_NO_DEBUGis defined — i.e. in everyrelease build the project ships. As a result,
getFileRecord()was never calledin release:
recstayed default-constructed,_lockTimeand_lockTimeoutwere both0, and the user-facing notification read "Lock will last for 0 minutes." everytime someone re-opened an already-locked file via Edit Locally.
The project's own assert header (
src/common/asserts.h) explicitly documents thatQ_ASSERT"is only present in debug builds", so this was a misuse of the macro.Fix
getFileRecord()return value into a local before asserting on it, sothe DB call always runs.
Q_ASSERTs for debug-build diagnostics.or not actually locked), show a generic notification without the bogus "0 minutes"
string and return early.
No behavior change in debug builds. In release builds the dialog now shows the real
remaining lock time, matching the
fileLockSuccesspath.TODO
Q_ASSERTside-effect inEditLocallyJob::fileAlreadyLocked()Checklist
AI (if applicable)