[Plugin] Notifications system (1.0)

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
[Plugin] Re: Notifications system (1.0)
« Reply #210, on June 25th, 2013, 12:08 AM »
Quote
Hmm, I think they do..? (Once you ucfirst them, of course...)
class Likes_Notifier for example... you'd declare new Likes_Notifier and Likes_Notifier is going to be the information made available to the autoloader. Normally, in autoloading cases you'd have the filename (notifiers/Likes.php) match the class name (Likes_Notifier), which it doesn't.

And an autoloader wouldn't help in wedit, wezip, westr, wesql or similar cases (but most of those aren't dynamically loading) or any similar 5-letter class naming scheme because there's no way to derive the filename from those class names.

And in any case, an autoloader is going to be largely screwed if you want plugins to use them because the plugin folders aren't part of the autoload list. (XenForo solves this aspect by essentially expecting plugins to add to the master Library/ folder so then you have Library/Author/System/Component deals, giving you classes named Author_System_Component, which IMHO is hideously long) And plugins will want to declare their own notifications at times.
Quote
It's actually quite tempting to ditch media ratings in favor of likes, but in that situation, things like fox.noisen.com lose some of their interest, at least for me -- since I like giving very precise ratings to these audio files. But really, in 95% of the cases, a like would even be preferable, because it's much easier to 'grasp' for users, who these days are so incredibly lazy...
I wouldn't ditch them. I'd make it an option. There are times ratings will be useful. There are times likes will be useful. There are times when both are unnecessary.
Quote
Dunno if it matters that much -- if id_object is included as "AND id_object ...", MySQL will prioritize the first test (if it's keyed) before this one, right..?
Sort of. When you're going through and adding clauses in MySQL, you generally want them to follow the ordering of the indexes.

In hindsight my index suggestion was probably naive but I'm kind of thinking on the hoof a bit here. If you have two clauses in a WHERE, MySQL is going to run them in that order. That means if you're doing two WHEREs on table contents, the WHERE clauses should follow the index.

This is kind of a chicken/egg scenario, the query should follow the index but the index is for the query's benefit. What is ideal is the characteristic of selectivity. The more selective something is - i.e. the criteria that lets you select a suitably useful subset of rows with the fewest characters - the better the index will be.

I suggested notification then what the notification is about, because that's logically the direction of selectivity, but I suspect on profiling it, putting the index on id_object then the type of notification (but making the index cover both fields in all cases) would be even better. It's still selecting the same rows, still selecting the same data, but the way it goes about it (which is what EXPLAIN will tell you) is faster. If you have to go through something row by row, better to have the fewest rows possible first.
Quote
Oh, you have no idea how many times I regretted including AeMe by default in Wedge... :^^;:
Not because it's not maintainable -- it is -- but for two reasons:
- too many 'loose licenses' in the code, meaning it's currently not compatible with Wedge's, and it will eventually force me to change most of the libraries to make it work, while I wouldn't have had to do it (so soon), had it been a plugin...
- my original plan was to ditch the attachment system, and replace it with media items uploaded to a default, fallback 'attachments' folder. Hmm... Well, I never got around to do that. Too much attachment code, too many things to change entirely. I could have done it at the beginning... But three years later, I'm just wary of huge code overhauls, and I don't see myself ever doing it, which is a pity...
Do you still want to do it? Most of the licensing issues aren't really issues - the outstanding issues are Exifer/Exifixer/Exifier and JWPlayer. Everything else for that just isn't a problem.

And I suspect it wouldn't be such a big deal to do the replacement - the real issue is doing it for *here* at this point in time. Though I'd suggest refactoring Aeva's code at this point so that action=media itself might still delegate tasks everywhere but it doesn't need to load hundreds of KB of code to do so every time (for busy galleries it can actually hammer a server, to the point where one 'firewall' mod for SMF typically considers it a DOS risk)

If you like, it's something I can tackle when I'm back?
Quote
Why is Notifications a plural, and Class-Notification a singular, for instance..? :P
Why is weNotif called that way (Wedge standard is "wefirstletters", all lowercase[1]), and why is it an object to begin with?
I asked Dragooon about that a while ago. Basically, he wanted to just objectify it because that's how he likes working. There's nothing wrong with that, and it does have some certain conveniences in terms of keeping everything together; it implicitly namespaces everything related to notifications and to a point I'd sort of like to do that with some of the other subsystems in Wedge, e.g. mail sending.

Of course it would also encourage standardisation of the naming system like you wanted to do.
Quote
I'm currently holding my changes, been doing so for a week now, I don't know where to go...

(Mind you, this isn't the only thing I've been stuck on; I'm very, extremely stuck on getting infinite scrolling to work 100%, and despite several internal rewrites, I still can't get it to behave better than it currently does.)
Yeah, I know that feeling. Related, please add my last commit (to Who's Online) to this site... the lack of debugging information is proving a little awkward, especially as I'm fairly sure I've seen two new malware bots hanging around... :/

I do have some of the changes fixed for the /e modifier deprecation but I'm not yet ready to commit them for a similar sort of reason
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

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 #211, on June 25th, 2013, 07:56 AM »Last edited on June 25th, 2013, 08:03 AM
Not replying to all, kinda stuck IRL but will reply to a couple of points
Quote
It's a bit fucked up... First of all, the notifiers aren't auto-loading, they're explicitly loaded and invoked in the initializer, and a hook is used to allow for other notifiers to plug into it. Seriously..? :-/
How else do I figure out what all notifiers are available?
Quote
Why is weNotif called that way (Wedge standard is "wefirstletters", all lowercase), and why is it an object to begin with?
You renamed it weNotif, it was WeNotif :P. It's an object because I wuv objects, seriously, it's just a personal preference.
Quote
This way, I can easily do a notification for a thought, with 'Likes likes thought' for instance... Likes.php could then get the 'thought' request, and if it doesn't support it, make a request to something, so that it autoloads it... From a plugin, for instance. But I'm not a specialist of object management, so... It's a bit over my skill level, I guess.
In the process you end up making Notifiers an if/else maze since they will be acting like two notifiers in one (really, likes_thought and likes_post will have almost nothing in common), they're better served as independent notifiers. And I was passing notifier objects because of type strictness and all (less headache and more clean stuff).
Quote
Also, I modified your template to ensure that the periodical e-mail doesn't use any HTML, which it did in your version, hmmm... Who cares, this e-mail has yet to be sent to anyone, anyway... :P
You know right that sendmail function is perfectly capable of handling HTML content, you just need to set the $send_html flag to true? Periodical e-mail already does that and when I received my periodical from localhost it was displaying it fine. Yeah though I wasn't too sure it did any good (the HTML formatting)
[Plugin] Re: Notifications system (1.0)
« Reply #212, on June 25th, 2013, 10:04 AM »
Okay, how can I disable infinite scrolling? It's annoying when trying to quick reply.

Oh what the hell, the idea of object type just hit me. One can have likes_post, likes_thought or simply likes with post, thought, media_comment etc. and mentions with different object type. You mind if I do this?
The way it's meant to be

Nao

  • Dadman with a boy
  • Posts: 16,079
[Plugin] Re: Notifications system (1.0)
« Reply #213, on June 27th, 2013, 03:22 PM »
Quote from Arantor on June 25th, 2013, 12:08 AM
class Likes_Notifier for example... you'd declare new Likes_Notifier and Likes_Notifier is going to be the information made available to the autoloader. Normally, in autoloading cases you'd have the filename (notifiers/Likes.php) match the class name (Likes_Notifier), which it doesn't.
Could easily rebuild the filename from the requested filename, it's just a matter of setting the convention..?
Quote
And an autoloader wouldn't help in wedit, wezip, westr, wesql or similar cases (but most of those aren't dynamically loading) or any similar 5-letter class naming scheme because there's no way to derive the filename from those class names.
I don't think we have the same definition of an autoloader, though... You're talking about spl_autoload_register, right..? I was thinking of something entirely Wedge-made, myself...
Quote
I wouldn't ditch them. I'd make it an option. There are times ratings will be useful. There are times likes will be useful. There are times when both are unnecessary.
Per-album option, then..?
Quote
This is kind of a chicken/egg scenario, the query should follow the index but the index is for the query's benefit. What is ideal is the characteristic of selectivity. The more selective something is - i.e. the criteria that lets you select a suitably useful subset of rows with the fewest characters - the better the index will be.
If I was MySQL (but I am not), I would do it this way...
- gather all of the AND elements in the WHERE.
- go through each of them, see if there's a matching index, if yes, use index, otherwise, skip the test for now, and look for the next test that has an index.
- when the loop is finished, if it's still TRUE, then go back to the beginning, and run a regular unindexed sort on all remaining entries, until we get FALSE.

Wouldn't it, err... Make more sense, to run queries this way...?
But as I said, I'm not MySQL... These guys probably know better than me, don't they..?
Quote
I suggested notification then what the notification is about, because that's logically the direction of selectivity, but I suspect on profiling it, putting the index on id_object then the type of notification (but making the index cover both fields in all cases) would be even better. It's still selecting the same rows, still selecting the same data, but the way it goes about it (which is what EXPLAIN will tell you) is faster. If you have to go through something row by row, better to have the fewest rows possible first.
I think I'll let you work on this one, hmm... ;)

Of course, ideally the notif table would be as empty as possible, so it wouldn't be much of a problem. (Thinking about it, do we have an OPTIMIZE query for this one at any point..? Because, I don't think we do, and it'd be useful...)
Quote
Do you still want to do it?
Do what? Ditch attachments..? Nope. I'm always thinking, "this is Wedge 2.0 material... I'll just write a frigging script to convert all current attachments to media items, and be done with it..."
Quote
Most of the licensing issues aren't really issues - the outstanding issues are Exifer/Exifixer/Exifier and JWPlayer. Everything else for that just isn't a problem.
Yes, true enough, I fixed the Highslide license issue years ago, but it took me weeks of work to rebuild my very own (Zoomedia), and I'm not looking forward to rewriting the rest of the code for our licensing purposes, I reckon...
Quote
And I suspect it wouldn't be such a big deal to do the replacement - the real issue is doing it for *here* at this point in time. Though I'd suggest refactoring Aeva's code at this point so that action=media itself might still delegate tasks everywhere but it doesn't need to load hundreds of KB of code to do so every time (for busy galleries it can actually hammer a server, to the point where one 'firewall' mod for SMF typically considers it a DOS risk)
Hmm...? There's a mod for SMF that advises against using AeMe..? Really?
Quote
If you like, it's something I can tackle when I'm back?
As I told you before (I think), I always feel like it's "my area", but since I haven't even started work on it yet, feel free to do it, after all, I can always alter your changes later, it's really not a big deal, and it'd get me started, I guess...! (Replacing JWPlayer with MediaElementJS is something that really scares the bejesus out of me, I'll admit... And it's not even that big a deal, I suppose...)
Quote
Yeah, I know that feeling. Related, please add my last commit (to Who's Online) to this site... the lack of debugging information is proving a little awkward, especially as I'm fairly sure I've seen two new malware bots hanging around... :/
I think I did it a few days ago, I'm not 100% sure though, I'll have to check...
Quote
I do have some of the changes fixed for the /e modifier deprecation but I'm not yet ready to commit them for a similar sort of reason
This one really annoys me... Especially since we can't do anonymous functions, because PHP 5.3, while the most commonly used version, is still not overwhelmingly spread, and PHP 5.2 is still installed on something like 43-45% of all servers, so it's a no-no, unfortunately...
Posted: June 27th, 2013, 03:06 PM
Quote from Dragooon on June 25th, 2013, 07:56 AM
Quote
Why is weNotif called that way (Wedge standard is "wefirstletters", all lowercase), and why is it an object to begin with?
You renamed it weNotif, it was WeNotif :P. It's an object because I wuv objects, seriously, it's just a personal preference.
Ah, yes, I remember now... Well, I guess I tried to make a concession, by only lowercasing the first letter, and not all, and I figured renaming it to 'wenot' immediately might confuse you, I guess...
Quote
In the process you end up making Notifiers an if/else maze since they will be acting like two notifiers in one (really, likes_thought and likes_post will have almost nothing in common),
Hmm, they do..?
Okay, let's take Likes.php as an example... I'll list the functions that are currently in my own version (I removed getPreview, as it's for topics only, will re-add it to non-topics, I guess...!)

   public function getURL(Notification $notification) --> this could be handled by the parent, for topics by default...
   public function getName() --> all right...
   public function getText(Notification $notification, $is_email = false) --> this one's pretty mandatory (see how I added $is_mail, too?)
   public function handleMultiple(Notification $notification, array &$data, array &$email_data) --> does it really need to be in here..?
   public function getProfile($id_member) --> this is going to be the same everywhere, maybe we could just standardize it...? $txt['we' . getName() . '_likes'], etc...? That way, we can put it in the main object... Yeah, that'd be nice...
   public function getEmail(Notification $notification, array $email_data) --> same here, could be standardized, I say...

All in all, there's not much to be done in a notifier, and if you're writing a notification for a thought, I'd even dare to say, it's the same: you'd want to have the 'default' code in a parent object, and the actual notifiers would just need to trust it, is all.
The issue, though, might be to create a new parent object for thought notifications, or media notifications, and I don't know how best to do that, especially, hmm, if we can't determine first-hand what type the notification is, can we...?
Quote
they're better served as independent notifiers. And I was passing notifier objects because of type strictness and all (less headache and more clean stuff).
Less headaches for you, maybe... ;)
Quote
You know right that sendmail function is perfectly capable of handling HTML content,
Yes...
Quote
you just need to set the $send_html flag to true? Periodical e-mail already does that and when I received my periodical from localhost it was displaying it fine. Yeah though I wasn't too sure it did any good (the HTML formatting)
http://academy.hubspot.com/blog/bid/109587/
Things like that...
Basically, HTML e-mail = higher chance of being flagged as spam, and thus not seen by the end user.

HTML e-mail is fine for a marketing newsletter campaign, where you're precisely trying to attract someone's attention, but it's no good for something that's utilitarian, and it also makes it look less 'personalized', because you can't help thinking that it's an advertisement, even if it isn't.

Also, HTML takes more bandwidth (for your server...!), tends to irritate people more, is less accessible, and is opened to tons of issues, like you have to do inline styles, which I hate, etc, etc...

All in all, it's really best to do plain text, and I'm going to go that way, but I'll still keep the HTML versions anyway, so if in the future you want to offers users to ability to choose between plain text and HTML (with plain text as default), you can always do it... Okay?
[Plugin] Re: Notifications system (1.0)
« Reply #214, on June 27th, 2013, 03:23 PM »
Quote from Dragooon on June 25th, 2013, 10:04 AM
Okay, how can I disable infinite scrolling? It's annoying when trying to quick reply.
Just... Don't keep moving your mouse wheel more than a couple of times after you reach the end of the page..?
Quote
Oh what the hell, the idea of object type just hit me. One can have likes_post, likes_thought or simply likes with post, thought, media_comment etc. and mentions with different object type. You mind if I do this?
I have no idea what you're suggesting, but I'm open to anything you have to offer...? Can you elaborate, then..?

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
[Plugin] Re: Notifications system (1.0)
« Reply #215, on June 27th, 2013, 06:31 PM »
Quote
I don't think we have the same definition of an autoloader, though... You're talking about spl_autoload_register, right..? I was thinking of something entirely Wedge-made, myself...
Um... kind of. Let me walk you through what an autoloader actually does (and it's irrelevant whether it's __autoload or spl_autoload_register)

An autoloader is something that does the loading of a class when it's called for without explicitly loading it. You just say $var = new Classname(); and if it is already loaded, fine, if not the autoloader function is called, and given the Classname and essentially expected to figure out how to load it.

__autoload is the simple version, one function, application-wide, that will receive every call for a class that doesn't currently exist. spl_autoload_register is the swish version, which lets you declare multiple functions that act as __autoload functions. They will be called in order and attempt to find the class name. Bear in mind plugins may well want to declare autoloaders. I'm strongly thinking of making SSI auto-loader driven rather than including every function every time it's used.

Short of having a list of names to files (e.g. wedit => Class-Editor.php), there's simply no way to do it. If we're going to do *that* we might as well not bother with an autoloader in the first place.
Quote
Per-album option, then..?
I was actually thinking per installation myself.
Quote
If I was MySQL (but I am not), I would do it this way...
- gather all of the AND elements in the WHERE.
- go through each of them, see if there's a matching index, if yes, use index, otherwise, skip the test for now, and look for the next test that has an index.
- when the loop is finished, if it's still TRUE, then go back to the beginning, and run a regular unindexed sort on all remaining entries, until we get FALSE.

Wouldn't it, err... Make more sense, to run queries this way...?
But as I said, I'm not MySQL... These guys probably know better than me, don't they..?
There are surprisingly few cases where every AND criteria will be matched on a single index, at least not when you get multi-table queries going. Part keys can be useful too.
Quote
Basically, HTML e-mail = higher chance of being flagged as spam, and thus not seen by the end user.

HTML e-mail is fine for a marketing newsletter campaign, where you're precisely trying to attract someone's attention, but it's no good for something that's utilitarian, and it also makes it look less 'personalized', because you can't help thinking that it's an advertisement, even if it isn't.
I'd be inclined to leave it out for this purpose but perhaps include it for newsletters. Technically, though, if you include both versions (which IIRC you're actually supposed to do for specification compliance) it seems less likely to be misread as spam. But then you're including two copies per email which is not cheap (and the poor mail queue gets hammered even more by it because the bulk insert can't do so many per cycle)[1]
 1. For those not familiar with how the mail queue works, it does something unlike every other insert in SMF/Wedge. Most inserts, even those with bulk lines to insert, are never going to get anywhere near the query packet limit, but the mail queue insertion is potentially very likely to do so. So the query is built bit by bit and then thrown as a single big-ass query, typically around 800KB in length, into the queue table. I have to admit I was impressed when I first saw it because it's showing an awareness of a problem most people don't even know exists.

Nao

  • Dadman with a boy
  • Posts: 16,079
[Plugin] Re: Notifications system (1.0)
« Reply #216, on June 27th, 2013, 11:11 PM »
Autoload: we could easily call the file Class-wedit.php, it doesn't look as good, but it's not much of a problem..?

Per-album: it's the best solution, because one might want to have 'likes' on a regular album, and 'ratings' on an audio album with multiple tracks that the author wants to know who prefers what, etc... (I feel like I made a grammar mistake in that sentence, hmm...)

AND: see, that's where it goes right over my head... I still have no clue about how indexes work, and not that I care much, either...

Email: let's start with just raw text e-mails, all right..?



Other: I'm still lost with these notification objects... An example: getProfile.
Notifications.php does $subscriber->getProfile($memID).
Class-Notifier.php does abstract public function getProfile($id_member)
Likes.php does public function getProfile($id_member), but it doesn't use $id_member...
Notifications.template.php does weNotif::getNotifier($notifier)->getProfile() (without an id_member)...

All in all, I'm a bit lost, but it gets worse with this: I need to have a 'default' getProfile, and I don't know how to declare it.
I changed Class-Notifier's to remove the abstract keyword, but I need to access the current notifier's name, so how do I do it..?

Code: [Select]
public function getProfile($id_member)
{
$name = $notification->getName();
...
}

