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

Addons avoid incorrect error message #22437

Merged
merged 1 commit into from
May 11, 2023
Merged

Conversation

emveepee
Copy link
Contributor

Description

With a multi-instance plugin incorrect error messages are logged if the addon has not been setup.

Normally in a typical single profile scenario Kodi multi-instance addons create the userdata settings directory when it is installed. When multiple profiles are in use the settings directory is not created unless the addon is enabled for that profile. There is no requirement to enable all addons.

This PR adds a return if the settings directory has not been created before checking for instance configuration. The return is the same as on failure.

Motivation and context

It is a low priority, but this is not an error, addons don't need to be enabled in all profiles.

This error is generated in the logs

2023-01-11 13:04:15.950 T:7616    debug <general>: Thread PVRGUIProgressHandler start, auto delete: false
2023-01-11 13:04:15.956 T:20468   error <general>: XFILE::CDirectory::GetDirectory - Error getting C:\workspace\nexus-build\portable_data\userdata\profiles\Martin\addon_data\pvr.hts\
2023-01-11 13:04:15.956 T:20468   error <general>: XFILE::CDirectory::GetDirectory - Error getting special://profile/addon_data/pvr.hts/

How has this been tested?

Tested with pvr.hts and pvr.nextpvr installed in the master profile. Create a new profile without enabling the addon to see the error messages.

Testing as been limited but because the return is the same it should be low risk.

What is the effect on users?

Screenshots (if appropriate):

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • None of the above (please explain below)

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

With wulti-instance addon return earlier if the addon hasn't been installed.
@fuzzard fuzzard added this to the Omega 21.0 Alpha 1 milestone Jan 15, 2023
@fuzzard fuzzard merged commit 34a127c into xbmc:master May 11, 2023
1 check passed
Copy link
Member

@ksooo ksooo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm late to the party but @emveepee maybe you can open a follow-up PR with the optimization I suggested?

Comment on lines +286 to 296
std::vector<AddonInstanceId> ret;

if (!XFILE::CDirectory::Exists(StringUtils::Format("special://profile/addon_data/{}/", m_id)))
{
ret.emplace_back(ADDON_FIRST_INSTANCE_ID);
return ret;
}

const std::string searchPath = StringUtils::Format("special://profile/addon_data/{}/", m_id);
CFileItemList items;
XFILE::CDirectory::GetDirectory(searchPath, items, ".xml", XFILE::DIR_FLAG_NO_FILE_DIRS);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emveepee sorry, I missed this PR, but would suggest to optimize this a bit to avoid two string instances with exactly the same content.

std::vector<AddonInstanceId> ret;
const std::string searchPath = StringUtils::Format("special://profile/addon_data/{}/", m_id);

if (!XFILE::CDirectory::Exists(searchPath))
{
  ret.emplace_back(ADDON_FIRST_INSTANCE_ID);
  return ret;
}

CFileItemList items;
XFILE::CDirectory::GetDirectory(searchPath, items, ".xml", XFILE::DIR_FLAG_NO_FILE_DIRS);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ksooo I don't even remember submitting the PR but I need to check what happens when special://masterprofile doesn't exist.

Copy link
Member

@ksooo ksooo May 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emveepee I will open the PR for the optimization I suggested. Not sure I got the message across and I'm very short on time currently. What you say about masterprofile might be valid, but is not at all related to my suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understood what you meant, I thought it was better to look for the issue first as this would be a pretty low priority PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants