diff options
author | Morris Jobke <hey@morrisjobke.de> | 2017-12-20 12:38:05 +0100 |
---|---|---|
committer | GitHub <noreply@github.com> | 2017-12-20 12:38:05 +0100 |
commit | dc8809e754c22addaab20401a7505dcc4082541c (patch) | |
tree | 3d976ec5e32d105967c2d76bfbb9f8ca694cd61f | |
parent | b6fcf59881b27f0a0b7541ff6e01921d7a23142e (diff) | |
parent | 0ff3c81fc14793ddb39d6d46549ec56cfec26ddd (diff) | |
download | nextcloud-server-dc8809e754c22addaab20401a7505dcc4082541c.tar.gz nextcloud-server-dc8809e754c22addaab20401a7505dcc4082541c.zip |
Merge pull request #7498 from nextcloud/fix_7497
Better handle avatars
-rw-r--r-- | core/js/jquery.avatar.js | 77 | ||||
-rw-r--r-- | core/js/placeholder.js | 5 | ||||
-rw-r--r-- | core/js/tests/specs/jquery.avatarSpec.js | 137 | ||||
-rw-r--r-- | lib/private/Avatar.php | 17 | ||||
-rw-r--r-- | lib/private/Server.php | 19 | ||||
-rw-r--r-- | lib/private/Template/JSConfigHelper.php | 1 | ||||
-rw-r--r-- | lib/public/IAvatar.php | 6 | ||||
-rw-r--r-- | settings/js/settings/personalInfo.js | 14 | ||||
-rw-r--r-- | tests/lib/AvatarTest.php | 2 |
9 files changed, 148 insertions, 130 deletions
diff --git a/core/js/jquery.avatar.js b/core/js/jquery.avatar.js index 54518c75cc7..958f0f9edd7 100644 --- a/core/js/jquery.avatar.js +++ b/core/js/jquery.avatar.js @@ -106,54 +106,39 @@ }); } - // If the displayname is not defined we use the old code path - if (typeof(displayname) === 'undefined') { - $.get(url).always(function(result, status) { - // if there is an error or an object returned (contains user information): - // -> show the fallback placeholder - if (typeof(result) === 'object' || status === 'error') { - if (!hidedefault) { - if (result.data && result.data.displayname) { - $div.imageplaceholder(user, result.data.displayname); - } else { - // User does not exist - setAvatarForUnknownUser($div); - } - } else { - $div.hide(); - } - // else an image is transferred and should be shown - } else { - $div.show(); - if (ie8fix === true) { - $div.html('<img width="' + size + '" height="' + size + '" src="'+url+'#'+Math.floor(Math.random()*1000)+'" alt="">'); - } else { - $div.html('<img width="' + size + '" height="' + size + '" src="'+url+'" alt="">'); - } - } - if(typeof callback === 'function') { - callback(); - } - }); - } else { - // We already have the displayname so set the placeholder (to show at least something) - if (!hidedefault) { - $div.imageplaceholder(displayname); - } + var img = new Image(); + + // If the new image loads successfully set it. + img.onload = function() { + $div.text(''); + $div.append(img); + $div.clearimageplaceholder(); - var img = new Image(); + if(typeof callback === 'function') { + callback(); + } + }; + // Fallback when avatar loading fails: + // Use old placeholder when a displayname attribute is defined, + // otherwise show the unknown user placeholder. + img.onerror = function () { + $div.clearimageplaceholder(); + if (typeof(displayname) !== 'undefined') { + $div.imageplaceholder(user, displayname); + } else { + setAvatarForUnknownUser($div); + $div.removeClass('icon-loading'); + } - // If the new image loads successfully set it. - img.onload = function() { - $div.show(); - $div.text(''); - $div.append(img); - $div.clearimageplaceholder(); - }; + if(typeof callback === 'function') { + callback(); + } + }; - img.width = size; - img.height = size; - img.src = url; - } + $div.addClass('icon-loading'); + $div.show(); + img.width = size; + img.height = size; + img.src = url; }; }(jQuery)); diff --git a/core/js/placeholder.js b/core/js/placeholder.js index f173e738676..5cf7b9095ad 100644 --- a/core/js/placeholder.js +++ b/core/js/placeholder.js @@ -2,7 +2,7 @@ * ownCloud * * @author John Molakvoæ - * @copyright 2016 John Molakvoæ <fremulon@protonmail.com> + * @copyright 2016-2017 John Molakvoæ <skjnldsv@protonmail.com> * @author Morris Jobke * @copyright 2013 Morris Jobke <morris.jobke@gmail.com> * @@ -47,7 +47,7 @@ * <div id="albumart" style="background-color: hsl(123, 90%, 65%); ... ">A</div> * */ - + /* * Alternatively, you can use the prototype function to convert your string to hsl colors: * @@ -156,5 +156,6 @@ this.css('text-align', ''); this.css('line-height', ''); this.css('font-size', ''); + this.removeClass('icon-loading'); }; }(jQuery)); diff --git a/core/js/tests/specs/jquery.avatarSpec.js b/core/js/tests/specs/jquery.avatarSpec.js index b9351d2a8a0..bdd1fdcc163 100644 --- a/core/js/tests/specs/jquery.avatarSpec.js +++ b/core/js/tests/specs/jquery.avatarSpec.js @@ -19,6 +19,13 @@ describe('jquery.avatar tests', function() { devicePixelRatio = window.devicePixelRatio; window.devicePixelRatio = 1; + + spyOn(window, 'Image').and.returnValue({ + onload: function() { + }, + onerror: function() { + } + }); }); afterEach(function() { @@ -39,6 +46,9 @@ describe('jquery.avatar tests', function() { $div.height(9); $div.avatar('foo'); + expect(window.Image).toHaveBeenCalled(); + window.Image().onerror(); + expect($div.height()).toEqual(9); expect($div.width()).toEqual(9); }); @@ -47,6 +57,9 @@ describe('jquery.avatar tests', function() { $div.data('size', 10); $div.avatar('foo'); + expect(window.Image).toHaveBeenCalled(); + window.Image().onerror(); + expect($div.height()).toEqual(10); expect($div.width()).toEqual(10); }); @@ -55,6 +68,9 @@ describe('jquery.avatar tests', function() { it('defined', function() { $div.avatar('foo', 8); + expect(window.Image).toHaveBeenCalled(); + window.Image().onerror(); + expect($div.height()).toEqual(8); expect($div.width()).toEqual(8); }); @@ -73,16 +89,10 @@ describe('jquery.avatar tests', function() { describe('no avatar', function() { it('show placeholder for existing user', function() { spyOn($div, 'imageplaceholder'); - $div.avatar('foo'); - - fakeServer.requests[0].respond( - 200, - { 'Content-Type': 'application/json' }, - JSON.stringify({ - data: {displayname: 'bar'} - }) - ); + $div.avatar('foo', undefined, undefined, undefined, undefined, 'bar'); + expect(window.Image).toHaveBeenCalled(); + window.Image().onerror(); expect($div.imageplaceholder).toHaveBeenCalledWith('foo', 'bar'); }); @@ -91,32 +101,23 @@ describe('jquery.avatar tests', function() { spyOn($div, 'css'); $div.avatar('foo'); - fakeServer.requests[0].respond( - 200, - { 'Content-Type': 'application/json' }, - JSON.stringify({ - data: {} - }) - ); + expect(window.Image).toHaveBeenCalled(); + window.Image().onerror(); expect($div.imageplaceholder).toHaveBeenCalledWith('?'); expect($div.css).toHaveBeenCalledWith('background-color', '#b9b9b9'); }); - it('show no placeholder', function() { + it('show no placeholder is ignored', function() { spyOn($div, 'imageplaceholder'); + spyOn($div, 'css'); $div.avatar('foo', undefined, undefined, true); - fakeServer.requests[0].respond( - 200, - { 'Content-Type': 'application/json' }, - JSON.stringify({ - data: {} - }) - ); + expect(window.Image).toHaveBeenCalled(); + window.Image().onerror(); - expect($div.imageplaceholder.calls.any()).toEqual(false); - expect($div.css('display')).toEqual('none'); + expect($div.imageplaceholder).toHaveBeenCalledWith('?'); + expect($div.css).toHaveBeenCalledWith('background-color', '#b9b9b9'); }); }); @@ -129,24 +130,24 @@ describe('jquery.avatar tests', function() { window.devicePixelRatio = 1; $div.avatar('foo', 32); - expect(fakeServer.requests[0].method).toEqual('GET'); - expect(fakeServer.requests[0].url).toEqual('http://localhost/index.php/avatar/foo/32'); + expect(window.Image).toHaveBeenCalled(); + expect(window.Image().src).toEqual('http://localhost/index.php/avatar/foo/32'); }); it('high DPI icon', function() { window.devicePixelRatio = 4; $div.avatar('foo', 32); - expect(fakeServer.requests[0].method).toEqual('GET'); - expect(fakeServer.requests[0].url).toEqual('http://localhost/index.php/avatar/foo/128'); + expect(window.Image).toHaveBeenCalled(); + expect(window.Image().src).toEqual('http://localhost/index.php/avatar/foo/128'); }); it('high DPI icon round up size', function() { window.devicePixelRatio = 1.9; $div.avatar('foo', 32); - expect(fakeServer.requests[0].method).toEqual('GET'); - expect(fakeServer.requests[0].url).toEqual('http://localhost/index.php/avatar/foo/61'); + expect(window.Image).toHaveBeenCalled(); + expect(window.Image().src).toEqual('http://localhost/index.php/avatar/foo/61'); }); }); @@ -158,17 +159,12 @@ describe('jquery.avatar tests', function() { it('default (no ie8 fix)', function() { $div.avatar('foo', 32); - fakeServer.requests[0].respond( - 200, - { 'Content-Type': 'image/jpeg' }, - '' - ); + expect(window.Image).toHaveBeenCalled(); + window.Image().onload(); - var img = $div.children('img')[0]; - - expect(img.height).toEqual(32); - expect(img.width).toEqual(32); - expect(img.src).toEqual('http://localhost/index.php/avatar/foo/32'); + expect(window.Image().height).toEqual(32); + expect(window.Image().width).toEqual(32); + expect(window.Image().src).toEqual('http://localhost/index.php/avatar/foo/32'); }); it('default high DPI icon', function() { @@ -176,37 +172,23 @@ describe('jquery.avatar tests', function() { $div.avatar('foo', 32); - fakeServer.requests[0].respond( - 200, - { 'Content-Type': 'image/jpeg' }, - '' - ); - - var img = $div.children('img')[0]; + expect(window.Image).toHaveBeenCalled(); + window.Image().onload(); - expect(img.height).toEqual(32); - expect(img.width).toEqual(32); - expect(img.src).toEqual('http://localhost/index.php/avatar/foo/61'); + expect(window.Image().height).toEqual(32); + expect(window.Image().width).toEqual(32); + expect(window.Image().src).toEqual('http://localhost/index.php/avatar/foo/61'); }); - it('with ie8 fix', function() { - sinon.stub(Math, 'random').callsFake(function() { - return 0.5; - }); - + it('with ie8 fix (ignored)', function() { $div.avatar('foo', 32, true); - fakeServer.requests[0].respond( - 200, - { 'Content-Type': 'image/jpeg' }, - '' - ); + expect(window.Image).toHaveBeenCalled(); + window.Image().onload(); - var img = $div.children('img')[0]; - - expect(img.height).toEqual(32); - expect(img.width).toEqual(32); - expect(img.src).toEqual('http://localhost/index.php/avatar/foo/32#500'); + expect(window.Image().height).toEqual(32); + expect(window.Image().width).toEqual(32); + expect(window.Image().src).toEqual('http://localhost/index.php/avatar/foo/32'); }); it('unhide div', function() { @@ -214,11 +196,12 @@ describe('jquery.avatar tests', function() { $div.avatar('foo', 32); - fakeServer.requests[0].respond( - 200, - { 'Content-Type': 'image/jpeg' }, - '' - ); + expect(window.Image).toHaveBeenCalled(); + window.Image().onload(); + + expect(window.Image().height).toEqual(32); + expect(window.Image().width).toEqual(32); + expect(window.Image().src).toEqual('http://localhost/index.php/avatar/foo/32'); expect($div.css('display')).toEqual('block'); }); @@ -232,12 +215,12 @@ describe('jquery.avatar tests', function() { observer.callback(); }); - fakeServer.requests[0].respond( - 200, - { 'Content-Type': 'image/jpeg' }, - '' - ); + expect(window.Image).toHaveBeenCalled(); + window.Image().onload(); + expect(window.Image().height).toEqual(32); + expect(window.Image().width).toEqual(32); + expect(window.Image().src).toEqual('http://localhost/index.php/avatar/foo/32'); expect(observer.callback).toHaveBeenCalled(); }); }); diff --git a/lib/private/Avatar.php b/lib/private/Avatar.php index 5893daa1804..afa9118c509 100644 --- a/lib/private/Avatar.php +++ b/lib/private/Avatar.php @@ -141,6 +141,7 @@ class Avatar implements IAvatar { try { $generated = $this->folder->getFile('generated'); + $this->config->setUserValue($this->user->getUID(), 'avatar', 'generated', 'false'); $generated->delete(); } catch (NotFoundException $e) { // @@ -161,6 +162,7 @@ class Avatar implements IAvatar { foreach ($avatars as $avatar) { $avatar->delete(); } + $this->config->setUserValue($this->user->getUID(), 'avatar', 'generated', 'true'); $this->user->triggerChange('avatar', ''); } @@ -177,6 +179,7 @@ class Avatar implements IAvatar { $ext = 'png'; $this->folder->newFile('generated'); + $this->config->setUserValue($this->user->getUID(), 'avatar', 'generated', 'true'); } if ($size === -1) { @@ -393,4 +396,18 @@ class Avatar implements IAvatar { return array(round($r * 255), round($g * 255), round($b * 255)); } + public function userChanged($feature, $oldValue, $newValue) { + // We only change the avatar on display name changes + if ($feature !== 'displayName') { + return; + } + + // If the avatar is not generated (so an uploaded image) we skip this + if (!$this->folder->fileExists('generated')) { + return; + } + + $this->remove(); + } + } diff --git a/lib/private/Server.php b/lib/private/Server.php index 44f5ea80cb7..4a851d67226 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -394,9 +394,10 @@ class Server extends ServerContainer implements IServerContainer { $userSession->listen('\OC\User', 'logout', function () { \OC_Hook::emit('OC_User', 'logout', array()); }); - $userSession->listen('\OC\User', 'changeUser', function ($user, $feature, $value, $oldValue) { + $userSession->listen('\OC\User', 'changeUser', function ($user, $feature, $value, $oldValue) use ($dispatcher) { /** @var $user \OC\User\User */ \OC_Hook::emit('OC_User', 'changeUser', array('run' => true, 'user' => $user, 'feature' => $feature, 'value' => $value, 'old_value' => $oldValue)); + $dispatcher->dispatch('OCP\IUser::changeUser', new GenericEvent($user, ['feature' => $feature, 'oldValue' => $oldValue, 'value' => $value])); }); return $userSession; }); @@ -1175,6 +1176,22 @@ class Server extends ServerContainer implements IServerContainer { $logger->info('Could not cleanup avatar of ' . $user->getUID()); } }); + + $dispatcher->addListener('OCP\IUser::changeUser', function (GenericEvent $e) { + $manager = $this->getAvatarManager(); + /** @var IUser $user */ + $user = $e->getSubject(); + $feature = $e->getArgument('feature'); + $oldValue = $e->getArgument('oldValue'); + $value = $e->getArgument('value'); + + try { + $avatar = $manager->getAvatar($user->getUID()); + $avatar->userChanged($feature, $oldValue, $value); + } catch (NotFoundException $e) { + // no avatar to remove + } + }); } /** diff --git a/lib/private/Template/JSConfigHelper.php b/lib/private/Template/JSConfigHelper.php index 60ac4bfecb0..551fc3b9b0d 100644 --- a/lib/private/Template/JSConfigHelper.php +++ b/lib/private/Template/JSConfigHelper.php @@ -254,6 +254,7 @@ class JSConfigHelper { $array['oc_userconfig'] = json_encode([ 'avatar' => [ 'version' => (int)$this->config->getUserValue($uid, 'avatar', 'version', 0), + 'generated' => $this->config->getUserValue($uid, 'avatar', 'generated', 'true') === 'true', ] ]); } diff --git a/lib/public/IAvatar.php b/lib/public/IAvatar.php index 369cafa00c1..a6731b63be9 100644 --- a/lib/public/IAvatar.php +++ b/lib/public/IAvatar.php @@ -77,4 +77,10 @@ interface IAvatar { * @since 9.0.0 */ public function getFile($size); + + /** + * Handle a changed user + * @since 13.0.0 + */ + public function userChanged($feature, $oldValue, $newValue); } diff --git a/settings/js/settings/personalInfo.js b/settings/js/settings/personalInfo.js index 3a4542df748..0a39e607762 100644 --- a/settings/js/settings/personalInfo.js +++ b/settings/js/settings/personalInfo.js @@ -60,7 +60,7 @@ function updateAvatar (hidedefault) { $displaydiv.avatar(user.uid, 145, true, null, function() { $displaydiv.removeClass('loading'); $('#displayavatar img').show(); - if($('#displayavatar img').length === 0) { + if($('#displayavatar img').length === 0 || oc_userconfig.avatar.generated) { $('#removeavatar').removeClass('inlineblock').addClass('hidden'); } else { $('#removeavatar').removeClass('hidden').addClass('inlineblock'); @@ -129,6 +129,7 @@ function avatarResponseHandler (data) { $warning.hide(); if (data.status === "success") { $('#displayavatar .avatardiv').removeClass('icon-loading'); + oc_userconfig.avatar.generated = false; updateAvatar(); } else if (data.data === "notsquare") { showAvatarCropper(); @@ -256,8 +257,14 @@ $(document).ready(function () { }); + var userSettings = new OC.Settings.UserSettings(); var federationSettingsView = new OC.Settings.FederationSettingsView({ - el: '#personal-settings' + el: '#personal-settings', + config: userSettings + }); + + userSettings.on("sync", function() { + updateAvatar(false); }); federationSettingsView.render(); @@ -362,6 +369,7 @@ $(document).ready(function () { type: 'DELETE', url: OC.generateUrl('/avatar/'), success: function () { + oc_userconfig.avatar.generated = true; updateAvatar(true); } }); @@ -392,7 +400,7 @@ $(document).ready(function () { // Load the big avatar var user = OC.getCurrentUser(); $('#avatarform .avatardiv').avatar(user.uid, 145, true, null, function() { - if($('#displayavatar img').length === 0) { + if($('#displayavatar img').length === 0 || oc_userconfig.avatar.generated) { $('#removeavatar').removeClass('inlineblock').addClass('hidden'); } else { $('#removeavatar').removeClass('hidden').addClass('inlineblock'); diff --git a/tests/lib/AvatarTest.php b/tests/lib/AvatarTest.php index 240aecc115e..9da719c26de 100644 --- a/tests/lib/AvatarTest.php +++ b/tests/lib/AvatarTest.php @@ -210,7 +210,7 @@ class AvatarTest extends \Test\TestCase { ->method('putContent') ->with($image->data()); - $this->config->expects($this->once()) + $this->config->expects($this->exactly(3)) ->method('setUserValue'); $this->config->expects($this->once()) ->method('getUserValue'); |