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

feat(xo-web/vm-import) Don't block the UI when dropping a big OVA file #4018

Merged
merged 5 commits into from Apr 4, 2019

Conversation

nraynaud
Copy link
Member

@nraynaud nraynaud commented Mar 6, 2019

Check list

Check items when done or if not relevant

  • PR reference the relevant issue (e.g. Fixes #007)
  • if UI changes, a screenshot has been added to the PR
  • CHANGELOG.unreleased.md:
    • enhancement/bug fix entry added
    • list of packages to release updated (${name} v${new version})
  • documentation updated
  • I have tested added/updated features (and impacted code)

Process

  1. create a PR as soon as possible
  2. mark it as WiP: (Work in Progress) if not ready to be merged
  3. when you want a review, add a reviewer
  4. if necessary, update your PR, and re- add a reviewer

@nraynaud nraynaud requested a review from pdonias March 6, 2019 22:22
@nraynaud nraynaud changed the title WIP don't block the UI when dropping a big OVA file Don't block the UI when dropping a big OVA file Mar 6, 2019
@nraynaud nraynaud changed the title Don't block the UI when dropping a big OVA file feat(xo-web/vm-import) Don't block the UI when dropping a big OVA file Mar 6, 2019
@nraynaud
Copy link
Member Author

nraynaud commented Mar 7, 2019

Confirmed that it doesn't block the UI with a big file, and can import a small file.
Left the temporary waiting pointer because sometimes dropping a big file take 2-3 seconds, in my case the time to spool up and external HDD.

const { name } = file

info(_('startVmImport'), name)

if (data.tables) {
Copy link
Member

Choose a reason for hiding this comment

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

data may be undefined.

@julien-f julien-f requested a review from pdonias March 27, 2019 14:53
const { name } = file

info(_('startVmImport'), name)

if (data && data.tables) {
Copy link
Member

Choose a reason for hiding this comment

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

Explicit tests: !== undefined.

@@ -12,10 +12,10 @@ const GRAIN_ADDRESS_OFFSET = 56
*/
export default async function readVmdkGrainTable(fileAccessor) {
const getLongLong = (buffer, offset, name) => {
if (buffer.length < offset + 8) {
if (buffer.byteLength < offset + 8) {
Copy link
Member

Choose a reason for hiding this comment

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

I have no idea if this change is ok.

@nraynaud nraynaud requested a review from pdonias April 1, 2019 17:57
@@ -4,6 +4,7 @@

- [SR/Disk] Disable actions on unmanaged VDIs [#3988](https://github.com/vatesfr/xen-orchestra/issues/3988) (PR [#4000](https://github.com/vatesfr/xen-orchestra/pull/4000))
- [Pool] Specify automatic networks on a Pool [#3916](https://github.com/vatesfr/xen-orchestra/issues/3916) (PR [#3958](https://github.com/vatesfr/xen-orchestra/pull/3958))
- [Import] Avoid blocking the UI when dropping a big OVA file on the UI (PR [#4018](https://github.com/vatesfr/xen-orchestra/pull/4018))
Copy link
Member

Choose a reason for hiding this comment

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

Please rebase on master and add xo-vmdk-to-vhd to the Changelog.

@nraynaud nraynaud force-pushed the nr-dont-block-ui-in-ova-import branch from 3ff7383 to a4bb915 Compare April 3, 2019 21:17
@nraynaud nraynaud requested a review from pdonias April 3, 2019 21:21
@pdonias
Copy link
Member

pdonias commented Apr 4, 2019

Don't forget to check the PR checkboxes when done or irrelevant so we know when it's ready to be merged :)

@pdonias pdonias merged commit 975de19 into master Apr 4, 2019
@pdonias pdonias deleted the nr-dont-block-ui-in-ova-import branch April 4, 2019 08:59
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.

None yet

2 participants