new-day-international / reddit

New Day fork of the code that powers reddit.com
Other
3 stars 1 forks source link

Name Picker - Phase 1 #88

Open memetic007 opened 11 years ago

memetic007 commented 11 years ago

Estimate: 1d

Data Entry Field for entering a username. Suggest names as characters are entered.

Display Common Name

For Phase One match on full list of all usernames in system. That should be satisfactory at least until we have a few thousand users.

examples from Facebook

namepicker001 nmaepicker002 nmaepicker003

memetic007 commented 11 years ago

Moved Picture and Home Town to Phase 2

memetic007 commented 11 years ago

Name Picker should have two modes of operation: single name, and multi-name.

Single name should work like the Facebook example above.

Multiname should allow multiple names to be entered into the field, with the ability to delete them, in a manner similar to the Gmail example below.

namepickermulti

inflector commented 11 years ago

Some relevant links:

memetic007 commented 11 years ago

The first one here http://www.jqueryrain.com/2012/03/35-best-ajax-jquery-autocomplete-tutorial-plugin-with-examples/ looks just about what we want in terms of UI behavior for the "single" field (ie "@" notification). Don't know about the back end.

http://textextjs.com/?ref=hn looks pretty close to what i had in mind for "multi-field names"....

haven't a clue if the "back ends" would scale to 10,000.

=jim

On Sat, Oct 12, 2013 at 11:47 AM, Curtis Faith notifications@github.comwrote:

Some relevant links:

  • List of various jQuery plugins to do autocompletion:

http://www.jqueryrain.com/2012/03/35-best-ajax-jquery-autocomplete-tutorial-plugin-with-examples/

  • From this page, I liked this one for multi-name picker:

http://textextjs.com/?ref=hn

  • This one does textareas (using the same hack from the next one from Codeacademy, I think)

http://www.amirharel.com/2011/03/07/implementing-autocomplete-jquery-plugin-for-textarea/

-

This one allows you to get the current cursor position from a text area:

https://github.com/Codecademy/textarea-helper

— Reply to this email directly or view it on GitHubhttps://github.com/new-day-international/reddit/issues/88#issuecomment-26199845 .

Jim Rutt JPR Ventures

ffunch commented 11 years ago

A few questions and remarks...

You have in mind for this to happen in which fields? I can imagine:

Facebook starts auto-completing if one does a @ or one starts with a capital letter. Do you want the same?

I didn't take apart the Facebook code, but I'm pretty sure they're not using ajax in that manner, and it isn't particularly necessary. It is an expensive and not very scalable thing to do. Since we're talking about users one is likely to be thinking about, the realm of possibilities is already very limited. Even, or particularly, if we're talking about 10,000 or less, it would be much more handy to have the whole list of likely candidates already downloaded, for example in a javascript array. I believe that's what Facebook is doing. And even if we need to generate custom lists for each user, updated regularly, that's less overhead than doing an ajax call per character, with a search on the server.

It would make sense if it were not just usernames that were autocompleted, but also names of spaces, I think.

ffunch commented 11 years ago

At first glance, TextExt seems like a good base platform for this. It isn't stuck with the idea of using Ajax to get suggestions, and is extensible with other ways of doing it. Lots and lots of options and possibility of doing one's own plugins.

memetic007 commented 11 years ago

responses to Flemming's questions above

You have in mind for this to happen in which fields? I can imagine:

  • the search field
  • the text of a new post
  • a comment text box

_Search_ is not necessary at this time. We'll want to Ajax search in the future (ala Google0 but probably not worth fooling with at all now.

More critical is that Name Picker should be plugged in any place where somebody might want to enter a Username:

Though of course without the requirement for the "@" trigger.

Facebook starts auto-completing if one does a @ or one starts with a capital letter. Do you want the same?

DON'T do the "starts with a Capital Letter" thing for Post and Comments. Very confusing on FB.

Enter name picker mode in posts and Comments on "@" if "@" is preceded by white space or is at the beginning of a line. FB seems to follow that rule.

I didn't take apart the Facebook code, but I'm pretty sure they're not using ajax in that manner, and it isn't >particularly necessary. It is an expensive and not very scalable thing to do. Since we're talking about users one >is likely to be thinking about, the realm of possibilities is already very limited. Even, or particularly, if we're >talking about 10,000 or less, it would be much more handy to have the whole list of likely candidates already >downloaded, for example in a javascript array. I believe that's what Facebook is doing.

There is a noticeable lag in the name picker coming up when I type "@" in Facebook. Much less so when typing "@" plus an initial character.

