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.

Sign up for GitHub

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

Jump to bottom

[gui] follow up for PR #11704 #11847

Merged
merged 4 commits into from
Mar 21, 2017
Merged

[gui] follow up for PR #11704 #11847

merged 4 commits into from
Mar 21, 2017

Conversation

xhaggi
Copy link
Member

@xhaggi xhaggi commented Mar 13, 2017

@ksooo as discussed at #11704.

@xhaggi xhaggi requested a review from ksooo March 13, 2017 16:54
@ksooo
Copy link
Member

ksooo commented Mar 13, 2017

You should change "actualitems" to "numactualitems" as well, then, for consistency reasons. Otherwise I see no point in the name change of "nonfolderitems" to "numnonfolderitems".

@xhaggi
Copy link
Member Author

xhaggi commented Mar 13, 2017 •
edited

Added the missing break, thanks.

You should change "actualitems" to "numactualitems" as well, then, for consistency reasons. Otherwise I see no point in the name change of "nonfolderitems" to "numnonfolderitems".

That's the reason why I want to double check new GUI labels etc. If it was merged it becomes hard to change. @phil65 does the same mistake if you ask me. Nothing will point-out that this info label will return the size of items. That's the reason why NumItems is prefixed with num. We should use the same pattern for new info labels if it makes sense and this is the case here.

@xhaggi
Copy link
Member Author

xhaggi commented Mar 13, 2017

I've added a separate commit which renames the info label ActualItems to NumAllItems. It leaves the old one in for backward compatibility.

@xhaggi
Copy link
Member Author

xhaggi commented Mar 13, 2017

I've dropped the backward compat stuff because ActualItems was merged into Leia only. In that case we don't need it.

@xhaggi
Copy link
Member Author

xhaggi commented Mar 16, 2017

Jenkins build this please

@ksooo
Copy link
Member

ksooo commented Mar 16, 2017

Have you seen my latest comments? I'm not sure the PR is ready to merge.

@xhaggi
Copy link
Member Author

xhaggi commented Mar 16, 2017

Which one?

ksooo
ksooo reviewed Mar 16, 2017
View reviewed changes
xbmc/GUIInfoManager.cpp Outdated
@@ -2407,7 +2407,7 @@ const infomap container_bools[] ={{ "onnext", CONTAINER_MOVE_NEXT },
{ "numpages", CONTAINER_NUM_PAGES },
{ "numitems", CONTAINER_NUM_ITEMS },
{ "numnonfolderitems", CONTAINER_NUM_NONFOLDER_ITEMS },
{ "actualitems", CONTAINER_ACTUAL_ITEMS },
{ "numallitems", CONTAINER_NUM_ALL_ITEMS },

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

xbmc/guilib/GUIBaseContainer.cpp Outdated
}
label = StringUtils::Format("%u", numItems);
}
break;
default:
break;

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@ksooo
Copy link
Member

ksooo commented Mar 16, 2017

Seems I forgot to submit my review. ;-)

@xhaggi
Copy link
Member Author

xhaggi commented Mar 21, 2017

jenkins build this please

@xhaggi xhaggi merged commit badda83 into xbmc:master Mar 21, 2017
@Rechi Rechi added the v18 Leia label Mar 21, 2017
@Rechi Rechi added this to the L 18.0-alpha1 milestone Mar 21, 2017
@xhaggi xhaggi deleted the follow-up-pr-11704 branch July 22, 2017 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ronie ronie ronie left review comments

@ksooo ksooo ksooo left review comments

Assignees
No one assigned
Labels
Component: GUI engine v18 Leia
Projects
None yet
Milestone
Leia 18.0-alpha1
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@xhaggi @ksooo @ronie @Rechi