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

Fix expansion of selective sync tree #10058

Merged
merged 4 commits into from
Aug 25, 2022
Merged

Fix expansion of selective sync tree #10058

merged 4 commits into from
Aug 25, 2022

Conversation

fmoc
Copy link
Contributor

@fmoc fmoc commented Aug 24, 2022

Before:
screenshot_2022-08-24_14-47-12

After:
screenshot_2022-08-24_14-49-07

@fmoc fmoc requested review from a team and dragotin August 24, 2022 12:50
@ownclouders
Copy link
Contributor

ownclouders commented Aug 24, 2022

@fmoc fmoc changed the base branch from master to 2.11 August 24, 2022 12:53
@fmoc fmoc marked this pull request as draft August 24, 2022 14:56
@fmoc
Copy link
Contributor Author

fmoc commented Aug 24, 2022

We decided to have another look.

@fmoc fmoc requested a review from erikjv August 24, 2022 18:29
@fmoc fmoc marked this pull request as ready for review August 24, 2022 18:29
@fmoc
Copy link
Contributor Author

fmoc commented Aug 24, 2022

I had another look and I think this new fix is significantly better than my last attempt. Now there is a check to make sure invalid QVariants won't slip through the check.

Copy link
Contributor

@dragotin dragotin left a comment

Choose a reason for hiding this comment

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

The whole patch seems bogus since the isReady flag is always true. Why don't you remove the isReady flag completely? There is one more usage in AccountSettings::slotCustomContextMenuRequested which can be cleaned up/removed as well.

Might be the better solution overall
@fmoc
Copy link
Contributor Author

fmoc commented Aug 25, 2022

The flag used to be used to query whether the root folder is ready. I just forgot to implement that same functionality for subfolders as well. It has got its use. I updated the patch to perform the check properly for subfolders as well, i.e., it's actually doing something useful now.

I think we actually have to review the return values of data() in other places as well in the future, since it may always return some invalid QVariant, which could be a problem in other places as well. I'll open an issue for that.

@dragotin
Copy link
Contributor

Currently, whenever you use data( index, FolderStatusDelegate::IsReady) you have access to the folder object as well. With that you could ask if the folder object is ready, which is what you're really interested in. Pulling that through the model makes that more complex in my opinion.
So again, my recommendation is: Drop the isReady from the model data and use folder->isReady() directly.

That removes alternative code pathes that actually do the same thing.

@fmoc
Copy link
Contributor Author

fmoc commented Aug 25, 2022

Makes sense, totally agree, thanks!

Copy link
Collaborator

@erikjv erikjv left a comment

Choose a reason for hiding this comment

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

Nice!

Copy link
Contributor

@dragotin dragotin left a comment

Choose a reason for hiding this comment

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

LGTM

@sonarcloud
Copy link

sonarcloud bot commented Aug 25, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@fmoc fmoc merged commit a1da631 into 2.11 Aug 25, 2022
@delete-merged-branch delete-merged-branch bot deleted the work/fix-selective-sync branch August 25, 2022 10:44
fmoc pushed a commit that referenced this pull request Aug 26, 2022
This commit restores the use of FolderStatusDelegate::data(...) to check whether a folder is ready. The two additions fix two different ways in which the client could crash.

Using data(...) makes sure we use the right data structure to look up the ready state for the subfolders. Using folder(...) rightfully causes an index out of range error.

This commit partially reverts #10058 and restores a previous version of said PR.
fmoc added a commit that referenced this pull request Aug 29, 2022
This commit restores the use of FolderStatusDelegate::data(...) to check whether a folder is ready. The two additions fix two different ways in which the client could crash.

Using data(...) makes sure we use the right data structure to look up the ready state for the subfolders. Using folder(...) rightfully causes an index out of range error.

This commit partially reverts #10058 and restores a previous version of said PR.
@jnweiger jnweiger mentioned this pull request Aug 31, 2022
55 tasks
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.

4 participants