From 392ef74ebaf6d67fdb75a51cfe181d45fac0398f Mon Sep 17 00:00:00 2001 From: Hyzual Date: Sat, 10 Jan 2015 17:36:37 +0100 Subject: [PATCH] Rewrites notification service - showNotification takes a song as parameter and picks the information it needs itself. That way we avoid player-directive.js having utils as a dependency. - shortened the names of the methods a bit. Had to rename them in settings.js - Used $interval rather than window.setInterval in player-directive.js and in notification-service.js. It's easier to unit test them that way ! --- app/common/notification-service.js | 51 ++++------- app/common/notification-service_test.js | 108 ++++++++++++++++++++++++ app/player/player-directive.js | 13 +-- app/player/player-directive_test.js | 31 +++---- app/settings/settings.js | 4 +- 5 files changed, 146 insertions(+), 61 deletions(-) create mode 100644 app/common/notification-service_test.js diff --git a/app/common/notification-service.js b/app/common/notification-service.js index dff787a..98cb152 100644 --- a/app/common/notification-service.js +++ b/app/common/notification-service.js @@ -3,12 +3,12 @@ * * Provides access to the notification UI. */ -angular.module('jamstash.notifications', ['jamstash.player.service']) +angular.module('jamstash.notifications', ['jamstash.player.service', 'jamstash.utils']) -.service('notifications', ['$rootScope', 'globals', 'player', function($rootScope, globals, player) { +.service('notifications', ['$rootScope', '$window', '$interval', 'globals', 'player', 'utils', + function($rootScope, $window, $interval, globals, player, utils) { 'use strict'; - var msgIndex = 1; this.updateMessage = function (msg, autohide) { if (msg !== '') { var id = $rootScope.Messages.push(msg) - 1; @@ -21,49 +21,34 @@ angular.module('jamstash.notifications', ['jamstash.player.service']) } }; this.requestPermissionIfRequired = function () { - if (window.Notify.isSupported() && window.Notify.needsPermission()) { + if (this.isSupported() && !this.hasPermission()) { window.Notify.requestPermission(); } }; - this.hasNotificationPermission = function () { - return (window.Notify.needsPermission() === false); + this.hasPermission = function () { + return !$window.Notify.needsPermission(); }; - this.hasNotificationSupport = function () { + this.isSupported = function () { return window.Notify.isSupported(); }; - var notifications = []; - this.showNotification = function (pic, title, text, type, bind) { - if (this.hasNotificationPermission()) { - //closeAllNotifications() - var settings = {}; - if (bind === '#NextTrack') { - settings.notifyClick = function () { + this.showNotification = function (song) { + if (this.hasPermission()) { + var notification = new Notify(utils.toHTML.un(song.name), { + body: utils.toHTML.un(song.artist + " - " + song.album), + icon: song.coverartthumb, + notifyClick: function () { player.nextTrack(); this.close(); - //TODO: Hyz: This should be in a directive, so we wouldn't have to use this. $rootScope.$apply(); - }; - } - if (type === 'text') { - settings.body = text; - settings.icon = pic; - } else if (type === 'html') { - settings.body = text; - } - var notification = new Notify(title, settings); - notifications.push(notification); - setTimeout(function (notWin) { - notWin.close(); - }, globals.settings.Timeout, notification); + } + }); + $interval(function() { + notification.close(); + }, globals.settings.Timeout); notification.show(); } else { console.log("showNotification: No Permission"); } }; - this.closeAllNotifications = function () { - for (var notification in notifications) { - notifications[notification].close(); - } - }; }]); diff --git a/app/common/notification-service_test.js b/app/common/notification-service_test.js new file mode 100644 index 0000000..ac5e569 --- /dev/null +++ b/app/common/notification-service_test.js @@ -0,0 +1,108 @@ +describe("Notifications service - ", function() { + 'use strict'; + + var notifications, $window, $interval, player, utils, mockGlobals, + NotificationObj; + beforeEach(function() { + mockGlobals = { + settings: { + Timeout: 30000 + } + }; + + module('jamstash.notifications', function ($provide) { + $provide.value('globals', mockGlobals); + }); + + inject(function (_notifications_, _$window_, _$interval_, _player_, _utils_) { + notifications = _notifications_; + $window = _$window_; + player = _player_; + utils = _utils_; + $interval = _$interval_; + }); + + spyOn(player, "nextTrack"); + spyOn(utils.toHTML, "un").and.callFake(function (arg) { return arg; }); + + // Mock the Notify object + $window.Notify = jasmine.createSpyObj("Notify", ["isSupported", "needsPermission", "requestPermission"]); + NotificationObj = jasmine.createSpyObj("Notification", ["show", "close"]); + spyOn($window, "Notify").and.callFake(function (title, settings) { + NotificationObj.simulateClick = settings.notifyClick; + return NotificationObj; + }); + }); + + it("can check whether we have the permission to display notifications in the current browser", function() { + $window.Notify.needsPermission.and.returnValue(false); + + expect(notifications.hasPermission()).toBeTruthy(); + expect($window.Notify.needsPermission).toHaveBeenCalled(); + }); + + it("can check whether the current browser supports notifications", function() { + $window.Notify.isSupported.and.returnValue(true); + + expect(notifications.isSupported()).toBeTruthy(); + expect($window.Notify.isSupported).toHaveBeenCalled(); + }); + + it("can request Notification permission for the current browser", function() { + spyOn(notifications, "isSupported").and.returnValue(true); + spyOn(notifications, "hasPermission").and.returnValue(false); + + notifications.requestPermissionIfRequired(); + + expect($window.Notify.requestPermission).toHaveBeenCalled(); + }); + + describe("When I show a notification, given a song,", function() { + var song; + beforeEach(function() { + song = { + coverartthumb: "https://backjaw.com/overquantity/outpitch?a=redredge&b=omnivoracious#promotement", + name: "Unhorny", + artist: "Saturnina Koster", + album: "Trepidate" + }; + spyOn(notifications, "hasPermission").and.returnValue(true); + }); + it("it checks the permissions, displays the title, the artist's name and the album picture in a notification", function() { + notifications.showNotification(song); + + expect(notifications.hasPermission).toHaveBeenCalled(); + expect($window.Notify).toHaveBeenCalledWith(song.name, { + body: song.artist + " - " + song.album, + icon: song.coverartthumb, + notifyClick: jasmine.any(Function) + }); + expect(NotificationObj.show).toHaveBeenCalled(); + }); + + it("when I click on it, it plays the next track of the queue", function() { + notifications.showNotification(song); + NotificationObj.simulateClick(); + + expect(player.nextTrack).toHaveBeenCalled(); + expect(NotificationObj.close).toHaveBeenCalled(); + }); + + it("given that the global Timeout setting is set to 10 seconds, it closes itself after 10 seconds", function() { + mockGlobals.settings.Timeout = 10000; + + notifications.showNotification(song); + $interval.flush(10001); + + expect(NotificationObj.close).toHaveBeenCalled(); + }); + + it("if we don't have the permission to display notifications, nothing happens", function() { + notifications.hasPermission.and.returnValue(false); + + notifications.showNotification(song); + + expect(NotificationObj.show).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/app/player/player-directive.js b/app/player/player-directive.js index 0f0f300..a3e430a 100644 --- a/app/player/player-directive.js +++ b/app/player/player-directive.js @@ -4,10 +4,10 @@ * Encapsulates the jPlayer plugin. It watches the player service for the song to play, load or restart. * It also enables jPlayer to attach event handlers to our UI through css Selectors. */ -angular.module('jamstash.player.directive', ['jamstash.player.service', 'jamstash.settings', 'jamstash.subsonic.service', 'jamstash.notifications', 'jamstash.utils', 'jamstash.persistence']) +angular.module('jamstash.player.directive', ['jamstash.player.service', 'jamstash.settings', 'jamstash.subsonic.service', 'jamstash.notifications', 'jamstash.persistence']) -.directive('jplayer', ['player', 'globals', 'subsonic', 'notifications', 'utils', '$window', 'persistence', - function(playerService, globals, subsonic, notifications, utils, $window, persistence) { +.directive('jplayer', ['player', 'globals', 'subsonic', 'notifications', '$interval', 'persistence', + function(playerService, globals, subsonic, notifications, $interval, persistence) { 'use strict'; return { restrict: 'EA', @@ -100,7 +100,7 @@ angular.module('jamstash.player.directive', ['jamstash.player.service', 'jamstas } else { $player.jPlayer('play'); if(globals.settings.NotificationSong) { - notifications.showNotification(newSong.coverartthumb, utils.toHTML.un(newSong.name), utils.toHTML.un(newSong.artist + ' - ' + newSong.album), 'text', '#NextTrack'); + notifications.showNotification(newSong); } } } @@ -111,6 +111,7 @@ angular.module('jamstash.player.directive', ['jamstash.player.service', 'jamstas }, function (newVal) { if(newVal === true) { $player.jPlayer('play', 0); + scope.scrobbled = false; playerService.restartSong = false; } }); @@ -131,9 +132,9 @@ angular.module('jamstash.player.directive', ['jamstash.player.service', 'jamstas scope.startSavePosition = function () { if (globals.settings.SaveTrackPosition) { if (timerid !== 0) { - $window.clearInterval(timerid); + $interval.cancel(timerid); } - timerid = $window.setInterval(function () { + timerid = $interval(function () { var audio = $player.data('jPlayer'); if (globals.settings.SaveTrackPosition && scope.currentSong !== undefined && audio !== undefined && audio.status.currentTime > 0 && audio.status.paused === false) { diff --git a/app/player/player-directive_test.js b/app/player/player-directive_test.js index d9a8d97..3d7e4ce 100644 --- a/app/player/player-directive_test.js +++ b/app/player/player-directive_test.js @@ -2,7 +2,7 @@ describe("jplayer directive", function() { 'use strict'; var element, scope, $player, playingSong, deferred, - playerService, mockGlobals, subsonic, notifications, persistence, $window; + playerService, mockGlobals, subsonic, notifications, persistence, $interval; beforeEach(function() { // We redefine globals because in some tests we need to alter the settings @@ -24,22 +24,16 @@ describe("jplayer directive", function() { $delegate.isLastSongPlaying = jasmine.createSpy('isLastSongPlaying'); return $delegate; }); - //TODO: Hyz: We shouldn't have to know the utils service just for that. Remove these calls and deal with this in the Notifications service. - // Mock the utils service - $provide.decorator('utils', function ($delegate) { - $delegate.toHTML.un = jasmine.createSpy('un'); - return $delegate; - }); $provide.value('globals', mockGlobals); }); spyOn($.fn, "jPlayer").and.callThrough(); - inject(function($rootScope, $compile, _player_, _subsonic_, _notifications_, _persistence_, _$window_, $q) { + inject(function($rootScope, $compile, _player_, _subsonic_, _notifications_, _persistence_, _$interval_, $q) { playerService = _player_; subsonic = _subsonic_; notifications = _notifications_; persistence = _persistence_; - $window = _$window_; + $interval = _$interval_; // Compile the directive scope = $rootScope.$new(); element = '
'; @@ -92,18 +86,20 @@ describe("jplayer directive", function() { scope.$apply(); - expect(notifications.showNotification).toHaveBeenCalled(); + expect(notifications.showNotification).toHaveBeenCalledWith(playingSong); }); }); }); - it("When the player service's restartSong flag is true, it restarts the current song and resets the flag to false", function() { + it("When the player service's restartSong flag is true, it restarts the current song, resets the restart flag to false and resets the scrobbled flag to false", function() { $.fn.jPlayer.and.stub(); playerService.restartSong = true; + scope.scrobbled = true; scope.$apply(); expect($player.jPlayer).toHaveBeenCalledWith('play', 0); expect(playerService.restartSong).toBeFalsy(); + expect(scope.scrobbled).toBeFalsy(); }); describe("When jplayer has finished the current song,", function() { @@ -223,11 +219,6 @@ describe("jplayer directive", function() { mockGlobals.settings.SaveTrackPosition = true; spyOn(persistence, "saveTrackPosition"); spyOn(persistence, "saveQueue"); - jasmine.clock().install(); - }); - - afterEach(function() { - jasmine.clock().uninstall(); }); it("every 30 seconds, it saves the current song's position and the playing queue", function() { @@ -236,7 +227,7 @@ describe("jplayer directive", function() { $player.data('jPlayer').status.paused = false; scope.startSavePosition(); - jasmine.clock().tick(30001); + $interval.flush(30001); expect(scope.currentSong.position).toBe(35.3877); expect(persistence.saveTrackPosition).toHaveBeenCalledWith(scope.currentSong); @@ -248,18 +239,18 @@ describe("jplayer directive", function() { $player.data('jPlayer').status.paused = true; scope.startSavePosition(); - jasmine.clock().tick(30001); + $interval.flush(30001); expect(persistence.saveTrackPosition).not.toHaveBeenCalled(); expect(persistence.saveQueue).not.toHaveBeenCalled(); }); it("if there was already a watcher, it clears it before adding a new one", function() { - spyOn($window, "clearInterval"); + spyOn($interval, "cancel"); scope.startSavePosition(); scope.startSavePosition(); - expect($window.clearInterval).toHaveBeenCalled(); + expect($interval.cancel).toHaveBeenCalled(); }); }); }); diff --git a/app/settings/settings.js b/app/settings/settings.js index 2d4477b..ad41dfa 100644 --- a/app/settings/settings.js +++ b/app/settings/settings.js @@ -36,13 +36,13 @@ if ($scope.settings.Server.indexOf('http://') != 0 && $scope.settings.Server.indexOf('https://') != 0) { $scope.settings.Server = 'http://' + $scope.settings.Server; } if ($scope.settings.NotificationSong) { notifications.requestPermissionIfRequired(); - if (!notifications.hasNotificationSupport()) { + if (!notifications.isSupported()) { alert('HTML5 Notifications are not available for your current browser, Sorry :('); } } if ($scope.settings.NotificationNowPlaying) { notifications.requestPermissionIfRequired(); - if (!notifications.hasNotificationSupport()) { + if (!notifications.isSupported()) { alert('HTML5 Notifications are not available for your current browser, Sorry :('); } }