Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

guilib: execute actions when going through navigation path #518

Merged
merged 3 commits into from

3 participants

@pieh
Collaborator

Hitcher described potential issue in http://forum.xbmc.org/showthread.php?t=112767. It looks like bug for me but it might be this way by design. Simplified problematic scenario:

We have controls:
id=1: currently focused, <onright>2</onright>
id=2: hidden, <onright>3</onright>
id=3: hidden, <onright>ExecuteSomeBuiltinFunction</onright>

We have control id==1 focused, press right and currently it works like this:
We're going from control 1 to control 2, control 2 isn't focusable so we're checking where should we go from there. We're going from control 2 to control 3, control 3 still isn't focusable, we don't have navigation path so we simply stop there and in the end nothing is happening. Should we execute builtin from control id==3 or not?

If we should - this PR changes/fixes current behaviour. If not - with some cosmetic changes this can clean <onfoo> handling a little bit, I think.

@HitcherUK

It's also worth noting that putting them in a grouplist and letting that handle the action doesn't work either.

Thanks.

@HitcherUK

No more thoughts anyone?

@HitcherUK

Hi, not trying to be be pushy here but I would like to know whether this will be fixed for Eden so I can make the relevant changes to XeeBo (Eden)?

Thanks.

@jmarshallnz
Owner

I would suggest that in the example above, it is not intuitive for the 3rd (hidden) control to execute it's action? I guess it depends on what the action was - if it was an action designed to emulate normal navigation, then it might still make sense. In general, I don't think it does?

@HitcherUK

But what if all the controls were inside a grouplist? Shouldn't the grouplist's onright ACTION be performed in the above scenario?

@jmarshallnz
Owner

Good question. Basically how grouplists work is that the grouplists actions are given to the controls inside the group, rather than executing from the grouplist control itself. For movement in the direction of the grouplist, however, it really only makes sense to give the internal controls navigation actions, and that's also what's done to the controls at the start and end. I suspect this would be possible to change but it might take quite a bit of futzing around.

I presume that atm you have a button that you navigate onto hidden outside the end of the list that has an appropriate action? In some cases this might actually be the cleaner way to execute it?

@HitcherUK

A little background -

The skin in question is XeeBo and it has a global side menu (which is actually a custom window) that is accessed by moving to the left of any window in the skin. From this menu you can select to go any where you please or, if say you changed your mind or simply wanted to see the little weather panel at the top of the menu, by pressing right return to the window you were in.This is achieved by closing the custom window so control is returned to the last focused control.

Now I've made a skin layout for the TV Next Aired script and obviously want to keep the global menu available via an action - ActivateWindow(51) - but because the script is made up of 7 lists for each day of the week and sometimes there might not be anything airing on one or more of the days I need to be able to open the custom window when they're hidden, and this is where I found my problem.
I've had to use a button for this particular case but it goes against the rest of the skin's navigational feel because it gets focus when returning to the script and not the lists.

Hope that explains things better.

Thanks.

@jmarshallnz
Owner

As I think about it, even if we were to change grouplists so that the grouplist action was assigned to the end controls, it still wouldn't fix the problem.

The only fix is treating the actions and navigation as the same, i.e. if you move onto a hidden control, then it will execute it's in all circumstances.

Will review the commits here more thoroughly, as I'm coming around to the idea.

@jmarshallnz
Owner

I think once Eden is out and the commits are cleaned up, I'd be happy for this to go in.

@HitcherUK

Awesome, thanks JM.

pieh added some commits
@pieh pieh change CGUIControl::GetNextAction to more general CGUIControl::GetNav…
…igationAction
a5c77d9
@pieh pieh don't continue to follow navigation path if we don't have valid id of…
… next control
ea04d36
@pieh pieh split GUIAction::Execute into GUIAction::ExecuteActions and CGUIContr…
…ol::Navigate

GUIAction::ExecuteActions will simply execute all non-navigation actions
CGUIControl::Navigate is meant to start navigation from control in given direction until we find focusable control (executing actions on its path)
bf300a0
@pieh
Collaborator

@jmarshallnz
I did cleanups You pointed earlier. Couldn't really split last commit into 2 because it would break execution of non-navigation actions in in intermediate revision.

@jmarshallnz
Owner

Looks good. As this potentially changes navigation paths in existing skins we probably need an xbmc.gui bump?

@pieh pieh was assigned
@pieh
Collaborator

Resurrection of another stalled pr. Is it in good shape to be merged in August? xbmc.gui bump would propably should go in at the end of merge window?

@jmarshallnz
Owner

It's fine to be merged in August - assigned to you.

@jmarshallnz jmarshallnz merged commit 397b43e into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 26, 2012
  1. @pieh
  2. @pieh
  3. @pieh

    split GUIAction::Execute into GUIAction::ExecuteActions and CGUIContr…

    pieh authored
    …ol::Navigate
    
    GUIAction::ExecuteActions will simply execute all non-navigation actions
    CGUIControl::Navigate is meant to start navigation from control in given direction until we find focusable control (executing actions on its path)
This page is out of date. Refresh to see the latest.
View
18 xbmc/guilib/GUIAction.cpp
@@ -38,7 +38,7 @@ CGUIAction::CGUIAction(int controlID)
SetNavigation(controlID);
}
-bool CGUIAction::Execute(int controlID, int parentID, int direction /*= 0*/) const
+bool CGUIAction::ExecuteActions(int controlID, int parentID) const
{
if (m_actions.size() == 0) return false;
bool retval = false;
@@ -47,21 +47,7 @@ bool CGUIAction::Execute(int controlID, int parentID, int direction /*= 0*/) con
{
if (it->condition.IsEmpty() || g_infoManager.EvaluateBool(it->condition))
{
- if (StringUtils::IsInteger(it->action))
- {
- CGUIMessage msg(GUI_MSG_MOVE, parentID, controlID, direction);
- if (parentID)
- {
- CGUIWindow *pWindow = g_windowManager.GetWindow(parentID);
- if (pWindow)
- {
- retval |= pWindow->OnMessage(msg);
- continue;
- }
- }
- retval |= g_windowManager.SendMessage(msg);
- }
- else
+ if (!StringUtils::IsInteger(it->action))
{
CGUIMessage msg(GUI_MSG_EXECUTE, controlID, parentID);
msg.SetStringParam(it->action);
View
4 xbmc/guilib/GUIAction.h
@@ -35,9 +35,9 @@ class CGUIAction
CGUIAction(int controlID);
/**
- * Execute actions, if action is paired with condition - evaluate condition first
+ * Execute actions (no navigation paths), if action is paired with condition - evaluate condition first
*/
- bool Execute(int controlID, int parentID, int direction = 0) const;
+ bool ExecuteActions(int controlID, int parentID) const;
/**
* Check if there is any action that meet its condition
*/
View
2  xbmc/guilib/GUIBaseContainer.cpp
@@ -700,7 +700,7 @@ bool CGUIBaseContainer::OnClick(int actionID)
if (selected >= 0 && selected < (int)m_items.size())
{
CGUIStaticItemPtr item = boost::static_pointer_cast<CGUIStaticItem>(m_items[selected]);
- item->GetClickActions().Execute(GetID(), GetParentID());
+ item->GetClickActions().ExecuteActions(GetID(), GetParentID());
}
return true;
}
View
6 xbmc/guilib/GUIButtonControl.cpp
@@ -307,17 +307,17 @@ void CGUIButtonControl::OnClick()
CGUIMessage msg(GUI_MSG_CLICKED, controlID, parentID, 0);
SendWindowMessage(msg);
- clickActions.Execute(controlID, parentID);
+ clickActions.ExecuteActions(controlID, parentID);
}
void CGUIButtonControl::OnFocus()
{
- m_focusActions.Execute(GetID(), GetParentID());
+ m_focusActions.ExecuteActions(GetID(), GetParentID());
}
void CGUIButtonControl::OnUnFocus()
{
- m_unfocusActions.Execute(GetID(), GetParentID());
+ m_unfocusActions.ExecuteActions(GetID(), GetParentID());
}
void CGUIButtonControl::SetSelected(bool bSelected)
View
49 xbmc/guilib/GUIControl.cpp
@@ -224,46 +224,50 @@ bool CGUIControl::OnAction(const CAction &action)
return false;
}
+bool CGUIControl::Navigate(int direction)
+{
+ if (HasFocus())
+ {
+ CGUIMessage msg(GUI_MSG_MOVE, GetParentID(), GetID(), direction);
+ return SendWindowMessage(msg);
+ }
+ return false;
+}
+
// Movement controls (derived classes can override)
void CGUIControl::OnUp()
{
- if (HasFocus())
- m_actionUp.Execute(GetID(), GetParentID(), ACTION_MOVE_UP);
+ Navigate(ACTION_MOVE_UP);
}
void CGUIControl::OnDown()
{
- if (HasFocus())
- m_actionDown.Execute(GetID(), GetParentID(), ACTION_MOVE_DOWN);
+ Navigate(ACTION_MOVE_DOWN);
}
void CGUIControl::OnLeft()
{
- if (HasFocus())
- m_actionLeft.Execute(GetID(), GetParentID(), ACTION_MOVE_LEFT);
+ Navigate(ACTION_MOVE_LEFT);
}
void CGUIControl::OnRight()
{
- if (HasFocus())
- m_actionRight.Execute(GetID(), GetParentID(), ACTION_MOVE_RIGHT);
+ Navigate(ACTION_MOVE_RIGHT);
}
bool CGUIControl::OnBack()
{
- return HasFocus() ? m_actionBack.Execute(GetID(), GetParentID(), ACTION_NAV_BACK) : false;
+ return Navigate(ACTION_NAV_BACK);
}
void CGUIControl::OnNextControl()
{
- if (HasFocus())
- m_actionNext.Execute(GetID(), GetParentID(), ACTION_NEXT_CONTROL);
+ Navigate(ACTION_NEXT_CONTROL);
}
void CGUIControl::OnPrevControl()
{
- if (HasFocus())
- m_actionPrev.Execute(GetID(), GetParentID(), ACTION_PREV_CONTROL);
+ Navigate(ACTION_PREV_CONTROL);
}
bool CGUIControl::SendWindowMessage(CGUIMessage &message)
@@ -890,22 +894,27 @@ bool CGUIControl::IsAnimating(ANIMATION_TYPE animType)
return false;
}
-int CGUIControl::GetNextControl(int direction) const
+bool CGUIControl::GetNavigationAction(int direction, CGUIAction& action) const
{
switch (direction)
{
case ACTION_MOVE_UP:
- return m_actionUp.GetNavigation();
+ action = m_actionUp;
+ return true;
case ACTION_MOVE_DOWN:
- return m_actionDown.GetNavigation();
+ action = m_actionDown;
+ return true;
case ACTION_MOVE_LEFT:
- return m_actionLeft.GetNavigation();
+ action = m_actionLeft;
+ return true;
case ACTION_MOVE_RIGHT:
- return m_actionRight.GetNavigation();
+ action = m_actionRight;
+ return true;
case ACTION_NAV_BACK:
- return m_actionBack.GetNavigation();
+ action = m_actionBack;
+ return true;
default:
- return -1;
+ return false;
}
}
View
5 xbmc/guilib/GUIControl.h
@@ -199,7 +199,10 @@ class CGUIControl
int GetControlIdLeft() const { return m_actionLeft.GetNavigation(); };
int GetControlIdRight() const { return m_actionRight.GetNavigation(); };
int GetControlIdBack() const { return m_actionBack.GetNavigation(); };
- virtual int GetNextControl(int direction) const;
+ bool GetNavigationAction(int direction, CGUIAction& action) const;
+ /*! \brief Start navigating in given direction.
+ */
+ bool Navigate(int direction);
virtual void SetFocus(bool focus);
virtual void SetWidth(float width);
virtual void SetHeight(float height);
View
2  xbmc/guilib/GUIEditControl.cpp
@@ -306,7 +306,7 @@ void CGUIEditControl::UpdateText(bool sendUpdate)
{
SEND_CLICK_MESSAGE(GetID(), GetParentID(), 0);
- m_textChangeActions.Execute(GetID(), GetParentID());
+ m_textChangeActions.ExecuteActions(GetID(), GetParentID());
}
SetInvalid();
}
View
12 xbmc/guilib/GUIWindow.cpp
@@ -833,7 +833,13 @@ bool CGUIWindow::OnMove(int fromControl, int moveAction)
while (control)
{ // grab the next control direction
moveHistory.push_back(nextControl);
- nextControl = control->GetNextControl(moveAction);
+ CGUIAction action;
+ if (!control->GetNavigationAction(moveAction, action))
+ return false;
+ action.ExecuteActions(nextControl, GetParentID());
+ nextControl = action.GetNavigation();
+ if (!nextControl) // 0 isn't valid control id
+ return false;
// check our history - if the nextControl is in it, we can't focus it
for (unsigned int i = 0; i < moveHistory.size(); i++)
{
@@ -951,12 +957,12 @@ void CGUIWindow::SetRunActionsManually()
void CGUIWindow::RunLoadActions()
{
- m_loadActions.Execute(GetID(), GetParentID());
+ m_loadActions.ExecuteActions(GetID(), GetParentID());
}
void CGUIWindow::RunUnloadActions()
{
- m_unloadActions.Execute(GetID(), GetParentID());
+ m_unloadActions.ExecuteActions(GetID(), GetParentID());
}
void CGUIWindow::ClearBackground()
Something went wrong with that request. Please try again.