[Plugin] Notifications system (1.0)

Nao

  • Dadman with a boy
  • Posts: 16,063
[Plugin] Re: Notifications system (1.0)
« Reply #30, on March 19th, 2013, 09:58 AM »
- Hmm, are you sure you can do it? Doesn't that bother you?

- @if member is a trick I only recently added, you'd have surprised me if you'd implemented it that way, actually ;)

- I can do that, if you want... CSS/JS is my area of expertise these days, after all :P

Also... Where the heck is the admin page for setting up notifications...? I couldn't find it. The language files mention "Admin > Plugins > Notifications", so I'm trying to update them to the correct position...

Ah, and the notification popup is incorrectly positioned in my local install. I was wondering, could you test on your side with the latest revision? I'm wondering if it's a browser issue or a bug I introduced when optimizing your JS code... (I did some pretty big changes and didn't know *where* to test, so I wouldn't be surprised.)
Posted: March 19th, 2013, 09:39 AM
Quote from Nao on March 19th, 2013, 09:39 AM
- @if member is a trick I only recently added, you'd have surprised me if you'd implemented it that way, actually ;)
I only recently added it in JS, sorry, it's been in Wess for a longer time ;)

I mixed things up in my head. Which reminds me: notifications.js was a stand-alone file because it was a plugin to begin with... Now that it's a core feature, we should be integrating it into script.js, all right? And add the infamous @if member to it, of course ;)

Dragooon

  • I can code! Really!
  • polygon.com has to be one of the best sites I've seen recently.
  • Posts: 1,841
[Plugin] Re: Notifications system (1.0)
« Reply #31, on March 19th, 2013, 10:16 AM »
Quote from Nao on March 19th, 2013, 09:58 AM
- Hmm, are you sure you can do it? Doesn't that bother you?
Not sure why it would bother me, I can definitely do it :P.
Quote from Nao on March 19th, 2013, 09:58 AM
- @if member is a trick I only recently added, you'd have surprised me if you'd implemented it that way, actually ;)

- I can do that, if you want... CSS/JS is my area of expertise these days, after all :P
Go ahead, it's a part of Wedge anyway
Quote from Nao on March 19th, 2013, 09:58 AM
Also... Where the heck is the admin page for setting up notifications...? I couldn't find it. The language files mention "Admin > Plugins > Notifications", so I'm trying to update them to the correct position...
There isn't one :P, there is only one setting for setting the number of days before which read notifications are pruned and that doesn't have an UI yet.
Quote from Nao on March 19th, 2013, 09:58 AM
Ah, and the notification popup is incorrectly positioned in my local install. I was wondering, could you test on your side with the latest revision? I'm wondering if it's a browser issue or a bug I introduced when optimizing your JS code... (I did some pretty big changes and didn't know *where* to test, so I wouldn't be surprised.)
I can have a look in the evening.
Quote from Nao on March 19th, 2013, 09:58 AM
I mixed things up in my head. Which reminds me: notifications.js was a stand-alone file because it was a plugin to begin with... Now that it's a core feature, we should be integrating it into script.js, all right? And add the infamous @if member to it, of course ;)
Yeah sure. The patch file was a quick job I did in the evening, it definitely isn't finished yet.
Posted: March 19th, 2013, 10:11 AM

Okay, quickly looking over the popup it seems to be correctly positioned in the latest revision.
The way it's meant to be

Nao

  • Dadman with a boy
  • Posts: 16,063
[Plugin] Re: Notifications system (1.0)
« Reply #32, on March 21st, 2013, 10:09 AM »
I've made a few changes to the notification stuff, and uploaded them here, but not committed, so you can easily compare with your local install...
More importantly -- I've (temporarily..?) disabled auto-markread when clicking the notification, so that I can actually have time to style the popup at all... :whistle:

I think it would be best if we went for an approach more similar to Facebook's. Not that all they do is great, but in particular, their notification system is pretty good, and pretty much the only thing I use on their site...
Quote from Dragooon on March 19th, 2013, 10:16 AM
Go ahead, it's a part of Wedge anyway
Hopefully today I'll have included that in script.js, but first I'll commit a changed version of notifications.js so that it's easier for you guys to track all the changes I made, then I'll make another commit where I merge both files together.

File size of original notifications.js when gzipped: 744 bytes
Current file size: 578 bytes

Ideally, getting it under 500 bytes before the merging would be satisfying for me. This will require rethinking some of the code, though, and I'll put the priority on getting it to look nice.
Quote
There isn't one :P, there is only one setting for setting the number of days before which read notifications are pruned and that doesn't have an UI yet.
Alrighty.
We should also make sure that all unread notifs are sent by e-mail before they're pruned... Is it accounted for?

Also, a strange error in the log...
A *guest* accessed a ;area=getunread page. I don't know how the hell they found the link to that..?! I tried logging off, but nothing showed up. Hmm, maybe it's in the footer JS...? I'll have a look again.

Dragooon

  • I can code! Really!
  • polygon.com has to be one of the best sites I've seen recently.
  • Posts: 1,841
[Plugin] Re: Notifications system (1.0)
« Reply #33, on March 21st, 2013, 10:19 AM »
Quote
We should also make sure that all unread notifs are sent by e-mail before they're pruned... Is it accounted for?
Unread notifications aren't pruned, only read ones. By default no emails are sent, you can change that in the profile area. The email area is perhaps one of the most unfinished part, it needs refinement and perhaps use of email templates.
Quote
More importantly -- I've (temporarily..?) disabled auto-markread when clicking the notification, so that I can actually have time to style the popup at all... :whistle:
Did you disable mark read on topic display (See Notification::markReadForNotifier, it handles all auto mark reads for areas other than the notifications action)? Because otherwise one way or the another it'll end up mark it read on click.

Nao

  • Dadman with a boy
  • Posts: 16,063
[Plugin] Re: Notifications system (1.0)
« Reply #34, on March 21st, 2013, 11:04 AM »
Quote from Dragooon on March 21st, 2013, 10:19 AM
Quote
We should also make sure that all unread notifs are sent by e-mail before they're pruned... Is it accounted for?
Unread notifications aren't pruned, only read ones.
I'm not sure it works like this... It seems to me that as soon as you click the popup, it gets marked as read, even if not visiting the topic itself. I may be wrong, but this is what I gathered yesterday... And I haven't looked at the notification code too deeply for now.
Quote
Did you disable mark read on topic display (See Notification::markReadForNotifier, it handles all auto mark reads for areas other than the notifications action)? Because otherwise one way or the another it'll end up mark it read on click.
That's not a problem... As long as I don't read the Birthday topic (which by tradition I'm not supposed to post on for a while :P), it won't get marked in my notifications. (Oh, and I'd appreciate if someone could post another @Nao in that topic, so that I get two notifications to style... :P)

Oh, I love the popup that comes up when I mention someone... It will need some refining, though! For instance, sometimes it doesn't close... (Like now. It's still opened.) Also, I tried locally to position the popup next to the name I entered, and while it works, which is great, it adds another gzipped kilobyte of JS, so I'll try to make it shorter or something, or maybe it's worth it I don't know...

Dragooon

  • I can code! Really!
  • polygon.com has to be one of the best sites I've seen recently.
  • Posts: 1,841
[Plugin] Re: Notifications system (1.0)
« Reply #35, on March 21st, 2013, 11:07 AM »
Quote
I'm not sure it works like this... It seems to me that as soon as you click the popup, it gets marked as read, even if not visiting the topic itself. I may be wrong, but this is what I gathered yesterday... And I haven't looked at the notification code too deeply for now.
That should not happen, some systems do this but I don't intend that.
Quote
Oh, I love the popup that comes up when I mention someone... It will need some refining, though! For instance, sometimes it doesn't close... (Like now. It's still opened.) Also, I tried locally to position the popup next to the name I entered, and while it works, which is great, it adds another gzipped kilobyte of JS, so I'll try to make it shorter or something, or maybe it's worth it I don't know...
I wanted it positioned next to the cursor but couldn't figure it out, so thanks! I'd like to see how you did it even if you don't commit it.

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
[Plugin] Re: Notifications system (1.0)
« Reply #36, on March 21st, 2013, 05:21 PM »Last edited on March 21st, 2013, 05:40 PM by Nao
There's an error been flagged in the log - and it still happened this morning but I'm not sure it's been fixed since.

(edited out)
 8: Undefined index: page_title_html_safe

(Equivalent of action=notification)
When we unite against a common enemy that attacks our ethos, it nurtures group solidarity. Trolls are sensational, yes, but we keep everyone honest. | Game Memorial

Nao

  • Dadman with a boy
  • Posts: 16,063
[Plugin] Re: Notifications system (1.0)
« Reply #37, on March 21st, 2013, 05:40 PM »
@Dragooon> I actually used an external jQuery plugin for that, but it needed to be rewritten a bit because it's old and incompatible. Basically, IE provides a x/y position in its select range object, so that's perfect, but other browsers don't, so the plugin creates a hidden textarea with no scrollbars, fills it with the text up to the caret position, and gets the caret position from that. Well, it's easy to get the vertical position, but as for the horizontal position, I'm not too sure, haven't delved too deeply into its code... All I know, is that it works ;)

@Arantor> Yup I did notice that, and I'm planning to fix it, but right now I'm lost in a larger rewrite of the notification code and that error gets in my way :lol: Also, I edited your link out -- it's a public page, and I don't want Google following a link that generates errors for guests..!

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
[Plugin] Re: Notifications system (1.0)
« Reply #38, on March 21st, 2013, 05:43 PM »
Oops >_<

In other news, we really gotta move it up to the top of the page.
[Plugin] Re: Notifications system (1.0)
« Reply #39, on March 22nd, 2013, 03:11 AM »
Also, the x/y position is off; when using main reply, non-WYSIWYG, the suggester position is the bottom left of the textarea.

Dragooon

  • I can code! Really!
  • polygon.com has to be one of the best sites I've seen recently.
  • Posts: 1,841
[Plugin] Re: Notifications system (1.0)
« Reply #40, on March 22nd, 2013, 03:52 AM »
Currently that's intended, I couldn't position it next to the cursor.

Nao

  • Dadman with a boy
  • Posts: 16,063
[Plugin] Re: Notifications system (1.0)
« Reply #41, on March 22nd, 2013, 10:38 AM »
Quote from Nao on March 21st, 2013, 05:40 PM
I actually used an external jQuery plugin for that, but it needed to be rewritten a bit because it's old and incompatible.
I'm the first surprised, but I managed to take it down to 340 gzipped bytes.... 8-)

Basically, I gave up on the plugin I was using, and took some ideas here and there. Which didn't work...

Here's how I did it. In times like that, I feel really smart... :lol:

- Getting the Y position: reproducing the same text (up to the caret position) in a div that inherits the textarea's font values. That's the classic way of doing it. It's quick, and it works: you just take the div's height (default overflow), and you're done :)

- Getting the X position: here's the tricky part -- the one I figured out by myself. I'm initializing a count variable, and getting the height from above. Then I add a single-pixel width span, and calculate the div's height again. If it's the same, increment the count and continue. If it's not, then it means the div overflowed, and I stop it. Well... By this point, guess what count stands for? It's the number of horizontal pixels between the caret and the right end of the div! Then, all I need to do is $test.width() - count... And voilà, we've got our X position!

Worst part is, it really works :lol:
It could be faster, though... I'll have to consider doing that in pure JavaScript (not jQuery), I don't know. I'll just see.

This isn't tested in Wysiwyg, though. I suspect it won't work there... And I'll have to do a different codepath for it. Which will increase the size of the code. Meh. Well, right now it's at 1130 bytes, gzipped, and it's short enough to warrant being enabled by default here. ;)
Posted: March 22nd, 2013, 10:34 AM

Bugger... Just did a quick benchmark.
With innerHTML: 8 seconds to execute!!
With .append(): 1.4 second.
It's still very slow... Dunno how I can optimize.

live627

  • Should five per cent appear too small / Be thankful I don't take it all / 'Cause I'm the taxman, yeah I'm the taxman
  • Posts: 1,667
A confident man keeps quiet.whereas a frightened man keeps talking, hiding his fear.

Dragooon

  • I can code! Really!
  • polygon.com has to be one of the best sites I've seen recently.
  • Posts: 1,841
[Plugin] Re: Notifications system (1.0)
« Reply #43, on March 22nd, 2013, 11:42 AM »
Use pure javascript? IMO textarea keyup callbacks should really use that, it is a world of a difference when it comes to speed.

Nao

  • Dadman with a boy
  • Posts: 16,063
[Plugin] Re: Notifications system (1.0)
« Reply #44, on March 22nd, 2013, 03:55 PM »
'kay, back from the outer world, immediately jumped onto this...
I was hoping changing .height() to .innerHeight would help, but it didn't.
After multiple tests, I managed to reduce the delay to under 200ms. Another stroke of genius, or maybe not: I simply increased the width of the span... If it's 5px instead of 1px, you need 5 times less additions to get the width, obviously... And a 5px difference doesn't matter much in the final result. It's not rocket science...

I'll still try to find other solutions. Apparently, just changing the display:inline of that span to display:inline-block seems to increase the speed, but I'm not sure if it's related to that... :^^;:
Posted: March 22nd, 2013, 01:01 PM

Doing Wysiwyg is a nightmare... :-/