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

Various Emscripten frontend fixes #199

Merged
merged 9 commits into from Oct 1, 2019
Merged

Conversation

AliceLR
Copy link
Owner

@AliceLR AliceLR commented Sep 29, 2019

This fixes various issues with the Emscripten port's frontend blocking the release of HTML5 builds.

  • Prevent browsers from blocking certain useful MZX shortcuts (tested and works in Firefox and Chrome; Opera won't disable F3 or left alt behavior).
  • Fix save corruption bug caused by not handling O_TRUNC (tested with memory, localStorage, and IndexedDB). Overwriting save files with a new save file would fail to wipe the data after the end of the file, causing MZX to detect a corrupt ZIP.
  • Fix issues with creating new files when using localStorage. Empty files would get converted to the empty string when entering localStorage. Empty strings evaluate to false, so some checks would fail to detect the existence of the empty file when retrieving it later. This seems to have been a problem even before this branch but the O_TRUNC fix made it worse.
  • Fix performance issues encountered with writing particularly large files by expanding the underlying data buffer to powers of two and tracking the real file length separately. This issue could be encountered when saving in DOOMZX and with the autosaves in Depot Dungeons.
  • Implement unlink. It seems to work fine but could be tested more.
  • Implement rmdir. Same as above.

Would be nice, but probably won't bother with:

  • Implement rename. For now it just prints a console error instead of crashing MZX. Prevented by FS API nightmare code
  • Fix mkdir. This uses mknod which intentionally fails for directories currently. The createNode function also ignores the incoming mode and errors when creating a directory. It'd be nice to create directories even if they aren't stored, but I'm not sure it's worth the effort and potentially breaking something right now.

Copy link
Contributor

@asiekierka asiekierka left a comment

Choose a reason for hiding this comment

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

The fact I'll have to re-implement some of these bothers me a little, especially for the storage part, as that code is shared between Zeta (for which I strongly care about having total authorship control) and MegaZeux (where that is not the case). It's not a big concern though so don't worry about it. I haven't gotten around to it mostly as I've been feeling a bit off during the past week, and thesis-related stress on top too.

arch/emscripten/web/src/index.js Outdated Show resolved Hide resolved
arch/emscripten/web/src/storage.js Outdated Show resolved Hide resolved
@AliceLR
Copy link
Owner Author

AliceLR commented Oct 1, 2019

Providing my buggy partial implementation of rename here for reference. This causes crashes after renaming a file too many times and I haven't even bothered to see what directory rename does (it probably just crashes, vfs_path isn't updated for any child nodes). For now this just isn't happening, though. I'm probably just merging this PR as-is so this thing can just be released already.

function vfs_rename(storage, oldPath, newPath) {
    // FIXME tfw no atomic way to do any of this
    // FIXME tfw the FS API sabotages this at every step
    if (oldPath == newPath) return true;
    if (!storage.canSet(oldPath)) return false;

    function vfs_rename_file(storage, oldFile, newFile) {
        var contents = storage.get(oldFile);
        if (contents !== null)
            if (storage.set(newFile, contents))
                if (storage.remove(oldFile))
                    return true;
        return false;
    }

    if (oldPath.slice(-1) == '/') {
        // Directory-- rename multiple files
        if (newPath.slice(-1) != '/') newPath += '/';

        var files = storage.list(a => a.startsWith(oldPath));
        var retval = true;
        for (var oldFile in files) {
            let newFile = oldFile.replace(oldPath, newPath);
            retval &= vfs_rename_file(storage, oldFile, newFile);
        }
        return retval;
    } else {
        // File
        return vfs_rename_file(storage, oldPath, newPath);
    }
}
                node.node_ops.rename = (oldNode, newDir, newName) => {
                    console.log("FS rename " + oldNode.vfs_path + ", " + newDir.vfs_path + ", " + newName);
                    let oldPath = oldNode.vfs_path;
                    let newPath = newDir.vfs_path + newName;
                    if (!vfs_rename(vfs, oldPath, newPath))
                        throw new FS.ErrnoError(EPERM);
                    oldNode.name = newName;
                    oldNode.vfs_path = newPath;
                };

@AliceLR AliceLR merged commit 0552ae2 into master Oct 1, 2019
@AliceLR AliceLR deleted the fix-emscripten-frontend branch October 1, 2019 18:06
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