Video-hub-app: Some problems with paths in the latest main branch

[CrendKing]

1. Now the package name from package.json is already "video-hub-app-3". But the settings directory is still "video-hub-app-2" in main.ts:

GLOBALS.settingsPath = pathToPortableApp ? pathToPortableApp : path.join(pathToAppData, 'video-hub-app-2');

1. I see you added the portable variable around 3 days ago. I like the idea, but the triggering environment variable name is PORTABLE_EXECUTABLE_DIR? Normally environment variables should have some prefix to identify which application it is for, since env is a global shared space. Also, it is for settings, not executable, right? Isn't something like VHA_SETTINGS_DIR better?
2. When I try to create a playlist, I got this exception:

1. Suppose I build a hub and start watching the folder. Now I remove a video file and reopen the app. If I proceed to "Delete old thumbnails", it can't find any thumbnail to delete. I need to close the app and reopen, then I can delete.

[CrendKing]

Here is the values of playlist and sourceFolderMap from createDotPlsFile():

playlist: [
  {
    cleanName: '<title>',
    duration: 123,
    fileName: '<filename>',
    ...
  },
  {
    ...
  },
  ...
]

sourceFolderMap: { '0': { path: 'C:\\Video', watch: false } }

The problematic line is

  for (let i = 0; i < playlist.length; i++) {

    const fullPath: string = path.join(
>     sourceFolderMap[i].path,
      playlist[i].partialPath,
      playlist[i].fileName
    );

    writeArray.push('File' + (i + 1) + '=' + fullPath );
    writeArray.push('Title' + (i + 1) + '=' + playlist[i].cleanName);
  }

I think it is bug to access sourceFolderMap[i]. There should be an inner loop for all the source folders. For example, changing it to sourceFolderMap[0].path would fix the exception, but only works if there is exactly one source folder.

[whyboris]

Thank you for starting the Issue. I merged in the video-hub-app-3 branch into main somewhat prematurely -- containing bugs. But I preferred that option over having to constantly reconcile all PRs against the unmerged branch.

I suspect there may be some other problems that will need fixing.

One space I suspect to have trouble is the index i (in other parts of the code) because I wanted it to be a number but objects convert each key to type string - so I might just end up typing everything to string in the future for consistency and fewer headaches. That change might mess up some .vha2 files created with the current version of the branch 😓 sorry 😓

I wonder if in your example the i has that string / number incongruence 🤔 -- you could do typeOf(i) to see if that's the problem. I suspect sourceFolderMap[1] fails but sourceFolderMap["1"] passes -- do let me know if that's the problem 😅

[whyboris]

The PORTABLE_EXECUTABLE_DIR is a recent addition to the code #461 to address the "Storing portable app settings near the executable" -- and it seems to be specific to electron-builder ... I didn't get a chance to test this change before merging.

[CrendKing]

I wonder if in your example the i has that string / number incongruence 🤔 -- you could do typeOf(i) to see if that's the problem. I suspect sourceFolderMap[1] fails but sourceFolderMap["1"] passes -- do let me know if that's the problem 😅

No. The problem is not type mismatch. The two arrays are irrelevant to each other. There is not any index or key that links them. For instance, in my previous example, I have 100 videos in the view, but only one source folder (C:\Video). Whenever i goes beyond 0, there will be exception.

What I'm suggesting is to have code like this:

for (let i = 0; i < playlist.length; i++) {
  for (const j in sourceFolderMap) {
    if (isItemInFolder(playlist[i], sourceFolderMap[j]) {
      const fullPath: string = path.join(
        sourceFolderMap[j].path,
        playlist[i].partialPath,
        playlist[i].fileName
      );

      writeArray.push('File' + (i + 1) + '=' + fullPath );
      writeArray.push('Title' + (i + 1) + '=' + playlist[i].cleanName);
      break;
    }
  }
}

Or if you can find out where the item is in, something like this:

for (let i = 0; i < playlist.length; i++) {
  const sourceFolderPath: string = getSourcePath(playlist[i], sourceFolderMap);
  const fullPath: string = path.join(
    sourceFolderPath,
    playlist[i].partialPath,
    playlist[i].fileName
  );

  writeArray.push('File' + (i + 1) + '=' + fullPath );
  writeArray.push('Title' + (i + 1) + '=' + playlist[i].cleanName);
}

The PORTABLE_EXECUTABLE_DIR is a recent addition to the code #461 to address the "Storing portable app settings near the executable" -- and it seems to be specific to electron-builder ... I didn't get a chance to test this change before merging.

OK I didn't know that is an electron thing. Well, guess I should blame Google for choosing a poor, non-prefixed global variable name.

On the other hand, do you consider adding a mechanism to store portable settings and document it? For example, if you detect settings.json in the same directory as Video Hub App 3.exe then use it instead of trying to creating one in AppData. This way by default it would still be exactly the same as now, but if users are aware and move the file, they can make it portable. Also if you introduce a "portable installer", you can just create a empty settings.json to jump start the portable settings mechanism.

[whyboris]

Thanks for looking more into the playlist thing -- I've not thought about that code much; I don't think I ever even tested it -- just slapped something together when I was upgrading the whole app to allow more than one source folder and moved on (my biggest aim then was to get the core functionality to work, with intent to fix things later ... now is that time 😝 😅 ).

Please feel free to open a PR for the playlist problems 🙇

My current goal is to finalize the core scanning/re-scanning/extraction-of-screenshots functionality before I spend time on other bugs (which I'm sure are around). I finally have a week vacation starting tomorrow -- so I hope to get more done 😊

I like your idea for placing a settings.json next to the portable version 👍 -- this may work out great (I still haven't tested the portable version with current code to see if it works). I would prefer if I didn't have to have two build steps when creating the portable and installer versions 🤔 -- we'll see how it goes.

ps -- I think the PORTABLE_EXECUTABLE_DIR is a electron-builder thing 🤔 -- not sure you should blame Google 😝

[CrendKing]

ps -- I think the PORTABLE_EXECUTABLE_DIR is a electron-builder thing 🤔 -- not sure you should blame Google 😝

Is electron-builder not by Electron guys or Google? Sorry I had no idea. I've never done anything about node.js before.

[whyboris]

I think the biggest issue is 3 - creating playlist _bug_

It is fixed by #554 with https://github.com/whyboris/Video-Hub-App/pull/554/commits/50967a5295c891ff3c8882036d98e93917fbbc93
Was a 1-liner 😎

This issue will be closed when I merge that PR. I think the other 3 issues are not a big deal (though please let me know if I underestimate their importance).

Since a lot of code has changed, perhaps it's better to open a new issue instead (feel free to copy/paste from this one if it's easier) 👌 🤝 🙇