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

Missing pieces: user mentions in Comments #2443

Closed
2 of 4 tasks
blizzz opened this issue Dec 1, 2016 · 26 comments
Closed
2 of 4 tasks

Missing pieces: user mentions in Comments #2443

blizzz opened this issue Dec 1, 2016 · 26 comments
Assignees
Labels
1. to develop Accepted and waiting to be taken care of enhancement feature: comments high

Comments

@blizzz
Copy link
Member

blizzz commented Dec 1, 2016

With 11 we put the cornerstones for user mentioning in comments (cf. #753). It's not end user friendly yet, though. For Nc 12 we have left to accomplish:

  • When writing or editing a comment and typing an @ in the beginning of a word followed by another character an auto-complete is offered. It only includes users that have access to the file. Results are supposed to show up with a slight delay. For scaling reasons, it needs to be figured out whether all possible users are fetched in the background in advance, or on demand (like search dialog). done in 13

  • Usernames are somehow allowed to contain whitespaces. They are currently not supported by our regex syntax. To be truly compatible we need to change the underlying markup and, for instance, wrap the mention into [ ]brackets (see below). Changes needs to be reflected into clients so they render properly (comments UI in sidebar, notifications, activites, …)

  • upon click on a comment notification, currently only the corresponding page within Files is being opened. Additionally, the sidebar should be opened with the Comments section active. The new comment should be highlighted, also.

Not fully supported username examples

"Let's talk about it today, @foobar."

Since we allow dots, @foobar and @foobar. are possible scenarios here. If you have both? Would be silly to insert a space between the username and the fullstop.

But even better it is that we allow white spaces in usernames now:

"@alice Bernadette wants to talk"

Username can be '@Alice Bernadette'or'@Alice'(or both).

For now, I improve the Regexp to have the 98% solution, butt when going with autocomplete I am afraid we need to wrap the mention into brackets to be able to support all these funny usernames.

Nice to have

  • subscription system (not further specified yet)

Update Jul 12th and again on Jul 14th

Implementation specs

Overview

  • JavaScript GUI Tool: OC.AutoComplete.GUI
    • fetches AutoComplete options once! (automatically, when opening Comments tab)
    • filters options client-side
    • inserts, following a consumer-defined template, e.g. [@$id] for comments
    • block-removes inserted mentions on backspace/delete
    • uses At.js for the actual presentation and interaction parts (best candiate after research, did not prototype yet)
  • API endpoint: /autocomplete/get/
    • Controller in core
    • gets users from sharees endpoint
    • Parameters
      • optional {itemType} being the thing you look users for, e.g. a file (→ file comments obviously)
      • optional {itemId} the corresponding identifier (requires itemType, throws Exception if missing)
      • optional {sorter} parameter to request a Sorter (default: $sorter = null → none)
    • returns sorted array with resulting objects, as assoc array with id, label and sourceas keys
  • ServerAPI: AutoCompleteManager
    • offers to register a Sorter, e.g. prioritizing who where commenting on the file already. (Might come as a second step, but Controller implementation should have this extension in mind)
@foobar
Copy link

foobar commented Dec 2, 2016

😅

@jancborchardt
Copy link
Member

Hahaha @foobar :D welcome

@blizzz two things:

  • we would do autocomplete suggestions across usernames and realnames, right?
  • do we insert a space automatically after the @-mentioned name, like Github? The lack of that in Discourse for example I find really strange

Let's push this because without autocompletion the feature is basically not really discoverable.

@blizzz
Copy link
Member Author

blizzz commented Dec 8, 2016

we would do autocomplete suggestions across usernames and realnames, right?

Basically I'd follow what sharing is doing. The typed string is passed to the user backends, and what they are looking at is their business. Local ones do username and display name i believe.

do we insert a space automatically after the @-mentioned name, like Github? The lack of that in Discourse for example I find really strange

Reasonable to do so, I agree.

Let's push this because without autocompletion the feature is basically not really discoverable.

And shouldn't be, I guess, it's more groundwork right now. Though it works if you know how to mention whom ;)

@jancborchardt jancborchardt added the 1. to develop Accepted and waiting to be taken care of label Feb 21, 2017
@jancborchardt
Copy link
Member

@blizzz any progress here or something to review?

@blizzz
Copy link
Member Author

blizzz commented Mar 7, 2017

nope, i've other prios so far unfortunately

@jancborchardt
Copy link
Member

One other thing:

When writing or editing a comment and typing an @ in the beginning of a word followed by another character an auto-complete is offered.

We should already offer the autocomplete as soon as the @ is typed. It should prioritize showing them in this order:

  • People who commented on this file previously.
  • People who can see this file / have it shared with them.
  • Contacts you most mentioned or interacted with.

@blizzz
Copy link
Member Author

blizzz commented Jul 12, 2017

People who can see this file / have it shared with them.

Those who cannot access it, should not be shown at all. Or, otherwise, showing them (then of course only those that potentially can get access) and eventually mentioning them would also include creating a share.

Contacts you most mentioned or interacted with.

Basically like above, but it would end up as federated or mail share. Thus, also only offering those we potentially can get access.

@blizzz
Copy link
Member Author

blizzz commented Jul 12, 2017

I added the Implementation specs part into the description and would follow this scheme. This should allow auto completion not only for File Comments, but in any other place too (e.g. calendar descriptions), while still not being bloated. Most requirements come from File Comments itself.

Perhaps you want to have a look and leave input? @schiessle @nickvergessen @LukasReschke @MorrisJobke (and anyone else is invited, too, of course). Beware, since I won't have much time left until i am off for a longer while until the conf, I am going to take on the implementation swiftly (with the risk of changes in case I oversaw a thing).

@nickvergessen
Copy link
Member

Could also do it like facebook does it with closed groups:
bildschirmfoto vom 2017-07-13 13-43-17

mentioning them would also include creating a share.

I would definetly not create a share because someone was mentioned in a comment, that's plain wrong Oo

@nickvergessen
Copy link
Member

The API endpoint sounds like it is going to duplicate a lot from the sharees thing.
Maybe it is possible to combine the two or even reuse the sharee thing here.
It's also already used by the spreed app and others.

@blizzz
Copy link
Member Author

blizzz commented Jul 13, 2017

The API endpoint sounds like it is going to duplicate a lot from the sharees thing.

Unfortunately it is not limited to sharing, users can also come from other sources: external storages, contacts, perhaps circles

@blizzz
Copy link
Member Author

blizzz commented Jul 13, 2017

Could also do it like facebook does it with closed groups:

What is a closed group?

I would definetly not create a share because someone was mentioned in a comment, that's plain wrong Oo

Would not rule it out completely, but if, then it should be very obvious. Looking forward for @jancborchardt feedback about it, since it is a consequence from his list at the end of #2443 (comment)

@jancborchardt
Copy link
Member

jancborchardt commented Jul 14, 2017

What I meant is that it should be possible to mention other people too, although the should of course be lower in the list.

The consequence of mentioning is not creating a share right away, but showing a box below the comment saying "The file is not shared with @\LukasReschke yet. [ Share ]". Clicking that share button would open the share tab with the username you mentioned prefilled in the input.

@blizzz
Copy link
Member Author

blizzz commented Jul 14, 2017

What I meant is that it should be possible to mention other people too, although the should of course be lower in the list.

What is the benefit of mentioning a person that never can see the comment, because the person has no access to the object commented on?

The consequence of mentioning is not creating a share right away, but showing a box below the comment saying "The file is not shared with @\LukasReschke yet. [ Share ]". Clicking that share button would open the share tab with the username you mentioned prefilled in the input.

Does this not go too much into the way of a user? Assuming you write a comment, you have some thoughts you want to type down. A box/message fading in (and perhaps additional click, and switching the context to a different tab and task) kicks you out of your train of thought.

There might be other solutions (decent highlight, but how to get to sharing? clicking on the mention can be misleading, and perhaps we'll have account pages some time in the future? Hover does not work on mobile. Or instead show an info box below after submitting).

All in all this would help to reduce the complexity of gathering users – we could just fetch them from the same source as the contacts menu does. And only let a sorter run over it.

@nickvergessen
Copy link
Member

nickvergessen commented Jul 14, 2017

What is the benefit of mentioning a person that never can see the comment, because the person has no access to the object commented on?

You can comment:

I got a complain with this screenshot from a costumer,
looking at the words @example used, we should fire him,
what do you think @manager2

With example not having access and manager2 having access

Unfortunately it is not limited to sharing, users can also come from other sources: external storages, contacts, perhaps circles

Contacts and circles are already returned by the sharee thing. I think it does deserve another name in the mean time

@blizzz
Copy link
Member Author

blizzz commented Jul 14, 2017

@nickvergessen tz, tz,tz, talking behind the back…

Really, thanks for the info. I did not knew it was all provided by the sharee API. That makes it all easier, and any handling of mentions to people who do not have access to the target object (File or whaterver) can be added at a later stage.

It still needs an own API endpoint, because this needs to go through some Sorter. On client-side we don't have enough info.

Let me update the description.
↑ Done

@jancborchardt
Copy link
Member

What is the benefit of mentioning a person that never can see the comment, because the person has no access to the object commented on?

Referring to a person, which is then linked to their profile so others can check it out. For example it's possible on Github in a private repo.

@blizzz blizzz self-assigned this Aug 30, 2017
@blizzz
Copy link
Member Author

blizzz commented Aug 30, 2017

Contacts and circles are already returned by the sharee thing.

For the record, ContactsMenu does not use the Sharee API, but ContactsStore which itself uses ContactsManager, which eventually searches addressbooks.

The advantage and disadvantage is that it goes not through the user backends (+faster -less complete -no search on backends).

Circles is using it's own endpoint where is searches through the User Manager, Group Manager and ContactsManager.

The Circles approach is interesting and makes sense for this use case as well. If not to say it is even exactly the same use case, except of sorting (even the whole UI!). We should consolidate a common endpoint in core, and perhaps moving the Circles approach thereto, is reasonable, and extend it for sorting.

cc @daita

@ArtificialOwl
Copy link
Member

Confirming that during the conference, I switch the "user" search to a small provider:

https://github.com/nextcloud/circles/blob/master/lib/Service/SearchService.php
https://github.com/nextcloud/circles/blob/master/lib/Search/

I have not wrote a way to 'programmatically' register a new provider because it is an app so I can update the list quickly enough.

@blizzz
Copy link
Member Author

blizzz commented Aug 30, 2017

Never mind the last comments… i am still a bit rusty from the time off.

sharees API backend provides this and more, too. Only, the logic is caught in the Controller. I talked with Björn on how to liberate it and put it into core, so this can be re-used from within NC, too.

@jancborchardt
Copy link
Member

@blizzz do you think the @-mention-suggestions will make it into 13? Anything up for review? :)

@blizzz
Copy link
Member Author

blizzz commented Oct 2, 2017

#6328 as necessary pre-work… i started a dev branch based on that will deal with the backend stuff. not ready yet, nevertheless, #6328 needs to get through first. All in all it should go into 13, otherwise i feel very very very very awful.

@MorrisJobke
Copy link
Member

@blizzz Seems to be delayed again, right? Move to 15?

@nextcloud-bot nextcloud-bot removed the stale Ticket or PR with no recent activity label Jun 25, 2018
@MorrisJobke
Copy link
Member

Nothing for 14 it seems -> moved to 15

@blizzz
Copy link
Member Author

blizzz commented Jun 29, 2018

The second item could be met with a proper solution for #10021, alas i won't have the resources left to tackle it

@nickvergessen
Copy link
Member

I have a local fix for mentions with spaces.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1. to develop Accepted and waiting to be taken care of enhancement feature: comments high
Projects
None yet
Development

No branches or pull requests

8 participants