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

fix for the files node in both music and video windows #159

Closed
wants to merge 8 commits into from
Closed

fix for the files node in both music and video windows #159

wants to merge 8 commits into from

Conversation

marcelveldt
Copy link
Contributor

@Ignoble61 it turns out that the shortcut returned by the sscript for the files entry was wrong, this little fix should cover that

@ghost
Copy link

ghost commented Mar 4, 2016

Can I ask what's wrong with it? I've only tested for videos as my music database needs rebuilding, but Kodi uses source://video/ when navigating from root node to files node, and the shortcut I have that Skin Shortcuts created - ActivateWindow(Videos,sources://video,return) - works...?

@marcelveldt
Copy link
Contributor Author

Yep it works, but it's missing the "add files" entry at the bottom

@ghost
Copy link

ghost commented Mar 4, 2016

So it is :) What an odd little bug. From a very quick test, it looks like the fact that Skin Shortcuts doesn't append a slash stops that entry from showing up 😕

Code looks fine if you want to merge this.

@marcelveldt
Copy link
Contributor Author

yeah, I know. Sounds to me like a Kodfi bug.
Want to merge it in like this or should we just append the trailing slash at all times ?

@ghost
Copy link

ghost commented Mar 4, 2016

Depends how much time you have :) (I'm just on my lunch break right now!) My personal preference would be to append the trailing slash so that we're consistently using library nodes, rather than mixing and matching paths. But I don't know off-hand whether all node types have or need the trailing slash - that would need testing before merging :)

Or we can merge the fix in now and switch to the slash as and when it's remembered to do so in the future :)

@marcelveldt
Copy link
Contributor Author

You're the (grand)father of the script so your call. I can update the code to make sure there is a trailing slash, that shouldn't harm at all.

@ghost
Copy link

ghost commented Mar 4, 2016

I would prefer the trailing slash, but only if you have the time to add + test it. More important is working code :)

@ghost
Copy link

ghost commented Mar 7, 2016

Have you decided what you want to do with this - merge in now, or add the trailing slash?

@marcelveldt
Copy link
Contributor Author

I will go with the slash route, I have time tomorrow night to fix and test it.

@ghost ghost mentioned this pull request Mar 30, 2016
@ghost
Copy link

ghost commented Apr 8, 2016

@marcelveldt It's been about a month since this was last touched. Are you still planning to go the 'slash route' when time allows, or would you prefer it be merged as it is, and the 'slash route' taken in the future?

(If at all possible, I'd like to see an update to the script on the repo in the not too distant future as I and others are increasingly using features only on git, but this fix - or a version of it - really needs to go in before that happens, and for preference time for a beta :))

@marcelveldt
Copy link
Contributor Author

Sorry I did the tests in the meanwhile and all was fine but I still have to write the code and submit a new PR. I was very busy last weekd with work so now I have to catch up with a lot of reports/requests on the forums ;-)

I will see if I get some time tomorrow to put the trailing fix in and prep a new beta on the repo.

Thanks for the reminder!

@marcelveldt
Copy link
Contributor Author

I'll close this one as my local fork has some issues I can't get fixed.
Just reforked and will do another PR

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

Successfully merging this pull request may close these issues.

1 participant