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

Throttle ui updates #9863

Merged
merged 6 commits into from
Jul 6, 2022
Merged

Throttle ui updates #9863

merged 6 commits into from
Jul 6, 2022

Conversation

TheOneRing
Copy link
Member

Issue: #9832

@TheOneRing TheOneRing requested review from dragotin and a team July 5, 2022 16:17
@TheOneRing TheOneRing marked this pull request as draft July 5, 2022 16:27
@TheOneRing
Copy link
Member Author

Some of the stuff in there really needs to run on an item level.
I have to take another look

@TheOneRing TheOneRing added this to the 2.10.2 milestone Jul 5, 2022
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.

Almost nice :)

// every connect has its own progressUpdated
connect(f, &Folder::progressInfo, this, [progressUpdated = std::make_unique<std::chrono::system_clock::time_point>(std::chrono::system_clock::now()), f, this](const ProgressInfo &info) mutable {
// throttle how often we update the progress
if (std::chrono::system_clock::now() - *progressUpdated > progressUpdateTimeOutC) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, I suggest using QElapsedTimer class. More clean imho.

@TheOneRing TheOneRing requested a review from a team July 6, 2022 08:54
@TheOneRing TheOneRing marked this pull request as ready for review July 6, 2022 08:54
@TheOneRing TheOneRing force-pushed the work/9832 branch 2 times, most recently from 1607e47 to 7fc04b0 Compare July 6, 2022 08:58
@ownclouders
Copy link
Contributor

ownclouders commented Jul 6, 2022

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.

Yeah.

roles << FolderStatusDelegate::SyncProgressItemString
<< FolderStatusDelegate::WarningCount
<< Qt::ToolTipRole;
const QVector<int> roles = { FolderStatusDelegate::SyncProgressItemString, FolderStatusDelegate::WarningCount, Qt::ToolTipRole };

if (progress.status() == ProgressInfo::Discovery) {
if (!progress._currentDiscoveredRemoteFolder.isEmpty()) {
pi->_overallSyncString = tr("Checking for changes in remote '%1'").arg(progress._currentDiscoveredRemoteFolder);
emit dataChanged(index(folderIndex), index(folderIndex), roles);
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should not check if progress.status() is different than the status that is stored currently. If it is not different we could save the dataChanged() emit completely, or not?

Copy link
Member Author

Choose a reason for hiding this comment

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

Those states are only emitted once per discovery.
Only Propagation or Done are emitted over and over again

// progress updates are expensive, throtle them
if (std::chrono::system_clock::now() - _lastProgressUpdated > progressUpdateTimeOutC) {
computeProgress(progress, pi);
_lastProgressUpdated = std::chrono::system_clock::now();
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, there is this QElapsedTimer which is very nice and neat...

Copy link
Member Author

Choose a reason for hiding this comment

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

QElapsedTimer does not support std::chrono so I would need to perform an ugly duration cast on the qint64 returned by it or on our progressUpdateTimeOutC.
And in the end we would replace a subtraction with lots of casts.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, progressUpdateTimeOutC would be a qint64, which would result in more clear code for me. But... yes.

changelog/unreleased/9836 Outdated Show resolved Hide resolved
changelog/unreleased/9832 Outdated Show resolved Hide resolved
src/gui/folderstatusmodel.cpp Show resolved Hide resolved
src/gui/folderstatusmodel.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@fmoc fmoc left a comment

Choose a reason for hiding this comment

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

Looks good. I have just one more suggestion. We should use acronyms more consistently.

Comment on lines +1 to +3
Enhancement: Run vfs downloads with a high priority

This should reduce the probability for timeouts when downloading vfs files in the Windows explorer.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Enhancement: Run vfs downloads with a high priority
This should reduce the probability for timeouts when downloading vfs files in the Windows explorer.
Enhancement: Run VFS downloads with a high priority
This should reduce the probability for timeouts when downloading VFS files in the Windows explorer.

@sonarcloud
Copy link

sonarcloud bot commented Jul 6, 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 7 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@TheOneRing TheOneRing merged commit ab8f9e3 into 2.10 Jul 6, 2022
@delete-merged-branch delete-merged-branch bot deleted the work/9832 branch July 6, 2022 16:57
TheOneRing added a commit that referenced this pull request Jul 14, 2022
In #9863 we started throtteling the the ui updates during sync, this throttles the updates even more.
TheOneRing added a commit that referenced this pull request Jul 14, 2022
In #9863 we started throtteling the the ui updates during sync, this throttles the updates even more.
TheOneRing added a commit that referenced this pull request Jul 20, 2022
In #9863 we started throtteling the the ui updates during sync, this throttles the updates even more.
TheOneRing added a commit that referenced this pull request Jul 20, 2022
In #9863 we started throtteling the the ui updates during sync, this throttles the updates even more.
TheOneRing added a commit that referenced this pull request Jul 21, 2022
In #9863 we started throtteling the the ui updates during sync, this throttles the updates even more.
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.

None yet

4 participants