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

Allow setting custom async progress callback timeout #725

Closed
wants to merge 2 commits into from

Conversation

GeorgesStavracas
Copy link
Contributor

@GeorgesStavracas GeorgesStavracas commented Mar 8, 2017

Flatpak is becoming a thing, and it makes heavy usage of OSTree, and GNOME Software allows managing Flatpak application. That exposed an issue where the progress is updated only once every 1 second, which works well for command line but not for so well for UIs.

This PR fixes that by adding an option to set the timeout to call the async progress callback.

@rh-atomic-bot
Copy link

Can one of the admins verify this patch?
I understand the following commands:

  • bot, add author to whitelist
  • bot, test pull request
  • bot, test pull request once

@@ -2589,6 +2589,7 @@ reinitialize_fetcher (OtPullData *pull_data, const char *remote_name, GError **e
* * override-url (s): Fetch objects from this URL if remote specifies no metalink in options
* * inherit-transaction (b): Don't initiate, finish or abort a transaction, usefult to do mutliple pulls in one transaction.
* * http-headers (a(ss)): Additional headers to add to all HTTP requests
* * update-frequency (i): Frequency to call the async progress callback, if any; only values higher than 0 are valid

Choose a reason for hiding this comment

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

You probably want to say "in milliseconds" here, just to make things clear

And why make it "i" if you only allow nonnegative values ?

@GeorgesStavracas
Copy link
Contributor Author

I'm not sure how my changes are related to the test failures...

@cgwalters
Copy link
Member

bot, add author to whitelist

@@ -2613,6 +2614,7 @@ ostree_repo_pull_with_options (OstreeRepo *self,
char **configured_branches = NULL;
guint64 bytes_transferred;
guint64 end_time;
guint update_frequency;
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be initialized to 0, right? In case we don't unpack anything.

When using Flatpak with GNOME Software, it is important to
show the progress of the download and install as close as
possible to the real progress.

However, OSTree forces the frequency to call the async
progress callback to 1 second, which causes an unpleasant
effect on the UI, specially when the download size is so
small that everything happens in less than 1 second.

Fix that by adding making OSTree read a custom 'update-frequency'
option and set the timeout source timeout to that. If
no custom frequency is passed, we assume the default 1
second timeout, maintaining the current behavior.
@GeorgesStavracas
Copy link
Contributor Author

Fixed the build errors (thanks @jlebon!)

@@ -252,6 +254,9 @@ ostree_builtin_pull (int argc, char **argv, GCancellable *cancellable, GError **
g_variant_builder_add (&builder, "{s@v}", "depth",
g_variant_new_variant (g_variant_new_int32 (opt_depth)));

g_variant_builder_add (&builder, "{s@v}", "update-frequency",
g_variant_new_variant (g_variant_new_int32 (opt_frequency)));
Copy link
Member

Choose a reason for hiding this comment

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

s/int32/uint32/

After commit 80b3edc introducing the option to set
a custom timeout, adapt the ostree program to be able to update
that.
@jlebon
Copy link
Member

jlebon commented Mar 8, 2017

Looks sane to me. Just one small nit, and then I think we can get this in!

I'm not sure how my changes are related to the test failures...

Yeah, the Travis tester is a bit flaky unfortunately.

@GeorgesStavracas
Copy link
Contributor Author

Nice catch, @jlebon. Updated the PR

@jlebon
Copy link
Member

jlebon commented Mar 8, 2017

@rh-atomic-bot r+ 9d3aa95

@rh-atomic-bot
Copy link

⌛ Testing commit 9d3aa95 with merge bb3a0e3...

rh-atomic-bot pushed a commit that referenced this pull request Mar 8, 2017
After commit 80b3edc introducing the option to set
a custom timeout, adapt the ostree program to be able to update
that.

Closes: #725
Approved by: jlebon
@rh-atomic-bot
Copy link

☀️ Test successful - status-atomicjenkins
Approved by: jlebon
Pushing bb3a0e3 to master...

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

5 participants