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

Rework, refactor, revamp web-ui #1872

Closed
wants to merge 14 commits into from

Conversation

abcdefghijorngarbosaxyz
Copy link

@abcdefghijorngarbosaxyz abcdefghijorngarbosaxyz commented Jul 1, 2023

Title

Use existing web-ui dist dir files

Description

Add a feat that checks if the dist dir already exists, use it instead, if not download the dist dir.

Related Issue

#1871

Motivation and Context

I like to reduce the server startup time. Also reduce internet data transmission.

How Has This Been Tested?

Some external pytest and running spotdl in poetry shell, as instructed in contributing.md

Screenshots (if appropriate)

running spotdl web with existing dist dir

image

running spotdl web with non-existing (deleted or first time user) dist dir

image

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project
  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have read the CONTRIBUTING document
  • I have read the CORE VALUES document
  • I have added tests to cover my changes
  • All new and existing tests passed

PS Had one error on mypy test but its not related to my changes :

Incompatible types in assignment (expression has type "AudioProvider", variable has type "Piped")  [assignment]

BREAKING CHANGE

Need to add release on the web-ui repo(currently none). A function checks for updates base on latest release on web-ui repo.

@abcdefghijorngarbosaxyz abcdefghijorngarbosaxyz changed the title Dev Use existing web-ui dist dir files, if already downloaded Jul 1, 2023
@xnetcat
Copy link
Member

xnetcat commented Jul 1, 2023

But we still have to check if we aren't running an outdated version of web ui. Maybe add a file in the web repo with version, and a flag to force the update of web ui. Also consider implementing web ui version check in the flag that checks for updates.

@abcdefghijorngarbosaxyz
Copy link
Author

But we still have to check if we aren't running an outdated version of web ui. Maybe add a file in the web repo with version, and a flag to force the update of web ui. Also consider implementing web ui version check in the flag that checks for updates.

I'm currently working on the web-ui. It gets the version from the package.json, which then bundles as string in the static files. Which then compares it to the latest version number and informs the user if they're running an outdated web-ui, allows them to update.

@abcdefghijorngarbosaxyz
Copy link
Author

Please allow me to convert this to a draft.

@abcdefghijorngarbosaxyz abcdefghijorngarbosaxyz marked this pull request as draft July 2, 2023 06:45
@abcdefghijorngarbosaxyz
Copy link
Author

@xnetcat I added a function that gets the current web-ui version and the latest version from github api. Will update the files if not latest, before starting the server.

@abcdefghijorngarbosaxyz abcdefghijorngarbosaxyz marked this pull request as ready for review July 3, 2023 03:23
@xnetcat
Copy link
Member

xnetcat commented Jul 4, 2023

PR looks good, but I am thinking that maybe I should convert it to PR for web-ui rework.?

@abcdefghijorngarbosaxyz
Copy link
Author

PR looks good, but I am thinking that maybe I should convert it to PR for web-ui rework.?

Of course. If you think its the appropriate.

@xnetcat xnetcat changed the title Use existing web-ui dist dir files, if already downloaded Rework, refactor, revamp Jul 11, 2023
@xnetcat xnetcat changed the title Rework, refactor, revamp Rework, refactor, revamp web-ui Jul 11, 2023
@abcdefghijorngarbosaxyz abcdefghijorngarbosaxyz marked this pull request as draft July 11, 2023 15:53
@xnetcat
Copy link
Member

xnetcat commented Jul 12, 2023

Just letting you know that if you need help with anything let me know!

@abcdefghijorngarbosaxyz
Copy link
Author

abcdefghijorngarbosaxyz commented Jul 13, 2023

Just letting you know that if you need help with anything let me know!

Haven't made real progress yet on the web-ui. Waiting on merge of #1865, if ever. And, also working on the desktop app.

Planning to implement #1779 in the app.

@xnetcat xnetcat linked an issue Jul 13, 2023 that may be closed by this pull request
@xnetcat
Copy link
Member

xnetcat commented Jul 13, 2023

Ahh I didn't know you needed #1865. Merging it right now.

@abcdefghijorngarbosaxyz
Copy link
Author

Actually i need help. I'm trying to add --output-dir arg on the cli, like --use-web-output-dir, but can't think any implemention other than concatinating the path to the output format like c:\users\user\path\to\dir\{title} - {artist}.{output-ext}

@xnetcat
Copy link
Member

xnetcat commented Jul 13, 2023

what do you need it for?

@abcdefghijorngarbosaxyz
Copy link
Author

what do you need it for?

Its not related to web-ui rework. LOL. Im implementing it in the desktop app. Please take a look at this.

And #1779 will be in the desktop app, not in the web-ui rework. I have some ideas but can't really implement it in Python(no knowledge on syntax and modules to use). But I can do it in Rust, so.

@xnetcat
Copy link
Member

xnetcat commented Jul 16, 2023

gj with #1779 few questions though. why CD to downloads, why does it need elevated perms?

@abcdefghijorngarbosaxyz
Copy link
Author

why CD to downloads

When link with custom uri is clicked, example spotdl://https://open.spotify.com/track/5sICkBXVmaCQk5aISGR3x1?si=8f3379819fb24ecf

It opens a new cmd window, then execute spotdl download "url" there. but cwd will be in win32 folder because cmd is there lol. so need to cd to other folders, like the current user's downloads folder atleast, so the downloaded file will be saved there.

Also if we remove the cmd start and just execute the spotdl download "url" i dont know where the browser starts the command and download. Maybe in the same folder where the browsers exe file is? I haven't checked.

why does it need elevated perms

I think registering a winreg key needs elevated access. I tried it with no elevation it gives a permission error.

@xnetcat
Copy link
Member

xnetcat commented Jul 16, 2023

thanks for explaining, maybe try adding some info about where songs are being downloaded when using uri opener etc and maybe add some logging to Inform users on what's happening.

@abcdefghijorngarbosaxyz
Copy link
Author

When a url like this spotdl://https://open.spotify.com/track/5sICkBXVmaCQk5aISGR3x1?si=8f3379819fb24ecf is clicked it will show alert on browser:

image

and opens a cmd window like this

image

Its exactly the spotdl download tui.

About the download folder maybe we can add the user's preferred path when installing like spotdl --install-uri-scheme "C:\some\folder". Maybe with sys.argv but I think its too hacky.

@xnetcat
Copy link
Member

xnetcat commented Jul 16, 2023

or get a module path then create a path to script and start it with the python or something.

traversing all $PATH directories isn't the best idea someone might be running py -m spotdl because their python environment is messed up

@abcdefghijorngarbosaxyz
Copy link
Author

I used print insted of the logger. For some reason the logger doesnt output on cmd when registering the winreg key, but it does work on PermissionError block. I like to reiterate that I'm noob in python so I need help in this 🤣

@abcdefghijorngarbosaxyz
Copy link
Author

or get a module path then create a path to script and start it with the python or something.

traversing all $PATH directories isn't the best idea someone might be running py -m spotdl because their python environment is messed up

Ill try this. Another use case is if the user is using a downloaded compile exe.

@xnetcat
Copy link
Member

xnetcat commented Jul 16, 2023

or get a module path then create a path to script and start it with the python or something.

traversing all $PATH directories isn't the best idea someone might be running py -m spotdl because their python environment is messed up

Ill try this. Another use case is if the user is using a downloaded compile exe.

yeah that's what I've meant by spotdl.exe

@xnetcat
Copy link
Member

xnetcat commented Jul 16, 2023

I used print insted of the logger. For some reason the logger doesnt output on cmd when registering the winreg key, but it does work on PermissionError block. I like to reiterate that I'm noob in python so I need help in this 🤣

logging is not initialized at this point of cli lifecycle. it's totally fine to use prints here

@abcdefghijorngarbosaxyz
Copy link
Author

Also, I think we'll need to implement the --output-dir arg so we can use it in the uri command like spotdl download "url" --output-dir "c:\some\folder" instead of having to cd into the folder.

So when custom uri is used the browser will show
image

instead of
image

The "trying to open windows command processor" will be confusing

@xnetcat
Copy link
Member

xnetcat commented Jul 16, 2023

there's already a --output flag, what would --output-dir change?

@abcdefghijorngarbosaxyz
Copy link
Author

In description the --output flag specifies the download file name format. Users can't control the uri command.
--output-dir can be used in spotdl --install-uri-scheme "C:\some\folder"

So the uri command will be spotdl download "url" --output-dir "C:\some\folder" instead of cd "C:\some\folder" && spotdl download "url"

The "trying to open windows command processor" will be confusing

If you really prefer the --output flag we can just use it and user will install the uri scheme with something like spotl --install-uri-scheme "C:\some\folder\{title} - {artist}.{output-ext}", but in this way, the output is fixed, at least with --output-dir, only the dir will be fixed, user can still change the output in config.json and will reflect on the uri command.

Hope you get what im meaning

@xnetcat
Copy link
Member

xnetcat commented Jul 16, 2023

file name is not required when passing a directory path to the --output flag

@xnetcat
Copy link
Member

xnetcat commented Jul 16, 2023

#1728

@abcdefghijorngarbosaxyz
Copy link
Author

file name is not required when passing a directory path to the --output flag

Oh I didnt know that. So we can just pass the output folder?

@abcdefghijorngarbosaxyz
Copy link
Author

It actually worked. Lol. Some new knowledge for me.
image

@xnetcat
Copy link
Member

xnetcat commented Jul 16, 2023

Ok I think I get what you mean now. Since we have to CD first, browser will show an alert with trying to open the windows command processor message. Maybe try doing spotdl -v && cd path && spotDL download url console gets cleared before downloading songs so this shouldn't be an issue.

@abcdefghijorngarbosaxyz
Copy link
Author

Having spotdl as the first argument wont launch a cmd window. No idea why.

@stale
Copy link

stale bot commented Sep 16, 2023

This issue has been automatically marked stale because there hasn't been any activity for the last 30 days.

@stale stale bot added the Stale Issue with no activity for the last 30 days label Sep 16, 2023
@stale stale bot closed this Oct 15, 2023
@xnetcat xnetcat reopened this Oct 16, 2023
@stale stale bot removed the Stale Issue with no activity for the last 30 days label Oct 16, 2023
Copy link

stale bot commented Dec 15, 2023

This issue has been automatically marked stale because there hasn't been any activity for the last 30 days.

@stale stale bot added the Stale Issue with no activity for the last 30 days label Dec 15, 2023
@stale stale bot closed this Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Stale Issue with no activity for the last 30 days
Projects
None yet
2 participants