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

Partial revert of imdb crashfix due to a introduced bug #12767

Merged
merged 1 commit into from
Sep 20, 2017

Conversation

lobermann
Copy link
Member

As discussed with @DaVukovic and @koying in slack this reverts part of my previous imdb crash fix as it was causing a bug in that content can no longer be added manually to the videolibrary.

I would appreciate if also somebody else can also try this PR, so that we are really sure it fixes the issue :-)

one part of the fix for the crashed on imdb caused an issue
that it is no longer possible to manually add content to
the videolibrary. Removing one of the checks for a non
existing VideoInfoTag is getting rid of that bug.
Copy link
Contributor

@koying koying left a comment

Choose a reason for hiding this comment

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

Thanks.
We probably want someone to confirm this fixes the issue, though

@DaVukovic
Copy link
Member

Probably I'm able to confirm the fix later in the evening. I will test it for sure, not later than tomorrow.

@DaVukovic
Copy link
Member

@koying @lobermann

I tested it just now and manually adding files works at least for movies, which doesn't worked before. So at least that is fixed. I assume that everything else will work, too.

I can do further tests tomorrow if needed. Just ping me in case.

@MilhouseVH
Copy link
Contributor

@DaVukovic
Copy link
Member

Probably not related because the user misses to mention that's the artwork which is missing. The scraping itself seems to work fine

See: https://forum.kodi.tv/showthread.php?tid=298461&pid=2643266#pid2643266

@arnova
Copy link
Member

arnova commented Sep 11, 2017

Please elaborate how this fixes your issue. I don't get why the check for empty Videoinfotag caused a problem as an empty tag triggered a crash here previously, right?

@koying
Copy link
Contributor

koying commented Sep 12, 2017

Because the tag is almost guaranteed to be empty, here, and the returnactually prevents it to be filled by the RefreshItemModal

The actual crash fix is, I assume, fixed by the other half of the reverted commit, which stays

@razzeee
Copy link
Member

razzeee commented Sep 13, 2017

jenkins build this please

@Rechi Rechi added the v18 Leia label Sep 17, 2017
@arnova
Copy link
Member

arnova commented Sep 17, 2017

@koying: Ah yes, now I see. Thanks for explaining it. +1 in that case.

@MartijnKaijser
Copy link
Member

Was this issue already fixed in Krypton branch?

@koying
Copy link
Contributor

koying commented Sep 18, 2017

The regression was introduced in 18.
I don't know if the original crash issue is present in 17 as well...

PS: original PR: #12373

@koying
Copy link
Contributor

koying commented Sep 20, 2017

jenkins build and merge

@brozikcz
Copy link

brozikcz commented Sep 20, 2017

The regression is in 17.4 version too.

https://forum.kodi.tv/showthread.php?tid=321180

@jenkins4kodi jenkins4kodi merged commit 71a59dc into xbmc:master Sep 20, 2017
@Rechi Rechi added this to the L 18.0-alpha1 milestone Sep 20, 2017
@Rechi Rechi added Backport: Needed Type: Fix non-breaking change which fixes an issue labels Sep 20, 2017
@koying
Copy link
Contributor

koying commented Sep 20, 2017

Ah, right. The regression was backported to 17.4

@MartijnKaijser
Copy link
Member

@lobermann care to create a fix for krypton?

@a1rwulf
Copy link
Member

a1rwulf commented Sep 21, 2017

afaik he is on vacation so I made the PR:
#12822

I did just checkout the Krypton branch and cherry-picked, I didn't even compile!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backport: Done Type: Fix non-breaking change which fixes an issue v18 Leia
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet