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

Addsource jsonrpc #5484

Closed
wants to merge 11 commits into from
Closed

Addsource jsonrpc #5484

wants to merge 11 commits into from

Conversation

topfs2
Copy link
Contributor

@topfs2 topfs2 commented Oct 10, 2014

Took out the addsource jsonrpc from the headless addition. @Montellese to review

@Tolriq
Copy link
Contributor

Tolriq commented Oct 10, 2014

Me again :)

Perhaps since adding this take : e99054a in account and handle allowsharing too to be complete ?

@@ -227,6 +234,74 @@ JSONRPC_STATUS CFileOperations::Download(const std::string &method, ITransportLa
return transport->Download(parameterObject["path"].asString().c_str(), result) ? OK : InvalidParams;
}

CONTENT_TYPE contentTypeFromString(const std::string &content) {

This comment was marked as spam.

@Montellese
Copy link
Member

Do we want to allow multiple directories per source like it can be done in the user interface? The API could support either a single string or an array of strings for the directory parameter.

Furthermore this is missing all the options that can be set per source (which are a bit ambiguous).

We'll probably have to extend the result of Files.GetSources to return all the options and having a Files.EditSource or something like that would be nice as well. But we can get Files.AddSource sorted out first and then we can start looking at the others.

@topfs2
Copy link
Contributor Author

topfs2 commented Oct 11, 2014

Yeah agree on the multiple directory, will change

{
ADDON::AddonPtr scraperAddon;
ADDON::CAddonMgr::Get().GetDefault(ADDON::ScraperTypeFromContent(contentTypeFromString(contents[1])), scraperAddon);
ADDON::ScraperPtr scraper = boost::dynamic_pointer_cast<ADDON::CScraper>(scraperAddon);

This comment was marked as spam.

@Montellese
Copy link
Member

Looks a lot nicer already.

@topfs2 topfs2 added the Helix label Oct 12, 2014
@topfs2
Copy link
Contributor Author

topfs2 commented Oct 16, 2014

@Montellese Kindof unsure on how we should do the parameters here if we want to support allowshare? It feels like its a property on the source, so unrelated to the scraper settings. But feels a bit odd to have name, paths, content, source_options, scraper_options?

@Montellese
Copy link
Member

@topfs2: allowshare is a porperty of the source and not the scraper. As long as we don't allow scraper selection I wouldn't allow setting them anyway. We can still add those later as scrapersettings or something like that if we want.

Maybe we should try to find better naming for the source options? And we should also add descriptions as to which make sense for which media type and what they do.

PS: You still have the contentTypeFromString method in your code but it's unused now.

@topfs2 topfs2 added v15 Isengard API change Type: Improvement non-breaking change which improves existing functionality and removed Helix labels Nov 3, 2014
@Montellese
Copy link
Member

Are you planning on getting this into shape for the next merge window?

@topfs2
Copy link
Contributor Author

topfs2 commented Jan 31, 2015

I could, not sure what was left to do on it though?

@Montellese
Copy link
Member

From reading the previous comments I'd say

  • remove your own CONTENT_TYPE contentTypeFromString(const std::string &content) as it's not used anymore.
  • find better names for the source settings and add some documentation for them
  • add a default value for every parameter / property that is not required
  • optionally add support for allowsharing (but since this is not even supported in the GUI right now it's not that important as long as it gets set to true by default).

@jez500
Copy link

jez500 commented Jan 14, 2017

Hi All,
Id love to integrate this into Chorus is there any plans to look at this again or has it been replaced with another PR I can follow?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Improvement non-breaking change which improves existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants