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.

Sign up for GitHub

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

Jump to bottom

Improve 'Handle local file editing' feature. Add loading popup. Add force sync before opening a file. #4990

Merged
merged 1 commit into from
Oct 10, 2022
Merged

Improve 'Handle local file editing' feature. Add loading popup. Add force sync before opening a file. #4990

merged 1 commit into from
Oct 10, 2022

Conversation

allexzander
Copy link
Contributor

Signed-off-by: alex-z blackslayer4@gmail.com

This will close #4981

@allexzander allexzander requested review from claucambra, mgallien and camilasan and removed request for mgallien September 30, 2022 13:48
@codecov
Copy link

codecov bot commented Sep 30, 2022 •
edited
Loading

Codecov Report

Merging #4990 (2f978aa) into master (47b2cc3) will increase coverage by 0.12%.
The diff coverage is n/a.

❗ Current head 2f978aa differs from pull request most recent head 7672cb7. Consider uploading reports for the commit 7672cb7 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #4990      +/-   ##
==========================================
+ Coverage   57.18%   57.31%   +0.12%     
==========================================
  Files         138      138              
  Lines       17394    17394              
==========================================
+ Hits         9947     9969      +22     
+ Misses       7447     7425      -22
Impacted Files Coverage Δ
src/libsync/propagatedownload.cpp 64.46% <0.00%> (-0.30%) ⬇️
src/libsync/syncengine.cpp 83.33% <0.00%> (+0.45%) ⬆️
src/libsync/vfs/cfapi/cfapiwrapper.cpp 74.31% <0.00%> (+1.81%) ⬆️
src/libsync/vfs/cfapi/vfs_cfapi.cpp 85.77% <0.00%> (+2.37%) ⬆️
src/libsync/vfs/cfapi/hydrationjob.cpp 56.08% <0.00%> (+3.70%) ⬆️

mgallien
mgallien requested changes Oct 7, 2022
View reviewed changes
src/gui/systray.cpp Outdated Show resolved Hide resolved
src/gui/folderman.cpp Outdated
Comment on lines 1495 to 1497
folderForFile->setProperty(editFileLocallyName, localFilePath);
folderForFile->startSync();
QObject::connect(folderForFile, &Folder::syncFinished, this, &FolderMan::slotSyncFinishedBeforeOpeningForLocalEditing);
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is too brittle
please use a lambda with capture for the file name
clean up in case of errors will be easier and you will also have a much easier way to track the filename once sync is completed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mgallien Why is it brittle? If the slot is invoked by a proper Folder instance, then, it will have a proper localFilePath set. The reason for making it a slot instead of lambda is that I could disconnect a signal, otherwise, you will end up having the file opened each time the sync is finished after any file was opened via this signal, as the connection to lambda doesn't go anywhere.

Anyways, I have implemented an alternative solution with a QMap of connections, even though, I'd prefer to just have a slot and make it all simpler.

@allexzander allexzander requested a review from mgallien October 7, 2022 16:16
@allexzander allexzander force-pushed the bugfix/local-editing-sync-and-loading-indicator branch from 2f978aa to 816eba4 Compare October 7, 2022 16:24
mgallien
mgallien reviewed Oct 10, 2022
View reviewed changes
src/gui/folderman.cpp Outdated Show resolved Hide resolved
@allexzander allexzander requested a review from mgallien October 10, 2022 08:51
@allexzander
Improve 'Handle local file editing' feature. Add loading popup. Add f…
7672cb7 
…orce sync before opening a file.'

Signed-off-by: alex-z <blackslayer4@gmail.com>
@allexzander allexzander force-pushed the bugfix/local-editing-sync-and-loading-indicator branch from 6ab1faf to 7672cb7 Compare October 10, 2022 14:13
@nextcloud-desktop-bot
Copy link

AppImage file: nextcloud-PR-4990-7672cb79036a2e1c22e1d6a07546ac92d34638b3-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

@sonarcloud
Copy link

sonarcloud bot commented Oct 10, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

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

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@allexzander allexzander merged commit 35dec82 into master Oct 10, 2022
@allexzander allexzander deleted the bugfix/local-editing-sync-and-loading-indicator branch October 10, 2022 14:57
@allexzander
Copy link
Contributor Author

/backport to stable-3.6

@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Oct 17, 2022
Closed
@mgallien mgallien added this to the 3.7.0 milestone Oct 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mgallien mgallien mgallien approved these changes

@claucambra claucambra Awaiting requested review from claucambra

@camilasan camilasan Awaiting requested review from camilasan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.7.0
Development

Successfully merging this pull request may close these issues.

Enhancements to "edit locally" feature
3 participants
@allexzander @nextcloud-desktop-bot @mgallien