Skip to content

Commit

Permalink
Fix: duplicate saving and stale webview. (#513)
Browse files Browse the repository at this point in the history
* Fix: duplicate saving and stale webview.

Fixed the duplicate save by saving only the timeline from the unsaved index.
Fixed the duplicate insertion by only applying timeline from the unsaved index.
Fixed bytes not showing after saving by forcing the webview to reload its raw
data pages.
Fixed diskFileSize state being updated with diskFileSize + editsDelta instead
of only diskFileSize.
Fixed diskFileSize state not being updated after document save.
Added tests to triple check the fix works in all places that used the
previous editTimeline.
Changed editTimeline to AllEditTimeline to prevent confusion with the
newly added unsavedEditTimeline

* Removed commented code

* Fixes race condition when writing in bulk.
  • Loading branch information
tomilho authored Apr 18, 2024
1 parent 7723cef commit 00c2123
Show file tree
Hide file tree
Showing 9 changed files with 157 additions and 46 deletions.
8 changes: 4 additions & 4 deletions media/editor/dataDisplay.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ export const DataDisplay: React.FC = () => {
const columnWidth = useRecoilValue(select.columnWidth);
const dimensions = useRecoilValue(select.dimensions);
const fileSize = useRecoilValue(select.fileSize);
const editTimeline = useRecoilValue(select.editTimeline);
const allEditTimeline = useRecoilValue(select.allEditTimeline);
const unsavedEditIndex = useRecoilValue(select.unsavedEditIndex);
const ctx = useDisplayContext();
const [pasting, setPasting] = useState<
Expand Down Expand Up @@ -169,8 +169,8 @@ export const DataDisplay: React.FC = () => {
// Whenever the edit timeline changes, update unsaved ranges.
useEffect(() => {
const unsavedRanges: Range[] = [];
for (let i = 0; i < editTimeline.ranges.length; i++) {
const range = editTimeline.ranges[i];
for (let i = 0; i < allEditTimeline.ranges.length; i++) {
const range = allEditTimeline.ranges[i];
// todo: eventually support delete decorations?
if (range.op !== EditRangeOp.Insert || range.editIndex < unsavedEditIndex) {
continue;
Expand All @@ -181,7 +181,7 @@ export const DataDisplay: React.FC = () => {
}
}
ctx.unsavedRanges = unsavedRanges;
}, [editTimeline, unsavedEditIndex]);
}, [allEditTimeline, unsavedEditIndex]);

useGlobalHandler(
"keydown",
Expand Down
2 changes: 1 addition & 1 deletion media/editor/dataDisplayContext.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ export class DisplayContext {
messageHandler.sendEvent({
type: MessageType.SetHoveredByte,
hovered: this._hoveredByte?.byte,
})
});
});
}

Expand Down
29 changes: 23 additions & 6 deletions media/editor/state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,13 @@ const diskFileSize = atom({
fx.setSelf(msg.replaceFileSize ?? undefined);
}
});
registerHandler(MessageType.Saved, () => {
const size = fx.getLoadable(diskFileSize).getValue();
if (size === undefined) {
return;
}
fx.setSelf(size + fx.getLoadable(unsavedEditTimeline).getValue().sizeDelta);
});
},
],
});
Expand All @@ -129,7 +136,7 @@ export const fileSize = selector({
key: "fileSize",
get: ({ get }) => {
const initial = get(diskFileSize);
const sizeDelta = get(editTimeline).sizeDelta;
const sizeDelta = get(unsavedEditTimeline).sizeDelta;
return initial === undefined ? initial : initial + sizeDelta;
},
});
Expand Down Expand Up @@ -349,20 +356,30 @@ export const unsavedEditIndex = atom({
],
});

