angular2-notifications icon indicating copy to clipboard operation
angular2-notifications copied to clipboard

Timeout - setting depends on computer/browser-speed

Open leomayer opened this issue 6 years ago • 5 comments

If I set a timeout of e.g. 5 sec, i.e. 5000 then depending on the computer I have either 8 secs or 30 secs. I guess that it is a timing issue, namely in the NotificationComponent you have


  startTimeOut(): void {
    this.steps = this.timeOut / 10;
    this.speed = this.timeOut / this.steps;
    this.start = new Date().getTime();
    this.zone.runOutsideAngular(() => this.timer = setTimeout(this.instance, this.speed));
  }

and if I have 5000 ==> steps=500 and the timeout is 10ms. Depending on the accuracy of the clock of the computer (and the load) 10ms might be way to low, especially since the rounding sums up to quite a large number.

Proposed solution:

  1. Either set from outside the amount of steps manually OR
  2. Just change the counter of 10 to some more reasonable value, e.g. 50 or 75

I guess with 50ms ==> steps=100 the deviation of few ms would sum up to less than one sec - independent of the load from the computer!

leomayer avatar Aug 14 '17 12:08 leomayer

Note: While e.g. Chromium takes 9secs on FF the same settings takes 35 secs!!!

leomayer avatar Aug 14 '17 13:08 leomayer

Thank you @leomayer, I'm completely tied up at the moment. Do you feel like submitting a PR?

flauc avatar Aug 14 '17 13:08 flauc

hmmm... don't know what you mean with PR.... Matter of fact:

  • I can adapt the code in our project
  • I never worked with GitHub nor have an idea how to submit changes :-/

leomayer avatar Aug 14 '17 14:08 leomayer

I did several changes on the code because if the timing lacks the whole code is a pain in the a**

The following changes fixes some other issues as well cuz it now relies on the passed time span instead of the counted steps. Consequently the dialog closes when the time passed by during hovering - and do NOT continue. I personally prefer this style although I agree this might be worth for discussions...


  startTimeOut(): void {
    this.steps = Math.round(this.timeOut / 50);
    this.speed = Math.round(this.timeOut / this.steps);
    this.start = new Date().getTime();
    this.zone.runOutsideAngular(() => this.timer = setTimeout(this.instance, this.speed));
  }

  private instance = () => {
    this.zone.runOutsideAngular(() => {
      const elapsedTime = (new Date().getTime() - this.start);
      this.diff = elapsedTime - (this.count * this.speed);
      this.count++;
      if (this.notYetFinished()) {
        if (!this.stopTime && this.showProgressBar) {
          this.zone.run(() =>
            this.progressWidth = Math.min(Math.round(elapsedTime / this.timeOut * 100), 100)
          )
          if (this.diff > this.speed) {
            // skip some steps because they are already consumed by the timeout
            this.count += Math.round(this.diff / this.speed);
            this.timer = setTimeout(this.instance, this.speed);
          } else {
            this.timer = setTimeout(this.instance, this.speed - this.diff);
          };
        }
      }
    })
  };

  private notYetFinished() {
    if ((new Date().getTime() - this.start) > this.timeOut) {
      this.zone.run(() => this.remove());
      return false;
    }
    return true;
  }

I guess the 50 ms are reasonable! This fix works on FF as well on Chrome - tested with timeout of 5000. Would be great if this could be a patch!

leomayer avatar Aug 14 '17 16:08 leomayer

Changes I did:

  private stopTime = false;
  private timer: any;
  private speed: 25; // time in ms for a new check if time has ellapsed
  private start: number;
  startTimeOut(): void {
    this.start = new Date().getTime();
    this.zone.runOutsideAngular(() => this.timer = setTimeout(this.instance, this.speed));
  }


  private instance = () => {
    this.zone.runOutsideAngular(() => {
      const elapsedTime = (new Date().getTime() - this.start);
      if (elapsedTime < this.timeOut) {
        if (!this.stopTime && this.showProgressBar) {
          this.zone.run(() =>
            this.progressWidth = Math.min(Math.round(elapsedTime / this.timeOut * 100), 100)
          )
          this.timer = setTimeout(this.instance, this.speed);
        }
      } else {
        this.zone.run(() => this.remove());
//        console.log(`finished after: ${elapsedTime} ms`)
      }
    })
  };

Which now ONLY relies on the time passed by, i.e. after each timeout (=25ms) ==> check if the total time passed by ==> if not, continue, if yes, abort... Adjust as well the progressbar according to the total time passed by...

Sorry, cannot create a patch :-/ But the above proposal fixes as well issue #211

leomayer avatar Aug 16 '17 06:08 leomayer