Skip to content

Schema/refresh phase2#516

Merged
subdavis merged 11 commits into
masterfrom
schema/refresh-phase2
Jan 4, 2021
Merged

Schema/refresh phase2#516
subdavis merged 11 commits into
masterfrom
schema/refresh-phase2

Conversation

@subdavis
Copy link
Copy Markdown
Contributor

@subdavis subdavis commented Dec 30, 2020

Phase 2: what really happened.

Phase 2 went differently than I expected. I doubled back on some choices from Phase 1, and it turned out less involved than I originally thought, so rather than try to make a bunch of changes in 1, then roll them back in 2, I'd prefer to just review 2 as if it were the only change. This will close #515

  • Revert to loadDetections / loadMetadata instead of loadDataset. There was no need to remove the decoupling between these methods. I was wrong that detections requires metadata: it only actually requires knowlege of the base data path. Extra reasoning is explaining inline in a comment in desktop/frontend/store/index.ts
  • Reorganize the folder structure a bit with desktop/frontend and desktop/backend for clarity.
  • Explain the 1 exception: keep loading detections directly from the renderer thread because it's a waste of compute and memory to buffer the whole file into memory twice (in the server thread, then in the client thread) to load it.
  • Keep recents and settings in localStorage. When I originally wanted to move them, I forgot the chicken-and-egg problem: you have to know where the file is on disk before you can load it. LocalStorage provides everything I was going to go to the trouble of implementing myself in the server (levelDB at a fixed location on disk) so it seems foolish to write code to end up with what you've already been given for free.
    • This leaves the minor annoyance of the backend having to ask the frontend for settings, but that's what IPC is for. It's not really as bad as I was making it out to be.
  • MIgrate to express.js webserver, add new endpoints for metadata save, metadata load, and import.
    • Media import could have been done with REST or IPC. Doesn't seem to matter to me which is used.
  • Changed the signature of SaveDetectionArgs since the object provided no benefit over a simpler array. This involves a simple server-side change.

Not included

Persistence for job run history (logs will not be populated if a job is loaded from this storage area)
Error handling for importMedia issues. Right now they just dump to console and nothing happens in the UI.
Error handling for when source files change location.

In this sense, this is more of a phase 1.5.

Some notes on the size of the changes

The size of this PR is misleading. I renamed some files (most notably frontend/api.ts) and reordered the contents. Many other files have been renamed, and git is considering them 100% changed for some reason.

It's probably closer to 850+, 500-

@subdavis subdavis requested a review from BryonLewis December 30, 2020 18:05
@subdavis subdavis mentioned this pull request Dec 30, 2020
Copy link
Copy Markdown
Collaborator

@BryonLewis BryonLewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Big PR and seems fully functional on Linux at least. Went through most of the standards of testing the loading, swapping, running pipelines and others for the electron version. I also tested to make sure the changes to loading meta/detections didn't have any adverse effect on the server. All seems stable. I didn't worry about windows testing as of this moment.

Most of what I have are just my standard annoying questions and wondering where stuff is heading towards. I do think we shouldn't do the moving of track.json in source media folder and instead do a copy.

Comment thread client/platform/desktop/backend/native/common.ts Outdated
Comment thread client/platform/desktop/backend/server.ts Outdated
Comment thread client/platform/desktop/backend/ipcService.ts Outdated
Comment thread client/platform/desktop/backend/native/common.ts
Comment thread client/platform/desktop/backend/native/common.ts
Comment thread client/platform/desktop/backend/native/common.spec.ts Outdated
Comment thread client/platform/desktop/frontend/store/index.ts
@subdavis subdavis merged commit b1575ec into master Jan 4, 2021
@subdavis subdavis deleted the schema/refresh-phase2 branch January 4, 2021 14:02
@subdavis subdavis mentioned this pull request Jan 4, 2021
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