Skip to content
This repository has been archived by the owner on Jul 8, 2021. It is now read-only.

Add files via upload #40

Merged
merged 1 commit into from
Mar 3, 2018
Merged

Add files via upload #40

merged 1 commit into from
Mar 3, 2018

Conversation

ronmat
Copy link
Contributor

@ronmat ronmat commented Feb 18, 2018

Please excuse me if this was not done correctly, this is my first pull request.
Related to issue #18

Seems to fix the issues with the Videos and Files not starting which hangs the program. It also seems to fix the timeout issues with courses failing to complete due to timeouts. The first issue is a workaround for the download library not recognizing that the API did not start and the other is just changing the timeout for retrying the download. Have not had a failure since I made these changes. There are probably better ways to do this but I am just learning JS and electron.

You can also get rid of the console.log() if you do not want them.

Thanks for contributing!

Please be sure you are following the guidelines at
https://github.com/FaisalUmair/udemy-downloader-gui/blob/master/CONTRIBUTING.md

Seems to fix the issues with downloads not starting and timing out.
@FaisalUmair
Copy link
Owner

FaisalUmair commented Feb 19, 2018

Hi, Thank you so much for the contribution.

I see that you have updated the set retry timeout so that the downloader gets some more time to resume the download in case of network failure.
But whats up with decreasing the number of threads? Have you experienced any issues like Udemy dropping the requesting due to multiple threads?

And issue #18 is mostly about resuming the download from the broken files when the user restarts the app (which I will add support for soon), but a part of the issue is also related to the downloader giving up on resuming at some point during the download which you claim to have fixed.

I am grateful that you fixed the issue. I will test this to make sure that it hasn't broken any other features. Once found to be working fine, I will merge the pull request and make it available in the next release.

Thanks :)

@ronmat
Copy link
Contributor Author

ronmat commented Feb 22, 2018

I wrote a longer response but the reply address was not the same so Git tossed it.

First preference this with I am just learning Javascript and App Development, the last real code work I did was in 8080 assembler to remove floppy based copy protections in the 80's.

The First issue I tried to address were where the Udemy API did not start the download or did not respond to the download request. The status returned was 0 and the error returned by the library did not return until after the timeout which caused a failed download and everything stopped. Re-issuing the dl.start() after 15 seconds is probably overkill since in all the courses that I watched the console either started in the first 3 seconds or never started. Probably the less hacker method would be to wait for the error and restart the download but this was quicker and worked every time.

Another issue was the timeout interval where the download would start and then just pause, timeout and fail. Increasing the timeout and the retry interval took care of this. The lib would return an error ( self.callback is not a function ) There is a pull request to fix this but the library has not been touched in 3 years. Just ignoring it an retrying worked.

There is another issue I did not address. In that case the program hangs at gathering course info and requires restarting the program. It only happens in courses where the data from udemy has "is_practice_test_course: true" in the course data. I would probably suggest just disabling downloads on these courses since what you may get from them might not be usable anyway.

Other than the one practice exam course the 120 other courses downloaded without a hitch after these changes, the definitely could be tuned a bit..

Ron

@FaisalUmair
Copy link
Owner

Hi Ron, Thank you so much for your great work. Since you have already tested it and you believe it improves the download functionality by a lot. I guess it doesn't need a second thought, I will soon merge this. I am currently working on a new update. I will make sure the changes you made are available in next release.

Thanks

@FaisalUmair FaisalUmair merged commit b1a4d16 into FaisalUmair:master Mar 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants