readium / readium-js-viewer

👁 ReadiumJS viewer: default web app for Readium.js library
BSD 3-Clause "New" or "Revised" License
553 stars 186 forks source link

Chrome app: Messages.SUCCESS worker event passes wrong object field (ebooks library payload) #544

Closed danielweck closed 8 years ago

danielweck commented 8 years ago

See Messages.SUCCESS below, and library vs. libraryItems field name:

https://github.com/readium/readium-js-viewer/blob/develop/src/js/workers/EpubLibraryWriter.js#L275

       onmessage = function(evt){
            var data = evt.data,
                msg = data.msg;

            var success = function(){
                postMessage({msg: Messages.SUCCESS, library: writer.libraryData});
            }
...

https://github.com/readium/readium-js-viewer/blob/develop/src/js/workers/WorkerProxy.js#L36

        worker.onmessage = function(evt){
            var data = evt.data;
            switch (data.msg){
...
                case Messages.SUCCESS:
                    if (callbacks.success){
                        callbacks.success(data.libraryItems);
                    }
                    cleanupWorker();
                    break;
...

This means that the success() callback function receives undefined, resulting in messing-up the local state in EpubLibraryManager (see _refreshLibraryFromWorker ()): https://github.com/readium/readium-js-viewer/blob/develop/src/js/EpubLibraryManager.js#L124

        _refreshLibraryFromWorker : function(callback, newLibraryData){
            this.libraryData = newLibraryData;
            callback();
        },

...which is for example used when unzipping / importing an EPUB archive into the library: https://github.com/readium/readium-js-viewer/blob/develop/src/js/EpubLibraryManager.js#L128

        handleZippedEpub : function(options){
            WorkerProxy.importZip(options.file, this.libraryData, {
                progress : options.progress,
                overwrite: options.overwrite,
                success: this._refreshLibraryFromWorker.bind(this, options.success),
                error : options.error
            });
            //Dialogs.showModalProgress()
            //unzipper.extractAll();
        },
danielweck commented 8 years ago

Fixed by https://github.com/readium/readium-js-viewer/commit/44ffe5a296767883483cb6f6df19aabffdac5e2b

danielweck commented 8 years ago

PS: I should have pointed out that although this.libraryData was set to undefined in the _refreshLibraryFromWorker() callback function, this did not have any observable consequences from the user standpoint. The library of eBooks is stored in a file called epub_library.json that happened to be reloaded at each import (see retrieveAvailableEpubs()): https://github.com/readium/readium-js-viewer/blob/develop/src/js/EpubLibraryManager.js#L53

        retrieveAvailableEpubs : function(success, error){
            if (this.libraryData){
                success(this.libraryData);
                return;
            }

            var self = this;

            var indexUrl = moduleConfig.epubLibraryPath
                        ? StorageManager.getPathUrl(moduleConfig.epubLibraryPath)
                        : StorageManager.getPathUrl('/epub_library.json');
...etc.

Also see the "save" function in EpubLibraryWriter: https://github.com/readium/readium-js-viewer/blob/develop/src/js/workers/EpubLibraryWriter.js#L21

        _saveLibraryIndex : function(success, error){
            var blob = new Blob([JSON.stringify(this.libraryData)]);
            StorageManager.saveFile('/epub_library.json', blob, success, error);
        },