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

Reorg to support electron build config #383

Merged
merged 13 commits into from
Oct 16, 2020
Merged

Reorg to support electron build config #383

merged 13 commits into from
Oct 16, 2020

Conversation

subdavis
Copy link
Contributor

@subdavis subdavis commented Oct 13, 2020

This peels apart the girder-specific parts of the client into platform/web-girder and leaves the main client in viame-web-common. Electron (and any future build configurations, such as a theoretical girderless client/server rewrite with flask or fastapi or something) will live in platforms/

This replicates the provide pattern with an API implementation defined in viame-web-common/apispec.ts, such that each platform will provide(implementation) and viame-web-common/components/Viewer.vue will inject(implementation).

This is the lowest-impact solution I could come up with. There are others. Feel free to discuss or suggest alternative approaches.

@BryonLewis
Copy link
Collaborator

This helped me understand much better what is going on here and how things will be structured. If I'm understanding correctly you place your api implementation in platform/{platform-type}/api which should be based on apispec.ts. Then you set up your skeleton for your platform picking and choosing from the viame-web-common the components to create it as well as implementing custom components needed for that platform.

@subdavis
Copy link
Contributor Author

Alright, this should be done. No real functional changes. Migrated a few more components to composition as it was convenient. Improved typescript coverage.

Copy link
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.

Some stuff with the RunPipelineMenu and some other general comments/questions.

client/platform/web-girder/views/ViewerLoader.vue Outdated Show resolved Hide resolved
client/package.json Show resolved Hide resolved
client/platform/web-girder/views/Home.vue Outdated Show resolved Hide resolved
@@ -18,7 +18,8 @@ module.exports = {
chainWebpack: (config) => {
config.output.strictModuleExceptionHandling(true);
config.resolve.symlinks(false);
config.resolve.alias.set('app', path.resolve(__dirname, 'app'));
config.resolve.alias.set('@', path.resolve(__dirname, 'viame-web-common'));
config.resolve.alias.set('viame-web-common', path.resolve(__dirname, 'viame-web-common'));
config.resolve.alias.set('vue-media-annotator', path.resolve(__dirname, 'src'));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can mark as resolved, would it make more sense for consistent language and vocabulary if we renamed ./src to ./vue-media-annotator? That way everything in it is already referred to by being the media-annotator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's do this later.

  1. It's common convention for the distributable package to live in src/
  2. This PR has already been mega-disruptive, and this change isn't necessary for the purpose of the PR.

Copy link
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.

Minor thing, missing the pipelines from the Annotator View.

Comment on lines 73 to 76
<template #title-right>
<export
v-if="dataset !== null"
:dataset-id="dataset._id"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you're still missing the pipelines from here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Argh. Right, sorry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 84d2d7a

Includes another fix where the Viewer could load with stale imagery from the last dataset because I moved the data to a vuex module so it no longer gets wiped when the component dies.

Fixed with a simple loading flag. This will have to change again in the electron branch, but I'm not sure how yet.

Copy link
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.

Looks good, pulled and tested to confirm it worked running pipelines.

@subdavis subdavis merged commit 7733330 into master Oct 16, 2020
@subdavis subdavis deleted the reorg branch October 16, 2020 18:28
@jjnesbitt jjnesbitt mentioned this pull request Oct 19, 2020
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