Skip to content

[PVR] : 4 fixes related to chanel icons #4227

Closed
wants to merge 5 commits into from

7 participants

@alexmaloteaux

This pr fixes 4 issues related to pvr channels icon :

1) When masterlockcode is set and user click on the logo button in channel manager, the password dialog box is shown twice and user need to enter the password 2 times.
This is resolved by removing and uneeded call to g_passwordManager.IsMasterLockUnlocked in CGUIDialogPVRChannelManager::OnClickButtonChannelLogo

2)When user set a user defined logo; the iconpath is emptied upon xbmc restart. This is fixed by setting the userseticon flag to true when user choose its own icon.

3) When the icon path related file does not exists anymore, it should be emptied. This is particulary boring when UserSetIcon is set because it will never been emptied regarding how updatefromclient works. This is fixed by veryfing if the file exists in CPVRChannelGroup::SearchAndSetChannelIcons

4) When user set a custom name for a channel in Channel manager, SearchAndSetChannelIcons should also search for an icon file based on that name.

@t-nelson

@opdenkamp @xhaggi for review please

@alexmaloteaux

@t-nelson splitting ?? how ?? they are all related to the same files and wont merge/build all i think

@t-nelson

http://stackoverflow.com/questions/4307095/git-how-to-split-up-a-commit-buried-in-history

In general we want at least one commit for each distinct feature added/bug fixed. You can make more if it's logical. They can always be squashed later. Multiple fixes per commit are a complete PITA to review and even further pain revert later if there's a problem with only part of it.

@alexmaloteaux

@t-nelson ok will know for the feature and won't happen again :) . I have not splitted the first 3 fixes in separated commits; Maybe for this times lets first @opdenkamp and @xhaggi review it, i guess they will figure out easily this pr. If not i ll got to the boring, time consuming tasks of splitting it and realeasing 1 by 1 .. Regards

@opdenkamp
Team Kodi member

before i have a look at it: please fix up the cosmetics. we use 2 spaces, no tabs

@alexmaloteaux

@xhaggi it was like this, so by adding bForceUserSetIconUpdate i m sure to not break anything.
do we need to know if an user has set the icon path to empty? -> i guess original devs would better answer this..

@alexmaloteaux

@xhaggi , @opdenkamp you want me to split it ?

@xhaggi
Team Kodi member
xhaggi commented Feb 20, 2014

yes please split your commit in logical parts and apply the cosmetics.

@alexmaloteaux

@xhaggi i already applied the cosmetics . Will check your link concerning the commit splitting thanks

@alexmaloteaux

@xhaggi @t-nelson after splitting can i push diretly to this pr or i need to create a new one ?

@fritsch
Team Kodi member
fritsch commented Feb 20, 2014

Force push

@t-nelson

To elaborate. When you force push the branch to your repo, github will automatically update this PR.

@MartijnKaijser MartijnKaijser added this to the Pending for inclusion milestone Feb 21, 2014
@alexmaloteaux

@xhaggi @t-nelson @opdenkamp ok hope it is better now ..

@t-nelson

Thanks for splitting this up. Would you mind dropping the "Fix Nx:" prefixes from the messages and prefixing them "[PVR]" instead.

@xhaggi
Team Kodi member
xhaggi commented Feb 21, 2014

Sorry but could you please add fix: to the commit message and use a short one line message + separated details if needed.

[pvr] fix: password prompt appears twice in channel manager

detailed description 
@t-nelson

Hehe. I think maybe we need an "XBMC git guidelines" in contributing.md.

@xhaggi
Team Kodi member
xhaggi commented Feb 21, 2014

1st commit (password prompt appears twice) is fine with me

@xhaggi
Team Kodi member
xhaggi commented Feb 21, 2014

@alexmaloteaux could you please extract the first commit into a separate PR so we could merge it independently.

@t-nelson

1st commit (password prompt appears twice) is fine with me

Something else seems wrong though. We shouldn't be prompting the user unless bPromptUser is set though, no?

@xhaggi
Team Kodi member
xhaggi commented Feb 21, 2014

@t-nelson we don't need to call IsMasterLockUnlocked() as IsProfileLockUnlocked() do it for us
https://github.com/xbmc/xbmc/blob/4372e0d63cd05961b81c0d08d1180e77f44f8de2/xbmc/GUIPassword.cpp?source=cc#L195

@t-nelson
@opdenkamp opdenkamp and 1 other commented on an outdated diff Feb 25, 2014
xbmc/pvr/channels/PVRChannelGroup.cpp
@@ -257,29 +268,63 @@ void CPVRChannelGroup::SearchAndSetChannelIcons(bool bUpdateDb /* = false */)
/* skip if an icon is already set */
if (!groupMember.channel->IconPath().empty())
- continue;
+ {
+ //check if the file exist or empty it
+ if(VerifyChannelIconPath(groupMember.channel))
@opdenkamp
Team Kodi member
opdenkamp added a note Feb 25, 2014

don't have time to test this right now, but does this work with icons on a webserver?

Don't know but as clue, it is CFile::Exists in the backend

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@opdenkamp opdenkamp commented on the diff Feb 25, 2014
xbmc/pvr/channels/PVRChannelGroup.cpp
CStdString strIconPathUid;
strIconPathUid = StringUtils::Format("%08d", groupMember.channel->UniqueID());
strIconPathUid = URIUtils::AddFileToFolder(strBasePath, strIconPathUid);
- SetChannelIconPath(groupMember.channel, strIconPath + ".tbn") ||
- SetChannelIconPath(groupMember.channel, strIconPath + ".jpg") ||
- SetChannelIconPath(groupMember.channel, strIconPath + ".png") ||
+ bool bIconFound =
+ SetChannelIconPath(groupMember.channel, strIconPath + ".tbn") ||
+ SetChannelIconPath(groupMember.channel, strIconPath + ".jpg") ||
+ SetChannelIconPath(groupMember.channel, strIconPath + ".png") ||
+
+ SetChannelIconPath(groupMember.channel, strIconPathLower + ".tbn") ||
+ SetChannelIconPath(groupMember.channel, strIconPathLower + ".jpg") ||
+ SetChannelIconPath(groupMember.channel, strIconPathLower + ".png") ||
@opdenkamp
Team Kodi member
opdenkamp added a note Feb 25, 2014

ugh, this is becoming more ugly every day. we should really clean this up. but since the original code was like this too, it's fine with me for gotham like this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@opdenkamp opdenkamp commented on the diff Feb 25, 2014
xbmc/pvr/channels/PVRChannelGroup.cpp
+
+ if(!bIconFound)
+ {
+ CStdString strSanitizedLowerChannelName = strSanitizedChannelName;
+ StringUtils::ToLower(strSanitizedLowerChannelName);
+ CStdString strIconPathLower2 = strBasePath + strSanitizedLowerChannelName;
+ // Do not search Lowercase if user only changed a letter case ...
+ if(strIconPathLower2 != strIconPathLower)
+ {
+ SetChannelIconPath(groupMember.channel, strIconPathLower2 + ".tbn") ||
+ SetChannelIconPath(groupMember.channel, strIconPathLower2 + ".jpg") ||
+ SetChannelIconPath(groupMember.channel, strIconPathLower2 + ".png") ;
+ }
+ }
+ }
+ }
@opdenkamp
Team Kodi member
opdenkamp added a note Feb 25, 2014

is there any fs caching used here in the file exists check? or else this code will be very slow on filesystems like nfs, and then we should get a dirlisting and search in there instead

No no caching ; but this always be added after merging i think

@opdenkamp
Team Kodi member
opdenkamp added a note Feb 28, 2014

this code will be really slow in that case unless it's being used on a local filesystem...

as you said it was like this before, i just used the same method

@opdenkamp
Team Kodi member
opdenkamp added a note Mar 4, 2014

yeah i know, it's still fugly

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

@xhaggi @opdenkamp regarding how the userseticon flag was used in SetIconPath before the PR. i think it is safe to remove the force flag and just make one little change as in the 5b452bd commit

@opdenkamp
Team Kodi member

I'll review over the week end.

@xhaggi xhaggi commented on an outdated diff Mar 4, 2014
xbmc/pvr/channels/PVRChannel.cpp
@@ -753,6 +750,11 @@ bool CPVRChannel::IsUserSetIcon(void) const
return m_bIsUserSetIcon;
}
+bool CPVRChannel::IsIconExists() const
+{
+ return (CFile::Exists(IconPath()));
@xhaggi
Team Kodi member
xhaggi added a note Mar 4, 2014

two spaces and we don't need the extra braces here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@xhaggi xhaggi closed this Mar 4, 2014
@xhaggi xhaggi reopened this Mar 4, 2014
@xhaggi
Team Kodi member
xhaggi commented Mar 4, 2014

sorry wrong button ;(

@xhaggi
Team Kodi member
xhaggi commented Mar 4, 2014

@alexmaloteaux would be nice if you could separate the commit 4372e0d into a new PR as i said before. This commit could be merged immediately and don't have to wait.

And please check the comments i've made ;)

@xhaggi xhaggi added the Gotham label Mar 4, 2014
@xhaggi
Team Kodi member
xhaggi commented Mar 9, 2014
@xhaggi xhaggi added PVR Fix and removed Fix labels Mar 12, 2014
Alexandre Ma... added some commits Feb 21, 2014
Alexandre Maloteaux [PVR] when a user set its own icon for the pvr logo, this one is rese…
…t to null upon xbmc restart because UserSetIcon flag is not set
fe5ffed
Alexandre Maloteaux [PVR] if an user set icon does not exists anymore it will never be de…
…leted from the database because updatefromclient do not check user set icons.
165578b
@alexmaloteaux

@xhaggi removed it from here

@xhaggi
Team Kodi member
xhaggi commented Mar 12, 2014

could you please squash the fixup commits and i'll review again

@alexmaloteaux

@Jalle19 renamed that, @xhaggi applied the cosmetics

@xhaggi
Team Kodi member
xhaggi commented Mar 15, 2014

could you please squash the commits

@t-nelson

@alexmaloteaux please squash the history down to one commit per fix and we'll get this in.

@alexmaloteaux

@t-nelson ok like this ?

@t-nelson

Fine by my eye.

@opdenkamp, @xhaggi, A last pass by one of you, please.

jenkins build this please

@xhaggi

UpdateChannel() is only used to update channel information changed by a user.
so here we need channel->SetIconPath(strIconPath, true); only and please change back the other stuff (method signature etc.)

@xhaggi UpdateChannel() is called by savelist--PersistChannel for all channels in a for loop; the userset flag is only set for those "set" by the user through the Item property set before. I think it is ok this way ...

Team Kodi member

okay then that's fine.

@xhaggi
Team Kodi member
xhaggi commented Mar 23, 2014

@alexmaloteaux finally please squash commit 19e3053 into 165578b

@opdenkamp
Team Kodi member

ok for gotham when squashed

@xhaggi
Team Kodi member
xhaggi commented Mar 25, 2014

@alexmaloteaux if you don't have time let me know. i will cherry pick your stuff and finish things so we can merge it.

@alexmaloteaux

@xhaggi i don't get it is it merged ?

Alexandre Maloteaux [PVR] If a user set a custom name for the pvr channel, this name shou…
…ld be used too when searching icons
d88053a
@alexmaloteaux

@xhaggi ok done ; read the comments i think it is better to keep updatechannel like this

@xhaggi
Team Kodi member
xhaggi commented Mar 25, 2014

sorry but you squashed different commits into one. (the last one in the one before)

@alexmaloteaux

@xhaggi one per fix with all the cosmetics and so on included in the last one

@xhaggi
Team Kodi member
xhaggi commented Mar 25, 2014

i told you that the last commit should be squashed into commit 165578b. you squashed the last one in the previous one. It would be nice if you could separate all the stuff related to commit 165578b into this commit. if you need help let me know.

@xhaggi

please use StringUtils::Empty instead of std:string()

@alexmaloteaux

@xhaggi the older commits history is deleted due to -force i think; only way is the boring part of git rebase ; git -p add ; manual stuff ; ... i guess; do you have an easier solution ? sorry fir misunderstood ..

@xhaggi
Team Kodi member
xhaggi commented Mar 26, 2014

@alexmaloteaux now you have to do a manual part. you could do a git rebase and edit the corresponing commits then c/p your changes and continue rebasing or you could start from scratch, rename your local branch, create a new one with the same name, c/p your changes then commit and so on and later do a force push.

@xhaggi

ahh not seen before .. indention

@xhaggi
Team Kodi member
xhaggi commented Mar 26, 2014

@alexmaloteaux BTW the commit history is not deleted, but you have to use a gui client for git which supports to show commits without branch assignments. or you know the commit id ;)

Alexandre Maloteaux [PVR] fix identation e49041d
@alexmaloteaux

@xhaggi theres is 3 commit (after final squash) and 3 fix; maybe just editing final commit messgae will do ; otherwise too boring .. :)

@Jalle19
Team Kodi member
Jalle19 commented Mar 26, 2014

Just merge it, no point in testing the guy's gitfu.

@xhaggi
Team Kodi member
xhaggi commented Mar 26, 2014

i did the magic git rebase/split stuff in PR #4477

@xhaggi xhaggi closed this Mar 26, 2014
@t-nelson
@t-nelson t-nelson removed the Gotham label Mar 27, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.