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

bug(5.0.3): Images not showing #168

Closed
eduardoRoth opened this issue Jul 3, 2018 · 41 comments · Fixed by #173
Closed

bug(5.0.3): Images not showing #168

eduardoRoth opened this issue Jul 3, 2018 · 41 comments · Fixed by #173
Labels

Comments

@eduardoRoth
Copy link
Member

Images are downloaded but not updated on screen. Images seem to be cached but not shown.
I'll update as soon as I have more detail.

@eduardoRoth
Copy link
Member Author

Sometimes images are shown, sometimes they don't. I have a share button that gets the image from the cache so I know images are downloaded and saved, but for some reason the component never gets the "finished" status and keeps showing the spinner.

@eduardoRoth
Copy link
Member Author

  1. Image download start as usual.
  2. Image's XHR calls finish with status code 200 (only 5)
  3. I have two GIFs that keep loading even after the XHR call is finished
  4. Component is not updated, keeps showing spinner

@eduardoRoth
Copy link
Member Author

More details about this error:

  1. Open app
  2. App inits Image Loader configuration
      this._image.setMaximumCacheSize(268435456);
      this._image.enableDebugMode();
      this._image.setCacheDirectoryName('custom-name');
      this._image.setFileNameCachedWithExtension(true);
    
  3. App sets root to page
  4. Page load calls API
  5. API returns five items
  6. Image Loader starts fetching image
  7. Spinner stuck loading
  8. Close app after a while
  9. Open app again, ImageLoader fetch from cache but image is not available (although no error)
  10. Move to another page
  11. New page call API and gets 5 different results
  12. ImageLoader load everything fine.

@eduardoRoth
Copy link
Member Author

On first run, something is happening in processQueue function that it never resolves or rejects in this part:

_this.file.writeFile(localDir, fileName, data)
                    .then(function (file) {
                        if (_this.shouldIndex) {
                            _this.addFileToIndex(file).then(function () {
                                _this.getCachedImagePath(currentItem.imageUrl).then(function (localUrl) {
                                    currentItem.resolve(localUrl);
                                    resolve();
                                    done();
                                    _this.maintainCacheSize();
                                }).catch(err => {
                                    console.error('currentItem', err);
                                    currentItem.reject();
                                });
                            });
                        }
                    }).catch(function (e) {
                        //Could not write image
                        console.error(e);
                        reject();
                        error(e);
                    });

@gmosornoza
Copy link

Hi, same issue over here. Any news?

@gmosornoza
Copy link

What i'm seeing is that the files are created with 0 filesize.

@eduardoRoth
Copy link
Member Author

@gmosornoza but it only happens the first time, if you close your app (fully close it) and then navigate to another page that has img-loader components, they'll load just fine.

@gmosornoza
Copy link

@eduardoRoth Not for me. In fact, first time only shows the fallback Image. If i close and reopen the app, then it shows the image as broken image.

Checking the cache directory in the device i noticed the zero filesize.

@eduardoRoth
Copy link
Member Author

First time opening the app:
image

Second time opening the app - confirms the zero byte files:
image

@eduardoRoth
Copy link
Member Author

image

@eduardoRoth
Copy link
Member Author

eduardoRoth commented Jul 4, 2018

This happens with @ionic-native 4.7.0 or 4.9.0

@eduardoRoth
Copy link
Member Author

eduardoRoth commented Jul 4, 2018

@felixbroehl could you take a look at this?

@felixbroehl
Copy link
Contributor

Did this worked before my changes were merged? Which platforms did you tested? I'm unable to reproduce such problems with my devices.

@gmosornoza
Copy link

@felixbroehl I'm using the plugin as is. Android Oreo device,

@gmosornoza
Copy link

gmosornoza commented Jul 4, 2018

window.resolveLocalFileSystemURL is rejecting with errorCode 1, in @ionic-native/file.

I trace until this:

File.prototype.resolveLocalFilesystemUrl = function (fileUrl) {
var _this = this;
return new Promise(function (resolve, reject) {
try {
window.resolveLocalFileSystemURL(fileUrl, function (entry) {
resolve(entry);
}, function (err) {
_this.fillErrorMessage(err);
reject(err);
});
}
catch (xc) {
_this.fillErrorMessage(xc);
reject(xc);
}
});
};

@eduardoRoth
Copy link
Member Author

eduardoRoth commented Jul 4, 2018

@felixbroehl Samsung S8 - Oreo 8.0.0

ionic info

cli packages: (/usr/local/lib/node_modules)

    @ionic/cli-plugin-proxy : 1.5.8
    @ionic/cli-utils        : 1.19.2
    ionic (Ionic CLI)       : 3.20.0

global packages:

    cordova (Cordova CLI) : 8.0.0

local packages:

    @ionic/app-scripts : 3.1.10
    Cordova Platforms  : android 7.1.0 ios 4.5.4
    Ionic Framework    : ionic-angular 3.9.2

System:

    ios-deploy : 1.9.2
    ios-sim    : 7.0.0
    Node       : v8.11.1
    npm        : 6.1.0
    OS         : macOS High Sierra
    Xcode      : Xcode 9.4.1 Build version 9F2000

Environment Variables:

    ANDROID_HOME     : not set
    HTTP_PROXY       : not set
    http_proxy       : not set
    HTTPS_PROXY      : not set
    https_proxy      : not set
    IONIC_HTTP_PROXY : not set
    PROXY            : not set
    proxy            : not set

Misc:

    backend : legacy

package.json Dependencies

        "@angular/animations": "5.2.10",
        "@angular/common": "5.2.10",
        "@angular/compiler": "5.2.10",
        "@angular/compiler-cli": "5.2.10",
        "@angular/core": "5.2.10",
        "@angular/forms": "5.2.10",
        "@angular/http": "5.2.10",
        "@angular/platform-browser": "5.2.10",
        "@angular/platform-browser-dynamic": "5.2.10",
        "@ionic-native/camera": "^4.9.0",
        "@ionic-native/core": "^4.9.0",
        "@ionic-native/device": "^4.9.0",
        "@ionic-native/file": "^4.9.0",
        "@ionic-native/onesignal": "^4.9.0",
        "@ionic-native/social-sharing": "^4.9.0",
        "@ionic-native/splash-screen": "^4.9.0",
        "@ionic-native/status-bar": "^4.9.0",
        "@ionic/storage": "2.1.3",
        "@ngx-translate/core": "8.0.0",
        "@ngx-translate/http-loader": "^2.0.0",
        "@types/wpapi": "^1.1.0",
        "cordova-android": "7.1.0",
        "cordova-ios": "4.5.4",
        "cordova-plugin-camera": "^4.0.3",
        "cordova-plugin-device": "^2.0.2",
        "cordova-plugin-file": "^6.0.1",
        "cordova-plugin-ionic-keyboard": "^2.1.2",
        "cordova-plugin-ionic-webview": "^1.2.1",
        "cordova-plugin-splashscreen": "^5.0.2",
        "cordova-plugin-statusbar": "^2.4.2",
        "cordova-plugin-whitelist": "^1.3.3",
        "cordova-plugin-x-socialsharing": "^5.4.1",
        "cordova-sqlite-storage": "^2.3.3",
        "es6-promise-plugin": "^4.2.2",
        "ionic-angular": "3.9.2",
        "ionic-image-loader": "^5.0.4",
        "ionic2-super-tabs": "^4.3.0",
        "ionicons": "3.0.0",
        "moment": "^2.22.2",
        "onesignal-cordova-plugin": "^2.4.1",
        "rxjs": "5.5.8",
        "sw-toolbox": "3.6.0",
        "wpapi": "^1.1.2",
        "zone.js": "0.8.26"

@felixbroehl
Copy link
Contributor

felixbroehl commented Jul 4, 2018

hmm, ok. personally i'm using it with an older cordova version. @eduardoRoth was 5.0.2 working for you with 8.0.0? If not this is not related to my latest changes, but i will try to investigate. If 5.0.2 showing the same behavior i think its related to the copying into a temp directory in the getCachedImagePath function. As @gmosornoza pointed out he traced it back to this.file.resolveLocalFilesystemUrl which is used in getCachedImagePath.

@eduardoRoth
Copy link
Member Author

@gmosornoza @felixbroehl checked my app settings and found that it had the "Storage" permission off, so I turned it on and tried again with no result... So I disallowed it again and suddendly it started working... really strange

Will check on it further...

@felixbroehl
Copy link
Contributor

ok this is strange :D

@felixbroehl
Copy link
Contributor

seems more like a problem with cordova than with this plugin

@eduardoRoth
Copy link
Member Author

Cleared cache again for my app and tried again... same problem 😢

@felixbroehl
Copy link
Contributor

this could be related to danielsogl/awesome-cordova-plugins#806

@felixbroehl
Copy link
Contributor

could you try this hack described by joewoodhouse? danielsogl/awesome-cordova-plugins#505

So, I've found a fix (I think) is to re-order the index.html of your app so that the polyfills.js (which includes zone.js and a few other things) is included before cordova.js. This seems to make it work reliably for me. Presumably this gives some clue as to what's going on, something to do with how zone.js patches things?

@eduardoRoth
Copy link
Member Author

@felixbroehl was just trying this change, and it seems to partially fix this issue. I need to use setMaximumCacheSize() function to set a maximum cache.

Tried both ways, with and without setMaximumCacheSize() and only with it, the images were loaded.

setMaximumCacheSize #27 (comment)
image

Is there any drawback to have the polyfills.js loaded before cordova?

@eduardoRoth
Copy link
Member Author

@gmosornoza could you try both fixes and see if it works for you know?

@felixbroehl
Copy link
Contributor

maybe this also is related to "if(this.shouldIndex)" and no else in your example

@felixbroehl
Copy link
Contributor

because this.shouldIndex is defined by the maximumCacheSize

@eduardoRoth
Copy link
Member Author

Yeah, just removed that if(this.shouldIndex) and it worked without setting a maximumCacheSize.

Why is there this property? @ihadeed

@felixbroehl
Copy link
Contributor

also wtf is this:
(this.config.maxCacheAge > -1) || (this.config.maxCacheSize > -1)
this means that maximumCacheSize should be set. @ihadeed what was this intended for?

@felixbroehl
Copy link
Contributor

felixbroehl commented Jul 4, 2018

there should be something like this:

this.http.get(currentItem.imageUrl, {
  responseType: 'blob',
  headers: this.config.httpHeaders,
}).subscribe(
  (data: Blob) => {
    if ((this.config.maxCacheSize > -1 && this.currentCacheSize <= this.config.maxCacheSize) || this.config.maxCacheSize == -1) {
      this.file.writeFile(localDir, fileName, data, {replace: true}).then((file: FileEntry) => {
        this.addFileToIndex(file).then(() => {
          this.getCachedImagePath(currentItem.imageUrl).then((localUrl) => {
            currentItem.resolve(localUrl);
            resolve();
            done();
            this.maintainCacheSize();
          });
        });
      }).catch((e) => {
        //Could not write image
        error(e);
      });
    } else {
      //if there is too less space
      currentItem.resolve(currentItem.imageUrl);
      resolve();
      done();
      this.maintainCacheSize();
    }
  },
  (e) => {
    //Could not get image via httpClient
    error(e);
  }
);

@felixbroehl
Copy link
Contributor

felixbroehl commented Jul 4, 2018

@eduardoRoth can you test it?

@gmosornoza
Copy link

gmosornoza commented Jul 4, 2018

It doesn't work for me.

The files were downloaded, but when they are written to the filesystem, the filesize keeps 0.

I setup the maximum cache size, tried to move cordova.js after polifyll but no success.

Edit: I get FileError code 1.

@eduardoRoth
Copy link
Member Author

@felixbroehl made the following change instead:

this is using your updated condition

private get shouldIndex(): boolean {
    return (this.config.maxCacheSize > -1 && this.currentCacheSize > this.config.maxCacheSize) || this.config.maxCacheSize == -1;
  }

Check if we have enough space in cache, if not, make some more removing the oldest one.

   this.file.writeFile(localDir, fileName, data, {replace: true}).then((file: FileEntry) => {
              if (!this.shouldIndex) {
                this.maintainCacheSize();
              }
              this.addFileToIndex(file).then(() => {
                this.getCachedImagePath(currentItem.imageUrl).then((localUrl) => {
                  currentItem.resolve(localUrl);
                  resolve();
                  done();
                  this.maintainCacheSize();
                });
              });
            }).catch((e) => {
              //Could not write image
              error(e);
            });

@felixbroehl
Copy link
Contributor

felixbroehl commented Jul 4, 2018

maybe we should rename shouldIndex to isCacheSizeExceeded or something like that

@gmosornoza
Copy link

Hi,

It worked changing the order of cordova.js and polifyll.js.

@eduardoRoth I was doing it wrong! The file to be edited is under /src/index.html (i was editing www/index.html)

Thank you all, it's working fine now.

@ajsystem
Copy link

ajsystem commented Jul 4, 2018

@felixbroehl same problem on Android 7.0.1

@felixbroehl
Copy link
Contributor

so maintainCacheSize already checking this. So I created a fork for this: https://github.com/felixbroehl/ionic-image-loader

Can somebody test this?
If its not working please also try to change the order of cordova.js and polyfill.js in your src/index.html

@eduardoRoth
Copy link
Member Author

eduardoRoth commented Jul 4, 2018

Also created a fork with this change, but removed shouldIndex property as it was only used just in that function and other with similar functionality.

Tested in device and emulator and works.

This does not fixes the polyfill.js workaround.

Pull request #171

image

@felixbroehl
Copy link
Contributor

Your fork seems good. I think we should document the polyfill.js workaround in the README and also ask @ihadeed to evaluate if there are consequences.

To bring @ihadeed up to date: It seems like the @ionic-native/file or cordova-plugin-file is not working correctly (promise resolve of this.file.writeFile is not called). Therefor images are not loaded on the first time. changing the order of cordova.js and polyfill.js in the src/index.html seems to do the trick.

@eduardoRoth
Copy link
Member Author

eduardoRoth commented Jul 5, 2018

@ihadeed @felixbroehl

Created PR #173 with suggested README.md changes.

@ihadeed
Copy link
Member

ihadeed commented Jul 5, 2018

@eduardoRoth @felixbroehl

I'll review the PRs soon.

  • The ImageLoader provider has a feature that indexes/caches the files in the memory. This allows us to check whether an image exists in the filesystem without calling @ionic-native/file and going through many layers of code to get an answer. It shaves time processing time in exchange of using slightly more memory. shouldIndex tells the provider whether we should index the filesystem or not.
  • I've done some testing a very long time ago with the maxCacheSize property and it seemed to work fine. It's possible that some other code change broke that feature.
  • I'm aware of the polyfill.js quirk; it's not something specific to this module, it happened to many users in the community while using different plugins. When cordova.js is loaded AFTER polyfills.js, zone.js code isn't able to wrap the promises with an NgZone which results in Angular's change detection failing to trigger when we get data back. I believe if you throw a console.log you will get the data back, but the view will not update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants