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

FIX: ensure real filename from videodb in playlists #12311

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@koying
Contributor

koying commented Jun 16, 2017

fixes issue when downloading subtitles side-by-side

@koying koying added the WIP label Jun 16, 2017

@MilhouseVH

This comment has been minimized.

Show comment
Hide comment
@MilhouseVH

MilhouseVH Jun 21, 2017

Contributor

Thanks @koying, this has fixed the subtitle download issue: https://forum.kodi.tv/showthread.php?tid=298462&pid=2606809#pid2606809

Unfortunately it doesn't appear to have had any effect on the watched statuses: https://forum.kodi.tv/showthread.php?tid=298461&pid=2605996#pid2605996

Contributor

MilhouseVH commented Jun 21, 2017

Thanks @koying, this has fixed the subtitle download issue: https://forum.kodi.tv/showthread.php?tid=298462&pid=2606809#pid2606809

Unfortunately it doesn't appear to have had any effect on the watched statuses: https://forum.kodi.tv/showthread.php?tid=298461&pid=2605996#pid2605996

@koying

This comment has been minimized.

Show comment
Hide comment
@koying

koying Jun 21, 2017

Contributor

Ok, thanks for the feedback

Contributor

koying commented Jun 21, 2017

Ok, thanks for the feedback

@mlburgett

This comment has been minimized.

Show comment
Hide comment
@mlburgett

mlburgett Jun 29, 2017

@koying, is there any info I can gather or tests I can run, to help with the 'watched statuses' issue @MilhouseVH refers to above?

mlburgett commented Jun 29, 2017

@koying, is there any info I can gather or tests I can run, to help with the 'watched statuses' issue @MilhouseVH refers to above?

@Rechi Rechi added the v18 Leia label Jun 29, 2017

@koying

This comment has been minimized.

Show comment
Hide comment
@koying

koying Jun 30, 2017

Contributor

@MilhouseVH Would you mind trying the current version, please

Contributor

koying commented Jun 30, 2017

@MilhouseVH Would you mind trying the current version, please

@MilhouseVH

This comment has been minimized.

Show comment
Hide comment
@MilhouseVH

MilhouseVH Jun 30, 2017

Contributor

@mlburgett can you try build #0629b which includes the latest commit from @koying.

Note this PR continues to be be included in regular test builds, so this updated PR with the new commit will be in tonight's #0630 build.

Contributor

MilhouseVH commented Jun 30, 2017

@mlburgett can you try build #0629b which includes the latest commit from @koying.

Note this PR continues to be be included in regular test builds, so this updated PR with the new commit will be in tonight's #0630 build.

@mlburgett

This comment has been minimized.

Show comment
Hide comment
@mlburgett

mlburgett Jun 30, 2017

@MilhouseVH, I don't see any change using #0629b, all my tv->recordings continue to show 'watched' status. Partially watched recordings do still show correct status (as they have since this first showed up)

mlburgett commented Jun 30, 2017

@MilhouseVH, I don't see any change using #0629b, all my tv->recordings continue to show 'watched' status. Partially watched recordings do still show correct status (as they have since this first showed up)

@da-anda

This comment has been minimized.

Show comment
Hide comment
@da-anda

da-anda Aug 20, 2017

Member

I suppose the new "DynPath" thingy could/should be use here instead of altering the actual "path" that basically acts like the file ID? @FernetMenta ?

Member

da-anda commented Aug 20, 2017

I suppose the new "DynPath" thingy could/should be use here instead of altering the actual "path" that basically acts like the file ID? @FernetMenta ?

@FernetMenta

This comment has been minimized.

Show comment
Hide comment
@FernetMenta

FernetMenta Aug 20, 2017

Member

@da-anda nope, DynPath only get evaluated by inputstream at a time when the file gets opened for playback.
but yes, id (path) should never be changed on a fileitem. better construct a new item insteead of chaning an existing one.

Member

FernetMenta commented Aug 20, 2017

@da-anda nope, DynPath only get evaluated by inputstream at a time when the file gets opened for playback.
but yes, id (path) should never be changed on a fileitem. better construct a new item insteead of chaning an existing one.

@da-anda

This comment has been minimized.

Show comment
Hide comment
@da-anda

da-anda Aug 20, 2017

Member
Member

da-anda commented Aug 20, 2017

@FernetMenta

This comment has been minimized.

Show comment
Hide comment
@FernetMenta

FernetMenta Aug 20, 2017

Member

@da-anda what do you call the "identity of the item"?

Member

FernetMenta commented Aug 20, 2017

@da-anda what do you call the "identity of the item"?

@da-anda

This comment has been minimized.

Show comment
Hide comment
@da-anda

da-anda Aug 21, 2017

Member

@FernetMenta well, the VFS path is basically the identiy of the fileItem, isn't it? As soon as you change the "path" property to something else you a) change the file type (your own words) and b) some things that rely on the VFS path won't work anymore (like some context menu stuff IIRC). Therefor IMO we neither should pass around stub fileItems (like we f.e. do for VideoInfoDialog which makes it impossible to implement a context menu there atm) nor alter the path.

Member

da-anda commented Aug 21, 2017

@FernetMenta well, the VFS path is basically the identiy of the fileItem, isn't it? As soon as you change the "path" property to something else you a) change the file type (your own words) and b) some things that rely on the VFS path won't work anymore (like some context menu stuff IIRC). Therefor IMO we neither should pass around stub fileItems (like we f.e. do for VideoInfoDialog which makes it impossible to implement a context menu there atm) nor alter the path.

@FernetMenta

This comment has been minimized.

Show comment
Hide comment
@FernetMenta

FernetMenta Aug 22, 2017

Member

@da-anda right. IMO we should make those methods private like the comment in the code suggests:
https://github.com/xbmc/xbmc/blob/master/xbmc/FileItem.cpp#L1698
Making use of SetPath is a step in the wrong direction an makes a fix harder.

Member

FernetMenta commented Aug 22, 2017

@da-anda right. IMO we should make those methods private like the comment in the code suggests:
https://github.com/xbmc/xbmc/blob/master/xbmc/FileItem.cpp#L1698
Making use of SetPath is a step in the wrong direction an makes a fix harder.

@pogo1975

This comment has been minimized.

Show comment
Hide comment
@pogo1975

pogo1975 Sep 2, 2017

i dont know if it was reported but whitout this patch if you disable play next video automaticly subtitles works well and downloading subtitles works, with this patch applied if you enable play next video automaticly subtitles works but if next video is played subs remains from the first one...

pogo1975 commented Sep 2, 2017

i dont know if it was reported but whitout this patch if you disable play next video automaticly subtitles works well and downloading subtitles works, with this patch applied if you enable play next video automaticly subtitles works but if next video is played subs remains from the first one...

@MilhouseVH

This comment has been minimized.

Show comment
Hide comment
@MilhouseVH

MilhouseVH Sep 7, 2017

Contributor

@koying: I don't know if this is ever going to be merged, but would you mind rebasing?

Contributor

MilhouseVH commented Sep 7, 2017

@koying: I don't know if this is ever going to be merged, but would you mind rebasing?

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