From 6367dd8a577e5d1eee1c3d938e2f2349627461f4 Mon Sep 17 00:00:00 2001 From: Juan Cruz Viotti Date: Thu, 10 Mar 2016 10:57:37 -0400 Subject: [PATCH 1/3] Implement NotifierService This service provides an easy-to-use and safe (regarding to memory leaks) way to emit data from services to controllers. This component will be used in `ImageWriterService` to emit the progress state instead of accepting an `onProgress` callback. --- lib/browser/modules/notifier.js | 74 ++++++++++++++++++++++++++ tests/browser/modules/notifier.spec.js | 50 +++++++++++++++++ 2 files changed, 124 insertions(+) create mode 100644 lib/browser/modules/notifier.js create mode 100644 tests/browser/modules/notifier.spec.js diff --git a/lib/browser/modules/notifier.js b/lib/browser/modules/notifier.js new file mode 100644 index 00000000..7295a09f --- /dev/null +++ b/lib/browser/modules/notifier.js @@ -0,0 +1,74 @@ +/* + * Copyright 2016 Resin.io + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +'use strict'; + +/** + * @module Etcher.notifier + */ + +const angular = require('angular'); +const notifier = angular.module('Etcher.notifier', []); + +/* + * Based on: + * http://www.codelord.net/2015/05/04/angularjs-notifying-about-changes-from-services-to-controllers/ + */ + +notifier.service('NotifierService', function($rootScope) { + + /** + * @summary Safely subscribe to an event + * @function + * @public + * + * @description + * We say "safely" since this subscribe function will listen + * to the scope's `$destroy` event and unbind itself automatically. + * + * @param {Object} scope - angular scope + * @param {String} name - event name + * @param {Function} callback - callback + * + * @example + * NotifierService.subscribe($scope, 'my-event', function() { + * console.log('Event received!'); + * }); + */ + this.subscribe = function(scope, name, callback) { + const handler = $rootScope.$on(name, function(event, data) { + return callback(data); + }); + + scope.$on('$destroy', handler); + }; + + /** + * @summary Emit an event + * @function + * @public + * + * @param {String} name - event name + * @param {*} data - event data + * + * @example + * NotifierService.emit('my-event', 'Foo'); + */ + this.emit = function(name, data) { + $rootScope.$emit(name, data); + }; + +}); diff --git a/tests/browser/modules/notifier.spec.js b/tests/browser/modules/notifier.spec.js new file mode 100644 index 00000000..8af60d63 --- /dev/null +++ b/tests/browser/modules/notifier.spec.js @@ -0,0 +1,50 @@ +'use strict'; + +const m = require('mochainon'); +const angular = require('angular'); +require('angular-mocks'); +require('../../../lib/browser/modules/notifier'); + +describe('Browser: Notifier', function() { + + beforeEach(angular.mock.module('Etcher.notifier')); + + describe('NotifierService', function() { + + let $rootScope; + let NotifierService; + + beforeEach(angular.mock.inject(function(_$rootScope_, _NotifierService_) { + $rootScope = _$rootScope_; + NotifierService = _NotifierService_; + })); + + it('should be able to emit an event without data', function() { + let spy = m.sinon.spy(); + NotifierService.subscribe($rootScope, 'foobar', spy); + NotifierService.emit('foobar'); + m.chai.expect(spy).to.have.been.calledOnce; + m.chai.expect(spy).to.have.been.calledWith(undefined); + }); + + it('should be able to emit an event with data', function() { + let spy = m.sinon.spy(); + NotifierService.subscribe($rootScope, 'foobar', spy); + NotifierService.emit('foobar', 'Hello'); + m.chai.expect(spy).to.have.been.calledOnce; + m.chai.expect(spy).to.have.been.calledWith('Hello'); + }); + + it('should emit the correct event', function() { + let spy1 = m.sinon.spy(); + let spy2 = m.sinon.spy(); + NotifierService.subscribe($rootScope, 'foobar', spy1); + NotifierService.subscribe($rootScope, 'foobaz', spy2); + NotifierService.emit('foobar'); + m.chai.expect(spy1).to.have.been.calledOnce; + m.chai.expect(spy2).to.not.have.been.called; + }); + + }); + +}); From 793001e133b991cab28e8c611c1c54eb4cce406e Mon Sep 17 00:00:00 2001 From: Juan Cruz Viotti Date: Thu, 10 Mar 2016 11:46:41 -0400 Subject: [PATCH 2/3] Move burn state to ImageWriterService Previously, the burn state lived in the controller, however if the user moved to another page (the settings page for example) and then returned, the progress state would be lost, leading to a broken progress bar. Fixes: https://github.com/resin-io/etcher/issues/190 --- lib/browser/app.js | 25 +++++++------- lib/browser/modules/image-writer.js | 39 ++++++++++++++++++---- lib/partials/main.html | 14 ++++---- tests/browser/modules/image-writer.spec.js | 29 ++++++++++++++++ 4 files changed, 81 insertions(+), 26 deletions(-) diff --git a/lib/browser/app.js b/lib/browser/app.js index 6e9cef30..216146a3 100644 --- a/lib/browser/app.js +++ b/lib/browser/app.js @@ -35,6 +35,7 @@ require('./browser/modules/settings'); require('./browser/modules/drive-scanner'); require('./browser/modules/image-writer'); require('./browser/modules/path'); +require('./browser/modules/notifier'); require('./browser/modules/analytics'); const app = angular.module('Etcher', [ @@ -47,6 +48,7 @@ const app = angular.module('Etcher', [ 'Etcher.settings', 'Etcher.drive-scanner', 'Etcher.image-writer', + 'Etcher.notifier', 'Etcher.analytics' ]); @@ -74,6 +76,8 @@ app.config(function($stateProvider, $urlRouterProvider) { app.controller('AppController', function( $q, $state, + $scope, + NotifierService, DriveScannerService, SelectionStateService, ImageWriterService, @@ -87,12 +91,16 @@ app.controller('AppController', function( AnalyticsService.logEvent('Restart'); if (!this.writer.isBurning()) { - this.state = { - progress: 0, - percentage: 0 - }; + this.writer.resetState(); } + NotifierService.subscribe($scope, 'image-writer:state', function(state) { + AnalyticsService.log(`Progress: ${state.progress}% at ${state.speed} MB/s`); + + // Show progress inline in operating system task bar + currentWindow.setProgressBar(state.progress / 100); + }); + this.scanner.start(2000).on('scan', function(drives) { // Notice we only autoselect the drive if there is an image, @@ -183,14 +191,7 @@ app.controller('AppController', function( device: drive.device }); - return self.writer.burn(image, drive, function(state) { - self.state = state; - AnalyticsService.log(`Progress: ${self.state.progress}% at ${self.state.speed} MB/s`); - - // Show progress inline in operating system task bar - currentWindow.setProgressBar(self.state.progress / 100); - - }).then(function() { + return self.writer.burn(image, drive).then(function() { AnalyticsService.logEvent('Done'); $state.go('success'); }).catch(dialog.showError).finally(function() { diff --git a/lib/browser/modules/image-writer.js b/lib/browser/modules/image-writer.js index c87aaec9..84c64cbc 100644 --- a/lib/browser/modules/image-writer.js +++ b/lib/browser/modules/image-writer.js @@ -30,14 +30,39 @@ if (window.mocha) { } require('./settings'); +require('./notifier'); const imageWriter = angular.module('Etcher.image-writer', [ - 'Etcher.settings' + 'Etcher.settings', + 'Etcher.notifier' ]); -imageWriter.service('ImageWriterService', function($q, $timeout, SettingsService) { +imageWriter.service('ImageWriterService', function($q, $timeout, SettingsService, NotifierService) { let self = this; let burning = false; + /** + * @summary Reset burn state + * @function + * @public + * + * @example + * ImageWriterService.resetState(); + */ + this.resetState = function() { + self.state = { + progress: 0, + speed: 0 + }; + }; + + /** + * @summary Burn progress state + * @type Object + * @public + */ + this.state = {}; + this.resetState(); + /** * @summary Check if currently burning * @function @@ -102,11 +127,10 @@ imageWriter.service('ImageWriterService', function($q, $timeout, SettingsService * @public * * @description - * This function will update `state.progress` with the current writing percentage. + * This function will update `ImageWriterService.state` with the current writing state. * * @param {String} image - image path * @param {Object} drive - drive - * @param {Function} onProgress - in progress callback (state) * * @returns {Promise} * @@ -117,7 +141,7 @@ imageWriter.service('ImageWriterService', function($q, $timeout, SettingsService * console.log('Write completed!'); * }); */ - this.burn = function(image, drive, onProgress) { + this.burn = function(image, drive) { if (self.isBurning()) { return $q.reject(new Error('There is already a burn in progress')); } @@ -129,13 +153,14 @@ imageWriter.service('ImageWriterService', function($q, $timeout, SettingsService // Safely bring the state to the world of Angular $timeout(function() { - return onProgress({ + self.state = { progress: Math.floor(state.percentage), // Transform bytes to megabytes preserving only two decimal places speed: Math.floor(state.speed / 1e+6 * 100) / 100 || 0 + }; - }); + NotifierService.emit('image-writer:state', self.state); }); }).finally(function() { diff --git a/lib/partials/main.html b/lib/partials/main.html index 820d07a6..716f0324 100644 --- a/lib/partials/main.html +++ b/lib/partials/main.html @@ -56,17 +56,17 @@ 3
- - Finishing... - Burn! - Starting... - + Finishing... + Burn! + Starting... + -

