Version 2, ridding of reliance on web-service #6

IT-MikeS · 2017-12-05T04:40:28Z

Putting all changes from v2 branch into new master branch pull request.

Done:

swap to module to do downloading and conversion on computer instead of using a web-service.

Needs doing:

Testing on platforms other than windows
Create user setting type store where a folder can be set as the download destination to improve flow for user.
Better comment code to reflect changes made to get to 2.x.x
Set application to use users platform default downloads folder as default store for converted files.

…e to download and convert right on the machine!

Big changes! No longer rely on an hosted webservice, we user ytdl-cor…

…ation so download and conversion are separated progress bars, set path for downloaded file to platform default downloads folder.

…ation so download and conversion are separated progress bars, set path for downloaded file to platform default downloads folder with display for user.

IT-MikeS · 2017-12-05T16:11:03Z

Update

Added:

Converted files go automatically to the appropriate platforms downloads folder.
Added local conversion function that I had forgot before so now files actually get turned into mp3
Separated the progress for download and conversion, downloading to 100% then converting to 100%

Currently working on:

Allowing user to set a default folder that will persist through application exit and re-open.
Creating better error handling and testing possible common errors.
Progressively commenting and refactoring to clean up names and make everything more understandable

Needs doing that I can not accomplish:

Testing application on other platforms

…nce between reopening of the app.

IT-MikeS · 2017-12-06T17:30:10Z

Update

Added:

Function to allow user to set a default folder that will persist through application exit and re-open.
Some more comments

leerob

This seems much more complicated than what we were doing previously... could you explain the advantages of doing this? Specifically getting and mp4 and then converting it to mp3 versus using yt-mp3.

leerob · 2017-12-07T15:52:19Z

public/scss/global.scss

@@ -93,3 +95,11 @@ body {
      cursor: pointer;
  }
 }
+
+.outputFolderDisplay {


Prefer to use BEM naming, otherwise use hypen-case versus camelCase.

Sure I'll make the adjustment

leerob · 2017-12-07T15:53:27Z

app/containers/app.container.js

-      return <ProgressBar progress={this.state.progress} messageText={this.state.progressMessage}/>;
+      return (
+        <div>
+          <div className="outputFolderDisplay">


This appears to be the same code in both the if and else block. Maybe we can make a new React component for this code? Something like OutputFolder.

Yeah I was thinking about that too, still fiddling with react but I'll get it there

leerob · 2017-12-07T15:53:52Z

app/containers/app.container.js

+        <div>
+          <div className="outputFolderDisplay">
+            <a href='#' onClick={this.changeOutputFolder}>Change</a>
+            <span>&nbsp;&nbsp;Current output folder:&nbsp;</span>{this.state.userDownloadsFolder}


Would it make more sense to use CSS padding versus   ?

Yeah this is more just for now working on fixing this

leerob · 2017-12-07T15:54:13Z

app/containers/app.container.js

+      return (
+        <div>
+          <div className="outputFolderDisplay">
+            <a href='#' onClick={this.changeOutputFolder}>Change</a>


Do you even need to put # if there's not actually a link? I'm not sure.

I do, but I'm not sure either

So the answer is no, but without it there is no "link" style to the "a" tag so I'll just remove the href and add css styling

I quite like the css I put on it better than the default look of a html link and no need for class just style the a tag inside of containing element

leerob · 2017-12-07T15:54:46Z

app/containers/app.container.js

+    let fileSelector = remote.dialog.showOpenDialog({properties: ['openDirectory'], title: 'Select folder to store files.'});
+
+    // If a folder was selected and not just closed, set the localStorage value to that path and adjust the state.
+    if(fileSelector) {


There should be a space between if and the ( e.g.

if (fileSelector)

leerob · 2017-12-07T15:55:33Z

app/containers/app.container.js

-        progress: Math.floor(progress)
-      });
+  changeOutputFolder() {
+    // Create an electron open dialog for selecting folders, this will take into account platform


Nit: Prefer to end comments that read as sentences with a period.

leerob · 2017-12-07T15:57:24Z

app/containers/app.container.js

+    this.setState({progressMessage: 'Converting...', progress: 0});
+
+    return new Promise((resolve, reject) => {
+      // reset the rate limiting trigger just encase.


Nit: Prefer to start inline comments with a capital letter.

IT-MikeS · 2017-12-07T18:21:53Z

The main benefit of using this over yt-mp3 is that we were bound to that service, this limits you in a few significant ways.

If they disappear the app breaks
They have only so many "workers" on their server which can lead to long wait times to download and convert a video
Progress tracking on the web-service can be spotty
Any time their API changes the app needs an update

As for why we download an mp4 then convert it to mp3: this is more of a limitation workaround, youtube videos provide audio in mp4 format, we could download this and leave it there, and most modern players and software will be able to read it, but we convert to mp3 to ensure the greatest compatibility across players and platforms.

leerob · 2017-12-07T19:00:46Z

Well, if it’s one line of HTML to get the styling versus a CSS class, then we should just do that. I was just curious.

…

On Dec 7, 2017, 12:52 PM -0600, Mike S ***@***.***>, wrote: @IT-MikeS commented on this pull request. In app/containers/app.container.js: > } } render() { if (this.state.showProgressBar) { - return <ProgressBar progress={this.state.progress} messageText={this.state.progressMessage}/>; + return ( + <div> + <div className="outputFolderDisplay"> + <a href='#' onClick={this.changeOutputFolder}>Change</a> So the answer is now but without it there is no "link" style to the "a" tag so I'll just remove the href and add css styling — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

leerob

I was able to pull things down and test them out. Everything appeared to work as excepted. I like having the two different loading bars for downloading and converting, good idea.

I think instead of saying Conversion successful! Resetting in 5 seconds, we can just say Conversion successful! and reset after 1 second.

I'm sorry I didn't articulate this more clearly earlier, but I guess what I had in mind for the folder selection was to have it hidden in a menu. That way it wasn't modifying the UI at all. So in the menu bar there could be a "Preferences" option and underneath that would be a "Download Folder" option. Clicking that would pop the modal asking for the location and then continue to save it to local storage like such.

That would prevent us from having the text at the bottom of the application. I want to keep the UI very simple. I think it's okay that it doesn't show which folder it's downloading to.

leerob · 2017-12-07T21:11:27Z

app/containers/app.container.js

@@ -4,6 +4,7 @@ import ProgressBar from '../components/ProgressBar';
 const ytdl = window.require('ytdl-core');
 const fs = window.require('fs-extra');
 import * as path from 'path';
+import OutputPath from "../components/OutputPath";


Should these imports be ordered? https://eslint.org/docs/rules/sort-imports

IT-MikeS · 2017-12-08T01:24:48Z

No worries, now hearing it, I like your idea for the output folder settings better. As for the conversion successful message your modification makes sense, I'll make both these changes as soon as I can and the sort order changes too

…u item, and app now resets after 1 second after successful download and conversion.

IT-MikeS · 2017-12-08T12:45:15Z

I made all the modifications in your last comment and I think it looks and feels very good, great suggestions!

… set folder.

leerob · 2017-12-08T15:46:11Z

app/containers/app.container.js


    // If a folder was selected and not just closed, set the localStorage value to that path and adjust the state.
    if (fileSelector) {
      let pathToStore = fileSelector[0];
      localStorage.setItem('userSelectedFolder', pathToStore);
      this.setState({userDownloadsFolder: pathToStore});
+      console.log(`New current path ${localStorage.getItem('userSelectedFolder')}`)


This probably isn't needed anymore

Yeah just forgot to take the log out lol

leerob · 2017-12-08T15:52:41Z

Do you happen to know what bitrate the mp3s get converted to?

Anyways, just tested everything and I totally agree. The menu option is awesome! I tried downloading short, medium, and long length videos. They all worked really well! Super excited that this now support videos that are 1 hour long. Maybe I'll test it with something that's like a day long haha.

Nice work on this! 🎉

leerob · 2017-12-08T15:56:50Z

Did you have any trouble packaging the app? It's saying it can't find the logo file, but it is definitely there.

IT-MikeS · 2017-12-08T16:02:46Z

For the answer to the bitrate: its a value we can set in the ffmpeg lib, right now its at 160 (cd quality)

As for the packaging issue, its the path to the icon, pushing a update that should resolve this.

IT-MikeS · 2017-12-08T16:15:12Z

That should resolve the issue you were having

IT-MikeS · 2017-12-11T13:16:31Z

Test building on windows, and tested with 24 hour video all is working well here

leerob · 2017-12-12T16:37:22Z

Looks good to me! This should allow the user to easily change bitrates from a menu option in the future as well, which is nice 🎉

IT-MikeS · 2017-12-12T17:47:01Z

That's what I thought too, was going to work on download queue and better preferences menu, what do you think?

leerob · 2017-12-12T18:02:34Z

That could be pretty neat! One thing I had thought about doing was utilizing Electron DL for something similar. Check it out: https://github.com/sindresorhus/electron-dl

…

On Dec 12, 2017, 11:47 AM -0600, Mike S ***@***.***>, wrote: That's what I thought too, was going to work on download queue and better preferences menu, what do you think? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or mute the thread.

IT-MikeS · 2017-12-13T13:15:50Z

Okay i'll work on it, as for electron-dl, I like the idea but I think we'll need to modify it a bit to make it work with youtube videos

IT-MikeS and others added 5 commits December 4, 2017 12:34

Big changes! No longer rely on an hosted webservice, we user ytdl-cor…

e784dc6

…e to download and convert right on the machine!

Merge pull request #1 from IT-MikeS/v2

d14131b

Big changes! No longer rely on an hosted webservice, we user ytdl-cor…

PR #5 changes in accordance with authors comments

e21f1b5

bump to reflect small changes from PR #5 review

bb20777

bump to reflect small changes from PR #5 review

7b384ff

IT-MikeS mentioned this pull request Dec 5, 2017

Doesn't Work on Ubuntu #2

Closed

IT-MikeS added 2 commits December 5, 2017 10:33

Forgot to add conversion code for mp4 to mp3, expanded on user inform…

bb7077d

…ation so download and conversion are separated progress bars, set path for downloaded file to platform default downloads folder.

Forgot to add conversion code for mp4 to mp3, expanded on user inform…

3ad62d0

…ation so download and conversion are separated progress bars, set path for downloaded file to platform default downloads folder with display for user.

IT-MikeS added 3 commits December 5, 2017 13:28

Commented on new download and conversion code, also cleaned up functions

ef8854e

Added functionality for user to change output location, with persiste…

df486f0

…nce between reopening of the app.

More comments and code clean-up

f1c1041

upgrade project dependancies

810f684

leerob suggested changes Dec 7, 2017

View reviewed changes

Changes made in accordance with PR review

cd84a06

leerob suggested changes Dec 7, 2017

View reviewed changes

Change download folder is now a menu option under the preferences men…

c242a80

…u item, and app now resets after 1 second after successful download and conversion.

Small tweak to make the change download prompt open to your currently…

f1ea8d6

… set folder.

leerob reviewed Dec 8, 2017

View reviewed changes

IT-MikeS added 2 commits December 8, 2017 10:58

Fix package icon not found issue

eb7d49c

Fix package icon not found issue

2f80178

leerob approved these changes Dec 12, 2017

View reviewed changes

leerob merged commit a866db1 into leerob:master Dec 12, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Version 2, ridding of reliance on web-service #6

Version 2, ridding of reliance on web-service #6

IT-MikeS commented Dec 5, 2017

IT-MikeS commented Dec 5, 2017

IT-MikeS commented Dec 6, 2017

leerob left a comment

leerob Dec 7, 2017

IT-MikeS Dec 7, 2017 •

edited

leerob Dec 7, 2017

IT-MikeS Dec 7, 2017

leerob Dec 7, 2017

IT-MikeS Dec 7, 2017

leerob Dec 7, 2017

IT-MikeS Dec 7, 2017

IT-MikeS Dec 7, 2017 •

edited

IT-MikeS Dec 7, 2017

leerob Dec 7, 2017

leerob Dec 7, 2017

leerob Dec 7, 2017

IT-MikeS commented Dec 7, 2017

leerob commented Dec 7, 2017 via email

leerob left a comment

leerob Dec 7, 2017

IT-MikeS commented Dec 8, 2017

IT-MikeS commented Dec 8, 2017

leerob Dec 8, 2017

IT-MikeS Dec 8, 2017

leerob commented Dec 8, 2017

leerob commented Dec 8, 2017

IT-MikeS commented Dec 8, 2017

IT-MikeS commented Dec 8, 2017

IT-MikeS commented Dec 11, 2017

leerob commented Dec 12, 2017

IT-MikeS commented Dec 12, 2017

leerob commented Dec 12, 2017 via email

IT-MikeS commented Dec 13, 2017

Version 2, ridding of reliance on web-service #6

Version 2, ridding of reliance on web-service #6

Conversation

IT-MikeS commented Dec 5, 2017

IT-MikeS commented Dec 5, 2017

Update

Added:

Currently working on:

Needs doing that I can not accomplish:

IT-MikeS commented Dec 6, 2017

Update

leerob left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IT-MikeS Dec 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IT-MikeS Dec 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IT-MikeS commented Dec 7, 2017

leerob commented Dec 7, 2017 via email

leerob left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IT-MikeS commented Dec 8, 2017

IT-MikeS commented Dec 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leerob commented Dec 8, 2017

leerob commented Dec 8, 2017

IT-MikeS commented Dec 8, 2017

IT-MikeS commented Dec 8, 2017

IT-MikeS commented Dec 11, 2017

leerob commented Dec 12, 2017

IT-MikeS commented Dec 12, 2017

leerob commented Dec 12, 2017 via email

IT-MikeS commented Dec 13, 2017

IT-MikeS Dec 7, 2017 •

edited

IT-MikeS Dec 7, 2017 •

edited