-
-
Notifications
You must be signed in to change notification settings - Fork 7
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
Desktop user experience overhaul #42
Changes from 36 commits
8783694
fa7610b
f97587a
8d62d24
3fb71b8
a389edd
70c3f50
0273ff3
d4a50d1
466372f
1701ee2
e344727
dc696fc
ab5e55d
692bc0f
f2ab8db
db4771d
3092c0e
bba46ab
792c738
e8cfa3f
1f88340
ab313bb
a9283ca
8d5245d
e0ec5b9
dc497b4
1831a8e
4da9d63
7b88549
ee22692
417b46f
9ab22bf
267c076
b357417
20dbd50
868f250
1d06fe1
7086885
997694d
1f0ee7e
7927c13
ca54ee2
d56b163
98b9655
db20234
46f6545
ac42a7e
7ea1b97
63f319a
1c91a16
109f692
9f6bde8
c69fac6
cd5af1d
3ba4aee
d5a3781
f016be6
a1b023b
d535880
e02aeb7
0346390
441ca55
cd97f98
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 |
---|---|---|
@@ -1,5 +1,5 @@ | ||
[submodule "core"] | ||
path = core | ||
url = https://github.com/gristlabs/grist-core | ||
branch = main | ||
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. 🚫We'll need to switch this back before merging (marking it as a blocker) 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. Right, but remember to merge that grist-core PR first! 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. this can happen now right @Spoffy ? |
||
branch = nativefs | ||
ignore = dirty |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
import { HomeModel } from "app/client/models/HomeModel"; | ||
|
||
export type NewDocument = { | ||
path: string, | ||
id: string | ||
} | ||
|
||
/** | ||
* Allows the Grist client to call into electron. | ||
* See https://www.electronjs.org/docs/latest/tutorial/ipc | ||
SleepyLeslie marked this conversation as resolved.
Show resolved
Hide resolved
|
||
*/ | ||
interface IElectronAPI { | ||
|
||
// The Grist client can use these interfaces to request the electron main process to perform | ||
// certain tasks. | ||
createDoc: () => Promise<NewDocument>, | ||
importDoc: (uploadId: number) => Promise<NewDocument>, | ||
|
||
// The Grist client needs to call these interfaces to register callback functions for certain | ||
// events coming from the electron main process. | ||
onMainProcessImportDoc: (callback: (fileContents: Buffer, fileName: string) => void) => void | ||
|
||
} | ||
|
||
declare global { | ||
interface Window { | ||
electronAPI: IElectronAPI, | ||
gristHomeModel: HomeModel, | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
export function electronOnly() { | ||
if (window.electronAPI === undefined) { | ||
throw Error("Sorry, this must be done from within the app."); | ||
SleepyLeslie marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
import { AppModel, reportError } from 'app/client/models/AppModel'; | ||
import { IMPORTABLE_EXTENSIONS } from 'app/client/lib/uploads'; | ||
import { ImportProgress } from 'app/client/ui/ImportProgress'; | ||
import { byteString } from 'app/common/gutil'; | ||
import { openFilePicker } from 'app/client/ui/FileDialog'; | ||
import { uploadFiles } from 'app/client/lib/uploads'; | ||
|
||
/** | ||
* Imports a document and returns its upload ID, or null if no files were selected. | ||
*/ | ||
async function docImport(app: AppModel, fileToImport?: File): Promise<number|null> { | ||
let files: File[]; | ||
|
||
if (fileToImport === undefined) { | ||
files = await openFilePicker({ | ||
multiple: false, | ||
accept: IMPORTABLE_EXTENSIONS.filter((extension) => extension !== ".grist").join(","), | ||
}); | ||
if (!files.length) { return null; } | ||
} else { | ||
files = [fileToImport]; | ||
} | ||
Spoffy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
const progressUI = app.notifier.createProgressIndicator(files[0].name, byteString(files[0].size)); | ||
const progress = ImportProgress.create(progressUI, progressUI, files[0]); | ||
try { | ||
const docWorker = await app.api.getWorkerAPI('import'); | ||
const uploadResult = await uploadFiles(files, {docWorkerUrl: docWorker.url, sizeLimit: 'import'}, | ||
(p) => progress.setUploadProgress(p)); | ||
|
||
return uploadResult!.uploadId; | ||
} catch (err) { | ||
reportError(err); | ||
return null; | ||
} finally { | ||
progress.finish(); | ||
progressUI.dispose(); | ||
} | ||
} | ||
|
||
export const homeImports = {docImport}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
import { HomeModel } from 'app/client/models/HomeModel'; | ||
import { electronOnly } from "app/client/electronOnly"; | ||
import { homeImports } from 'app/client/ui/HomeImports'; | ||
|
||
async function createDocAndOpen() { | ||
electronOnly(); | ||
const doc = await window.electronAPI.createDoc(); | ||
if (doc) { | ||
window.location.assign("/o/docs/" + doc.id); | ||
} | ||
} | ||
|
||
async function importDocAndOpen(home: HomeModel, fileToImport: File) { | ||
electronOnly(); | ||
const uploadId = await homeImports.docImport(home.app, fileToImport); | ||
if (uploadId === null) { return; } | ||
const doc = await window.electronAPI.importDoc(uploadId); | ||
if (doc) { | ||
window.location.assign("/o/docs/" + doc.id); | ||
} | ||
SleepyLeslie marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
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. These function signatures differ from the ones in core, which could cause us problems with compatibility. 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. For Note that there should also be an |
||
// The ? is for external visitors over the network. electronAPI is set by electron's preload script | ||
// and is undefined for non-electron visitors. An error here will make the entire page fail to load. | ||
window.electronAPI?.onMainProcessImportDoc((fileContents: Buffer, fileName: string) => { | ||
(async() => { | ||
while (!window.gristHomeModel || !window.gristHomeModel.app) { | ||
await new Promise(resolve => setTimeout(resolve, 100)); | ||
} | ||
importDocAndOpen(window.gristHomeModel, new File([fileContents], fileName)); | ||
})(); | ||
}); | ||
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. Where is 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. Also, can you recommend a comment, for why we need to wait for this here? As I'm not sure when 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.
Here, as a part of the core PR. https://github.com/gristlabs/grist-core/blob/2f84093129c02767a83330811bd330bb2732ee55/app/client/ui/AppUI.ts#L99-L100 There is also has some contextual information about this (I PM'd you). I hate this polling approach myself, but I wasn't able to find a better way to ensure 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. Awesome, I'll look deeper into this! Thank you :) |
||
|
||
export const newDocMethods = { createDocAndOpen, importDocAndOpen }; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
import * as path from "path"; | ||
import { HomeDBManager } from "app/gen-server/lib/homedb/HomeDBManager"; | ||
import { fileExists } from "./utils"; | ||
|
||
export class DocRegistry { | ||
|
||
private idToPathMap: Map<string, string>; | ||
private pathToIdMap: Map<string, string>; | ||
private db: HomeDBManager; | ||
|
||
Spoffy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
private constructor(dbManager: HomeDBManager) { | ||
this.db = dbManager; | ||
} | ||
|
||
public static async create(dbManager: HomeDBManager) { | ||
// Allocate space. | ||
const dr = new DocRegistry(dbManager); | ||
dr.idToPathMap = new Map<string, string>; | ||
dr.pathToIdMap = new Map<string, string>; | ||
SleepyLeslie marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// Go over all documents we know about. | ||
for (const doc of await dr.db.getAllDocs()) { | ||
// All documents are supposed to have externalId set. | ||
const docPath = doc.options?.externalId; | ||
if (docPath && fileExists(docPath)) { | ||
// Cache the two-way mapping docID <-> path. | ||
dr.idToPathMap.set(doc.id, docPath); | ||
dr.pathToIdMap.set(docPath, doc.id); | ||
} else { | ||
// Remove this document - it should not appear in a DB for Grist Desktop. | ||
await dr.db.deleteDocument({ | ||
userId: (await dr.getDefaultUser()).id, | ||
urlId: doc.id | ||
}); | ||
} | ||
} | ||
return dr; | ||
Spoffy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
SleepyLeslie marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
public lookupById(docId: string): string | null { | ||
return this.idToPathMap.get(docId) ?? null; | ||
} | ||
|
||
public lookupByPath(docPath: string): string | null { | ||
return this.pathToIdMap.get(docPath) ?? null; | ||
} | ||
|
||
public async getDefaultUser() { | ||
const user = await this.db.getUserByLogin(process.env.GRIST_DEFAULT_EMAIL as string); | ||
if (!user) { throw new Error('cannot find default user'); } | ||
return user; | ||
} | ||
|
||
public async registerDoc(docPath: string): Promise<string> { | ||
const defaultUser = await this.getDefaultUser(); | ||
const wss = this.db.unwrapQueryResult(await this.db.getOrgWorkspaces({userId: defaultUser.id}, 0)); | ||
for (const doc of wss[0].docs) { | ||
if (doc.options?.externalId === docPath) { | ||
// We might be able to do better. | ||
throw Error("DocRegistry cache incoherent. Please try restarting the app."); | ||
} | ||
} | ||
const docId = this.db.unwrapQueryResult(await this.db.addDocument({ | ||
userId: defaultUser.id, | ||
}, wss[0].id, { | ||
name: path.basename(docPath, '.grist'), | ||
options: { externalId: docPath }, | ||
})); | ||
this.pathToIdMap.set(docPath, docId); | ||
this.idToPathMap.set(docId, docPath); | ||
return docId; | ||
} | ||
|
||
} |
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.
Can we safely bump core to 20 if Desktop runs happily under 20? Are there any specific changes needed to make this run?
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.
I don't think there'll be much trouble if any at all. I did this because latest electron-builder requires Node v20+. No other changes were needed and it just worked.
I have been using v22.6.0 for development. From my experience, Desktop worked perfectly with v22, so you might even want to try that.