Implement initial UI

[djalmaaraujo]

@linusg Adding you here to track the work you are doing.

[linusg]

@djalmaaraujo Sure, sorry for the rant. I have got a basic wrapper for the script working, still not completely happy and thus improving it one more time. Expect the code in a few minutes, just one question:

At the time my instantiation looks like this:

window.spotifyDownloader = new SpotifyDownloader('/home/linus/.virtualenvs/spotdl/bin/python', '/home/linus/spotify-downloader/spotdl.py');

Not sure if there's a more straightforward way to create an object accessible in all react components than window.spotifyDownloader, but let's talk about this later. I'm concerned about having hardcodd paths like this in the git. Any advice?

[sdhutchins]

Hmm yeah that's a bit problematic. I've been reading on the other threads. Trying to keep up with you guys lol

[linusg]

Also, just to let you know, I've started some work on the spotify-downloader side. Ritiek is fine with adding several error codes, so here we go: https://github.com/ritiek/spotify-downloader/issues/162.

[linusg]

So, even though I have not pushed yet for obvious reasons, here's a GIF:

The file is indeed showing up:

The new error codes in spotify-downloader (not yet published) work fine too:

I got the exit callback working, so next would be a loader icon showing up while it's fetching.

So, for now I'm waiting for anyone's response in terms of pushing these paths. :wink:

[linusg]

Another huge update for you guys:

Here's the PR, please comment and comment and comment! https://github.com/djalmaaraujo/spotifube/pull/12

(Sorry, I've been super busy & excited and forgot to split into multiple commit, so it's one huge thing)

[linusg]

Update: you will need the newest commit of the return-codes branch: https://github.com/ritiek/spotify-downloader/tree/return-codes

[sdhutchins]

I would probably edit how the errors show up and what not, but I like the general look of this.

[linusg]

Sure, any sketches, explanations or even actual code changes are appreciated 🙂

[sdhutchins]

A thought on the spotdl.py script and paths...

1. We could ask the user to place spotdl.py in their path or...
2. We could tell the user to place it in their home path.

In python, you can easily get to the home path cross platform:

from pathlib import Path
home = str(Path.home())

I don't know an equivalent way of doing that w/ electron right now.

3. We could have an option at some point wiht this app to download/install spotdl & its dependencies...that would ensure it's installed where we want for the app to work.

[ritiek]

Just my $0.02, maybe could ask the user to point towards spotdl.py's location from the GUI and then just call the script from the same path with necessary arguments?

It should be possible to run the spotdl.py from any current working directory as of ritiek/spotify-downloader@d39272d6a2854f203482c5fbdaa55fb9de1f3db9. I've merged this change and also the branch return-codes with ritiek/spotify-downloader#164 to master.

[djalmaaraujo]

@linusg Awesome awesome work. Looking pretty good. Just one heads up. Since you are running "bash" commands, this file: src/renderer/spotdl/index.js could be inside main folder, right? renderer is just for UI things.

@ritiek Awesome suggestion. Maybe the first run could have this. We can use https://www.npmjs.com/package/electron-config to save this and the default output folder, although right now I would ask for a new folder on every playlist, since we don't have a preferences UI yet.

[djalmaaraujo]

@linusg I will try to get sometime later and do some refactoring in the CSS part. We should try to split the component into smaller parts, but I wouldn't matter right now.

[linusg]

Yep, was not too sure where tu put the file as it somehow mixes up pretty much. Ok, CSS refactoring can be done soon or later. I can look into the package you've suggested and implement a very basic settings dialog. It would be the most clean way in my eyes to also have the Python binary and the script as a config value, not just in PATH, as venvs are very common at least for pythonistas.

[sdhutchins]

Just my $0.02, maybe could ask the user to point towards spotdl.py's location from the GUI and then just call the script from the same path with necessary arguments?

Definitely the easy way for starting off things assuming they know where it is lol! Users can be...somewhwat confused.

[linusg]

Definitely the easy way for starting off things assuming they know where it is lol! Users can be...somewhwat confused.

I mean, they had to manually clone the repository as it's the only way we provide the spotdl.py script for now. Not on PyPI, any Linux distro repos or similar. And the have to issue a command with the python binary, I think we might expect such a basic level of knowledge :wink:

So, I tried implementing a first startup dialog including two inputs (might change this to a file dialog later, for now it's really simple but avoids the hardcoded path). You'll see and understand, but I'm stuck:

On compiling I get this:

WARNING in ./node_modules/conf/index.js
25:27-43 Critical dependency: the request of a dependency is an expression

It simply won't run and I have no idea how to resolve this. Various searches brought some ideas what the issue is about but no satisfying solution yet.

Code is this:

import { Config } from 'electron-config';
import React from 'react';

const config = new Config();

export class InitialConfig extends React.Component {
  ...
}

Later, in the Main component there's simply a <InitialConfig />.

Please help!

[sdhutchins]

Can you push that to a separate branch so that I can check it out and debug? @linusg

[linusg]

Im on the phone right now, but later I can push the (not working) code to the initial-ui branch or create yet another one. What would you prefer?

[sdhutchins]

@linusg I'd say create a new branch, broken-config. After debugging, can push to the initial-ui since it is a "working" demo.

[djalmaaraujo]

@linusg I don't know if it's going to work or the real problem is this, but might be. Try to remove the {} in this line import { Config } from 'electron-config'; should work.

correct would be:

import Config from 'electron-config';

[linusg]

Ah, come on, that's not fair :wink: Still not at home but I'll try for sure, thanks to both of you!

[linusg]

So, this did not work. Sorry, @djalmaaraujo :disappointed: But here you go, sorry for being late: https://github.com/djalmaaraujo/spotifube/commit/6d66934ce7850c4da857ad1831a46e92608a4af3 (branch config-dialog) I've changed it back to an import with curly braces, did not work either way.

[linusg]

Ah, I noticed I pushed it without the braces (https://github.com/djalmaaraujo/spotifube/commit/6d66934ce7850c4da857ad1831a46e92608a4af3#diff-92bdc4e9b9726f98a6f40f4b6d5bf621R1). I'm tired, until tomorrow folks.

[linusg]

So, just another fast update: I found an (closed) issue (and a dup) on their GitHub: https://github.com/sindresorhus/electron-store/issues/3 So it's Webpack's fault, but I have no idea how to integrated the proposed solution as we don't even have a webpack config (somehow managed by electron-webpack?). Feel free to investigate.

[djalmaaraujo]

@linusg I am using this module in another project. Try electron-store instead of -config.

const Store = require('electron-store');
const store = new Store();

ps: try the latest version. yarn add electron-store

[linusg]

Still no luck with electron-strore (neither with and without braces nor with require or import), seems to be a problem caused by webpack with the underlying conf module for both electron-config and electron-store.

WARNING in ./node_modules/conf/index.js
25:27-43 Critical dependency: the request of a dependency is an expression

[linusg]

Just a heads up, electron-config seems to be electron-store: https://github.com/sindresorhus/electron-store/issues/4 :wink:

[sdhutchins]

@linusg you get this working? I've been reading on the above thread? Currently been testing it out.

[linusg]

Stop working on this specific issue, I got it. Spent some hours on it, read a lot of docs and hat to upgrade electron-webpack for being able to extend their webpack config. Here you go, the config-dialog branch is gone: https://github.com/djalmaaraujo/spotifube/pull/12/commits

[linusg]

When using wrong paths, it fails with a "unknown error" (I noticed this accidentally), we might change this to a more specific error. However, the dialog works and appears, whenever there's not both the script and the python binary found in the config. Right after the initial config is done and saved, it will not show up anymore.

Please tell me what you think, and what you're working on or planning to work on.

@djalmaaraujo indeed splitting up the sass code should be done soon or later :wink:

[djalmaaraujo]

Will test soon. Not on the computer now.

On Sat, Dec 2, 2017, 8:51 PM Linus Groh notifications@github.com wrote:

When using wrong paths, it fails with a "unknown error" (I noticed this accidentally), we might change this to a more specific error. However, the dialog works and appears, whenever there's not both the script and the python binary found in the config. Right after the initial config is done and saved, it will not show up anymore.

Please tell me what you think, and what you're working on or planning to work on.

@djalmaaraujo https://github.com/djalmaaraujo indeed splitting up the sass code should be done soon or later 😉

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/djalmaaraujo/spotifube/issues/11#issuecomment-348719222, or mute the thread https://github.com/notifications/unsubscribe-auth/AAANSnfK47i-RzoOoQu4eB0v0lMLC-IAks5s8bhDgaJpZM4QvAoZ .

-- Djalma Araújo +351 910623833 nossomos.cc

[sdhutchins]

Tested it and it works well @linusg! What are you about to turn to working on next? I'll fill in things along the way.

[linusg]

Glad to hear! Please feel free to pick a task you would like to work on, as I don't have anything uncommited for now. Of course there's a lot to do even for stage 0, reading the created txt file and displaying the number of songs, downloading (Button, dialog, choosing a path, next time taking the path as default but still being able to choose another one), more and proper error handling, refactoring of the Sass code, improving the initial config (checking the existence of the paths), adding more explanations, probably tooltips, ... So, you may tell me what you like and I choose my next task then. :smile:

[sdhutchins]

Right now, I'm just sort of playing around, but I'm interested in working on reading the created txt file and displaying the number of songs as well as making some minor edits to the sass, but not much.

[djalmaaraujo]

@sdhutchins just do it, don't think, hehehehe. :)

[linusg]

Yup, that would be a great next step @sdhutchins! I think it might be easier for us to not use the -l flag of the spotdl.py script but rather the -s flag to download each song separately. This way we can easily implement a progress widget and calculate the percentage (songs_downloaded/songs_total). Just to keep in mind, except you guys have something different in mind.

[linusg]

And are we still continuing working on the initial-ui branch until we have a working download button or can we merge this into master and continue with a new one for parsing and displaying the txt file and another one for download? Still no complains on my code, thought there would be more critical reviews hehe :wink:

[djalmaaraujo]

@linusg I think if @sdhutchins tested your code, we can merge to master so she can work in a new feature. I will do a review now and merge your code.

[djalmaaraujo]

@linusg Did an initial review. Can you please do some changes before I merge?

[djalmaaraujo]

@sdhutchins You can always start from his branch and later rebase with master.

[sdhutchins]

@djalmaaraujo will do.

[djalmaaraujo]

Code reviewed and merged. New branch, New PR.

[djalmaaraujo]

Wohoo 🍬🍬🍭🍭🍧🍧🍦🍦🍰🍰🎂🎂

[linusg]

Nice! But this issue is still in progress, right? 😉 Can't wait for the initial release...

[djalmaaraujo]

@linusg I think we have enough for initial UI. We already have colors, sizes, etc. Let's finish this issue once for all.