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
blizzz opened this issue Dec 1, 2016 · 26 comments
Closed

Missing pieces: user mentions in Comments #2443

blizzz opened this issue Dec 1, 2016 · 26 comments

Comments

@blizzz
Copy link
Member

@blizzz 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)
@blizzz blizzz added this to the Nextcloud 12.0 milestone Dec 1, 2016
@blizzz blizzz mentioned this issue Dec 1, 2016
3 of 7 tasks complete
@MorrisJobke MorrisJobke mentioned this issue Dec 1, 2016
35 of 47 tasks complete
@foobar
Copy link

@foobar foobar commented Dec 2, 2016

😅

@jancborchardt
Copy link
Member

@jancborchardt jancborchardt commented Dec 8, 2016

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 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 ;)

@nickvergessen nickvergessen mentioned this issue Dec 14, 2016
37 of 59 tasks complete
@jancborchardt jancborchardt added this to TODO in Communication Feb 21, 2017
@jancborchardt
Copy link
Member

@jancborchardt jancborchardt commented Feb 21, 2017

@blizzz any progress here or something to review?

@blizzz
Copy link
Member Author

@blizzz blizzz commented Mar 7, 2017

nope, i've other prios so far unfortunately

@jancborchardt
Copy link
Member

@jancborchardt jancborchardt commented Apr 30, 2017

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 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 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

@nickvergessen nickvergessen commented Jul 13, 2017

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

@nickvergessen nickvergessen commented Jul 13, 2017

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 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 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 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 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 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 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

@jancborchardt jancborchardt 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?

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 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

@daita
Copy link
Member

@daita daita commented Aug 30, 2017

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 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

@jancborchardt jancborchardt commented Oct 2, 2017

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

@blizzz
Copy link
Member Author

@blizzz 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

@MorrisJobke MorrisJobke commented Jun 25, 2018

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

@nextcloud-bot nextcloud-bot removed the stale label Jun 25, 2018
@MorrisJobke
Copy link
Member

@MorrisJobke MorrisJobke commented Jun 29, 2018

Nothing for 14 it seems -> moved to 15

@blizzz
Copy link
Member Author

@blizzz 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

@nickvergessen nickvergessen commented Oct 12, 2018

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
Projects
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants
You can’t perform that action at this time.