Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add share manager and the share objects #4033

Merged
merged 11 commits into from
Oct 30, 2015
Merged

Add share manager and the share objects #4033

merged 11 commits into from
Oct 30, 2015

Conversation

rullzer
Copy link
Contributor

@rullzer rullzer commented Oct 29, 2015

Step closer to #3737

Pretty big PR (I'm sorry... I'll promise to do better).

Basically this introduces the abstraction I want. And which should help significantly in making the user/group sharing easier to implement.

There is now a ShareManager, this is the entry point to do anything with shares. Be it creating, or fetching.

Then there is the Share class which represents a generic share and there is the LinkShare class. Since link shares are just like regular shares but slightly different :P. Share objects allow for interaction with the object. E.g. setting the password etc.

The sharedialog code is converted to use the new code and still works for me so far. And is a lot simpler.

TODO:

  • Triplle check share dialog functionality
  • Check share dialog functionality against older servers
  • More comments
  • Maybe some cleanup?
  • Connect error reporting from manager/share

@guruz @dragotin @ckamm please have a look. More commits will follow. But since it kind of works now

@rullzer rullzer added this to the 2.1-next milestone Oct 29, 2015
* This is needed so we can update the share objects properly
*
* @param reply The reply
* @param value To what did we set a varialble (if we set any).
Copy link
Contributor

Choose a reason for hiding this comment

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

variable

@rullzer
Copy link
Contributor Author

rullzer commented Oct 29, 2015

Addressed your comments :) thanks. Already looking better.

{
setPath(path() + QString("/%1").arg(id));
setPath(path() + "/" + id);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should use QLatin1Char('/') here rather than just "/"

@dragotin
Copy link
Contributor

Nice PR.
One last thing: Do you have some kind of error handling with the jobs, in case they fail? I did not see that.
Also, please add proper headers to the files.

@rullzer
Copy link
Contributor Author

rullzer commented Oct 29, 2015

@dragotin no error handling still needs to be done. I plan to do that very simple for now (just like we had it before, show the status code in the footer). But now that we are properly abstracted away we can actually work on proper error handling and reporting in future PR's.

For now pass it on to the gui. So at least they know something is wrong.
@rullzer
Copy link
Contributor Author

rullzer commented Oct 29, 2015

Ok now there is error reporting. Much in the same way it was before. So no improvements there yet. But it seems to be working again.

Now the ShareTypes and Permissions are part of the Share class (which is
a bit better abstracted away).
@rullzer
Copy link
Contributor Author

rullzer commented Oct 29, 2015

Now using QFlags to have better type checking in the code. They are less strict than enum classes but I think they fit better in the Qt coding way here.

@rullzer
Copy link
Contributor Author

rullzer commented Oct 29, 2015

Please let me know if there are further comments. Else I'll merge somewhere tomorrow.

rullzer added a commit that referenced this pull request Oct 30, 2015
Add share manager and the share objects
@rullzer rullzer merged commit d38b190 into master Oct 30, 2015
@rullzer rullzer deleted the share_object branch October 30, 2015 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants