[pvr] fix: duplicate parent dir item for recordings #4277

Merged
merged 2 commits into from Mar 4, 2014

Projects

None yet

6 participants

@xhaggi
Member
xhaggi commented Feb 25, 2014

The current integration of the parent dir item for recordings results in a duplicate parent dir item if you add pvr://recordings as video source. This implements a more safer way for handling the parent dir item.

Discussed at http://forum.xbmc.org/showthread.php?tid=187320

@FernetMenta ping

@opdenkamp opdenkamp was assigned by xhaggi Feb 25, 2014
@FernetMenta
Member

@xhaggi thanks. I am traveling and can't test before end of week.

@MartijnKaijser MartijnKaijser added this to the Pending for inclusion milestone Feb 27, 2014
@xhaggi
Member
xhaggi commented Feb 27, 2014

@jmarshallnz it would be nice if you could also have a look at this.

@t-nelson
Contributor

Who's adding the parent dir item in this case? They should already know about the setting.

@xhaggi
Member
xhaggi commented Feb 27, 2014

CGUIMediaWindow handles this by checking the view state. I override CGUIViewStatePVR::HideParentDirItems to handle when we hide or show it, it delegates first to CGUIViewState::HideParentDirItems(), because there the setting is checked. A special case is that we don't want to display the parent dir item at the root level only in pvr recording window, so this is done there.

@t-nelson
Contributor

Could this be done in CGUIMediaDirectory::GetDirectory() with an "&&
items.GetPath() != strParentPath" ?

@xhaggi
Member
xhaggi commented Feb 27, 2014

I'm not sure and can't check this because I'm traveling until tomorrow, but we need this only within the pvr recordings window and I don't want to change the default behavior.

@t-nelson
Contributor

Ok.

@opdenkamp's button.

@FernetMenta
Member
@xhaggi
Member
xhaggi commented Mar 2, 2014

jenkins build this please

@opdenkamp
Member

Sorry carnaval, can't review now :)

Op 2 mrt. 2014 om 21:14 heeft Sascha Woo notifications@github.com het volgende geschreven:

jenkins build this please


Reply to this email directly or view it on GitHub.

@jmarshallnz jmarshallnz commented on an outdated diff Mar 2, 2014
xbmc/pvr/windows/GUIWindowPVR.cpp
@@ -363,3 +364,18 @@ void CGUIWindowPVR::FrameMove()
}
CGUIMediaWindow::FrameMove();
}
+
+bool CGUIWindowPVR::GetDirectory(const CStdString &strDirectory, CFileItemList &items)
+{
+ bool bReturn = CGUIMediaWindow::GetDirectory(strDirectory, items);
+
+ CGUIWindowPVRCommon* view = GetActiveView();
+
+ // remove parent dir item on recordings root folder
+ if (view == m_windowRecordings &&
+ items.GetPath() == "pvr://recordings/" &&
+ !m_guiState->HideParentDirItems())
+ items.Remove(0);
@jmarshallnz
jmarshallnz Mar 2, 2014 Member

If you're going to leave it like this, you'll need to at least check that items.Size() > 0 and that items[1]->IsParentFolder() prior to removing the item.

@xhaggi xhaggi [pvr] fix: duplicate parent dir item for recordings
The current integration of the parent dir item for recordings results
in a duplicate parent dir item if you add pvr://recordings as
video source. This implements a more safer way for handling the parent
dir item.
3758dd2
@xhaggi
Member
xhaggi commented Mar 3, 2014

@jmarshallnz i found a solution to determine, if the parent dir item should be hidden in GUIViewStatePVR::HideParentDirItems(). The path of m_items never gets updated before HideParentDirItems() is called. So i updated the path and everything works fine.

@jmarshallnz jmarshallnz commented on the diff Mar 3, 2014
xbmc/pvr/windows/GUIWindowPVRRecordings.cpp
@@ -402,6 +402,9 @@ bool CGUIWindowPVRRecordings::OnContextButtonMarkWatched(const CFileItemPtr &ite
void CGUIWindowPVRRecordings::BeforeUpdate(const CStdString &strDirectory)
{
+ // set items path to current directory
+ m_parent->m_vecItems->SetPath(strDirectory);
+
@jmarshallnz
jmarshallnz Mar 3, 2014 Member

if the listing fails for some reason, does this get reset somewhere later (e.g. in Update/GetDirectory) ? If so, fine.

@jmarshallnz jmarshallnz commented on an outdated diff Mar 3, 2014
xbmc/pvr/windows/GUIWindowPVR.cpp
@@ -34,6 +34,7 @@
#include "dialogs/GUIDialogBusy.h"
#include "dialogs/GUIDialogKaiToast.h"
#include "threads/SingleLock.h"
+#include "utils/StringUtils.h"
@jmarshallnz
Member

Much nicer. If the cosmetics are taken care of and it handles a failure in update/getdirectory then all good.

@t-nelson
Contributor
t-nelson commented Mar 3, 2014

I think this discards a feature where we show the parent item regardless of user settings in the event that a list is empty.

@xhaggi
Member
xhaggi commented Mar 3, 2014

@t-nelson sorry i don't know what you mean? could you please explain it in detail.

@jmarshallnz
Member

I don't think so, as that is taken care of in CGUIMediaWindow::Update() or thereabouts ?

@jmarshallnz
Member

jenkins build this please

@xhaggi
Member
xhaggi commented Mar 4, 2014

is something wrong with git on arm and linux

unknown revision or path not in the working tree.
Use '--' to separate paths from revisions
@jmarshallnz jmarshallnz merged commit 960df53 into xbmc:master Mar 4, 2014

1 check failed

default Merged build #312 failed in 3 min 43 sec
Details
@jmarshallnz
Member

It happens if there's a communication issue.

@jmarshallnz
Member

Ah, missed there was a fixup commit - a bit too quick on the button :p

@jmarshallnz jmarshallnz added the Gotham label Mar 4, 2014
@xhaggi
Member
xhaggi commented Mar 4, 2014

revert and merge again?

@jmarshallnz
Member

No, it's fine - not the end of the world :)

@jmarshallnz jmarshallnz removed the Gotham label Mar 8, 2014
@xhaggi xhaggi deleted the xhaggi:fix-recordings-parent-dir branch Mar 10, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment