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

Bunch of usability improvements, some bug fixes and functional enhancements #56

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

jikamens
Copy link

Thanks for these scripts! I found them quite useful and I was glad not to have to write them from scratch myself!

I'm submitting some changes which I believe are improvements. I've tried to isolate them logically in separate commits.

The only change I'm unsure about is 583f17c. Your code uses the "storeId" field, if it is returned by the API, rather than the "id" field. I found that none of the songs for which the API returned a "storeId" were actually being added to my playlists. When I modified the code to always use "id", all of those songs were imported properly into my playlists.

Since I don't understand the API or your usage of it well enough to understand why "storeId" was being used in your code, I don't know what the global ramifications of my change are. All I know is that, for me at least, the import worked properly without "storeId", and improperly with it.

I've mentioned this change first because as I said it's the one change I can't say for certain is correct. Here are the other changes, which I'm much more confident about:

  • Use argparse for parsing the command line in both ImportList.py and ExportLists.py.
  • Add support for an INI file.
  • Make the username be specified on the command line or in the INI file rather than hard-coding it in preferences.py.
  • Make it possible to specify the android ID on the command line or in the INI file or have the script automatically generate and save a random android ID into the INI file for future use (necessary because the script wouldn't work for me when I used MobileClient.FROM_MAC_ADDRESS, and I assume others will have the same problem).
  • Add the ability to automatically replace existing playlists with the same name when importing one, so the user doesn't end up with many playlists with the same name.
  • Allow the password to be specified in the INI file. Not recommended for long-term use, I know, but it's awful useful in the short term when you need to run the script over and over to import a lot of playlists and/or debug issues with the script.
  • Be smarter about picking the best match for a track and about dropping duplicates. In particular:
    • When there's more than one match, and some of the matches have already been selected and others haven't, then select one of the matches that hasn't been selected yet, rather than selecting the first match.
    • When there's more than one match, prefer exact matches for title, album, or artist, over substring matches.

Jonathan Kamens added 8 commits March 28, 2017 10:38
In preparation for adding a configuration file and command-line
arguments, add code to use argparse to parse the command line. This is
at this point nonfunctional with the exception that the code now
enforces exactly the correct number of arguments on the command line
rather than silently ignoring extra arguments.
Allow the Android ID to be specified in the configuration file or on
the command line, or randomly generated and saved in the configuration
file automatically.
The code was using the storeId field in songs to import them, when it
existed. I don't know why it was doing that, but it wasn't working for
me -- songs added via storeId simply weren't appearing in my
playlists. Removing that logic and always using the id, never the
storeId, fixed this problem. This change may not be "correct", but all
I know is that the code wasn't working for me without the change and
is working with it.
When searching for a track, if there are multiple matches and some of
them are already selected, then use the ones that aren't.

When there are multiple search results, prefer exact matches rather
than substring matches for title, album, artist.
@nicman23
Copy link

nicman23 commented Apr 1, 2017

if a song is not found then i get:


Traceback (most recent call last):
  File "ImportList.py", line 301, in <module>
    search_result = search_for_track(smart_details)
  File "ImportList.py", line 125, in search_for_track
    top_result = best_match(details, search_results)
  File "ImportList.py", line 86, in best_match
    return pairs[0][0]

@jikamens
Copy link
Author

jikamens commented Apr 2, 2017

Fixed in 17d8ede, I hope.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants