refactor: [DC-257] Folder to ensure it always has a sync engine #12501
Draft
refactor: [DC-257] Folder to ensure it always has a sync engine #12501
Conversation
to new static class. static class was chosen over namespace to ensure *all* related decls and defs are in one file.
|
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
missing tr function
also added a new, still empty folder builder shell as this will be used to construct the folder deps so they can be injected to the ctr instead of all this crazy code in the ctr itself
"fixed" is relative, as the tests in this area are confusing and I worry some results are not correct. The tests are "fixed" insofar as the necessary prerequisite for calling addFolder is now fulfilled (ie the local folder actually exist on disk before calling addFolder)
modSpike
changed the title
also started cleaning up the crazy variety of smart pointers and refs related to these types. They are all QObject subs so the goal is that they should be owned by Folder, and wrapped in QPointer in other folder related classes that "need" them.
so far so good but if there are deeper problems they will assuredly show up on windows
and PASS!
…t of line with reality in terms of vfs instance ownership (side effect of never really caring in favor of using QSharedPointer, which is exactly why we should not use shared pointers unless there is true deomonstrated need!) exacerbating the problem is that the FakeFolder::switchToVfs impl shares just a few bits on common with the real Folder::changeVfsMode that in my personal opinion it's not really testing modern behavior of the subsystem at all. This needs deep review.
I hope this finally fixes the alternating crashes on win vs mac - it's still just a bandaid until we review and fix these tests, especially the fake folder impl.
erikjv
reviewed
Apr 20, 2026
| QFAIL(qUtf8Printable(error)); | ||
| }); | ||
| QSignalSpy spy(vfs.get(), &OCC::Vfs::started); | ||
| QObject::connect(vfs, &OCC::Vfs::error, vfs, [](const QString &error) { QFAIL(qUtf8Printable(error)); }); |
Contributor
There was a problem hiding this comment.
not your fault, but assert here instead of a QFAIL. QFail here will only print a message, and execution continues until other problems occur later.
Contributor
Author
There was a problem hiding this comment.
I'm not sure it's fully desired for the tests to stop in this specific case. In theory (at least in my mind) we may want to have tests that cover handling of vfs errors in the app? Let's discuss this one in more detail.
|
|
||
| // don't use QVERIFY outside of the test slot | ||
| if (spy.isEmpty() && !spy.wait()) { | ||
| QFAIL("VFS Setup failed"); |
Contributor
There was a problem hiding this comment.
same as above: assert here please.
these are all over the place and it's often the case that one or other values are needed but not all. key point is it carries the vfs pointer which we want to restrict as much as possible. now that is private with getter only, and checks have been added for nullptr on all consumers that arise from SyncOptions (so far). got rid of dead regex related stuff in syncoptions so now all that is left is the vfs pointer, move to trash and parallel network job count
…into refactor/dc-257
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
see https://kiteworks.atlassian.net/browse/DC-257