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

Change splash db upgrade logic #10731

Merged
merged 1 commit into from Oct 27, 2016

Conversation

@phate89
Copy link
Contributor

commented Oct 19, 2016

Change the splash screen to show the message of upgrading database only when we're actually upgrading a database.

Description

Motivation and Context

The old method wassn't aware if there was some upgrade and what db was doing the upgrade

How Has This Been Tested?

Tested upgrading my old mysql databases

Screenshots (if appropriate):

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the Code guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the CONTRIBUTING document
  • I have added tests to cover my change
  • All new and existing tests passed

@phate89 phate89 force-pushed the phate89:update_db branch from f1ca47f to 77f9b00 Oct 19, 2016

@phate89

This comment has been minimized.

Copy link
Contributor Author

commented Oct 19, 2016

@Paxxi @DaveTBlake for review/test

@DaveTBlake

This comment has been minimized.

Copy link
Member

commented Oct 19, 2016

A welcome follow on from #10483 that only shows when migration tasks happening rather than every start up. @MilhouseVH, @tamland and @fritsch may be interested in this too.

#. Localized addon database name for splash screen
#: xbmc/addons/AddonDatabase.h
msgctxt "#24152"
msgid "Addon"

This comment has been minimized.

Copy link
@phil65

phil65 Oct 19, 2016

Member

nitpick: Normally we use "Add-on" instead of "Addon".

This comment has been minimized.

Copy link
@MartijnKaijser

MartijnKaijser Oct 19, 2016

Member

You might as well repeat the complete sentence each time instead of stitching the strings together.

This comment has been minimized.

Copy link
@DaveTBlake

DaveTBlake Oct 20, 2016

Member

The other view is that storing the same text over and over is wasteful.
But is it a translation issue e.g. in some languages the object needs to go else where in the phrase?

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2016

Thanks @phate89 - it works well, I've tested upgrading MyMusic50 to MyMusic60 and MyVideos90 to MyVideos107, the latter is particularly time consuming. I'll include this PR in future builds.

There does however remain one issue: when <splash>false</splash> is added to advancedsettings.xml, there is no splash displayed, and also no migration messages (even when Music and Videos databases are being migrated). All the user sees is a totally blank (black) screen, with no messages.

IMHO the splash should be forced to appear along with the migration messages, regardless of the <splash> setting, whenever a migration is taking place.

One other nitpick would be to position the text a little higher, just in case of any overscanning or weird resolution issues - it seems a little unnecessary to position the text quite so low down (it appears to be close to the bottom edge of my 1920x1080 1:1 pixel screen).

@DaveTBlake

This comment has been minimized.

Copy link
Member

commented Oct 19, 2016

+1
Tested with a large MyMusic48 to MyMusic60.

I too would put the text a little higher, and probably shouldn't capatalise "Database", e.g.
"Music database migration in progress - please wait..."
But minor stuff, and I would happily live with it.

@ksooo ksooo added the Type: Fix label Oct 20, 2016

@phate89 phate89 force-pushed the phate89:update_db branch from 77f9b00 to 29c0849 Oct 20, 2016

@phate89

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2016

I updated the pr.. Now we update the text adding dot every second..
If someone is familiar with our CThread should check my code...

@phate89 phate89 force-pushed the phate89:update_db branch from 29c0849 to eec784c Oct 20, 2016

@tamland

This comment has been minimized.

Copy link
Member

commented Oct 20, 2016

Sorry but not liking how this is currently implemented. The database should not have a dependency on the global application messenger. Instead it should be passed in as a callback/another agnostic structure, or the entire migration refactored and uncoupled from the database. This is why I didn't bothered with it in the first place.

Sleep(1000);
}
}
Show();

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Oct 20, 2016

Member

Show must only be called by the render thread.

This comment has been minimized.

Copy link
@phate89

phate89 Oct 20, 2016

Author Contributor

@FernetMenta I started with something simpler and I moved to something out of my area.. Thanks for checking the pr.. Can you point me to an example how I can I call it every second from render thread? I really know nothing about this part..

This comment has been minimized.

Copy link
@Paxxi

Paxxi Oct 20, 2016

Member

You can use app messenger and post a guide message, those are handled on the correct thread. Fernet might have a better idea though

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Oct 20, 2016

Member

This is dead end and it should become clearer now that the approach of showing upgrade info with CSplash was an abuse of this class. The splash image is shown on init before the main loop gets active. This is also before app messenger can be used.
I recommend to revert the upgrade messages and start from scratch.

This comment has been minimized.

Copy link
@phate89

phate89 Oct 20, 2016

Author Contributor

As I said it's out of my area and I have n idea where to start so starting from scratch is probably too hard for me.. I'll close the pr so if someone wants to try it can..

@Paxxi

This comment has been minimized.

Copy link
Member

commented Oct 20, 2016

@tamland trust that passing in a callback could be cleaner but more work. This is a step in the right direction and app messenger is purposely designed for this with very few dependencies itself.
Any work that removes some lower level dependency of a higher level is progress and I encourage it. Will review the implementation later when I'm at a computer

@phate89

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2016

@tamland I added a new commit that moves the update part (the same update procedure, no refactor for now) to databasemanager. So only databasemanager has a dependency on applicationmessenger..Is it better?

@phate89 phate89 closed this Oct 20, 2016

@phate89 phate89 reopened this Oct 21, 2016

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Oct 21, 2016

you could offload the migrations tasks to an extra thread. then the main thread can wait in init for this task to be completed and display splash and messages accordingly.

@phate89

This comment has been minimized.

Copy link
Contributor Author

commented Oct 21, 2016

@FernetMenta thanks for the suggestions. Any help is appreciated.
The main problem wasn't to actually update from main thread but update the text every second to keep something moving.
You are suggesting move the loop in capplication? Something like

CDatabaseManager::GetInstance().StartInitialize();
while (CDatabaseManager::GetInstance().IsRunning())
{
  CSplash::GetInstance().Show(updatedMsg);
  if (CDatabaseManager::GetInstance().IsRunning())
    sleep (1000);
}
@FernetMenta

This comment has been minimized.

Copy link
Member

commented Oct 22, 2016

@phate89 yes, this will hold processing of the main thread until upgrade has finished. This is intended behaviour.

@phate89 phate89 force-pushed the phate89:update_db branch 2 times, most recently from 95190fc to 2beef54 Oct 22, 2016

@phate89

This comment has been minimized.

Copy link
Contributor Author

commented Oct 22, 2016

@FernetMenta I updated the pr.. I moved the update logic to the db manager and moved the db upgrade and the addon migration to a separate thread (If I understood well our cthread)

@phate89 phate89 force-pushed the phate89:update_db branch from 2beef54 to ba69721 Oct 22, 2016

if (CDatabaseManager::GetInstance().IsRunning())
Sleep(1000);
}
}

This comment has been minimized.

Copy link
@tamland

tamland Oct 22, 2016

Member

This is a job for std::future. Make MigrateAddons return one when migration happens and wait for it to complete here.

This comment has been minimized.

Copy link
@phate89

phate89 Oct 22, 2016

Author Contributor

Is it supported also in Android?

@tamland

This comment has been minimized.

Copy link
Member

commented Oct 22, 2016

Not exactly thread safe.. Try creating a task and send it off with CJobManager::Submit instead, or spawn a new thread; don't inherit from Cthread. Things like CAddonSystemSettings shouldn't be a thread.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Oct 22, 2016

@phate89 using CThread may work but is not the best choice. What you really want to do is spawning an upgrade thread if required. Look into CJob and CJobManager. Those classes do what you need here.

EDIT : cross post :) @tamland had the same idea

@phate89

This comment has been minimized.

Copy link
Contributor Author

commented Oct 22, 2016

Thanks from the tip.. it'' quite nice ttt see we already have so many classes ready for several tasks.. I''l look into CJobManager and report back

@phate89 phate89 force-pushed the phate89:update_db branch from ba69721 to 786083b Oct 22, 2016

@phate89

This comment has been minimized.

Copy link
Contributor Author

commented Oct 22, 2016

@tamland @FernetMenta I updated the pr using CJobManager::Submit. I hope I got it right this time :|

{
if (CDatabaseManager::GetInstance().m_bIsUpgrading)
CSplash::GetInstance().Show(std::string(i, ' ') + localizedStr + std::string(i, '.'));
if (notFinished)

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Oct 23, 2016

Member

is is already a condition of the for loop. why do you test again?

This comment has been minimized.

Copy link
@phate89

phate89 Oct 23, 2016

Author Contributor

I added it because the work might end while drawing the message and might avoid to wait 1 sec in that case.. not sure if it's worth it

{
for (int i = 1; i <= 3 && notFinished; ++i)
{
if (CDatabaseManager::GetInstance().m_bIsUpgrading)

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Oct 23, 2016

Member

what is the difference between m_bIsUpgrading and notFinished?

This comment has been minimized.

Copy link
@phate89

phate89 Oct 23, 2016

Author Contributor

notFinished is about the whole procedure. Not only the actual upgrade but also checking if there iis an old db and copying if we don't find one. Isupgrading is when the actual migration from an old one to a new one happens. That's the slow part we want to display.
There are 2 different because there's no simple check ahead of time if we need to do some upgrade. For every database we get back version by version to find if there is a ddatabase. If the upgrade fails we keep going back..

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Oct 24, 2016

Looks ok from my pov. I can't say much about the upgrade itself

@phate89 phate89 force-pushed the phate89:update_db branch from 06ccbe0 to a184181 Oct 24, 2016

@phate89

This comment has been minimized.

Copy link
Contributor Author

commented Oct 24, 2016

jenkins build this please

@phate89 phate89 force-pushed the phate89:update_db branch 2 times, most recently from f9e72b7 to 438e7fc Oct 24, 2016

@tamland

This comment has been minimized.

Copy link
Member

commented Oct 24, 2016

Getting closer:) but you need to use std::future or events, and wait. With the sleeps the migration is up to 1 second slower than it need to be.

@@ -141,13 +141,15 @@ std::vector<std::string> CAddonSystemSettings::MigrateAddons(std::function<void(

if (CSettings::GetInstance().GetInt(CSettings::SETTING_ADDONS_AUTOUPDATES) == AUTO_UPDATES_ON)
{
onMigrate();
m_bIsMigratingAddons = true;

This comment has been minimized.

Copy link
@tamland

tamland Oct 24, 2016

Member

This is unnecessary. You can achieve exactly the same by using the callback, without the need for a public member. In the callback you a set a bool that belongs to CApplication. And since you have the notFinished bool, it's not necessary to set anything to false at the end.

@phate89

This comment has been minimized.

Copy link
Contributor Author

commented Oct 24, 2016

@tamland
I know waiting is better but we also want to update the text every second to show some progress. It's something that can take several minutes so moving dots helps to see we're not stuck.

@tamland

This comment has been minimized.

Copy link
Member

commented Oct 24, 2016

That's what it does. If you pass 1 second to the wait method it returns ready or timeout after 1 second. If it's ready before that it will be interrupted.

@phate89 phate89 force-pushed the phate89:update_db branch from 438e7fc to 67c2cf9 Oct 24, 2016

@phate89

This comment has been minimized.

Copy link
Contributor Author

commented Oct 24, 2016

@tamland I'm dumb of course you're right.. i updated it using our CEvent class

@@ -1123,8 +1124,22 @@ bool CApplication::Initialize()
g_curlInterface.Unload();

// initialize (and update as needed) our databases
CSplash::GetInstance().Show(g_localizeStrings.Get(24150));
CDatabaseManager::GetInstance().Initialize();
CEvent event;

This comment has been minimized.

Copy link
@tamland

tamland Oct 24, 2016

Member

To avoid race condition this should be set to manual reset, ie. CEvent event(true). Then I think we're good:)

@phate89 phate89 force-pushed the phate89:update_db branch from 67c2cf9 to 5c5b448 Oct 24, 2016

@phate89

This comment has been minimized.

Copy link
Contributor Author

commented Oct 24, 2016

now it should be ok.. thanks @FernetMenta @tamland for all the help..
jenkins build this please

@phate89 phate89 force-pushed the phate89:update_db branch from 5c5b448 to f344ca0 Oct 25, 2016

@MartijnKaijser MartijnKaijser added this to the Krypton 17.0-beta5 milestone Oct 25, 2016

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Oct 25, 2016

win32 upgrade from v14 and all is fine. thx
jenkins build this please.

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Oct 26, 2016

@FernetMenta @tamland if all good please merge :)

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Oct 26, 2016

fine by me, @tamland your button

@tamland tamland merged commit 7d97806 into xbmc:master Oct 27, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins4kodi You did a great job. Have a cookie.
Details

@phate89 phate89 deleted the phate89:update_db branch Nov 30, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.