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] package size metadata #9971

Merged
merged 6 commits into from Jun 19, 2016

Conversation

@tamland
Copy link
Member

commented Jun 12, 2016

Adds a size element (intended for repositories to set) which is the size of downloadable zip file. It can be displayed in a readable form with the Listitem.AddonSize into label.

This pr also includes some much needed db cleanup that should make it a bit less painful to add more metadata fields in the future.

while (i < units.size() && value >= 999.5)
{
++i;
value = bytes / std::pow(1024.0, i);

This comment has been minimized.

Copy link
@fritsch

fritsch Jun 12, 2016

Member
value /= 1024

would save the pow

This comment has been minimized.

Copy link
@stefansaraev

stefansaraev Jun 15, 2016

Contributor

hmh. 999.5... ? and kB should be KB (those have different meanings)

This comment has been minimized.

Copy link
@tamland

tamland Jun 16, 2016

Author Member

@stefansaraev yes, 999.5. this is a smarter rounding that scales decimals with significance. See tests.

Also, I've never seen "KB" being used..

This comment has been minimized.

Copy link
@MartijnKaijser

MartijnKaijser Jun 16, 2016

Member

maybe add line note to mention it's intentional?

@stefansaraev

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2016

nice.

std::string StringUtils::FormatFileSize(uint64_t bytes)
{
const std::array<std::string, 5> units{"B", "kB", "MB", "GB", "TB"};
if (bytes < 1000)

This comment has been minimized.

Copy link
@stefansaraev

stefansaraev Jun 15, 2016

Contributor

1024

This comment has been minimized.

Copy link
@tamland

tamland Jun 16, 2016

Author Member

same. intentional

@tamland tamland force-pushed the tamland:addon_package_size branch from e69137b to e938f9a Jun 18, 2016
@tamland

This comment has been minimized.

Copy link
Member Author

commented Jun 18, 2016

Ok, added a docstring that should hopefully make it clearer how it's intended to work (there are also tests that shows the output of different sizes)

@tamland

This comment has been minimized.

Copy link
Member Author

commented Jun 18, 2016

jenkins build this please

@tamland tamland force-pushed the tamland:addon_package_size branch from e938f9a to 635401b Jun 18, 2016
@tamland

This comment has been minimized.

Copy link
Member Author

commented Jun 18, 2016

jenkins build this please

@tamland tamland merged commit 32a6916 into xbmc:master Jun 19, 2016
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
default You did a great job. Have a cookie.
Details
@HitcherUK

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2016

@tamland Any chance of getting ListItem.Property(Addon.Size) added to the addon info dialog as well please?

@phil65

This comment has been minimized.

Copy link
Member

commented Jun 19, 2016

I think it's listitem.addonsize and should already be available there?

@stefansaraev

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2016

yes. $INFO[ListItem.AddonSize] worked for me when testing this PR.

@HitcherUK

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2016

Looks like I've jumped the gun a bit as this isn't in the nightlies yet, thanks.

@Memphiz

This comment has been minimized.

Copy link
Member

commented on xbmc/addons/AddonDatabase.cpp in 8f7b6a2 Aug 28, 2016

@tamland hey there - as you were the last one touching this code i wanted to ask you about this crash report here:
http://forum.kodi.tv/showthread.php?tid=285493&pid=2403470#pid2403470

  • any idea what might be the issue (already requested the addons.db for verification)? I think we have to somehow make sure that this is really json in there before deserializing it or gracefully bail out.

This comment has been minimized.

Copy link
Member Author

replied Aug 29, 2016

Do you know which line is the problem? I can't tell from the backtrace.. Everything that's inserted goes through SerializeMetadata above so as long as Deserialize matches it should be impossible for it to contain invalid data.

Maybe there's an issue with CVariant? The only schema change should be this: f9de660#diff-130a411077441f19c8eb4b80f5b382d7R95 But as far as I can tell, CVariant should handle converting 'null' to vector..

This comment has been minimized.

Copy link
Member

replied Sep 7, 2016

Well i got an adodn db - this one here:

http://forum.kodi.tv/showthread.php?tid=285493&pid=2408437#pid2408437

While it has a different stacktrace - it indeed crashes on my osx system. But its unrelated to this. But maybe you have an idea if this is expected with circular dependencies:

http://pastebin.com/1EJnAyXG

@phil65 as far as i can see it is an circular dependency between script.module.t9.search and script.module.kodi65 which have both eachother in the required section. Not sure if we support this or not?

This comment has been minimized.

Copy link
Member

replied Sep 7, 2016

@Memphiz circular was fixed way back in v14 or so but perhaps it has raised it's head again

This comment has been minimized.

Copy link
Member

replied Sep 7, 2016

Yeah maybe - i think people started complaining since the aug 24th atv4 testbuild i posted - the one before from 24th july was ok they stated. Wouldn't count on those information though. Who are the team devs with proper knowledge of this code part?

@Memphiz

This comment has been minimized.

Copy link
Member

commented on 8f7b6a2 Aug 30, 2016

@tamland i've got the DB in Case you wanna beat me to it ;)
http://forum.kodi.tv/showthread.php?tid=285493&pid=2404398#pid2404398

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