-
-
Notifications
You must be signed in to change notification settings - Fork 407
Adds download/upload attachment archive UIs #1551
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
base: main
Are you sure you want to change the base?
Changes from 34 commits
ab5cf00
44ec8f1
22c61a4
b6787f4
d471af8
310f0e4
db00fb6
a11a2f8
ece3119
e2c7a02
970002f
006a532
1cbead6
1857f7d
714c645
06799d9
a61d9d2
b2e41c8
09feac2
ee3db1e
ab72147
94c3239
80bf482
414c922
b306297
e15f0ae
7314244
106f686
808745f
45b004e
11e5397
46691bc
d615998
ed65e9a
f48dd7c
697dc3a
0407dc4
edc597f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,9 +11,11 @@ import {makeT} from 'app/client/lib/localization'; | |
import {cssMarkdownSpan} from 'app/client/lib/markdown'; | ||
import {reportError} from 'app/client/models/AppModel'; | ||
import type {DocPageModel} from 'app/client/models/DocPageModel'; | ||
import {reportWarning} from 'app/client/models/errors'; | ||
import {urlState} from 'app/client/models/gristUrlState'; | ||
import {KoSaveableObservable} from 'app/client/models/modelUtil'; | ||
import {AdminSection, AdminSectionItem} from 'app/client/ui/AdminPanelCss'; | ||
import {openFilePicker} from 'app/client/ui/FileDialog'; | ||
import {hoverTooltip, showTransientTooltip, withInfoTooltip} from 'app/client/ui/tooltips'; | ||
import {bigBasicButton, bigPrimaryButton} from 'app/client/ui2018/buttons'; | ||
import {cssRadioCheckboxOptions, radioCheckboxOption} from 'app/client/ui2018/checkbox'; | ||
|
@@ -226,11 +228,11 @@ export class DocSettingsPage extends Disposable { | |
}), | ||
]), | ||
|
||
isDocOwner ? this._buildTransferDom() : null, | ||
isDocOwner ? this._buildAttachmentStorageSection() : null, | ||
); | ||
} | ||
|
||
private _buildTransferDom() { | ||
private _buildAttachmentStorageSection() { | ||
const INTERNAL = 'internal', EXTERNAL = 'external'; | ||
|
||
const storageType = Computed.create(this, use => { | ||
|
@@ -350,13 +352,76 @@ export class DocSettingsPage extends Disposable { | |
]), | ||
]), | ||
), | ||
this._buildAttachmentUploadSection(), | ||
]); | ||
} | ||
|
||
private _buildAttachmentUploadSection() { | ||
const isUploadingObs = Observable.create(this, false); | ||
const buttonText = Computed.create(this, (use) => use(isUploadingObs) ? t('Uploading...') : t('Upload')); | ||
|
||
const uploadButton = cssSmallButton( | ||
dom.text(buttonText), | ||
dom.on('click', | ||
async () => { | ||
// This may never return due to openFilePicker. Anything past this point isn't guaranteed | ||
// to execute. | ||
const file = await this._pickAttachmentsFile(); | ||
if (!file) { | ||
return; | ||
} | ||
isUploadingObs.set(true); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the presence of the comment above, I think it is better to test if the owner is not disposed after promise returns? (Here and below). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure what you mean, can you describe the change in more detail? Are you suggesting we check if the owner is disposed before setting the observable? If so, for my benefit learning GrainJS, what's the reason to do that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are doing an async operation. By the time the promise is resolved, someone could move to another page, destroying all observables you are setting. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've pushed some guards for this. Is this something that Observables should be handling internally? It feels like once disposed, they should either error or do nothing if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let me know if the additional checks are sufficient! |
||
try { | ||
await this._uploadAttachmentsArchive(file); | ||
} finally { | ||
isUploadingObs.set(false); | ||
} | ||
}), | ||
dom.prop('disabled', isUploadingObs), | ||
testId('upload-attachment-archive') | ||
); | ||
|
||
return dom.create(AdminSectionItem, { | ||
id: 'uploadAttachments', | ||
name: withInfoTooltip( | ||
dom('span', t('Upload missing attachments'), testId('transfer-header')), | ||
'uploadAttachments', | ||
), | ||
value: uploadButton, | ||
}); | ||
} | ||
|
||
// May never finish - see `openFilePicker` for more info. | ||
private async _pickAttachmentsFile(): Promise<File | undefined> { | ||
const files = await openFilePicker({ | ||
multiple: false, | ||
accept: ".tar", | ||
}); | ||
return files[0]; | ||
} | ||
|
||
private async _uploadAttachmentsArchive(file: File) { | ||
try { | ||
const uploadResult = await this._gristDoc.docApi.uploadAttachmentArchive(file); | ||
this._gristDoc.app.topAppModel.notifier.createNotification({ | ||
title: "Attachments upload complete", | ||
message: `${uploadResult.added} attachment files reconnected`, | ||
level: 'info', | ||
canUserClose: true, | ||
expireSec: 5, | ||
}); | ||
} catch (err) { | ||
reportWarning(err.toString(), { | ||
key: "attachmentArchiveUploadError", | ||
title: "Attachments upload failed", | ||
level: 'error' | ||
}); | ||
} | ||
} | ||
|
||
private async _reloadEngine(ask = true) { | ||
const docPageModel = this._gristDoc.docPageModel; | ||
const handler = async () => { | ||
await docPageModel.appModel.api.getDocAPI(docPageModel.currentDocId.get()!).forceReload(); | ||
await this._gristDoc.docApi.forceReload(); | ||
document.location.reload(); | ||
}; | ||
if (!ask) { | ||
|
@@ -377,7 +442,6 @@ export class DocSettingsPage extends Disposable { | |
} | ||
|
||
private async _startTiming() { | ||
const docPageModel = this._gristDoc.docPageModel; | ||
modal((ctl, owner) => { | ||
this.onDispose(() => ctl.close()); | ||
const selected = Observable.create<TimingModalOption>(owner, TimingModalOption.Adhoc); | ||
|
@@ -387,7 +451,7 @@ export class DocSettingsPage extends Disposable { | |
if (selected.get() === TimingModalOption.Reload) { | ||
page.set(TimingModalPage.Spinner); | ||
await this._gristDoc.docApi.startTiming(); | ||
await docPageModel.appModel.api.getDocAPI(docPageModel.currentDocId.get()!).forceReload(); | ||
await this._gristDoc.docApi.forceReload(); | ||
ctl.close(); | ||
urlState().pushUrl({docPage: 'timing'}).catch(reportError); | ||
} else { | ||
|
@@ -551,10 +615,9 @@ export class DocSettingsPage extends Disposable { | |
} | ||
|
||
private async _doSetEngine(val: EngineCode|undefined) { | ||
const docPageModel = this._gristDoc.docPageModel; | ||
if (this._engine.get() !== val) { | ||
await this._docInfo.documentSettingsJson.prop('engine').saveOnly(val); | ||
await docPageModel.appModel.api.getDocAPI(docPageModel.currentDocId.get()!).forceReload(); | ||
await this._gristDoc.docApi.forceReload(); | ||
} | ||
} | ||
} | ||
|
@@ -698,7 +761,6 @@ function stillInternalCopy(inProgress: Observable<boolean>, ...args: IDomArgs<HT | |
}); | ||
} | ||
|
||
|
||
const cssContainer = styled('div', ` | ||
overflow-y: auto; | ||
position: relative; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this used anywhere else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only in 'uploadFiles', which it was extracted from.
It was going to be used elsewhere, but I guess I went a different route.
I'm happy to leave this in though if you are, as it splits up a long function (
uploadFiles
), making it more readable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine with leaving, maybe just remove the export, to detect dead code more easily in the future.