Display Avatar pictures in the Settingsdialog #5482

dragotin · 2017-01-22T13:02:51Z

This is a little beauty feature, to utilize owncloud/core#26872 . Hope you like it.

Looks like this:

The avatar pixmap is fetched from the server by the Connectionvalidator, once it has validated the user name, it queries the avatar pixmap. If the server does not have the avatar route, an empty pixmap is stored.

The avatar image is fetched from the server async, thus connect a signal from the account if the avatar changes. Server feature owncloud/core#26872 is needed.

ogoffart

Looks good. Only the timeout need to be added.

ogoffart · 2017-01-23T09:34:59Z

src/gui/settingsdialog.cpp

@@ -236,6 +261,9 @@ void SettingsDialog::accountRemoved(AccountState *s)
        }
    }

+    if( _actionForAccount.contains(s->account().data()) ) {


Small detail: No need of the if here. remove does nothing if it is not there.

ogoffart · 2017-01-23T09:39:57Z

src/libsync/connectionvalidator.cpp

+
+        AvatarJob *job = new AvatarJob(_account, this);
+        QObject::connect(job, SIGNAL(avatarPixmap(QPixmap)), this, SLOT(slotAvatarPixmap(QPixmap)));
+


Missing

job->setTimeout(timeoutToUseMsec);

The default timeout is 5 minutes, and that's way to long for the connection validator which must finish much earlier in case of bad connection.

(And actually, since we increased the amount of jobs, timeoutToUseMsec might need to be lowered too. But I'm wondering if it would be possible to fetch the capabilities, user name, and the avatar in parallel.)

I guess capabilities fetching could run in parallel to fetching the username, and following on that, the avatar, because the avatar needs the username. Maybe the fetch job of the avatar (at least) should be owned by the account, and could become fully async?

ogoffart · 2017-01-23T09:49:14Z

src/libsync/connectionvalidator.h

@@ -119,6 +119,7 @@ protected slots:

    void slotCapabilitiesRecieved(const QVariantMap&);
    void slotUserFetched(const QVariantMap &);
+    void slotAvatarPixmap(const QPixmap&);


The ASCII art chart need to be updated at the beginning of this class.

dragotin · 2017-01-23T10:17:31Z

Btw, I did not mean to push this to 2.3. Probably this PR should have gone to master, but your choice.

Added Avatar fetching

The server displays the avatar cut into a circle, and so we do.

dragotin · 2017-01-23T20:38:40Z

Updated the Ascii-art accordingly and set a 20 seconds timeout.

Also, I added code that cuts a circle out of the avatar image, similar to what the server does. Looks better.

ogoffart

It does not work for me. If I use the same url as in the webinterface, it works tough. But what URL should we use then. @DeepDiver1975 ?

ogoffart · 2017-01-24T09:26:53Z

src/libsync/networkjobs.cpp

+AvatarJob::AvatarJob(AccountPtr account, QObject *parent)
+    : AbstractNetworkJob(account, QString(), parent)
+{
+    _avatarUrl = Utility::concatUrlPath(account->url(), QString("remote.php/dav/avatars/%1/128.png").arg(account->davUser()));


Does not work for me with this URL. (owncloud master)
The URL that works for me is index.php/avatar/%1/128

@DeepDiver1975 What's the exact URL of the avatars?

Hmm, I think the pull request that is implementing the avatar images is not yet merged in core: owncloud/core#26872

Then let's delay this PR to master/2.4? sorry for that.

ogoffart · 2017-01-24T09:31:34Z

src/libsync/connectionvalidator.cpp

@@ -252,7 +253,18 @@ void ConnectionValidator::slotUserFetched(const QVariantMap &json)
    QString user = json.value("ocs").toMap().value("data").toMap().value("id").toString();
    if (!user.isEmpty()) {
        _account->setDavUser(user);
+
+        AvatarJob *job = new AvatarJob(_account, this);
+        job->setTimeout(20*1000);


(I think it should use the same timeout as all the other jobs in this file, for consistency)

@ogoffart wouldn''t it make more sense to set it in the constructor of AvatarJob anyway?

jturcotte · 2017-02-10T16:44:40Z

src/libsync/networkjobs.cpp

+{
+    int http_result_code = reply()->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt();
+
+    QPixmap avPixmap;


This seems to cause the FolderManTest to fail on your PR, since that needs a QGuiApplication. And you don't seem to need a QPixmap either, since you're creating a new one in circleMask anyway, so storing it as a QImage could be enough to fix that test.

That fixes the test suite.

dragotin · 2017-03-09T21:36:57Z

5e33898 fixes the testsuite by using QImage instead of QPixmap

dragotin · 2017-03-09T21:37:30Z

@ogoffart @jturcotte please consider merging

guruz · 2017-04-12T15:07:03Z

(This would need to be merged in 2.4/master not 2.3)

guruz · 2017-04-12T15:11:21Z

Bummer. Still needs all the SettingsDialogMac code.
@shadone ? :-P

guruz · 2017-04-12T15:12:23Z

Why does Jenkins complain? https://jenkins.owncloud.org/job/owncloud-client/job/client/job/PR-5482/102/console

[ 13%] Building CXX object src/libsync/CMakeFiles/owncloudsync.dir/networkjobs.cpp.o
/var/lib/jenkins/workspace/cloud-client_client_PR-5482-YZILCXVUZD7LRF3ZRLYP2JS3Q6G7ZWAKK3GHZ7TY3RB7BIKKIFNA/src/libsync/networkjobs.cpp: In member function ‘virtual void OCC::AvatarJob::start()’:
/var/lib/jenkins/workspace/cloud-client_client_PR-5482-YZILCXVUZD7LRF3ZRLYP2JS3Q6G7ZWAKK3GHZ7TY3RB7BIKKIFNA/src/libsync/networkjobs.cpp:598:47: error: ‘davRequest’ was not declared in this scope
     setReply(davRequest("GET", _avatarUrl, req));
                                               ^
src/libsync/CMakeFiles/owncloudsync.dir/build.make:330: recipe for target 'src/libsync/CMakeFiles/owncloudsync.dir/networkjobs.cpp.o' failed
make[2]: *** [src/libsync/CMakeFiles/owncloudsync.dir/networkjobs.cpp.o] Error 1
CMakeFiles/Makefile2:1084: recipe for target 'src/libsync/CMakeFiles/owncloudsync.dir/all' failed
make[1]: *** [src/libsync/CMakeFiles/owncloudsync.dir/all] Error 2
Makefile:147: recipe for target 'all' failed
make: *** [all] Error 2

guruz · 2017-04-12T15:34:22Z

It's not easy to adjust this for macOS since we also need API changes in src/3rdparty/qtmacgoodies :-|

I propose this gets merged first and then do a commit later for macOS support.

Code looks ok to me. So fine to merge in master/2.4

guruz · 2017-04-12T15:55:52Z

I pushed the macOS support..

CLAassistant · 2017-04-12T15:55:53Z

All committers have signed the CLA.

guruz · 2017-04-12T15:57:27Z

src/gui/settingsdialog.cpp

-    auto accountAction = createColorAwareAction(QLatin1String(":/client/resources/account.png"),
-                                                brandingSingleAccount ? tr("Account") : s->account()->displayName());
+    QAction *accountAction;
+    QImage avatar = s->account()->avatar();


I think this will never be called since accountAdded is called before the ConnectionValidator can run?

dragotin · 2017-04-13T07:45:20Z

Hmm, I added a missing email address to my github account now again. Lets see if I have now signed the CLA again.

ogoffart · 2017-04-13T09:41:51Z

I have the same error locally:

src/libsync/networkjobs.cpp:598:14: error: use of undeclared identifier 'davRequest'

Because the API changed with 4a1a5fa

Also fix compilation because of 22370fdbdadd06f0cacd249a8d7a32f0d3c1374e Pull request #5482

ogoffart · 2017-04-13T09:55:59Z

Merged in master via commit 8cb3a77

dragotin added 3 commits January 22, 2017 13:52

New network job AvatarJob: GETs the account avatar from server.

b49dd02

Make the Account store the avatar pixmap.

e05d6bf

The avatar pixmap is fetched from the server by the Connectionvalidator, once it has validated the user name, it queries the avatar pixmap. If the server does not have the avatar route, an empty pixmap is stored.

SettingsDialog: Display the user avatar as action icon if available.

d466a05

The avatar image is fetched from the server async, thus connect a signal from the account if the avatar changes. Server feature owncloud/core#26872 is needed.

dragotin requested review from ckamm and ogoffart January 22, 2017 13:07

ogoffart reviewed Jan 23, 2017

View reviewed changes

dragotin added 3 commits January 23, 2017 21:34

Update ASCII-Art describing the connection check flow.

2a12610

Added Avatar fetching

ConnectionValidator: Set a 20 second timeout for Avatar request.

c00e3e8

Avatar: Set a circle mask around the avatar image.

e95b73d

The server displays the avatar cut into a circle, and so we do.

ogoffart reviewed Jan 24, 2017

View reviewed changes

guruz added the DO NOT MERGE YET label Jan 24, 2017

guruz added this to the 2.4.0 milestone Jan 24, 2017

jturcotte reviewed Feb 10, 2017

View reviewed changes

Avatar: Use QImage instead of QPixmap to avoid dep on QApplication.

5e33898

That fixes the test suite.

guruz mentioned this pull request Mar 15, 2017

Adding dav resource for avatars owncloud/core#26872

Merged

14 tasks

Avatar: macOS support

554d1b8

guruz reviewed Apr 12, 2017

View reviewed changes

guruz removed the DO NOT MERGE YET label Apr 12, 2017

ogoffart added a commit that referenced this pull request Apr 13, 2017

Merge remote-tracking branch 'origin/avatar_pics'

8cb3a77

Also fix compilation because of 22370fdbdadd06f0cacd249a8d7a32f0d3c1374e Pull request #5482

ogoffart closed this Apr 13, 2017

ogoffart deleted the avatar_pics branch April 13, 2017 09:56

BloodyIron mentioned this pull request Jan 3, 2018

Avatar requests use the new WebDAV endpoint regardless of the server version #6279

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display Avatar pictures in the Settingsdialog #5482

Display Avatar pictures in the Settingsdialog #5482

dragotin commented Jan 22, 2017 •

edited

Loading

ogoffart left a comment

ogoffart Jan 23, 2017

ogoffart Jan 23, 2017

dragotin Jan 23, 2017

ogoffart Jan 23, 2017

dragotin commented Jan 23, 2017

dragotin commented Jan 23, 2017

ogoffart left a comment

ogoffart Jan 24, 2017

dragotin Jan 24, 2017

guruz Jan 24, 2017

dragotin Jan 24, 2017

ogoffart Jan 24, 2017

dragotin Jan 25, 2017

jturcotte Feb 10, 2017

dragotin commented Mar 9, 2017

dragotin commented Mar 9, 2017

guruz commented Apr 12, 2017

guruz commented Apr 12, 2017

guruz commented Apr 12, 2017

guruz commented Apr 12, 2017

guruz commented Apr 12, 2017

CLAassistant commented Apr 12, 2017 •

edited

Loading

guruz Apr 12, 2017

dragotin commented Apr 13, 2017

ogoffart commented Apr 13, 2017 •

edited

Loading

ogoffart commented Apr 13, 2017


		AvatarJob *job = new AvatarJob(_account, this);
		QObject::connect(job, SIGNAL(avatarPixmap(QPixmap)), this, SLOT(slotAvatarPixmap(QPixmap)));

Display Avatar pictures in the Settingsdialog #5482

Display Avatar pictures in the Settingsdialog #5482

Conversation

dragotin commented Jan 22, 2017 • edited Loading

ogoffart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dragotin commented Jan 23, 2017

dragotin commented Jan 23, 2017

ogoffart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dragotin commented Mar 9, 2017

dragotin commented Mar 9, 2017

guruz commented Apr 12, 2017

guruz commented Apr 12, 2017

guruz commented Apr 12, 2017

guruz commented Apr 12, 2017

guruz commented Apr 12, 2017

CLAassistant commented Apr 12, 2017 • edited Loading

Choose a reason for hiding this comment

dragotin commented Apr 13, 2017

ogoffart commented Apr 13, 2017 • edited Loading

ogoffart commented Apr 13, 2017

dragotin commented Jan 22, 2017 •

edited

Loading

CLAassistant commented Apr 12, 2017 •

edited

Loading

ogoffart commented Apr 13, 2017 •

edited

Loading