Split sharedialog code #3773

rullzer · 2015-09-07T14:42:57Z

In preparation of the full desktop sharing (#3737) the sharedialog code had to be somewhat unentangled.

There now is more code in libsync and the gui is cleaned up a bit.

OCSJob class added. To allow a nice abstraction also for the new sharee endpoint code (which is required)
OcsShareJob class is now in libsync and has more generic functions like createShare, setPassword etc. So the GUI part can just say do this.
ShareDialog is cleaned up

My tests show that everything still works as expected but please run your own tests to make sure I did not break anything!

CC: @dragotin @guruz @ogoffart and others that want to test!

phil-davis · 2015-09-07T14:53:22Z

src/libsync/ocssharejob.h

+    /**
+     * Set the password of a share
+     *
+     * @param password The apssword of the share, if the password is empty the


s/apssword/password

ogoffart · 2015-09-08T14:03:36Z

Ideally, my long term goal would be that libsync would be for the sync engine.
Adding these jobs there goes against my goal.

rullzer · 2015-09-08T14:07:58Z

@ogoffart ah mm, then we could also move the jobs to the gui for now...
but also they are not really gui... yet is not ideal...

I'll move them to the gui for now.

guruz · 2015-09-08T14:47:29Z

Not sure I agree with @ogoffart .. sharing will be more prominent in future.. hm

dragotin · 2015-09-08T14:49:59Z

Not agreeing with @ogoffart is hardly an option.

rullzer · 2015-10-04T20:23:38Z

So finally my qt5 setup is running :)
All the files are now back in gui. But we might want to move to an libocs or libshare in the not to distant future. But I need somebody with some cmake-fu for that.

Please tests and review :)

phil-davis · 2015-10-05T01:45:19Z

src/gui/ocssharejob.h

+    explicit OcsShareJob(AccountPtr account, QObject *parent = 0);
+
+    /**
+     * Constructors for exisiting shares of which we know the shareId


s/exisitng/existing/

rullzer · 2015-10-05T08:50:08Z

@phil-davis thanks. Fixed

guruz · 2015-10-14T06:28:10Z

I'm not working on oC these days, maybe @danimo @dragotin can have a look.

dragotin · 2015-10-14T08:32:34Z

src/gui/ocssharejob.cpp

+    QList<QPair<QString, QString> > getParams;
+    getParams.append(qMakePair(QString::fromLatin1("path"), path));
+    setGetParams(getParams);
+


I think an API that provides a method addParam( QPair()) instead of the setGetParams(QList()) would be nicer because it would save to set up the List and all the pairing here. Could we also use just an addParam() rather than addGetParam() and addPostParam()?

Yes I like that. let me take care of that.

dragotin · 2015-10-14T08:36:29Z

Apart from my comment above (which is kind of minor but maybe still nice to have) I like this very much 👍

As usual my question: Do we see any chance to (unit-) test our job implementations, especially with a focus on error handling that we do? Any idea?

ckamm · 2015-10-14T08:53:40Z

@rullzer Feel free to defer my comment about splitting this into several classes and result parsing to later. I think this is already way nicer right now! :)

There is now a generic OCSJob which must be inherited by other jobs. This is in prepartion for the other OCS job that will come (for the Sharee API endpoint for example). More logic is moved from the sharedialog to the OcsShareJob. So in the GUI code we now only say what we want (a new share, set the password etc). And the code in libsync will make that happen. Error handling is for now still done in the GUI part. For now the ocsjob and ocssharejob live in gui but probabaly we should create a libshare or libocs at some point.

* Comments * Use the path of the abstractnetworkjob

rullzer · 2015-10-16T06:12:29Z

Fixes are on their way. Basically what I noticed is that yet another layer of abstraction would be nice. So that we have share objects.

Then the gui interacts with a share object (sets password etc). The object in the background then makes sure that the proper jobs are fired etc. That should make the gui code even cleaner and allow us to have really proper separation between the logic and the gui. But also that is future work.

* Pass the share_id to the functions that need it

rullzer · 2015-10-16T06:33:39Z

Ok pushed. Lots of commits (but squashing will come later).
Code is a lot cleaner now. But we still have room for improvement.

@ckamm I addressed most of your comments I think. Thanks.

@ckamm @dragotin Could you have another look before I start squashing commits and merge it.

phil-davis · 2015-10-16T06:41:24Z

src/gui/ocsjob.h

+ * @ingroup gui
+ *
+ * Base class for jobs that talk to the OCS endpoints on the server.
+ * All the comminication logic is handled in this class.


s/comminication/communication/
I will just add low-value comments by finding the comment typos, and along the way maybe some day be able to learn how this beast all fits together and then contribute something actually useful :)

ckamm · 2015-10-16T08:53:28Z

@rullzer The fixes so far look good. Unfortunately I won't have time for another full review next week.

rullzer · 2015-10-16T10:51:39Z

Thanks @ckamm @phil-davis

guruz · 2015-10-21T14:39:37Z

src/gui/ocssharejob.h

+        CREATE = 4,
+        DELETE = 8,
+        SHARE = 16,
+        ALL = 31


Why not READ | UPDATE | ... ?

Split sharedialog code

rullzer added feature:sharing 3 - Done labels Sep 7, 2015

rullzer added this to the 2.1-next milestone Sep 7, 2015

rullzer force-pushed the split_sharedialog branch from b5d49f2 to 4b789f9 Compare September 7, 2015 14:45

phil-davis reviewed Sep 7, 2015
View reviewed changes

rullzer force-pushed the split_sharedialog branch from 4b789f9 to b94aa63 Compare October 4, 2015 20:21

phil-davis reviewed Oct 5, 2015
View reviewed changes

rullzer force-pushed the split_sharedialog branch from b94aa63 to adc7e9f Compare October 5, 2015 08:49

ckamm self-assigned this Oct 14, 2015

dragotin reviewed Oct 14, 2015
View reviewed changes

rullzer added 7 commits October 15, 2015 20:05

Added setPublicUpload to OcsShareJob

3ea944d

OCSJob -> OcsJob and more docs

db1f4d4

Now add parameters in a less crappy way

a34b663

Thumbnailjob fixes

4441053

* Comments * Use the path of the abstractnetworkjob

Remove unused members

0371933

Move permissions to OcsShareJob

f95fea9

Now only 1 constructor to ocssharejob

40ab3ee

* Pass the share_id to the functions that need it

rullzer force-pushed the split_sharedialog branch from adc7e9f to 40ab3ee Compare October 16, 2015 06:30

phil-davis reviewed Oct 16, 2015
View reviewed changes

rullzer added 2 commits October 16, 2015 12:48

Use overloaded functions

0e97fbb

Typos

4a04dc1

guruz reviewed Oct 21, 2015
View reviewed changes

src/gui/ocssharejob.h

CREATE = 4,

DELETE = 8,

SHARE = 16,

ALL = 31

Copy link

Contributor

guruz Oct 21, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not READ | UPDATE | ... ?

rullzer added a commit that referenced this pull request Oct 21, 2015

Merge pull request #3773 from owncloud/split_sharedialog

ec351e0

Split sharedialog code

rullzer merged commit ec351e0 into master Oct 21, 2015

rullzer deleted the split_sharedialog branch October 21, 2015 14:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split sharedialog code #3773

Split sharedialog code #3773

rullzer commented Sep 7, 2015

phil-davis Sep 7, 2015

ogoffart commented Sep 8, 2015

rullzer commented Sep 8, 2015

guruz commented Sep 8, 2015

dragotin commented Sep 8, 2015

rullzer commented Oct 4, 2015

phil-davis Oct 5, 2015

rullzer commented Oct 5, 2015

guruz commented Oct 14, 2015

dragotin Oct 14, 2015

rullzer Oct 14, 2015

dragotin commented Oct 14, 2015

ckamm commented Oct 14, 2015

rullzer commented Oct 16, 2015

rullzer commented Oct 16, 2015

phil-davis Oct 16, 2015

ckamm commented Oct 16, 2015

rullzer commented Oct 16, 2015

guruz Oct 21, 2015

Split sharedialog code #3773

Split sharedialog code #3773

Conversation

rullzer commented Sep 7, 2015

phil-davis Sep 7, 2015

Choose a reason for hiding this comment

ogoffart commented Sep 8, 2015

rullzer commented Sep 8, 2015

guruz commented Sep 8, 2015

dragotin commented Sep 8, 2015

rullzer commented Oct 4, 2015

phil-davis Oct 5, 2015

Choose a reason for hiding this comment

rullzer commented Oct 5, 2015

guruz commented Oct 14, 2015

dragotin Oct 14, 2015

Choose a reason for hiding this comment

rullzer Oct 14, 2015

Choose a reason for hiding this comment

dragotin commented Oct 14, 2015

ckamm commented Oct 14, 2015

rullzer commented Oct 16, 2015

rullzer commented Oct 16, 2015

phil-davis Oct 16, 2015

Choose a reason for hiding this comment

ckamm commented Oct 16, 2015

rullzer commented Oct 16, 2015

guruz Oct 21, 2015

Choose a reason for hiding this comment