Skip to content

Improve automatic synchronization process#4

Closed
przybylski wants to merge 6 commits into
masterfrom
configurable_sync_time
Closed

Improve automatic synchronization process#4
przybylski wants to merge 6 commits into
masterfrom
configurable_sync_time

Conversation

@przybylski

@przybylski przybylski commented Jun 11, 2016

Copy link
Copy Markdown
Member

Currently time between synchronization is constant and is equal to 3h. Let's give users freedom to specify the time between synchronizations.

@tobiasKaminsky

Copy link
Copy Markdown
Member

Great idea!
To avoid user complaining that syncing is too often we should limit to a minimum of 60 minutes?

@AndyScherzinger

Copy link
Copy Markdown
Member

Also think this is a good idea and yes 60 minutes being the absolute lowest end configurable is the right thing to do.

What do you think @jancborchardt from a UX perspective. ATM the preference would end up in the Details section.

@przybylski

Copy link
Copy Markdown
Member Author

Personally I wouldn't restrict users to set any non negative value they want. But if I get outvoted I'll change it

@tobiasKaminsky

Copy link
Copy Markdown
Member

Or we could show a little warning/info that a value <60 could be battery consumption.

@jancborchardt

Copy link
Copy Markdown
Member

Hm, a custom setting like that seems strange. Is there no more intelligent way to solve this without people needing to fiddle with settings. Cause if people change it and are still unhappy with the results, people will feel stupid or frustrated. If the setting is not there in the first place, that’s less likely.

@przybylski

Copy link
Copy Markdown
Member Author

So, drop?

@tobiasKaminsky

Copy link
Copy Markdown
Member

The more "intelligent" approach is maybe the best way to go?
So to get a better understanding: why is it fixed to 3h? A balance between battery consumption and update frequency?
Maybe something like:

  • measure sync process
  • if > x minutes then +1h pause otherwise decrease sync time

@b1ggjoe

b1ggjoe commented Jun 14, 2016

Copy link
Copy Markdown

Is there a way to offer a choice? For Example:

Sync Frequency:

  • Instant
  • Every 5 Minutes
  • Every 15 Minutes
  • Every 30 Minutes
  • Every Hour

With another option for 'Manual' sync on demand?

J.

On Tuesday, June 14, 2016, Tobias Kaminsky notifications@github.com wrote:

The more "intelligent" approach is maybe the best way to go?
So to get a better understanding: why is it fixed to 3h? A balance between
battery consumption and update frequency?
Maybe something like:

  • measure sync process
  • if > x minutes then +1h pause otherwise decrease sync time


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#4 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/ALfB7z_b5TckoB5HMtQTxqZxOl_SM50yks5qLwbygaJpZM4IzfxQ
.

@przybylski

Copy link
Copy Markdown
Member Author

Instant as in? generally modified files should be uploaded to server but I am not sure how this function evolved over the time.
For more intelligent way. there is a lot of approaches which can be taken, from probing, not syncing when user is sleeping, etc.
There are a lot of paths to explore.

Why it was 3hrs? Random number :P i didn't give too much thinking when setting the sync time gap for 3h.

@ghost

ghost commented Jun 20, 2016

Copy link
Copy Markdown

The choice should be given imho.
A simple warning that setting it too low could cause battery drain should be enough, users will be smart enough not to set it too low.

@jancborchardt

Copy link
Copy Markdown
Member

@tobiasKaminsky yes, I’d prefer we go for a more intelligent approach. This is exactly details that the app should take care of and no user should need to fiddle with. :)

@tobiasKaminsky

Copy link
Copy Markdown
Member

But currently I am cluless how we can implement such an intelligent mechanism.

@AndyScherzinger

Copy link
Copy Markdown
Member

There would have to be a more or less smart algorithm taking the files changed frequency into account. In case at least one files change is tracked within an hour the sync can happen every hour, if files change less frequently raise the time between detected changes. This needs quite some change occurrence tracking.

@jancborchardt

Copy link
Copy Markdown
Member

Would really be cool if we can have something like that, at least step by step. Because introducing a setting for it results in the 2 big problems which are always there on introducing settings for every little thing:

  1. They obscure the other, actually important settings.
  2. They are very difficult to remove once introduced and people will complain even if the new solution is better and more automatic.

@AndyScherzinger

Copy link
Copy Markdown
Member

That's true but we would also have to think this through a little more even if we start step by step. So we need to have an answer how such an algorithm would play nicely with other sync relevant things like WiFi and Charging because having features like that for instant upload but then ignoring this for sync might confuse users.