couple of questions: _it would be much more handy to have the whole list of likely candidates already downloaded, for example in a javascript array._

Can the array be downloaded once per session and used by all pages or does it have to be downloaded for each page?

Can it be loaded asynchronously (like images)?

And even if we need to generate custom lists for each user, updated regularly, that's less overhead than doing >an ajax call per character, with a search on the server.

Overhead might not be a big issue.... this is a human interface mediated functionality used for ancillary functionalities... at a rate (guesstimate) of averaging once per day per user(???). That would be 10000 at day at 10,000 users. Overhead might not matter at that scale.

The decision might be around responsiveness... AJAX is always going to have some latency. A preloaded array could be nearly instantaneous... but I wouldn't want to take a page load hit to achieve that.... hence the questions above.

It would make sense if it were not just usernames that were autocompleted, but also names of spaces, I think.

yes that would make sense if one were entering Space Names. i can't think of any places, though, where that functionality is required.

ffunch commented 11 years ago

Alright, so we'll start with the places where a Username directly is what's called for, sending a message, following, adding moderators, etc.

Yes, the javascript array kind of thing I have in mind could be loaded asynchronously and once per session, or maybe just changing once daily or something like that.

Later, if it gets used for something more frequent and high volume, like referencing other people in comments, it would be quite possible to keep such an array of likely users up to date by a websocket type of mechanism, adding new users, newly created spaces, etc. in near real time.

myers commented 11 years ago

This looks like another js lib to look (as mentioned in standup) http://ichord.github.io/At.js/

ffunch commented 11 years ago

That one looks really good, I agree. First one I've seen that does it right with an @ kind of thing in-place.

ffunch commented 11 years ago

So, I'm using textext for now.

I can't find any page for following that allows entry of a username, so I'm leaving that out.

So, there's the message compose (templates/messagecompose.html), and there's adding a moderator, or an approved submitter, or banning a user, all 3 last ones sharing the same code (templates/userlist.html).

I combined the javascript into one file, namepicker.js, and the css into one file as well, textext-core-autocomplete.css. So, just three lines to throw into the template for getting the usernames and to link to the javascript and css. It will add the namepicker to input fields of class .namepicker.

I had to do some custom CSS to make the fields look more or less like the regular fields, but there might be minor differences. For example, in Chrome on OSX, the To field doesn't get a blue outline when it is in focus. It took me a while to figure out that it is the browser doing that, and not the CSS of the site. It is, however, possible to turn it off with outline: none, which we seem to be doing in a number of places.

memetic007 commented 11 years ago

I can't find any page for following that allows entry of a username, so I'm leaving that out.

On the Follow Page, Click the button in the right column that say "Manage Follow List".

It will route you to:

http://test.lightnetb.org/prefs/follow

On Mon, Oct 21, 2013 at 4:17 PM, Flemming Funch notifications@github.comwrote:

So, I'm using textext for now.

I can't find any page for following that allows entry of a username, so I'm leaving that out.

So, there's the message compose (templates/messagecompose.html), and there's adding a moderator, or an approved submitter, or banning a user, all 3 last ones sharing the same code (templates/userlist.html).

I combined the javascript into one file, namepicker.js, and the css into one file as well, textext-core-autocomplete.css. So, just three lines to throw into the template for getting the usernames and to link to the javascript and css. It will add the namepicker to input fields of class .namepicker.

I had to do some custom CSS to make the fields look more or less like the regular fields, but there might be minor differences. For example, in Chrome on OSX, the To field doesn't get a blue outline when it is in focus. It took me a while to figure out that it is the browser doing that, and not the CSS of the site. It is, however, possible to turn it off with outline: none, which we seem to be doing in a number of places.

— Reply to this email directly or view it on GitHubhttps://github.com/new-day-international/reddit/issues/88#issuecomment-26752294 .

Jim Rutt JPR Ventures

ffunch commented 11 years ago

I'll add that very soon, then. Otherwise the test site is updated with this change for the other pages. Give me some feedback on whether it looks and works ok.

inflector commented 11 years ago

Where's a good place to check for it?

ffunch commented 11 years ago

http://test.lightnetb.org/message/compose/

or moderators, etc, in the settings for a space

inflector commented 11 years ago

In compose, I get an error saying: "That user doesn't exist" if I pick a name from the list like 'Jim_Rutt_007'

memetic007 commented 11 years ago

I checked it out in "compose message", "edit moderators", "edit approved submitters", and "ban user"

All appeared to work nicely.

On Mon, Oct 21, 2013 at 5:15 PM, Curtis Faith notifications@github.comwrote:

