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

[addons] rework repository updating #7874

Merged
merged 8 commits into from
Sep 7, 2015
Merged

Conversation

tamland
Copy link
Member

@tamland tamland commented Aug 23, 2015

This mainly changes the current global updating job to one per repository. Each repository can now be updated individually. For simplicity the auto updating still update all at the same time to "synchronize" the update times for the next update. This can be changed in the future, possibly adding more options for how/when to update.

It now actually schedule updates using a timer instead of continuously checking in the main loop, which removes a lot of unnecessary updating and workarounds.

Additionally, it decouples repository updating from addon updating and installing so that there is one way to install updates instead of two.

TODO:
The progress dialog doesn't contain any useful info (nor progress) atm. This is natural consequence of making the updates concurrent. Should probably be changed to a infinite spinner or something.

@tamland tamland added Type: Fix non-breaking change which fixes an issue Type: Improvement non-breaking change which improves existing functionality WIP PR that is still being worked on labels Aug 23, 2015
@tamland tamland force-pushed the repo_updater branch 4 times, most recently from d6be0a7 to 61b8593 Compare August 30, 2015 09:26
#empty strings from id 24148 to 24991
#empty strings from id 24148 to 24990

#: xbmc/filesystem/AddonsDirectory.cpp

This comment was marked as spam.

@da-anda
Copy link
Member

da-anda commented Sep 1, 2015

I'm wondering if the error message needs to be an OK dialogue - IIRC other errors only trigger a kaitoast

@tamland
Copy link
Member Author

tamland commented Sep 1, 2015

@da-anda Examples? Can change it, but using an OK dialog is at least consistent with browsing unavailable remote folders. The dialog is only shown when you try to enter an unavailable repository, it's not any background action that will steal focus or anything.

@tamland tamland force-pushed the repo_updater branch 2 times, most recently from 5b27ae5 to 50950cd Compare September 1, 2015 15:25
@tamland tamland removed the WIP PR that is still being worked on label Sep 1, 2015
@tamland tamland added this to the Jarvis 16.0-alpha3 milestone Sep 1, 2015
@da-anda
Copy link
Member

da-anda commented Sep 1, 2015

@tamland thanks for adding the contextual note. Ignore my comment regarding the dialog OK - I mixed things up. Seems consistent as is.

@tamland
Copy link
Member Author

tamland commented Sep 2, 2015

Needs xcode.

And @Montellese for review? Didn't you poke in here recently?

{
//Unbroken
CLog::Log(LOGDEBUG, "CRepositoryUpdateJob[%s] addon '%s' unbroken '%s'",
m_repo->ID().c_str(), addon->ID().c_str());

This comment was marked as spam.

@mkortstiege
Copy link
Member

Please pick mkortstiege@0716a87 for the xcode sync.

@Montellese
Copy link
Member

I still need to review 31a3ea7 but that probably won't happen today.

@Montellese
Copy link
Member

Spotted a few minors. Apart from that it's not easy to review but it looks ok.

@tamland tamland force-pushed the repo_updater branch 2 times, most recently from d316a9c to 5d4ffc0 Compare September 3, 2015 13:13
@tamland
Copy link
Member Author

tamland commented Sep 3, 2015

Ok, hopefully I covered everything. I also moved LastRepoUpdate to repository updater as suggested.

jenkins build this please

@tamland
Copy link
Member Author

tamland commented Sep 3, 2015

jenkins build this please

@Montellese
Copy link
Member

Failed on win32 due to int <--> enum conversions.

@tamland
Copy link
Member Author

tamland commented Sep 3, 2015

Hm, I don't understand this error. I'm not converting from int to enum..

@hudokkow
Copy link
Member

hudokkow commented Sep 4, 2015

jenkins build this please

1 similar comment
@tamland
Copy link
Member Author

tamland commented Sep 4, 2015

jenkins build this please

@tamland
Copy link
Member Author

tamland commented Sep 4, 2015

@Montellese, @Paxxi @otherwindowsdevs Can you please have a look. win32 refuse to compile that enum.

{
OK,
NOT_MODIFIED,
ERROR

This comment was marked as spam.

tamland and others added 7 commits September 4, 2015 12:08
notify updater every time the list of active repositories change
remove unnecessary waiting for updates to complete.
properly ask for an update when repository data is invalid/never
fetched, and show info dialog instead of silently failing
@tamland
Copy link
Member Author

tamland commented Sep 4, 2015

lets see, jenkins build and merge

@tamland
Copy link
Member Author

tamland commented Sep 4, 2015

jenkins build this please

@tamland
Copy link
Member Author

tamland commented Sep 4, 2015

Good to go?

@MartijnKaijser
Copy link
Member

@Montellese good to go?

@Montellese
Copy link
Member

Haven't fully reviewed anymore after the fixes but it looks like my concerns have been addressed so yes.

tamland added a commit that referenced this pull request Sep 7, 2015
[addons] rework repository updating
@tamland tamland merged commit c889533 into xbmc:master Sep 7, 2015
@popcornmix
Copy link
Member

There is some evidence that this PR causes bad effects when a non-responding repo is present.
See: http://forum.kodi.tv/showthread.php?tid=231092&pid=2111696#pid2111696

@mkortstiege
Copy link
Member

As wrote on slack, tamland@61121d7 is causing this. Due to this one it's not possible to install from .zip.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Fix non-breaking change which fixes an issue Type: Improvement non-breaking change which improves existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants