Add extend handlers for openUrl, load/save list #991

tinchoz49 · 2020-05-18T17:50:58Z

This PR adds support for openUrl, load/save list handlers:

Update USAGE.MD

new Webamp({
  handleAddUrlEvent() {
    // return Tracks[] to load
  },
  handleLoadListEvent() {
    // return Tracks[] to load
  },
  handleSaveListEvent(tracks: PlaylistTrack[]) {
    // async operation to save the tracks
  },
})

tinchoz49 · 2020-05-18T23:09:33Z

also, do you think that we could allow the user to define the id of the track?

like:

webamp.appendTracks([
    { id: 'track1', url: 'https://example.com/track1.mp3'}
]);

here: https://github.com/captbaritone/webamp/blob/master/packages/webamp/js/actionCreators/files.ts#L224

const { id =  Utils.uniqueId(), defaultName, metaData, duration } = track;

This can be helpful for tracking the songs outside of the webamp app.

captbaritone

Great start, and very close! A few comments but overall this looks good.

I don't think we can let the user supply the ID since we can't ensure it won't collide with values we've created. Could you explain, perhaps with an example, how being able to define the ID would be useful?

captbaritone · 2020-05-18T23:49:40Z

packages/webamp/js/actionCreators/files.ts

+      const tracks = await handleLoadListEvent();
+
+      if (tracks != null) {
+        await dispatch(removeAllTracks());


Why are we awaiting here?

Fixed. I don't know why I tought the removeAllTracks was async.

captbaritone · 2020-05-18T23:53:39Z

packages/webamp/js/actionCreators/files.ts

+  return async (dispatch, getState, { handleSaveListEvent }) => {
+    if (handleSaveListEvent) {
+      const state = getState();
+      const trackIds = getVisibleTrackIds(state);


This returns the ids of the tracks that are visible in the window. If the window is shorter this list is shorter. I think you want getTrackOrder.

I tought it was a selector to get the non deleted tracks, changed to getTrackOrder worked,

captbaritone · 2020-05-18T23:55:15Z

packages/webamp/js/components/PlaylistWindow/ListMenu.tsx

-        onClick={() => alert("Not supported in Webamp")}
-      />
+      <div className="save-list" onClick={() => saveFilesToList()} />
+      <div className="load-list" onClick={() => addFilesFromList()} />


I don't think we need these arrow functions:

<div className="save-list" onClick={saveFilesToList} /> <div className="load-list" onClick={addFilesFromList} />

captbaritone · 2020-05-18T23:56:46Z

packages/webamp/js/types.ts

@@ -688,6 +688,9 @@ export interface Extras {
  handleTrackDropEvent?: (
    e: React.DragEvent<HTMLDivElement>
  ) => Track[] | null | Promise<Track[] | null>;
+  handleAddUrlEvent?: () => Track[] | null | Promise<Track[] | null>;
+  handleLoadListEvent?: () => Track[] | null | Promise<Track[] | null>;
+  handleSaveListEvent?: (tracks: PlaylistTrack[]) => Promise<undefined>;


I think the type PlaylistTrack is an internal type that is not really exposed in the public API. Could we convert these to Track before returning them to the user without losing anything valuable?

Does asking the user to return a promise here add any value? Currently we don't do anything with the promise.

captbaritone · 2020-05-18T23:57:11Z

packages/webamp/js/actionCreators/files.ts

+      const state = getState();
+      const trackIds = getVisibleTrackIds(state);
+      const tracks = getTracks(state);
+      await handleSaveListEvent(trackIds.map((id) => tracks[id]));


Do we need to await here?

captbaritone · 2020-05-18T23:59:27Z

packages/webamp/demo/js/index.js

@@ -158,6 +158,29 @@ async function main() {
        return null;
      }
    },
+    handleAddUrlEvent() {
+      const url = window.prompt("Paste your URL");


Sadly due to CORs this won't work for most URLs. Thanks for including this in the PR so that we can see how it would be used, but lets remove them before we actually merge.

captbaritone · 2020-05-19T00:28:10Z

@remigallego would this API be useful for https://winampify.io? If so, would this approach work?

tinchoz49 · 2020-05-19T05:10:12Z

I don't think we can let the user supply the ID since we can't ensure it won't collide with values we've created. Could you explain, perhaps with an example, how being able to define the ID would be useful?

Maybe I will not need it, I'm going to try a different approach.

My problem was that I'm going to load songs from a key/value store and then at some point I want to persist a playlist of the songs that I have loaded. I need to know which tracks I already have it in my drive and which one don't.

Using the title of the songs is not best option because you can have duplicate songs.

So that's why I thought a custom ID.

const webamp = new Webamp({
    handleSaveListEvent(tracks) {
      tracks.forEach(track => {
        if (!store.has(track.id)) {
           // persist the blob track 
        }
      })
      
      // save the playlist
    }
});

webamp.appendTracks([
  { 
    id: file.key, 
    blob: file
  }
]);

btw this is only a problem if I'm using blob tracks, I can identify a track by the url.

captbaritone

Very close. Just a few more things.

captbaritone · 2020-05-19T16:22:33Z

packages/webamp/demo/js/index.js

+    },
+    handleSaveListEvent(tracks) {
+      console.log(tracks);
+    },


I think we can remove both of these as well.

captbaritone · 2020-05-23T03:21:55Z

yarn.lock

+    url "0.10.3"
+    uuid "3.3.2"
+    xml2js "0.4.19"
+


I think these changes to yarn.lock are incorrect. I'm guessing you don't need any of them.

captbaritone · 2020-05-23T03:24:07Z

packages/webamp/js/actionCreators/files.ts

+          return {
+            url: track.url,
+            metaData: {
+              artist: track.artist || "",


I'm generally not a fan of || "". If the field is nullable, pass null, if it's not nullable then it's required and passing it an empty value is going to cause unexpected behavior. Could we just null out the entire metaData object if any of the required fields are missing?

I try to follow same paths than here:

webamp/packages/webamp/js/actionCreators/files.ts

Line 318 in da7e20c

artist: artist ? artist : "",

Touché 😛

captbaritone · 2020-05-23T03:25:06Z

packages/webamp/js/actionCreators/files.ts

+              album: track.album,
+              albumArtUrl: track.albumArtUrl || "",
+            },
+          };


For bonus points we could move the generation of this array of tracks into a selector in the selectors file. (Not required)

Sure, I name it getUserTracks since Track is an object type to interact with the playlist user and also because getTracks was already taked.

captbaritone · 2020-05-24T07:38:14Z

Looks great to me! Are you up to adding it to the usage.md docs?

tinchoz49 · 2020-05-24T15:31:30Z

Great, I updated the usage.

captbaritone · 2020-05-24T15:33:21Z

Fantastic! I’ll merge this now and try to push an alpha release to NPM later today for you to use/test.

Thanks for all your work on this. Great idea.

tinchoz49 · 2020-05-24T15:37:07Z

thank you!!

captbaritone · 2020-05-24T20:35:44Z

I just published a beta version 1.4.1-beta03 and tried it out here. Seems to work well!

Would you take it for a spin in your project and report back to confirm that it worked? I'll be publishing a new release in a little bit and hopefully you can move there soon.

tinchoz49 added 2 commits May 18, 2020 13:56

Added missing handlers

c9783d0

Merge new yarn workspace version

89e07e3

captbaritone requested changes May 19, 2020

View reviewed changes

tinchoz49 added 3 commits May 19, 2020 01:18

Minor changes

7d3c47f

Minor changes

46fbc35

Replace PlaylistTrack by Track

da7e20c

tinchoz49 requested a review from captbaritone May 19, 2020 05:10

captbaritone requested changes May 23, 2020

View reviewed changes

tinchoz49 added 3 commits May 24, 2020 02:37

Minor changes

47c92e8

Fixed yarn.lock

db3754a

Added getUserTracks selector

3176b38

Added usage doc

eea6924

captbaritone merged commit 2101c74 into captbaritone:master May 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add extend handlers for openUrl, load/save list #991

Add extend handlers for openUrl, load/save list #991

tinchoz49 commented May 18, 2020 •

edited

Loading

tinchoz49 commented May 18, 2020

captbaritone left a comment

captbaritone May 18, 2020

tinchoz49 May 19, 2020

captbaritone May 18, 2020

tinchoz49 May 19, 2020

captbaritone May 18, 2020

captbaritone May 18, 2020

captbaritone May 18, 2020

captbaritone May 18, 2020

captbaritone May 18, 2020

captbaritone commented May 19, 2020 •

edited

Loading

tinchoz49 commented May 19, 2020

captbaritone left a comment

captbaritone May 19, 2020

captbaritone May 23, 2020

captbaritone May 23, 2020

tinchoz49 May 24, 2020

captbaritone May 24, 2020

captbaritone May 23, 2020

tinchoz49 May 24, 2020 •

edited

Loading

captbaritone commented May 24, 2020

tinchoz49 commented May 24, 2020

captbaritone commented May 24, 2020

tinchoz49 commented May 24, 2020

captbaritone commented May 24, 2020

Add extend handlers for openUrl, load/save list #991

Add extend handlers for openUrl, load/save list #991

Conversation

tinchoz49 commented May 18, 2020 • edited Loading

tinchoz49 commented May 18, 2020

captbaritone left a 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

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

captbaritone commented May 19, 2020 • edited Loading

tinchoz49 commented May 19, 2020

captbaritone left a 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

Choose a reason for hiding this comment

tinchoz49 May 24, 2020 • edited Loading

Choose a reason for hiding this comment

captbaritone commented May 24, 2020

tinchoz49 commented May 24, 2020

captbaritone commented May 24, 2020

tinchoz49 commented May 24, 2020

captbaritone commented May 24, 2020

tinchoz49 commented May 18, 2020 •

edited

Loading

captbaritone commented May 19, 2020 •

edited

Loading

tinchoz49 May 24, 2020 •

edited

Loading