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 splash to be JPG also #13289

Merged
merged 3 commits into from Jan 4, 2018

Conversation

@pkerling
Copy link
Member

commented Jan 2, 2018

Description

Load Splash.jpg before Splash.png

Motivation and Context

To have more fancy splash screens, allow loading from JPG instead of PNG for reasonable file sizes

How Has This Been Tested?

Screenshots (if appropriate):

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Improvement (non-breaking change which improves existing functionality)
  • 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
@pkerling pkerling added this to the L 18.0-alpha1 milestone Jan 2, 2018
@pkerling pkerling self-assigned this Jan 2, 2018
@pkerling pkerling requested review from koying and Memphiz Jan 2, 2018
@pkerling

This comment has been minimized.

Copy link
Member Author

commented Jan 2, 2018

@mkortstiege This drops GUIWindowSplash that you added some years ago. Can you assess whether it's OK to drop it now or do we still need it?

@@ -3,7 +3,7 @@ xbmc/assets/*
xbmc/lib/*
xbmc/libs/*
xbmc/obj/*
xbmc/res/drawable/splash.png
xbmc/res/drawable*/splash.*

This comment has been minimized.

Copy link
@koying

koying Jan 2, 2018

Contributor

Have to be capital Splash.*, now

@@ -119,13 +119,13 @@ python: | xbmc/assets
res:
mkdir -p xbmc/res xbmc/res/raw xbmc/res/values images
@echo "native_arch=$(CPU)" > xbmc/res/raw/xbmc.properties
cp -fp $(CMAKE_SOURCE_DIR)/media/Splash.png xbmc/res/drawable/splash.png
cp -fp $(CMAKE_SOURCE_DIR)/media/Splash.* xbmc/res/drawable

This comment has been minimized.

Copy link
@koying

koying Jan 2, 2018

Contributor

xbmc/res/drawable/
Same below

@@ -183,7 +183,8 @@ apk-clean:
rm -rf xbmc/obj
rm -rf xbmc/res/raw
rm -rf xbmc/res/values
rm -f xbmc/res/drawable/splash.png
rm -f xbmc/res/drawable/splash.*
rm -f xbmc/res/drawable-xxxhdpi/splash.*

This comment has been minimized.

Copy link
@koying

koying Jan 2, 2018

Contributor

Have to be capital Splash.*, now

@koying

This comment has been minimized.

Copy link
Contributor

commented Jan 2, 2018

You also have to change https://github.com/xbmc/xbmc/blob/master/tools/android/packaging/xbmc/res/layout/activity_splash.xml#L14 to a camel case

android:src="@drawable/Splash" />
@pkerling

This comment has been minimized.

Copy link
Member Author

commented Jan 2, 2018

@koying Very correct, thanks. I kind of want to change it to the lower-case variant everywhere now, but I guess this would only further complicate the situation.

@notspiff

This comment has been minimized.

Copy link
Contributor

commented Jan 2, 2018

it shouldn't complicate much. afaik we look for the file using a "case-fuzzing" function.

@pkerling

This comment has been minimized.

Copy link
Member Author

commented Jan 2, 2018

@notspiff doesn't seem to work sadly. Crashes when I just rename to lowercase.

@mkortstiege

This comment has been minimized.

Copy link
Member

commented Jan 2, 2018

@pkerling I am not entirely sure if the actual "real" window is of any use these days. Back then, we needed it for the master lock and profile handling. We also wanted it for some more advanced progress when doing main app init and database upgrades afaik.

@koying

This comment has been minimized.

Copy link
Contributor

commented Jan 2, 2018

Looks good now. Will test to be sure ;)

std::string CUtil::GetSplashPath()
{
std::array<std::string, 4> candidates {{ "special://home/media/Splash.jpg", "special://home/media/Splash.png", "special://xbmc/media/Splash.jpg", "special://xbmc/media/Splash.png" }};
auto it = std::find_if(candidates.begin(), candidates.end(), [](std::string const& file) { return XFILE::CFile::Exists(file); });

This comment has been minimized.

Copy link
@notspiff

notspiff Jan 2, 2018

Contributor

pass this through CSpecialProtocol::TranslatePathConvertCase, or the better but more risky fix which is to fix CSpecialProtocolFile::TranslatePath

@pkerling

This comment has been minimized.

Copy link
Member Author

commented Jan 2, 2018

@mkortstiege the updates that show text should be handled by CRenderSystemBase I guess?

@mkortstiege

This comment has been minimized.

Copy link
Member

commented Jan 2, 2018

@pkerling

This comment has been minimized.

Copy link
Member Author

commented Jan 2, 2018

Filename changed to lowercase splash everywhere (I hope...)

@koying

This comment has been minimized.

Copy link
Contributor

commented Jan 2, 2018

Would be good to switch to lowercase, as droid does not support drawables with a capital in the name :(
Sorry for the noise...

@da-anda

This comment has been minimized.

Copy link
Member

commented Jan 2, 2018

samfisher is redoing my splash in 3D this very second to make it more awesome, so we might want to wait on his version before we merge it?

edit: or just leave the splash.jpg out for now. Up to you

@koying
koying approved these changes Jan 2, 2018
Copy link
Contributor

left a comment

Screen turns to black during profile changes (not sure how it was before really) but no showstopper

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Jan 2, 2018

Same happens on master so nothing new i think.

@pkerling

This comment has been minimized.

Copy link
Member Author

commented Jan 2, 2018

jenkins build this please

@Memphiz
Memphiz approved these changes Jan 2, 2018
Copy link
Member

left a comment

iOS changes look ok thx

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Jan 2, 2018

jenkins build this please

@xhaggi

This comment has been minimized.

Copy link
Member

commented Jan 2, 2018

We introduced the splash window to stay in background until you logged in to your profile if you had masterlock enabled. #7650 please verify if this is fixed before you remove the window.

@pkerling

This comment has been minimized.

Copy link
Member Author

commented Jan 2, 2018

@xhaggi I tried some combinations of profile selection and master mode but I can't get to the situation you describe. What exactly do I need to do to have the splash window visible in the background on current master?

@xhaggi

This comment has been minimized.

Copy link
Member

commented Jan 2, 2018

Check the forum thread. Enable masterlock with a password or code, restart Kodi, masterlock should appear, then move the mouse around the screen outside the masterlock dialog.

@pkerling

This comment has been minimized.

Copy link
Member Author

commented Jan 2, 2018

Ah I missed the ask for master lock code on startup since it's in a different settings page. Oddly enough this also does not work for me on master, i.e. there is no splash image in the background and the dialogs leave strange trails when closing. Can anyone else check?

@xhaggi

This comment has been minimized.

Copy link
Member

commented Jan 2, 2018

With current master I got the following result. The render issue is back :(

screenshot007

Used Xperience1080 because Estuary uses a full size keyboard dialog.
screenshot006

@pkerling

This comment has been minimized.

Copy link
Member Author

commented Jan 2, 2018

Then what should we do with this PR? It does not make the issue worse, but should I leave in the splash window for now?

@xhaggi

This comment has been minimized.

Copy link
Member

commented Jan 2, 2018

Okay found the commit which re-introduce the rendering issue if master lock is enabled.
9f6eb4d#diff-02cbf300ebd15bda2591e96d935b0f87R1506

Remove the condition (newWindowID != WINDOW_SPLASH) will fix it.

@pkerling you have to leave the CGUIWindowSplash.

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Jan 2, 2018

Perhaps add a code comment why that part is actually there (or some one really fixes itl

@pkerling

This comment has been minimized.

Copy link
Member Author

commented Jan 2, 2018

@xhaggi Are you sure it's OK to just change the else if to else? From the commit it sounds like it was done to fix some other issue.

@xhaggi

This comment has been minimized.

Copy link
Member

commented Jan 2, 2018

I'll add a PR for it and ping the right dev.

EDIT: #13294

@xhaggi

This comment has been minimized.

Copy link
Member

commented Jan 2, 2018

Are you sure it's OK to just change the else if to else? From the commit it sounds like it was done to fix some other issue.

I explain why it's wrong in the PR description.

@xhaggi

This comment has been minimized.

Copy link
Member

commented Jan 2, 2018

Perhaps add a code comment why that part is actually there

We did that while we introduced the the splash window https://github.com/xbmc/xbmc/blob/master/xbmc/Application.cpp#L1126

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Jan 3, 2018

@pkerling can you adjust accordingly now #13294 has been merged?

@pkerling

This comment has been minimized.

Copy link
Member Author

commented Jan 3, 2018

Sure, the verdict is to leave CGUIWindowSplash and the code it duplicates as-is?

@@ -70,6 +69,8 @@ class CUtil
static int GetDVDIfoTitle(const std::string& strPathFile);

static bool IsPicture(const std::string& strFile);
/// Get resolved filesystem location of splash image
static std::string GetSplashPath();

This comment has been minimized.

Copy link
@xhaggi

xhaggi Jan 4, 2018

Member

could you please add a valid doxy comment

This comment has been minimized.

Copy link
@pkerling

pkerling Jan 4, 2018

Author Member

What are you missing here?

This comment has been minimized.

Copy link
@xhaggi

xhaggi Jan 4, 2018

Member
/*!
 \brief Return the resolved filesystem ...
 \return the path ....
 */

This comment has been minimized.

Copy link
@pkerling

pkerling Jan 4, 2018

Author Member

Isn't that the same but longer? /// is picked up by doxygen and used for the brief.

This comment has been minimized.

Copy link
@xhaggi

xhaggi Jan 4, 2018

Member

sure, but be consistent would be appreciated and you could differ the brief from the return description.

This comment has been minimized.

Copy link
@pkerling

pkerling Jan 4, 2018

Author Member

/// is already used all over the place. And honestly I don't know what more I could say in \return. It would be just the same information as in the brief but worded differently, which is not helpful.

This comment has been minimized.

Copy link
@xhaggi

xhaggi Jan 4, 2018

Member

ok leave it if that is such a big thing.

if (!XFILE::CFile::Exists(splashImage))
splashImage = "special://xbmc/media/Splash.png";

std::string splashImage {CUtil::GetSplashPath()};

This comment has been minimized.

Copy link
@xhaggi

xhaggi Jan 4, 2018

Member

What's the reason for the curly braces? why not std::string splashImage = CUtil::GetSplashPath()?

This comment has been minimized.

Copy link
@pkerling

pkerling Jan 4, 2018

Author Member

Matter of taste. I don't really care tbh.

This comment has been minimized.

Copy link
@xhaggi

xhaggi Jan 4, 2018

Member

please be consistent because you use = in IOSExternalTouchController.mm

This comment has been minimized.

Copy link
@pkerling

pkerling Jan 4, 2018

Author Member

Yeah I went for minimal changes there because it was only one line, but in principle you're of course right. I think it would be good to have some of this stuff defined in the coding style guidelines maybe?

@xhaggi

This comment has been minimized.

Copy link
Member

commented Jan 4, 2018

Sure, the verdict is to leave CGUIWindowSplash and the code it duplicates as-is?

Yep, there is some dupe code (CGUIImage) but nothing else because you moved the path logic to CUtil::GetSplashPath().

@pkerling pkerling force-pushed the pkerling:splash-jpg branch from e150a10 to 879b083 Jan 4, 2018

CLog::Log(LOGINFO, "load splash image: %s", CSpecialProtocol::TranslatePath(splashImage).c_str());
std::string splashImage = CUtil::GetSplashPath();
CLog::Log(LOGINFO, "load splash image: %s", splashImage.c_str());

This comment has been minimized.

Copy link
@xhaggi

xhaggi Jan 4, 2018

Member

ahh, could you please remove this line .. I think it's a left-over or change the log level to debug.

This comment has been minimized.

Copy link
@ksooo

ksooo Jan 4, 2018

Member

I would eliminate splashImage variable => CLog::Log(LOGINFO, "load splash image: %s", CUtil::GetSplashPath().c_str());

Or make the var at least const.

This comment has been minimized.

Copy link
@ksooo

ksooo Jan 4, 2018

Member

@xhaggi it was LOGINFO before. So, why do you want to change that?

This comment has been minimized.

Copy link
@xhaggi

xhaggi Jan 4, 2018

Member

The logging of the path makes no sense here. It's not logged in CRenderSystemBase so why should we do it here? I've introduced it in the initial PR so it should be fine if I say drop it ;)

This comment has been minimized.

Copy link
@ksooo

ksooo Jan 4, 2018

Member

okay, I was just curious.

This comment has been minimized.

Copy link
@pkerling

pkerling Jan 4, 2018

Author Member

removed

if (!XFILE::CFile::Exists(splashImage))
splashImage = "special://xbmc/media/Splash.png";

std::string splashImage = CUtil::GetSplashPath();
m_splashImage = std::unique_ptr<CGUIImage>(new CGUIImage(0, 0, 0, 0, g_graphicsContext.GetWidth(),
g_graphicsContext.GetHeight(), CTextureInfo(splashImage)));

This comment has been minimized.

Copy link
@ksooo

ksooo Jan 4, 2018

Member

I would eliminate splashImage variable => CTextureInfo(CUtil::GetSplashPath())

Or make the var at least const.

This comment has been minimized.

Copy link
@pkerling

pkerling Jan 4, 2018

Author Member

sure, why not

pkerling added 3 commits Jan 2, 2018
CUtil only has static functions. Instantiating it should be a
compile-time error.
@pkerling pkerling force-pushed the pkerling:splash-jpg branch from 879b083 to 3c82695 Jan 4, 2018
@xhaggi

This comment has been minimized.

Copy link
Member

commented Jan 4, 2018

jenkins build this please

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Jan 4, 2018

Only a test failed on osx.

@xhaggi
xhaggi approved these changes Jan 4, 2018
Copy link
Member

left a comment

@MartijnKaijser your button

@MartijnKaijser MartijnKaijser merged commit 138fa39 into xbmc:master Jan 4, 2018
1 check failed
1 check failed
default Sorry, building this PR failed. Please check the logs for the errors.
Details
@xhaggi

This comment has been minimized.

Copy link
Member

commented Jan 4, 2018

Thanks @pkerling for your contribution

@da-anda

This comment has been minimized.

Copy link
Member

commented Jan 4, 2018

@MartijnKaijser we could have waited for the reworked splash from samfisher (which is progressing very well). But well, now my borked splash will be featured in one nightly build :)

@pkerling pkerling deleted the pkerling:splash-jpg branch Jan 4, 2018
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.