Merge pull request #7498 from nextcloud/fix_7497

Better handle avatars

core/js/jquery.avatar.js

@@ -106,54 +106,39 @@
 				});
 		}
 
-		// If the displayname is not defined we use the old code path
+		var img = new Image();
-		if (typeof(displayname) === 'undefined') {
+
-			$.get(url).always(function(result, status) {
+		// If the new image loads successfully set it.
-				// if there is an error or an object returned (contains user information):
+		img.onload = function() {
-				// -> show the fallback placeholder
+			$div.text('');
-				if (typeof(result) === 'object' || status === 'error') {
+			$div.append(img);
-					if (!hidedefault) {
+			$div.clearimageplaceholder();
-						if (result.data && result.data.displayname) {
+
-							$div.imageplaceholder(user, result.data.displayname);
+			if(typeof callback === 'function') {
-						} else {
+				callback();
-							// User does not exist
+			}
-							setAvatarForUnknownUser($div);
+		};
-						}
+		// Fallback when avatar loading fails:
-					} else {
+		// Use old placeholder when a displayname attribute is defined,
-						$div.hide();
+		// otherwise show the unknown user placeholder.
-					}
+		img.onerror = function () {
-				// else an image is transferred and should be shown
+			$div.clearimageplaceholder();
-				} else {
+			if (typeof(displayname) !== 'undefined') {
-					$div.show();
+				$div.imageplaceholder(user, displayname);
-					if (ie8fix === true) {
+			} else {
-						$div.html('<img width="' + size + '" height="' + size + '" src="'+url+'#'+Math.floor(Math.random()*1000)+'" alt="">');
+				setAvatarForUnknownUser($div);
-					} else {
+				$div.removeClass('icon-loading');
-						$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(typeof callback === 'function') {
+				callback();
+			}
+		};
 
-			// If the new image loads successfully set it.
+		$div.addClass('icon-loading');
-			img.onload = function() {
+		$div.show();
-				$div.show();
+		img.width = size;
-				$div.text('');
+		img.height = size;
-				$div.append(img);
+		img.src = url;
-				$div.clearimageplaceholder();
-			};
-
-			img.width = size;
-			img.height = size;
-			img.src = url;
-		}
 	};
 }(jQuery));

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));

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');
+			$div.avatar('foo', undefined, undefined, undefined, undefined, 'bar');
-
-			fakeServer.requests[0].respond(
-				200,
-				{ 'Content-Type': 'application/json' },
-				JSON.stringify({
-					data: {displayname: '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(
+			expect(window.Image).toHaveBeenCalled();
-				200,
+			window.Image().onerror();
-				{ 'Content-Type': 'application/json' },
-				JSON.stringify({
-					data: {}
-				})
-			);
 
 			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(
+			expect(window.Image).toHaveBeenCalled();
-				200,
+			window.Image().onerror();
-				{ 'Content-Type': 'application/json' },
-				JSON.stringify({
-					data: {}
-				})
-			);
 
-			expect($div.imageplaceholder.calls.any()).toEqual(false);
+			expect($div.imageplaceholder).toHaveBeenCalledWith('?');
-			expect($div.css('display')).toEqual('none');
+			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(window.Image).toHaveBeenCalled();
-			expect(fakeServer.requests[0].url).toEqual('http://localhost/index.php/avatar/foo/32');
+			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(window.Image).toHaveBeenCalled();
-			expect(fakeServer.requests[0].url).toEqual('http://localhost/index.php/avatar/foo/128');
+			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(window.Image).toHaveBeenCalled();
-			expect(fakeServer.requests[0].url).toEqual('http://localhost/index.php/avatar/foo/61');
+			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(
+			expect(window.Image).toHaveBeenCalled();
-				200,
+			window.Image().onload();
-				{ 'Content-Type': 'image/jpeg' },
-				''
-			);
 
-			var img = $div.children('img')[0];
+			expect(window.Image().height).toEqual(32);
-
+			expect(window.Image().width).toEqual(32);
-			expect(img.height).toEqual(32);
+			expect(window.Image().src).toEqual('http://localhost/index.php/avatar/foo/32');
-			expect(img.width).toEqual(32);
-			expect(img.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(
+			expect(window.Image).toHaveBeenCalled();
-				200,
+			window.Image().onload();
-				{ 'Content-Type': 'image/jpeg' },
-				''
-			);
 
-			var img = $div.children('img')[0];
+			expect(window.Image().height).toEqual(32);
-
+			expect(window.Image().width).toEqual(32);
-			expect(img.height).toEqual(32);
+			expect(window.Image().src).toEqual('http://localhost/index.php/avatar/foo/61');
-			expect(img.width).toEqual(32);
-			expect(img.src).toEqual('http://localhost/index.php/avatar/foo/61');
 		});
 
-		it('with ie8 fix', function() {
+		it('with ie8 fix (ignored)', function() {
-			sinon.stub(Math, 'random').callsFake(function() {
-				return 0.5;
-			});
-
 			$div.avatar('foo', 32, true);
 
-			fakeServer.requests[0].respond(
+			expect(window.Image).toHaveBeenCalled();
-				200,
+			window.Image().onload();
-				{ 'Content-Type': 'image/jpeg' },
-				''
-			);
 
-			var img = $div.children('img')[0];
+			expect(window.Image().height).toEqual(32);
-
+			expect(window.Image().width).toEqual(32);
-			expect(img.height).toEqual(32);
+			expect(window.Image().src).toEqual('http://localhost/index.php/avatar/foo/32');
-			expect(img.width).toEqual(32);
-			expect(img.src).toEqual('http://localhost/index.php/avatar/foo/32#500');
 		});
 
 		it('unhide div', function() {
@@ -214,11 +196,12 @@ describe('jquery.avatar tests', function() {
 
 			$div.avatar('foo', 32);
 
-			fakeServer.requests[0].respond(
+			expect(window.Image).toHaveBeenCalled();
-				200,
+			window.Image().onload();
-				{ 'Content-Type': 'image/jpeg' },
+
-				''
+			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(
+			expect(window.Image).toHaveBeenCalled();
-				200,
+			window.Image().onload();
-				{ 'Content-Type': 'image/jpeg' },
-				''
-			);
 
+			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();
 		});
 	});

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();
+	}
+
 }

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
+			}
+		});
 	}
 
 	/**

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',
 				]
 			]);
 		}

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);
 }

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');

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');