export const editTimeline = selector({
key: "editTimeline",
/**
* Timeline of all edits of the document. Includes both saved
* and unsaved edits.
*/
export const allEditTimeline = selector({
key: "allEditTimeline",
get: ({ get }) => buildEditTimeline(get(edits)),
});

export const unsavedEditTimeline = selector({
key: "unsavedEditTimeline",
get: ({ get }) => {
return buildEditTimeline(get(edits).slice(get(unsavedEditIndex)));
},
});

export const editedDataPages = selectorFamily({
key: "editedDataPages",
get:
(pageNumber: number) =>
async ({ get }) => {
const pageSize = get(dataPageSize);
const { ranges } = get(editTimeline);
const { ranges } = get(unsavedEditTimeline);
const target = new Uint8Array(pageSize);

const it = readUsingRanges(
{
read: (offset, target) => {
Expand Down Expand Up @@ -403,7 +420,7 @@ const rawDataPages = selectorFamily({
(pageNumber: number) =>
async ({ get }) => {
get(reloadGeneration); // used to trigger invalidation

get(unsavedEditIndex); // used to trigger invalidation when the user saves
const pageSize = get(dataPageSize);
const response = await messageHandler.sendRequest<ReadRangeResponseMessage>({
type: MessageType.ReadRangeRequest,
Expand Down
73 changes: 57 additions & 16 deletions shared/hexDocumentModel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ export class HexDocumentModel {
}

/**
* Gets the document size, accounting for all edits.
* Gets the document disk size.
* Returns undefined if infinite.
*/
public async size(): Promise<number | undefined> {
Expand All @@ -138,8 +138,24 @@ export class HexDocumentModel {
if (diskSize === undefined) {
return undefined;
}
return diskSize;
}

/**
* Gets the document size, accounting for all edits.
* Returns undefined if infinite.
*/
public async sizeWithEdits() {
if (!this.isFiniteSize) {
return undefined;
}

const diskSize = await this.getSizeInner();
if (diskSize === undefined) {
return undefined;
}
const { sizeDelta } = this.getUnsavedEditTimeline();

const { sizeDelta } = this.getEditTimeline();
return diskSize + sizeDelta;
}

Expand Down Expand Up @@ -182,10 +198,28 @@ export class HexDocumentModel {
}

/**
* Reads bytes in their edited state starting at the given offset.
* Reads bytes in their edited state starting at the given offset. Includes saved and unsaved edits.
*/
public readWithEdits(fromOffset = 0, chunkSize = 128 * 1024): AsyncIterableIterator<Uint8Array> {
return readUsingRanges(this.accessor, this.getEditTimeline().ranges, fromOffset, chunkSize);
public readWithAllEdits(
fromOffset = 0,
chunkSize = 128 * 1024,
): AsyncIterableIterator<Uint8Array> {
return readUsingRanges(this.accessor, this.getAllEditTimeline().ranges, fromOffset, chunkSize);
}

/**
* Reads bytes with only the unsaved changes at the given offset.
*/
public readWithUnsavedEdits(
fromOffset = 0,
chunkSize = 128 * 1024,
): AsyncIterableIterator<Uint8Array> {
return readUsingRanges(
this.accessor,
this.getUnsavedEditTimeline().ranges,
fromOffset,
chunkSize,
);
}

/**
Expand All @@ -196,9 +230,6 @@ export class HexDocumentModel {
if (toSave.length === 0) {
return Promise.resolve();
}

this._unsavedEditIndex += toSave.length;

return this.saveGuard.execute(async () => {
// for length changes, we must rewrite the entire file. Or at least from
// the offset of the first edit. For replacements we can selectively write.
Expand All @@ -212,8 +243,10 @@ export class HexDocumentModel {
);
} else {
// todo: technically only need to rewrite starting from the first edit
await this.accessor.writeStream(this.readWithEdits());
await this.accessor.writeStream(this.readWithUnsavedEdits());
}
this._unsavedEditIndex += toSave.length;
this.getUnsavedEditTimeline.forget();

this.getSizeInner.forget();
});
Expand All @@ -226,7 +259,8 @@ export class HexDocumentModel {
this._unsavedEditIndex = 0;
this._edits = [];
this.accessor.invalidate?.();
this.getEditTimeline.forget();
this.getAllEditTimeline.forget();
this.getUnsavedEditTimeline.forget();
this.getSizeInner.forget();
}

Expand All @@ -238,8 +272,8 @@ export class HexDocumentModel {
public makeEdits(edits: readonly HexDocumentEdit[]): HexDocumentEditReference {
const index = this._edits.length;
this._edits.push(...edits);
this.getEditTimeline.forget();

this.getAllEditTimeline.forget();
this.getUnsavedEditTimeline.forget();
return {
undo: () => {
// If the file wasn't saved, just removed the pending edits.
Expand All @@ -249,12 +283,14 @@ export class HexDocumentModel {
} else {
this._edits.push(...edits.map(reverseEdit));
}
this.getEditTimeline.forget();
this.getAllEditTimeline.forget();
this.getUnsavedEditTimeline.forget();
return this._edits;
},
redo: () => {
this._edits.push(...edits);
this.getEditTimeline.forget();
this.getAllEditTimeline.forget();
this.getUnsavedEditTimeline.forget();
return this._edits;
},
};
Expand All @@ -273,9 +309,14 @@ export class HexDocumentModel {
/**
* Gets instructions for returning file data. Returns a list of contiguous
* `Range` instances. Each "range" issues instructions until the next
* "offset". This can be used to generate file data.
* "offset". This can be used to generate file data. Includes unsaved and
* saved edits.
*/
private readonly getEditTimeline = once(() => buildEditTimeline(this._edits));
private readonly getAllEditTimeline = once(() => buildEditTimeline(this._edits));

private readonly getUnsavedEditTimeline = once(() => {
return buildEditTimeline(this._edits.slice(this._unsavedEditIndex));
});
}

export async function* readUsingRanges(
Expand Down
8 changes: 5 additions & 3 deletions src/fileSystemAdaptor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -194,9 +194,11 @@ class NativeFileAccessor implements FileAccessor {

writeBulk(ops: readonly FileWriteOp[]): Promise<void> {
return this.handle.borrow<void>(async fd => {
for (const { data, offset } of ops) {
fd.write(data, 0, data.byteLength, offset);
}
return Promise.all(
ops.map(({ data, offset }) => {
return fd.write(data, 0, data.byteLength, offset);
}),
);
});
}

Expand Down
14 changes: 7 additions & 7 deletions src/hexDocument.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,13 +95,13 @@ export class HexDocument extends Disposable implements vscode.CustomDocument {
public get uri(): vscode.Uri {
return vscode.Uri.parse(this.model.uri);
}

/**
* Reads data including edits from the model, returning an iterable of
* Uint8Array chunks.
* Reads data including unsaved edits from the model, returning an iterable
* of Uint8Array chunks.
*/
public readWithEdits(offset: number): AsyncIterableIterator<Uint8Array> {
return this.model.readWithEdits(offset);
public readWithUnsavedEdits(offset: number): AsyncIterableIterator<Uint8Array> {
return this.model.readWithUnsavedEdits(offset);
}

/**
Expand All @@ -111,7 +111,7 @@ export class HexDocument extends Disposable implements vscode.CustomDocument {
public async readBufferWithEdits(offset: number, length: number): Promise<Uint8Array> {
const target = new Uint8Array(length);
let soFar = 0;
for await (const chunk of this.model.readWithEdits(offset)) {
for await (const chunk of this.model.readWithUnsavedEdits(offset)) {
const read = Math.min(chunk.length, target.length - soFar);
target.set(chunk.subarray(0, read), soFar);
soFar += read;
Expand Down Expand Up @@ -297,7 +297,7 @@ export class HexDocument extends Disposable implements vscode.CustomDocument {

const newFile = await accessFile(targetResource);
this.lastSave = Date.now();
await newFile.writeStream(this.model.readWithEdits());
await newFile.writeStream(this.model.readWithUnsavedEdits());
this.lastSave = Date.now();
this.model.dispose();
this.model = new HexDocumentModel({
Expand Down
4 changes: 2 additions & 2 deletions src/searchRequest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ export class LiteralSearchRequest implements ISearchRequest {
isCaseSensitive ? undefined : caseInsensitiveEquivalency,
);

for await (const chunk of document.readWithEdits(0)) {
for await (const chunk of document.readWithUnsavedEdits(0)) {
if (this.cancelled || collector.capped) {
yield collector.final();
return;
Expand Down Expand Up @@ -155,7 +155,7 @@ export class RegexSearchRequest implements ISearchRequest {
const encoder = new TextEncoder();
const collector = new ResultsCollector(await document.size(), this.cap);

for await (const chunk of document.readWithEdits(0)) {
for await (const chunk of document.readWithUnsavedEdits(0)) {
if (this.cancelled || collector.capped) {
yield collector.final();
return;
Expand Down
Loading

0 comments on commit 00c2123

Please sign in to comment.