Where's a good place to check for it?

— Reply to this email directly or view it on GitHubhttps://github.com/new-day-international/reddit/issues/88#issuecomment-26757281 .

Jim Rutt JPR Ventures

ffunch commented 11 years ago

Hm, I'm getting "that user doesn't exist" as well, when I try to send. I'll dig into that.

memetic007 commented 11 years ago

yep, confirmed. While Name Picker worked nicely to insert the names into the entry boxes, in my first pass i didn't actually try the functionality. When I did, they all failed with the "That user doesn't exist" error.

memetic007 commented 11 years ago

Let's hold off on deployment of Name Picker until we roll out Notifications #94

ffunch commented 11 years ago

I found the problem with the user doesn't exist thing. The textext library was formatting the data to submit with JSON.stringify. I've overridden that behavior now.

The problem now is that the fields that have the namepicker don't get cleared at submit, which would be confusing for users. Submission is done by ajax. In principle that would be easy to fix, if I could just find out where that part is done, which is in no way obvious.

myers commented 11 years ago
ffunch commented 11 years ago

Well, this version of the namepicker only looks up usernames, and it only does one, for the places where one and only one username is what's called for. It doesn't yet:

It really only makes it easier to enter one username where you're supposed to type one. The other things would be nice, and should fit into one of the next phases, I think.

Although, do we really want regular names to show instead of usernames? We've made sure that the username is both their real name and it is unique. So I'm not sure we want that part to work like in Facebook.

inflector commented 11 years ago

Our latest thinking is that we're going to want the picker to go off the non-appended names, but place links to the specific user's profile. We want to drop the _1 etc. eventually almost everywhere. Most people don't have more than one friend with the same exact name. Duplicate names also rarely show up in communities, though they show up regularly in larger populations.

ffunch commented 11 years ago

Ah, ok, that changes things, of course.

Would you like that now, for this phase? I.e. that the list shows Real Names, not Real_Name_IDs, and when you pick one, it shows that you've picked that Real Name, but secretly stores and submits the Real_Name_ID?

Duplicate names are maybe rare, but would have to be considered. Choosing between Curtis Faith and Curtis Faith is impossible. Do I show the identifying number in parenthesis after the name, or what?

And, the trouble with showing the Real Name in the field, even though we really looked up and found a unique Real_Name_ID, is that the user can't see the difference between a found and a not-found name, unless it shows formatted in a particular way, like in the tag demo here: http://textextjs.com/?ref=hn or the way Facebook does it with a friend identified in a comment. Which is probably the best solution.

ffunch commented 11 years ago

Seems to make the most sense to push this out a little bit.

The Real Name vs Real_Name_ID goes well together with Name Picker Phase 2, because that also requires storing several other fields together with the usernames: hometown & picture.

There are two other logical phases, I think:

Phase 4? : Choosing multiple names in one field. For example, for sending a message to several people. That's relatively easily done by a plugin in the TextExt library, but the code processing the submitted form would need to be modified. For example, sending a message doesn't currently support more than one recipient. It would need to be decided where multiple names at one time is needed.

Phase 5? : Referencing other users in arbitrary places in a post or comment, by starting with an @, Facebook or GitHub style. It is doubtful that TextExt will be able to do that. At.js can. The identified username needs to be coded in some fashion in the text source. An html element, like a span, with a particular attribute could handle that, but it needs to be something that works with Markdown, and which will show the name linked when it is displayed in the text. While editing the post/comment, the name should maybe also have a unique formatting, but that kind of conflicts with the whole Markdown idea, so maybe it is enough that it starts with @.

inflector commented 11 years ago

The identified username needs to be coded in some fashion in the text source. An html element, like a span, with a particular attribute could handle that, but it needs to be something that works with Markdown, and which will show the name linked when it is displayed in the text. While editing the post/comment, the name should maybe also have a unique formatting, but that kind of conflicts with the whole Markdown idea, so maybe it is enough that it starts with @.

This should probably be done by putting '@Jim_Rutt_1' in the markdown. We would then extend snudown markdown to allow it to lookup names and such to create the link when it finds an '@' symbol. So in this case, the markdown would use the Real Name ID and output a link that uses both the Real Name (for display) and the Real Name ID for the link itself.

The much clumsier alternative would be to have the name picker create a markdown link element like: Jim Rutt which results in Jim Rutt.

ffunch commented 11 years ago

That makes sense

ffunch commented 10 years ago

Alright, so I've now managed to use At.js for the @part with little difficulty. I still need to make it look and work more closely the same way.

Where it gets more tricky is when I need to have a more complex list, of anything other than just a list of usernames. For example, to show their picture, their name, and their location. That's where it seems like a little less of a brilliant idea to use two different libraries for this, as I'll need to figure out twice how to do this, in two different ways, and they might not be able to use the same representation of that list as input. Still probably less trouble than doing this all ourselves, even though I'm tempted.

myers commented 10 years ago

Code review:

You define "def username_exists" and "def username_to_display_name" 4 times with the same implementation. Couldn't you just define it once?

This should go away:

Use the logging system here to avoid unicode problems. There should be no new print statements. Another advantage to the logging system is you shouldn't need to manual flush.

Is there a race condition here? If two notifications came in at the same time for a user couldn't process 1 load the user with N set as the number of notifications, and increment, but process 2 loads at the same time and also increments, then both save with N+1 as the value?

This introduces a bunch of globals in Javascript like "usernames", and "userdata". Can we at least create a top level namespace like "Namepicker" that they can live in? So move "usernames" to "Namepicker.usernames"

Can we create a thing fullname prefix constant collection? change: if thing._fullname[0:2] == 't5': to if ThingType.is_subreddit(thing._fullname): Even that sucks. The best would be: if isinstance(thing, Subreddit): but maybe there is some reason you don't want to ref "Subreddit" directly?

no need for the outer ()

Testing it out:

I open the front page, and click notify on one time. I start filling out my name "Myers", but there isn't a name picker that pops up.

I fillout "Myers_Carpenter_1" and "item notifications have been sent." It stays there for a while. I would expect it would fade out and disappear after a few seconds.

That notification didn't come to myself. I don't seen any extra queues runners I should be running.

Space notifications seem to work. Got a name picker and everything. The only problem is the text box is replaced by the message "space notifications have been sent", so if I wanted to send more notifications I can't.

I commented on a link adding the text "@Myers_Carpenter_1" and the name picker worked. When the comment was shown "@Myers_Carpenter_1" was gone, but my name wasn't there, even after reloading the page.

These meaningless debug messages where printed out while doing this test: redditheader.html (from user) 0 0 0 MessageController.GET_listing: mark None, self.mark true MessageController.query notifications clear_notification_count Inside Message.listing() redditheader.html (from user) 0 0 0

inflector commented 10 years ago

You define "def username_exists" and "def username_to_display_name" 4 times with the same implementation. Couldn't you just define it once?

There are four different callback implementations for a reason. Might not be a good reason, but I did it that way on purpose. In particular, the callbacks serve two distinct purposes:

1) To allow snudown to know whether or not to process an @notification as a notification to a user or not, and if so to supply the display name in that event.

2) To allow the code to know which users have been notified in any particular post or comment.

So for the first case, we have a global set of callbacks that are installed whenever a page is served. These callbacks are self-contained and do not change any state.

For the second case, we have three different callbacks: one for posting links, one for posting comments, and one for editing comments. These callbacks all change state outside the scope of the callback.

I implemented these three callbacks as local Python functions that have access to a locally scoped set variable that is used to add Accounts that are referenced via @notify in the text. I generally prefer to have local scoped functions, i.e. defs inside other defs for this sort of callback so you can easily see what's going on in one place.

So the code might appear the same, or similar, but usages are actually all different in important ways.

This method also made it very easy to implement the test cases for the snudown tests, if you look at test_snudown.py, you can see callbacks that stub in, or mock, the lookup of the user defined in the test case.

This should go away:

I thought adding git_backup as an ignore would be useful since it makes it easy to backup the git directory and not have to worry about git then picking up the changes as an addition.

Use the logging system here to avoid unicode problems. There should be no new print statements.

Agreed. I'm using print instead of log.info etc. whenever I intend to remove them. I use the log.info etc. when I want to provide useful logging information on a continuing basis.

These meaningless debug messages where printed out while doing this test: redditheader.html (from user) 0 0 0 MessageController.GET_listing: mark None, self.mark true MessageController.query notifications clear_notification_count Inside Message.listing() redditheader.html (from user) 0 0 0

I'll clear these out...

I believe the other comments apply to code that Flemming wrote

myers commented 10 years ago

@ffunch I think this is bug:

Create a new text item, post it and arrive on the item's page. Go to edit the text. Type "@Myers". Expected: Name picker UI. What happens now: Nothing. No errors in the javascript console either.

myers commented 10 years ago

Just as a note: When I was running this code I noticed that the notification tab in messages would show both notifications I had sent and received.

ffunch commented 10 years ago

The namepicker wasn't working in an edited post at all. Now it is.