@AndyScherzinger
AndyScherzinger force-pushed the configurable_sync_time branch 4 times, most recently from 927b0f5 to 437ee61 Compare August 8, 2016 22:02
@AndyScherzinger
AndyScherzinger force-pushed the configurable_sync_time branch 3 times, most recently from 00d5dc1 to adc723f Compare August 18, 2016 14:14
@AndyScherzinger
AndyScherzinger force-pushed the configurable_sync_time branch 4 times, most recently from d3888f6 to 00afde0 Compare September 8, 2016 11:21
@AndyScherzinger
AndyScherzinger force-pushed the configurable_sync_time branch 3 times, most recently from 81043c7 to 5d69079 Compare April 28, 2017 08:48
@tobiasKaminsky

Copy link
Copy Markdown
Member

How to deal with this?
To summarize, we want to have a (smart) approach and not have the user customize it.

@mario @AndyScherzinger @przybylski

@jancborchardt jancborchardt changed the title Allow user to specify time between automatic synchronizations Improve automatic synchronization process Jul 3, 2017
@jancborchardt

Copy link
Copy Markdown
Member

What’s the current state of this then? It should definitely not be a setting, yes.

@mario

mario commented Jul 3, 2017

Copy link
Copy Markdown
Contributor

@jancborchardt no state. We can talk about it for 2.0. The entire sync stuff needs to be changed anyway - let's first get proper auto upload (new files), then proper synchronization of files/folders inside our directory and then we'll talk all the other details.

@mario mario mentioned this pull request Sep 5, 2017
@AndyScherzinger
AndyScherzinger force-pushed the configurable_sync_time branch 2 times, most recently from df5d63c to ac76361 Compare June 12, 2018 15:31
@codecov

codecov Bot commented Jun 12, 2018

Copy link
Copy Markdown

Codecov Report

Merging #4 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##             master      #4      +/-   ##
===========================================
- Coverage      6.49%   6.47%   -0.02%     
  Complexity        1       1              
===========================================
  Files           307     307              
  Lines         30075   30105      +30     
  Branches       4314    4318       +4     
===========================================
- Hits           1952    1950       -2     
- Misses        27831   27862      +31     
- Partials        292     293       +1
Impacted Files Coverage Δ Complexity Δ
.../com/owncloud/android/ui/activity/Preferences.java 0% <0%> (ø) 0 <0> (ø) ⬇️
.../owncloud/android/syncadapter/FileSyncAdapter.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...in/java/com/owncloud/android/datamodel/OCFile.java 63.03% <0%> (-0.78%) 0% <0%> (ø)

@AndyScherzinger

Copy link
Copy Markdown
Member

I did a (quite complex and time consuming) rebase of this PR to have it up to date with the master again for our toolchain.

We should though decide on what to do, since keeping very long living PRs alive it close to impossible and adds no value, plus for housekeeping reasons I also vote to delete any branch that won't be used in the next month, same for PRs.

What do you think folks?

@jancborchardt

Copy link
Copy Markdown
Member

From the user perspective it’s still the same situation, as mentioned above and @tobiasKaminsky also mentioned, there should be a more intelligent approach to this than a setting. It’s too technical and sync frequency is really not something any user (however technical) should need to fiddle with.

Basically, we need to have a proper discussion and specification about this before a pull request.

@mario

mario commented Jun 13, 2018

Copy link
Copy Markdown
Contributor

This is related to sync adapter I guess. I've always planned on removing it and syncing on demand WHEN needed instead (either in-jobs or when you visit a directory).

Sure, pre-fetching is sometimes good, but at the current state it sometimes works and sometimes doesn't which is quite bad for the user experience too.

@AndyScherzinger
AndyScherzinger force-pushed the configurable_sync_time branch 4 times, most recently from 5d4420e to ed6a48e Compare June 28, 2018 10:18
@nextcloud-android-bot

Copy link
Copy Markdown
Collaborator

Lint

TypeMasterPR
Warnings113113
Errors

FindBugs (new)

Warning TypeNumber
Bad practice Warnings35
Correctness Warnings167
Internationalization Warnings14
Malicious code vulnerability Warnings10
Multithreaded correctness Warnings9
Performance Warnings124
Security Warnings202
Dodgy code Warnings139
Total700

FindBugs (master)

Warning TypeNumber
Bad practice Warnings35
Correctness Warnings167
Internationalization Warnings14
Malicious code vulnerability Warnings10
Multithreaded correctness Warnings9
Performance Warnings124
Security Warnings202
Dodgy code Warnings139
Total700

@tobiasKaminsky

Copy link
Copy Markdown
Member

With #2886 there is no real need of having an automated sync as we remove "keep available offline", so this can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants