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

Share tags wip #10487

Closed
wants to merge 4 commits into from
Closed

Share tags wip #10487

wants to merge 4 commits into from

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Aug 18, 2014

Okay, I guess to get this reviewed, I need to formally submit it as a PR, even though it's not entirely finished yet.

So here's my work-in-progress branch for #3812. Tested with contacts only, but nothing contacts-specific in the code, so it should work with other apps that use tags as well.

Known issues:

  • If the owner and the sharee both have a tag of the same name, there will probably some collision with unknown outcome (untested).
  • If the sharee tags a contact of their own with a shared tag, that contact will be counted in the owner's tagged items count as well, i.e. in the number right to the tag's name in the left column of the Contacts app.

@ghost
Copy link

ghost commented Aug 18, 2014

Thanks a lot for your contribution! Contributions to the core repo require a signed contributors agreement http://owncloud.org/contribute/agreement/ Alternatively you can add a comment here stating that this contribution is MIT licensed. Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

@ockham
Copy link
Contributor Author

ockham commented Aug 18, 2014

Okay, I'll fix those issues scrutinizer points out with separate commits. Comments are still welcome.

@jancborchardt
Copy link
Member

@ockham I haven’t tested yet, but does this integrate with the existing »tag-like« mechanisms in the individual apps?

  • Contacts has groups
  • Calendar has categories
  • News has folders
  • (Music will have playlists)
  • Bookmarks has tags

There should not be an additional »tags« element for these apps, but the existing means should be used for this.

@ockham
Copy link
Contributor Author

ockham commented Aug 18, 2014

Abso-taggin'-lutely.

@scrutinizer-notifier
Copy link

A new inspection was created.

@MorrisJobke
Copy link
Contributor

If this should used by other apps, we should have a public interface.

@ockham
Copy link
Contributor Author

ockham commented Aug 19, 2014

There is one already: OCP\ITags. I'm just modifying its backend as to include tags for shared items (which the backend looks up on its own).

@MorrisJobke
Copy link
Contributor

Okay. Never heard of that before :(

@jancborchardt
Copy link
Member

Please review, everyone who was interested in the tags support from #3812: @alexisread @bernic @fpiraneo (and even @tanghus cause I know you will like this. :)

@ockham
Copy link
Contributor Author

ockham commented Aug 27, 2014

@owncloud-bot retest this please

@ockham ockham mentioned this pull request Aug 27, 2014
@ockham
Copy link
Contributor Author

ockham commented Aug 27, 2014

Next iteration: #10679

@ockham ockham closed this Aug 27, 2014
@ockham ockham mentioned this pull request Sep 29, 2014
@lock lock bot locked as resolved and limited conversation to collaborators Aug 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants