release/react

[TonyBrobston]

@lasley Here's what I have so far.

Everything seems right, however it doesn't rip the tracks selected. I'm wondering if I'm passing the parameters to the socket.emit( incorrectly.

When I click "Rip Tracks" it fires the actionRipTracks from api.js

actionRipTracks(
    this.props.discName,
    this.props.driveId,
    trackIds
);

In this case the discName = "MARVELS_THE_AVENGERS", driveId = "/dev/sr0", and trackIds = "['1']".

When I dug in further to look at what your master branch does, I found this:

_this._socket_cmd('rip_track', {
    'save_dir': save_dir,
    'drive_id': drive_id,
    'track_ids': checked_boxes
})

This makes it seem like I have the incorrect attribute names in the json.

Old looks to be:
rip_track
{
'save_dir': 'MARVELS_THE_AVENGERS',
'drive_id': '/dev/sr0',
'track_ids': ['1']
}

New looks to be:
doRipTracks
{
'discName': 'MARVELS_THE_AVENGERS',
'driveId': '/dev/sr0',
'trackIds': ['1']
}

Am I thinking of this correctly?

[TonyBrobston]

I needed to conditionally display <DiscInfo />. If DiscInfo's constructor fired without tracks available, I got some wonky behavior.

[lasley]

Better way to do this would be something like:

discInfo = () => {
  if(...) {
    return null
  }
  return <DiscInfo ... />
}

Then just discInfo() when using

[TonyBrobston]

If I set the name to the trackId and used the event, I was able to determine if a track was selected without using jQuery, which I thought was nice; we could likely eliminate jQuery completely.

[lasley]

Yeah jQuery is superfluous once React comes into play. I just didn't quite realize it back then ;)

[TonyBrobston]

I was hoping to use a .map( function here, however for every track that I didn't return it would default to undefined. The result I wanted was something like: ['1', '5'], however map would produce [undefined, '1', undefined, undefined, undefined, '5']. I'm wondering if a filter or reduce method would be a better option.

[lasley]

Yep you want reduce

[TonyBrobston]

I'll add this back once I get ripping working.

[TonyBrobston]

I just noticed makemkv.js, this explains a lot. I'll start looking here. Maybe adding some console logs will help me better understand what's going on.

[TonyBrobston]

Kind of confused. driveId is a Set, but you're interacting with it like it's an array. Not sure which you meant for it to be, so I stuck with a set.

[lasley]

Yep this was wrong. Looks like the conditional doesn't do anything, so not really sure what I was going for honestly

[TonyBrobston]

Hoping this actually removes the last element from the set and returns it; rather than just returning it. Hoping to test drive some of this stuff later on, but just trying to get things working quick-and-dirty for now with minimal change.

[lasley]

It doesn't, and you can't access sets by index either. Better would probably be to just switch the set back to an array - it was just to clear up an edge case when someone submits the same track twice. It would only be possible if there are two UIs open talking to the server at the same time, and both submit the same track for ripping (within milliseconds of each other). Totally not going to happen outside of a lab

[lasley]

But if you did want to handle this:

const ripTracks = [...this.ripQueue.driveId];
const ripTrack = ripTracks.pop();
this.ripQueue.driveId.delete(ripTrack);

Seems like a lot of work though, when we could accomplish the same functionality by just guarding the array push for the track

[TonyBrobston]

Found a few more small problems. Fixed a few, now I'm on to another issue, not quite sure what "file" argument this stacktrace is referring to at the moment. More investigating to do.

Spawning "(exitCode, stdErr, stdOut) => {
            this.busyDevices.delete(driveId);
            let strOutput = stdOut.join('\n'),
                isSuccess = !exitCode && strOutput.indexOf('1 titles saved.') !== -1;
            callback(trackId, isSuccess, strOutput);
        }" with args: "--noscan,mkv,--cache=1024,--profile=/opt/node-makemkv/conversion_profile.xml,dev:/dev/sr1,0,/var/done/THE_ONION_MOVIE"
child_process.js:381
    throw new TypeError('"file" argument must be a non-empty string');
    ^

TypeError: "file" argument must be a non-empty string
    at normalizeSpawnArguments (child_process.js:381:11)
    at exports.spawn (child_process.js:494:38)
    at MakeMkv._spawn (/opt/node-makemkv/makemkv.js:457:23)
    at MakeMkv.ripTrack (/opt/node-makemkv/makemkv.js:259:14)
    at MakeMkv.ripTracks (/opt/node-makemkv/makemkv.js:226:14)
    at MakeMkvServer.doRipTracks (/opt/node-makemkv/server.js:250:22)
    at Socket.client.on (/opt/node-makemkv/server.js:207:51)
    at Socket.emit (events.js:180:13)
    at /opt/node-makemkv/node_modules/socket.io/lib/socket.js:513:12
    at process._tickCallback (internal/process/next_tick.js:176:11)

[lasley]

@TonyBrobston - I just saw this & it looks totally promising! I'll review this in depth and get back to you. Sorry for the crap state of this project code - it's been a while since this thing has seen some love.

[TonyBrobston]

@lasley Thanks for the reply. No need to apologize about the state of things, lol. I'm just super glad this repo exists. As I find time I'll likely try to make improvements. Life has been busy, I imagine as it gets colder here in Iowa I'll find more time.

If you help me with the remainder of getting this branch working, I'll slowly add fixes and enhancements as I use it. I've recently been using jest and eslint and have some interest in adding them to this repo.

You've done an amazing job so far. I owe you a beer.

[lasley]

@TonyBrobston

Do you use the mainline branch? It's definitely worth checking out, even if it's not React.

I've recently been using jest and eslint and have some interest in adding them to this repo.

Agreed on both of these. There's a long ago branch where I was adding lint and tests, but that was lost to the ether. Pretty sure I dropped it when the tests identified a crapton of bugs I didn't want to fix 😆