+

diff --git a/tests/browser/modules/image-writer.spec.js b/tests/browser/modules/image-writer.spec.js index 3bcf4bb2..7748856d 100644 --- a/tests/browser/modules/image-writer.spec.js +++ b/tests/browser/modules/image-writer.spec.js @@ -23,6 +23,35 @@ describe('Browser: ImageWriter', function() { ImageWriterService = _ImageWriterService_; })); + describe('.state', function() { + + it('should be reset by default', function() { + m.chai.expect(ImageWriterService.state).to.deep.equal({ + progress: 0, + speed: 0 + }); + }); + + }); + + describe('.resetState()', function() { + + it('should be able to reset the state', function() { + ImageWriterService.state = { + progress: 50, + speed: 3 + }; + + ImageWriterService.resetState(); + + m.chai.expect(ImageWriterService.state).to.deep.equal({ + progress: 0, + speed: 0 + }); + }); + + }); + describe('.isBurning()', function() { it('should return false by default', function() { From 9c5748b54e2c09715859353e13e462548867064c Mon Sep 17 00:00:00 2001 From: Juan Cruz Viotti Date: Thu, 10 Mar 2016 12:05:28 -0400 Subject: [PATCH 3/3] Reset burn state in FinishController --- lib/browser/app.js | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/lib/browser/app.js b/lib/browser/app.js index 216146a3..531331f2 100644 --- a/lib/browser/app.js +++ b/lib/browser/app.js @@ -90,10 +90,6 @@ app.controller('AppController', function( AnalyticsService.logEvent('Restart'); - if (!this.writer.isBurning()) { - this.writer.resetState(); - } - NotifierService.subscribe($scope, 'image-writer:state', function(state) { AnalyticsService.log(`Progress: ${state.progress}% at ${state.speed} MB/s`); @@ -214,11 +210,12 @@ app.controller('NavigationController', function($state) { this.open = shell.openExternal; }); -app.controller('FinishController', function($state, SelectionStateService, SettingsService) { +app.controller('FinishController', function($state, SelectionStateService, SettingsService, ImageWriterService) { this.settings = SettingsService.data; this.restart = function(options) { SelectionStateService.clear(options); + ImageWriterService.resetState(); $state.go('main'); }; });