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

[gui] smartplaylisteditor: convert spinners etc to regular buttons #10594

Merged
merged 15 commits into from Oct 6, 2016

Conversation

@phil65
Copy link
Member

commented Sep 29, 2016

Next one...this time the order-by button in Smartplaylisteditor.
this PR adds the same header file to GUIDialogSmartPlaylistEditor.cpp as #10593, so remind me to update the PR which gets pulled last. :)
Could have put all commits into one big PR, so sorry for the additional pinging.

EDIT: all PRs merged into this one.

EDIT: FInished!
screenshot031

@mention-bot

This comment has been minimized.

Copy link

commented Sep 29, 2016

@phil65, thank you for improving Kodi! According to the last 5 commits, we found the potential reviewers: @Montellese, @mkortstiege and @notspiff. Final approval needs to be given by the component maintainer.

@phil65 phil65 changed the title Playlistorder spinner [gui] change order-by selection from spinner to selectdialog Sep 29, 2016
@phil65 phil65 force-pushed the phil65:playlistorder_spinner branch from 3b33eae to 77ea84a Sep 30, 2016
@phil65

This comment has been minimized.

Copy link
Member Author

commented Sep 30, 2016

Added another commit which converts two more buttons.
If it helps testing or whatever I could also merge the other PR´s into this one.

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2016

Merge them (10593 and 10594) and I'll include them both in test builds - keeping them separate makes that a bit trickier (as they're both patching the same code).

@phil65

This comment has been minimized.

Copy link
Member Author

commented Sep 30, 2016

@MilhouseVH done.
This also includes now #10592, #10593 and #10589

@phil65 phil65 changed the title [gui] change order-by selection from spinner to selectdialog [gui] smartplaylisteditor: convert spinners etc to regular buttons Sep 30, 2016
@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2016

Thanks, will include in the next build.

@phil65

This comment has been minimized.

Copy link
Member Author

commented Sep 30, 2016

...aaand another commit.

@phil65 phil65 force-pushed the phil65:playlistorder_spinner branch 4 times, most recently from 7393ad2 to d7a93f4 Sep 30, 2016
@phil65 phil65 force-pushed the phil65:playlistorder_spinner branch from d7a93f4 to 1489216 Sep 30, 2016
@phil65 phil65 force-pushed the phil65:playlistorder_spinner branch 2 times, most recently from ca4dde2 to 89e07d7 Sep 30, 2016
Copy link
Member

left a comment

Looks good apart from the minors. This will require all skins to adjust the dialog right?

{
if (item == static_cast<int>(m_playlist.m_ruleCombination.m_rules.size()))
OnRuleAdd();
else if (item < 0 || item > static_cast<int>(m_playlist.m_ruleCombination.m_rules.size()))

This comment has been minimized.

Copy link
@Montellese

Montellese Oct 1, 2016

Member

Please move this condition to the top. IMO early returns should always be at the top if there are multiple conditions. Furthermore after an early return you don't need an else.

else
{
CSmartPlaylistRule rule = *std::static_pointer_cast<CSmartPlaylistRule>(m_playlist.m_ruleCombination.m_rules[item]);
if (CGUIDialogSmartPlaylistRule::EditRule(rule,m_playlist.GetType()))

This comment has been minimized.

Copy link
@Montellese

Montellese Oct 1, 2016

Member

Space after rule,

CGUIMessage msg(GUI_MSG_ITEM_SELECTED, GetID(), CONTROL_LIMIT);
OnMessage(msg);
m_playlist.m_limit = msg.GetParam1();
const int limits[] = {0, 10, 25, 50, 100, 250, 500, 1000, -1 };

This comment has been minimized.

Copy link
@Montellese

Montellese Oct 1, 2016

Member

You can use an std::vector to get rid of the awkward -1 special item and then you can use a ranged for loop.

This comment has been minimized.

Copy link
@Paxxi

Paxxi Oct 2, 2016

Member

Ranged for loop works for plain array as well, the requirements are that std:begin and end exists and those are implemented by default

This comment has been minimized.

Copy link
@Montellese

Montellese Oct 2, 2016

Member

@Paxxi: correct but the -1 would have to go in that case as well which is my main point/concern.

void CGUIDialogSmartPlaylistEditor::OnName()
{
std::string name = m_playlist.m_playlistName;
if (CGUIKeyboardFactory::ShowAndGetInput(name, CVariant{g_localizeStrings.Get(16012)}, false))

This comment has been minimized.

Copy link
@Montellese

Montellese Oct 1, 2016

Member

You can use CVariant{ 16012 } directly. The CGUIKeyboardFactory::ShowAndGetInput() knows how to deal with that.

@@ -546,6 +548,37 @@ void CGUIDialogSmartPlaylistEditor::HighlightItem(int item)
OnMessage(msg);
}

std::vector<CGUIDialogSmartPlaylistEditor::PLAYLIST_TYPE> CGUIDialogSmartPlaylistEditor::GetAllowedTypes(std::string mode)

This comment has been minimized.

Copy link
@Montellese

Montellese Oct 1, 2016

Member

const std::string& mode

dialog->Reset();
dialog->SetHeading(CVariant{20427});
int selected = -1;
for (unsigned int i = 0; i < fields.size(); i++)

This comment has been minimized.

Copy link
@Montellese

Montellese Oct 1, 2016

Member

ranged for loop.

This comment has been minimized.

Copy link
@phil65

phil65 Oct 1, 2016

Author Member

i need i... and I would need sth like for count, element in enumerate(fields) How do I do that in Cpp? :)

This comment has been minimized.

Copy link
@Montellese

Montellese Oct 1, 2016

Member

Right it doesn't work with ranged for loop. But if you use iterators there's a method called std::distance() and you use it like size_t pos = std::distance(fields.begin(), field); where field is the iterator.

This comment has been minimized.

Copy link
@phil65

phil65 Oct 2, 2016

Author Member

does that really make the code easier in this case? :)

This comment has been minimized.

Copy link
@Montellese

Montellese Oct 2, 2016

Member

It makes it more generic in case something around it changes.

if (!isValid)
m_rule.m_operator = (CDatabaseQueryRule::SEARCH_OPERATOR)std::get<1>(validOperators[0]);
m_rule.SetParameter("");
UpdateButtons();

This comment has been minimized.

Copy link
@Montellese

Montellese Oct 1, 2016

Member

Please throw in some empty lines in this code block (e.g. before an if statement and after the early return). It's hard to read if it's so dense.


void CGUIDialogSmartPlaylistRule::OnOperator()
{

This comment has been minimized.

Copy link
@Montellese

Montellese Oct 1, 2016

Member

Unneeded newline

{

std::vector< std::pair<std::string, int> > labels = GetValidOperators(m_rule);
CGUIDialogSelect* dialog = (CGUIDialogSelect*)g_windowManager.GetWindow(WINDOW_DIALOG_SELECT);

This comment has been minimized.

Copy link
@Montellese

Montellese Oct 1, 2016

Member

static_cast

void CGUIDialogSmartPlaylistRule::OnOperator()
{

std::vector< std::pair<std::string, int> > labels = GetValidOperators(m_rule);

This comment has been minimized.

Copy link
@Montellese

Montellese Oct 1, 2016

Member

const auto

@phil65

This comment has been minimized.

Copy link
Member Author

commented Oct 1, 2016

@Montellese
addressed all of your comments in a separate commit, except for the loops, see question above.
Also added two more casts to get rid of some compiler warnings.
Yes, this needs some trivial changes for all skins.

@@ -546,6 +549,37 @@ void CGUIDialogSmartPlaylistEditor::HighlightItem(int item)
OnMessage(msg);
}

std::vector<CGUIDialogSmartPlaylistEditor::PLAYLIST_TYPE> CGUIDialogSmartPlaylistEditor::GetAllowedTypes(const std::string& mode)
{

This comment has been minimized.

Copy link
@phil65

phil65 Oct 1, 2016

Author Member

Should I move this method somewhere else? The method does not depend on state and perhaps better belongs to CSmartPlaylist?

@phil65 phil65 force-pushed the phil65:playlistorder_spinner branch 2 times, most recently from 3b1196e to 03c1348 Oct 2, 2016
@phil65

This comment has been minimized.

Copy link
Member Author

commented Oct 2, 2016

@Montellese switched to using iterators. Anything else?
Thx for reviewing.

@phil65 phil65 force-pushed the phil65:playlistorder_spinner branch from 03c1348 to 03dbc20 Oct 4, 2016
@phil65

This comment has been minimized.

Copy link
Member Author

commented Oct 4, 2016

jenkins build this please

@phil65

This comment has been minimized.

Copy link
Member Author

commented Oct 5, 2016

@Montellese general skinner´s sentiment was to merge this, even though we´re already in a later beta, since needed skin changes are very trivial and our skinners are quite good in keeping up with these changes. Are you fine with this?

@Montellese

This comment has been minimized.

Copy link
Member

commented Oct 6, 2016

Fine with me but don't forget to squash the cleanup commit(s).

@phil65 phil65 force-pushed the phil65:playlistorder_spinner branch from 03dbc20 to 46ff271 Oct 6, 2016
@phil65

This comment has been minimized.

Copy link
Member Author

commented Oct 6, 2016

All follow-up stuff squashed into one commit.
jenkins build this please.

@phil65 phil65 added the v17 Krypton label Oct 6, 2016
@phil65 phil65 merged commit 0aacf5c into xbmc:master Oct 6, 2016
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
jenkins.kodi.tv You did a great job. Have a cookie.
Details
@phil65 phil65 deleted the phil65:playlistorder_spinner branch Oct 6, 2016
@jingai

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2017

Why was the Order button (id=19 in SmartPlaylistEditor.xml) changed from togglebutton to button? It only toggles between two values.

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