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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move eject drive and eject disk to new contextmenu system #10890

Merged
merged 1 commit into from Dec 12, 2016

Conversation

@Razzeee
Copy link
Member

commented Nov 7, 2016

Description

Marvel your eyes, as your now able to eject your disks and drives right from your homescreen 馃殌

Motivation and Context

No ejection from the homescreen 鈽笍

How Has This Been Tested?

Only tested eject disk, as I have no drive to eject in my reach.

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the Code guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the CONTRIBUTING document
  • I have added tests to cover my change
  • All new and existing tests passed
@ksooo

This comment has been minimized.

Copy link
Member

commented Nov 8, 2016

Not sure whether it is a good idea to introduce the new cpp and header file in the xbmc source root folder.

@ksooo

This comment has been minimized.

Copy link
Member

commented Nov 8, 2016

Otherwise, this is a nice one.

But who nowadays is still using physical disks?. /me runs ;-)

@Razzeee

This comment has been minimized.

Copy link
Member Author

commented Nov 8, 2016

I'm just moving it because it's there, I'm not using it either.
And yes, if you have a better idea then a class in root, let me know. We will have more cases where context menus are not addon/video/music specific

@ksooo

This comment has been minimized.

Copy link
Member

commented Nov 8, 2016

if you have a better idea then a class in root, let me know.

sorry, no idea. maybe it is even okay to have it in the root directory for "global" context menus. @tamland ?

@Razzeee

This comment has been minimized.

Copy link
Member Author

commented Dec 8, 2016

jenkins build this please

2 similar comments
@Razzeee

This comment has been minimized.

Copy link
Member Author

commented Dec 10, 2016

jenkins build this please

@Razzeee

This comment has been minimized.

Copy link
Member Author

commented Dec 12, 2016

jenkins build this please

@Razzeee Razzeee merged commit 5342ce9 into xbmc:master Dec 12, 2016

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
jenkins4kodi You did a great job. Have a cookie.
Details

@Razzeee Razzeee deleted the Razzeee:contextmenu-eject branch Dec 12, 2016

@ksooo

This comment has been minimized.

Copy link
Member

commented Dec 13, 2016

@Razzeee hmm, why the hurry? Imo, this one needs to be reworked. Comments follow...

@ksooo
Copy link
Member

left a comment

Could you please address my comments and open a follow-up PR? Thnaks.


bool CEjectDisk::IsVisible(const CFileItem& item) const
{
return item.IsRemovable() && (item.IsDVD() || item.IsCDDA());

This comment has been minimized.

Copy link
@ksooo

ksooo Dec 13, 2016

Member

Imo here a

#ifdef HAS_DVD_DRIVE
  return item.IsRemovable() && (item.IsDVD() || item.IsCDDA());
#else
  return false;
#endif

would be a good choice to 100% prevent this item to appear on builds compiled without dvd drive support.

*/

#include "ContextMenus.h"
#include "guilib/GUIWindowManager.h"

This comment has been minimized.

Copy link
@ksooo

ksooo Dec 13, 2016

Member

Is this include really needed?

But I'm missing #include "FileItem.h"

{
if (item->IsDVD() || item->IsCDDA())
{
buttons.Add(CONTEXT_BUTTON_EJECT_DISC, 13391); // Eject/Load CD/DVD!

This comment has been minimized.

Copy link
@ksooo

ksooo Dec 13, 2016

Member

CONTEXT_BUTTON_EJECT_DISC is no longer used now and schould be removed from dialogs/GUIDialogContextMenu.h

This comment has been minimized.

Copy link
@Razzeee

Razzeee Dec 13, 2016

Author Member

Good catch

}
else // Must be HDD
{
buttons.Add(CONTEXT_BUTTON_EJECT_DRIVE, 13420); // Eject Removable HDD!

This comment has been minimized.

Copy link
@ksooo

ksooo Dec 13, 2016

Member

CONTEXT_BUTTON_EJECT_DRIVE is no longer used now and schould be removed from dialogs/GUIDialogContextMenu.h

This comment has been minimized.

Copy link
@Razzeee

Razzeee Dec 13, 2016

Author Member

Good catch

@MartijnKaijser MartijnKaijser modified the milestone: L 18.0-alpha1 Dec 13, 2016

@Razzeee

This comment has been minimized.

Copy link
Member Author

commented Dec 13, 2016

Nobody had anything to say for 1 and a half month. I don't exactly call this hurry.

Master is open for pre alpha again and I feel like having PRs to be perfect and not giving people the doubt to deliver follow up PRs is also hurting us more then helping. Just my 2 cent :)

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Dec 18, 2016

@ksooo

This comment has been minimized.

Copy link
Member

commented Jul 12, 2017

@Razzeee just discovered that this PR introduces another regression: "Eject" is no longer available in the context menu in Kodi's file manager. Before this, PR, for example, "Eject" appeared in the context menu of a mounted USB stick. Reason is that file manager does not use the new context menu system, but CGUIDialogContextMenu, where this PR removes the items in question from. :-/

@Razzeee

This comment has been minimized.

Copy link
Member Author

commented Sep 3, 2017

So it seems that if "fails" when asking for the m_listProvider as it's null for that case.
Here:

if (m_listProvider)
{
int selected = GetSelectedItem();
if (selected >= 0 && selected < static_cast<int>(m_items.size()))
{
m_listProvider->OnContextMenu(m_items[selected]);
return true;
}
}
return false;

@tamland might have the insight on how to correctly tackle this

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