search

jansmolders86 / mediacenterjs

A HTML/CSS/Javascript (NodeJS) based Media center
http://mediacenterjs.com
1.29k stars 245 forks source link

Reworked Backend #12

Closed hoffi closed 11 years ago

hoffi commented 11 years ago

Here is my Pull-Request for my Backend-Rework.

READY TO MERGE :+1:

Changes:

Feel free to comment changes you don't like or don't understand why i made them.

jansmolders86 commented 11 years ago

This is such an amazing amount of work Stefan! Really amazing. I can't thank you enough!

hoffi commented 11 years ago

Rework is finished from my side :)

jansmolders86 commented 11 years ago

Amazing work hoffi! I'll merge this tomorrow! This really is great work! Thanks so much. I'll let you know when the merge has been completed.

Thanks again!

jansmolders86 commented 11 years ago

Hej Hoffi,

I have a question, you removed the SQL binaries (these will be create when used). Is this something Linux based systems do automatically? Because Windows cannot do this, so that is way I added the binaries. Maybe I'm just overlooking some of your code though :)

Thanks!

hoffi commented 11 years ago

Yes there is no need to specifiy the path to the sqlite binary as long the path is stored in the path environment variable. But im not sure about windows.

jansmolders86 commented 11 years ago

Thanks Hoffi, that makes sense. Although I personally prefer the binaries to be defined from the code. This means people have to perform less steps to install the application (lowering the bar to actually give MCJS a try)

I've found a couple of things not working for me:

Again amazing job! I'm really grateful! So, I'm happy to fix these issues if you want to move on to something else, but if you are willing to address these issues, these are what I have found and I would be very grateful. I can add the issues I find as Github issues in your fork if you want :)

hoffi commented 11 years ago

Oh yeah, somehow overlooked the placeholder image in movies and music. Also fixed the setup.

When exactly happens the problem with the blank boxes?

jansmolders86 commented 11 years ago

Thanks Hoffi for fixing these minor bugs so fast! The blank boxes happen when using a clean setup (clean database). But it might be just a Windows thing due to the fact that SQLite was not added to my system variables. I'm not sure yet. :)

Does the music player load individual songs in your setup? Or could that be a Windows bug as well?

hoffi commented 11 years ago

Looks like a windows problem, i have no problems with a clean setup.

I will check the musicplayer on monday

jansmolders86 commented 11 years ago

Hej Hoffi, Thanks for adding the placeholder images. I guess it's also needed to write these placeholders when the scraper gives an undefined. Otherwise you get the following error:

 C:\Users\jan.smolders\Documents\GitHub\hoffi\mediacenterjs\apps\movies\metadata-fetcher.js:110
            moviedb.movieInfo({ id: result.results[0].id }, function(err, response) {
                                                                    ^
 TypeError: Cannot read property 'id' of undefined

That and the music player are the only issues I found the rest works really well! When these two issues have been addressed we can merge! :)

jansmolders86 commented 11 years ago

Thank you Stefan for fixing the error for me. I Implemented what I learned from your code in the plugin-manager, and I really learned a lot from your code! Thank you. But sadly I have not been able to fix the music problem myself yet. If you have the time to take a look I would be very happy since we can merge it then :)

jansmolders86 commented 11 years ago

Hi, I fixed the music problems.

I've started merging this but I've made quite a lot of changes. Do you mind if I merge it the same way as I did with the plugin-manager (which means your Github stats will not be updated)? Or do you want me to push all my changes to your branch and then merge everything?

hoffi commented 11 years ago

Ok, i could not reproduce the music problem, so thanks ;) I don't mind how you merge them. You can do it how you want :)

jansmolders86 commented 11 years ago

Cool! Thanks. It was basically a Windows only problem (slashes where interpreted wrong). Merging this is quite a task though. Hopefully I'll be done tomorrow. Would you be so kind and test it when the merge has been pushed? Just in case I messed something up :) ow, btw one question why did you decide to use a config template? Just curious :)

hoffi commented 11 years ago

Sure i can test it :)

I added a config template cause it was annoying for me to always remove my config from a commit because it contained my local path and so on. I also had to clean all things up when i added new settings to it.

With the template you don't have to mind about those things and i personally think this is more clean :)

jansmolders86 commented 11 years ago

It's done :) Great idea with the config template. I tested it with all kinds of crazy files and folders and it worked great. I hope the same can be said for your end. If so, I'll bump the version and close this thread. Thanks again!

hoffi commented 11 years ago

Great! :) i will test it today in the evening.

hoffi commented 11 years ago

Hi,

does not look like everything was merged correctly. Here are the problems i found:

jansmolders86 commented 11 years ago

Thanks for testing Hoffi I'll fix it tomorrow, and let you know.

jansmolders86 commented 11 years ago

Hej Hoffi, I have an update for you:

What I certainly have learn is to use seperate branches before merging to the master. It's a real bummer to push something to master that is not working properly. I would suggest working with sprints or releases as soon as possible. Developing them in a seperate branch and then merging them to the master when tested. This way the master will stay stable and less buggy. Do you agree?

Also, shall we close this pull request if everything works on your end?

jansmolders86 commented 11 years ago

I'll close this Pull request Hoffi. If issues arise feel free to open a new issue for it :)