Where do I get $notification from..? Do I have to declare it in the param list, and then get rid of $memID and stuff which is never used anyway, or... What?

How complicated...

But, my ultimate goal is to enable notifications to be as simple as possible, and this is one of the obstacles on my way. ;)

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
[Plugin] Re: Notifications system (1.0)
« Reply #217, on June 27th, 2013, 11:22 PM »
Quote
Autoload: we could easily call the file Class-wedit.php, it doesn't look as good, but it's not much of a problem..?
Yeah, we'd just have to do that for all the classes.
Quote
Per-album: it's the best solution, because one might want to have 'likes' on a regular album, and 'ratings' on an audio album with multiple tracks that the author wants to know who prefers what, etc... (I feel like I made a grammar mistake in that sentence, hmm...)
(There's no grammar mistake I can discern) I can get behind that.
Quote
AND: see, that's where it goes right over my head... I still have no clue about how indexes work, and not that I care much, either...
If you wanted to know, have a read of "High Performance MySQL" (2nd edition at least) by Baron Schwartz and others, it's pretty awesome. But very specialised and generally not worth worrying about too much. Basically, as long as a query doesn't generally have a filesort in it when EXPLAINed.
Quote
Email: let's start with just raw text e-mails, all right..?
Works for me.
Quote
I changed Class-Notifier's to remove the abstract keyword, but I need to access the current notifier's name, so how do I do it..?
That would make sense, but how often are we actually going to see notifiers that actually have commonality?

As far as current notifier's name, get_class($this) seems the likely candidate?

Nao

  • Dadman with a boy
  • Posts: 16,079
[Plugin] Re: Notifications system (1.0)
« Reply #218, on June 27th, 2013, 11:43 PM »
- Is it feasible?

- It just needs implementing... And, well... As usual, AeMe is not my priority..... :^^;:

- I hate the very fact that explains are so obscure to a non-specialist, to me it makes no sense, and thus has limited interest.

- Good!

- Hmm, I just realized how silly my question was... Doing $this->getName() was enough, uh. Now, if I could understand why getData and getObject are defined in Class-Notification, rather than Class-Notifier..? Shouldn't notifiers be allowed to tamper with these, in the first place..?

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
[Plugin] Re: Notifications system (1.0)
« Reply #219, on June 27th, 2013, 11:51 PM »
Quote
- Is it feasible?
Sure it is. We just rename the classes, declare an autoloader, and do a loadSource on the name if it fits our normal naming convention (or the file exists with the right name). We just get to stop doing loadSource() explicitly everywhere, which is kind of nice.
Quote
- I hate the very fact that explains are so obscure to a non-specialist, to me it makes no sense, and thus has limited interest.
They're obscure, sure, but they contain a *lot* of information in a small space, which is why they're obscure.

As for notifications, @Dragooon is probably the person to ask.

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 #220, on June 28th, 2013, 02:43 PM »
I'm replying in random order, too much stuff. If I miss something let me know.
Quote
- Hmm, I just realized how silly my question was... Doing $this->getName() was enough, uh. Now, if I could understand why getData and getObject are defined in Class-Notification, rather than Class-Notifier..? Shouldn't notifiers be allowed to tamper with these, in the first place..?
The plugin handling notifier sends the data anyway, so notifier doesn't need to really tamper once the notification is issued. Plus you can't change ID of the object in any case, since that's fixed for a notification.
Quote
I have no idea what you're suggesting, but I'm open to anything you have to offer...? Can you elaborate, then..?
I'm suggesting what you've been saying, implement a new column called object_type and add that to the classes. Basically, that way likes can be for thought, post, media, media_comment etc. and similarly mentions can be for thought, post etc. without having multiple notifiers.
Quote
Just... Don't keep moving your mouse wheel more than a couple of times after you reach the end of the page..?
Very hard to do on a trackpad :P
Quote
I changed Class-Notifier's to remove the abstract keyword, but I need to access the current notifier's name, so how do I do it..?
Wait, what? Why? That's an abstract class, I'm fairly sure it'll cause a fatal error if you just remove the abstract keyword...even if it doesn't have any abstract function it'll always be an abstract class. It's not supposed to be created as an instance.
Quote
All in all, there's not much to be done in a notifier, and if you're writing a notification for a thought, I'd even dare to say, it's the same: you'd want to have the 'default' code in a parent object, and the actual notifiers would just need to trust it, is all.
If code can be standardised, go for it. However we can always leave them if a notifier needs to do something. getProfile also handles profile configuration fields (which I actually use for some of my notifiers), so it's useful for that but can have a default implementation I guess.
Quote
The issue, though, might be to create a new parent object for thought notifications, or media notifications, and I don't know how best to do that, especially, hmm, if we can't determine first-hand what type the notification is, can we...?
No point, really. There will be slight differences which need to be handled on a per notifier basis, I think it's best to abstract things into notifier and type and let notifier handle everything instead of standardising too much stuff.

Nao

  • Dadman with a boy
  • Posts: 16,079
[Plugin] Re: Notifications system (1.0)
« Reply #221, on June 28th, 2013, 02:47 PM »
Dragooon, can you tell me something? It may not be you, maybe someone else, but I noticed that one notification type (the default, I think) uses id_object to store the topic ID and $data['id_msg'] to store the message ID, while another uses id_object for id_msg, and $data['topic'] (in mentions, I think) for id_topic...

That doesn't make sense to me, because both point to a post, so which is the better one..? Which makes the most sense, in id_object? I would say id_msg, right..? But that's not the default...

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 #222, on June 28th, 2013, 02:53 PM »
It's on a case by case basis, it has nothing to do with what's the best behaviour, mostly related to how one would want to be notified if they're to be notified multiple times on that object (handleMultiple)

Basically, in a case of mentions, I always believed any individual post having a mention should be told to the user in a separate notification, hence I chose id_msg as the object. However in the case of Quote, I thought it's enough to be notified once in the topic that you've been quoted and then the guy can catch up with the rest. I'm not sure what Likes does since I didn't code that one, although I guess it should be on a per-msg basis and use id_msg as the object. The counterpart is stored in $data mostly for generating the complete URL.

Nao

  • Dadman with a boy
  • Posts: 16,079
[Plugin] Re: Notifications system (1.0)
« Reply #223, on June 28th, 2013, 03:05 PM »
Quote from Dragooon on June 28th, 2013, 02:43 PM
I'm replying in random order, too much stuff. If I miss something let me know.
I don't re-read my old posts, so it doesn't matter, when I need to make a decision, I'll just go with my instincts, if you didn't comment on it earlier.
Quote
The plugin handling notifier sends the data anyway, so notifier doesn't need to really tamper once the notification is issued. Plus you can't change ID of the object in any case, since that's fixed for a notification.
I'm using get_class, as per Pete's suggestion (more specifically, get_class($this), since get_class() returns the parent' class.)

I'm still not sure why we should provide a Notification object to so many notifier methods, since AFAIK, we could easily store that in the notifier object's properties themselves, couldn't we..??
Actually, I'd be inclined to do that, in the first place...

Also, I'll be doing things this way, I think...:

- Notifier (for posts)
  - Likes_Notifier
  - Notifier_Thoughts (for thoughts)
     - Likes_Notifier_Thoughts (or something in that line, for thought likes.)

Notifier_Thoughts can basically inherit from Notifier, and override all functions that directly use a post ID, and feed them a thought ID instead, making it simpler to inherit when you create a new notification, e.g. 'reply to your post', and 'reply to your thought', one would just need to create an additional notifier object, and inherit from the other class, simple enough... Or is it incorrect in how it's thought out..? I don't think it is, but you tell me.
Quote
I'm suggesting what you've been saying, implement a new column called object_type and add that to the classes.
It took me a long time to bring myself to do without a 'type', and now you're going my way instead... Hold your horses, we might be missing ourselves on our way to the other side... :lol:
Quote
Basically, that way likes can be for thought, post, media, media_comment etc. and similarly mentions can be for thought, post etc. without having multiple notifiers.
But why did you change your mind? I can live with having to rewrite some of my code (the stuff I explained just above isn't implemented yet, I'm only finishing up the e-mail stuff), but I'd like to know if you know what you're doing.. ;)
Quote
Quote
Just... Don't keep moving your mouse wheel more than a couple of times after you reach the end of the page..?
Very hard to do on a trackpad :P
I have no idea how that works, to be honest... Didn't even know they had wheels, uh..?
Quote
Wait, what? Why? That's an abstract class, I'm fairly sure it'll cause a fatal error if you just remove the abstract keyword...
Nope... I'm not an OO specialist, but abstract just means "this is a method that the child MUST declare", right..? Just removing the abstract keyword, and writing your own code in the first place, just turns it into a placeholder, which you can decide to override, if you want, but nothing more...
Quote
even if it doesn't have any abstract function it'll always be an abstract class. It's not supposed to be created as an instance.
I'm not sure why these classes are abstract in the first place, but with my rewrite, Notifier no longer has any abstract methods, meaning it can be a 'regular' class, and I'm not sure what the difference is, so if you could tell me before I commit... Is it okay, if I remove the abstract keyword for it, perchance..?[1]
Quote
If code can be standardised, go for it.
I'm working hard on that, but it's all to make creating notifications easier, really.
Ideally, a "basic" notification wouldn't need any contents in its class (except for loading language files in the constructor), and could simply get away with a Notification::issue call, and the appropriate language strings.
 1. Somehow, yesterday's link reawakened some long gone words in my mind... :lol:
[Plugin] Re: Notifications system (1.0)
« Reply #224, on June 28th, 2013, 03:08 PM »
Quote from Dragooon on June 28th, 2013, 02:53 PM
It's on a case by case basis, it has nothing to do with what's the best behaviour, mostly related to how one would want to be notified if they're to be notified multiple times on that object (handleMultiple)
Oh, speaking about that one... All of the notifiers I've got under my hand, return false for this, so I changed the default value to return false, this way, we don't have to reproduce the method every time, eh eh...
Quote
Basically, in a case of mentions, I always believed any individual post having a mention should be told to the user in a separate notification, hence I chose id_msg as the object. However in the case of Quote, I thought it's enough to be notified once in the topic that you've been quoted and then the guy can catch up with the rest.
I don't have your Quote notifier, so I don't know what it has, and you'll have to rewrite it yourself, I guess...
Quote
I'm not sure what Likes does since I didn't code that one, although I guess it should be on a per-msg basis and use id_msg as the object. The counterpart is stored in $data mostly for generating the complete URL.
So I should use id_msg in id_object by default, right..?