Wedge

Nao · « Reply #**195**, on June 19th, 2013, 06:08 PM »

Quote from Dragooon on June 19th, 2013, 01:27 PM

This seems better suited for my generic button controls idea. Basically I wanted notifiers to be able to add their own control buttons which can do whatever they want, that way they can have more control on a per-notifier basis instead of we determining their behaviour.

Hmmm... That, I could possibly do, I guess... I'll have a look. That would also, I guess, solve some of our problems...

Basically, we'd need to move the preview action to another function in the Notifier object, is that it..?

Quote

Do whatever the hell you want with emails, I can use all the help I can get there. I have no idea how to simplify those :P

Yeah, I still don't know either, since generic strings at the end of the e-mail are cool, but they also imply one shouldn't use HTML in the notification language string, either... Hmm...

spoogs

Probably Already reported but couldn't find it anywhere

I'm showing 1 new notification after having previewed and viewed the post I'm being notified about. The notification relates to likes for this post.

Hmmm I clicked the "x" to close and it eventually marked it as read it seems since the entry can still be seen in "View All"... is this how it's supposed to work now?

Also @Nao in reference to feedback, I'm not sure I follow whats being said/asked, I actually assumed you were speaking to the more technically inclined. Could be that I'm usually with a drink at hand most times as I'm browsing or brain fried late nights. Dont mind spitting my opinions around at all but when it seems to get technical I tend to leave that for those more in the know.

Nao · « Reply #**197**, on June 20th, 2013, 05:20 PM »

There's a filter for unread notifs (default), and a filter for all notifs, including read ones... Just in case you want to quickly access a notification you previewed and closed too quickly, for instance...

As for feedback, I just left it a couple of days, to see if anyone had anything to say, but I guess I'll have to make my decisions myself, eh... ;)

Dragooon · « Reply #**198**, on June 20th, 2013, 06:09 PM »

Quote from Nao on June 19th, 2013, 06:05 PM

Quote from Dragooon on June 19th, 2013, 08:25 AM
Quote
No. They can't disable likes notifs for topics, while keeping likes notifs for media items... See what I mean?
They will be able to. Since the notifier for topics will never be same as the notifier for media items, it'll be a different notifier. Like there are different notifier for quotes and mentions. getObjectType is more or less redundant with individual notifier, the whole idea is to implement different notifiers for different individual items.
Why don't notifier (plugins) inherit from the Notifications class rather than Notifier, anyway..?

Because they are Notifiers not Notifications :P, it's sort of subjective I guess. That made most sense to me.

Quote from Nao on June 19th, 2013, 06:08 PM

Quote from Dragooon on June 19th, 2013, 01:27 PM
This seems better suited for my generic button controls idea. Basically I wanted notifiers to be able to add their own control buttons which can do whatever they want, that way they can have more control on a per-notifier basis instead of we determining their behaviour.
Hmmm... That, I could possibly do, I guess... I'll have a look. That would also, I guess, solve some of our problems...

Basically, we'd need to move the preview action to another function in the Notifier object, is that it..?

I'm not sure, I was thinking of a new function returning an array of buttons which are then embedded.

Arantor · « Reply #**199**, on June 20th, 2013, 06:11 PM »

Notifications = overall system, Notifier = something in the system of which many instances will be created.

It's like the difference between boards and topics ;)

Nao · « Reply #**200**, on June 20th, 2013, 06:20 PM »

The problem is that I often find myself stuck in object hell...

Example: want to have getPreview() return whatever the notifier wants, but have it return topic stuff by default. Moved code from Notifications.php to Class-Notifier.php, with no special treatment... And, I don't even know if it's the correct target. I suppose it is, but sure..? Nope.

Also, I really think it's best to store the object type in its own field in the database, rather than in the data field. Makes it easier to avoid duplicate entries... And to me, it's better than creating multiple notifier names, one for each object type... Urgh!

Dragooon · « Reply #**201**, on June 20th, 2013, 07:08 PM »

Quote from Nao on June 20th, 2013, 06:20 PM

The problem is that I often find myself stuck in object hell...

Example: want to have getPreview() return whatever the notifier wants, but have it return topic stuff by default. Moved code from Notifications.php to Class-Notifier.php, with no special treatment... And, I don't even know if it's the correct target. I suppose it is, but sure..? Nope.

Also, I really think it's best to store the object type in its own field in the database, rather than in the data field. Makes it easier to avoid duplicate entries... And to me, it's better than creating multiple notifier names, one for each object type... Urgh!

Why would you want to have it return topic stuff by default? The thing is, I don't want to pre-define a lot of behaviour, mostly because then authors don't touch and the experience becomes bad again. Plus I don't want to add pre-defined behaviour for "special" object types, that goes against my model. I want it to be absolutely "core", in the sense it should be as unassuming as possible. I'm assuming a few cases (mostly such as getIcon in which it returns the avatar) because they are more or less global and can apply to all. That is why in my view getObjectType is absolutely redundent and not a good idea. I don't think having multiple notifiers is a bad thing, mostly because it gives every notifier an equal chance to implement the same experience for themselves and we don't define anything for them (any notifier that is in the core can't do anything more or less than any other notifier that might be a plugin)

Nao · « Reply #**202**, on June 22nd, 2013, 11:52 AM »

Quote from Dragooon on June 20th, 2013, 07:08 PM

Why would you want to have it return topic stuff by default?

Because that's what most notifications will be about... This is a forum, right..?

Quote

The thing is, I don't want to pre-define a lot of behaviour, mostly because then authors don't touch and the experience becomes bad again. Plus I don't want to add pre-defined behaviour for "special" object types, that goes against my model. I want it to be absolutely "core", in the sense it should be as unassuming as possible. I'm assuming a few cases (mostly such as getIcon in which it returns the avatar)

Perhaps because I imposed this one upon you... ;)

Quote

because they are more or less global and can apply to all.

Then it should be called getAvatar and not even be extensible... ;)

Quote

That is why in my view getObjectType is absolutely redundent and not a good idea. I don't think having multiple notifiers is a bad thing, mostly because it gives every notifier an equal chance to implement the same experience for themselves and we don't define anything for them (any notifier that is in the core can't do anything more or less than any other notifier that might be a plugin)

There are many problems in your implementation. There are good ideas, too, but there are problems. I didn't look much into your system until recently, because it was a lot to take in, and I should have really.
First of all, the different files are still fuzzy even to me. Class-Notification.php and Notifications.php are too much alike in their naming. I've come to learn that basically, Notifications.php should have been called Subs-Notification(s).php, since it's a helper class, but having the action handler within it makes little sense. This is probably why support for class::method wasn't added until recently to Wedge's action router, and it was added specifically for your code... Not a biggie, but it makes one wonder whether this was well thought out..? Maybe it should not be a class at all in the first place...

Also, maybe Notification::issue shouldn't be a method. Maybe it should be issue_notification(...), and this redirects to your Notification object, or something... Notification::issue implies that the notif object is loaded, which shouldn't be needed, at least not at all times. I don't know. Maybe it should, but it's just strange to me.

Notifiers are stored in the database with a plural in the name. A single notifications should be sorted as a 'like' or 'mention', not as a 'likes' or 'mentions'...

I have a hard time understanding why id_object gets its own database field. I mean, is that only for a case where you'd want to mark all notifications from a single topic as read once you read that topic..? But (1) there's no way to be sure the user has read the entire topic anyway, (2) I don't even think the code is currently checking unread notifications for a topic being read. Or is it..?

I could possibly agree with having different notifiers for the same notification type, but would have to be reassured on a couple of points...
- Could be done as an extra plugin, e.g. a class named Thought_Like(s)_Notifier, that extends Likes_Notifier, and can override all of its functions. Is that even doable..? (This would allow someone else to release a plugin that adds like notifications to their own plugin-created data types, without having to rewrite the notifier at all.)
- Could be done within the same notifier class, e.g. when returning the notifier name, getName() should return 'like' or 'like_thought', depending on the origin. Of course, this implies that the notifier keeps track of what type it is, internally.
- Doesn't disrupt all existing SQL queries that rely on id_object to filter items, especially since right now these queries seem to imply that id_object is always a topic.
(BTW, I'm surprised that id_object doesn't even get its own key..?!)

I think that's all for now, uh...

Arantor · « Reply #**203**, on June 22nd, 2013, 05:33 PM »

Without looking at the code (been snowed under with other code)... here's my take on a few things:

Quote

I have a hard time understanding why id_object gets its own database field.

For handling multiple operations to the same thing. E.g. if three people like a post, you'd want to tie them all together using the id_object. I mean we *don't* do that but we *could*. And it's on that field that we do it.

Quote

- Could be done as an extra plugin, e.g. a class named Thought_Like(s)_Notifier, that extends Likes_Notifier, and can override all of its functions. Is that even doable..?

It is, under certain conditions. Firstly, the parent class must be loaded (or invokable through an autoloader) before the child class, and secondly whatever invokes the child class will need the full class name and that seems a bit long for the current system.

Autoloading is currently tricky because the class names don't really match the filenames but we could do something with it if we wanted.

Quote

- Could be done within the same notifier class, e.g. when returning the notifier name, getName() should return 'like' or 'like_thought', depending on the origin. Of course, this implies that the notifier keeps track of what type it is, internally.

We have a gigantic data field for stuffing things like that into so it's no big deal to track it and then generate an appropriate message in getText().

See, there's more potential for likes than we're currently using; there's no reason why we couldn't attach likes to media items, for example. I know media items have ranking, but I can see the use for likes on media items as an additional option.

Quote

(BTW, I'm surprised that id_object doesn't even get its own key..?!)

It'd be better served as a key on (notifier (6), id_object) really from what I remember.

Quote

Also, maybe Notification::issue shouldn't be a method. Maybe it should be issue_notification(...), and this redirects to your Notification object, or something... Notification::issue implies that the notif object is loaded, which shouldn't be needed, at least not at all times. I don't know. Maybe it should, but it's just strange to me.

This is something I've raised in the past, actually. issue_notification() needs to handle loading all the relevant files (including the right file for a given notifier) and then we don't need to load it all each page (which would be handy)

Right now, the notifications core doesn't just load each page but every notifier does too. I can kind of see that need, to load each notifier, so that you know what you have available... I dunno.

Dragooon · « Reply #**204**, on June 22nd, 2013, 06:13 PM »

Quote

Because that's what most notifications will be about... This is a forum, right..?

I want it to be impartial, plus you got a huge freaking gallery inside the forum so I'd doubt that it's just a forum :P.

Quote

Perhaps because I imposed this one upon you... ;)

And it was a good idea, and it should be extensible. Maybe a crazy plugin author gets a good idea and implements something else for their notifier, it doesn't hurt to keep that option open.

Quote

First of all, the different files are still fuzzy even to me. Class-Notification.php and Notifications.php are too much alike in their naming. I've come to learn that basically, Notifications.php should have been called Subs-Notification(s).php, since it's a helper class, but having the action handler within it makes little sense. This is probably why support for class::method wasn't added until recently to Wedge's action router, and it was added specifically for your code... Not a biggie, but it makes one wonder whether this was well thought out..? Maybe it should not be a class at all in the first place...

Notifications.php is the "controller" here, it handles the notifications logic. If you have a better name go ahead, but I've thought a lot about my code and to me it makes perfect sense.

Quote

Also, maybe Notification::issue shouldn't be a method. Maybe it should be issue_notification(...), and this redirects to your Notification object, or something... Notification::issue implies that the notif object is loaded, which shouldn't be needed, at least not at all times. I don't know. Maybe it should, but it's just strange to me.

Hm, maybe. Notification::issue kind of made sense to me since Notification class is sort of the "model" for doing the DB interactions with individual Notification rows. Although it can be made different, not sure.

Quote

Notifiers are stored in the database with a plural in the name. A single notifications should be sorted as a 'like' or 'mention', not as a 'likes' or 'mentions'...

I read it like this notifier handles "likes" so it gets the name "likes", although this is a completely arbitrary issue since no one really reads the DB for grammar.

Quote

I have a hard time understanding why id_object gets its own database field. I mean, is that only for a case where you'd want to mark all notifications from a single topic as read once you read that topic..? But (1) there's no way to be sure the user has read the entire topic anyway, (2) I don't even think the code is currently checking unread notifications for a topic being read. Or is it..?

Why shouldn't id_object be a field? It is the object for which the notification is issued, and has a bunch of uses including multiple handling as @Arantor said. Anything stored in data field is mostly (almost purely) for presentation purposes only. As far as key goes...I forgot :P.

And in my world, there is no "object type". There is the notifications core, and then there are notifiers who do whatever they want. I don't want to abstract it further.

live627 · « Reply #**205**, on June 22nd, 2013, 11:33 PM »

Quote

And it was a good idea, and it should be extensible. Maybe a crazy plugin author gets a good idea and implements something else for their notifier, it doesn't hurt to keep that option open.

* live627 chuckles

And that crazy author is me.

Arantor · « Reply #**206**, on June 23rd, 2013, 01:36 AM »

What did you do? :P

live627 · « Reply #**207**, on June 23rd, 2013, 04:22 AM »

hehehe award image instead of user avatar

Arantor · « Reply #**208**, on June 23rd, 2013, 04:45 AM »

Yeah, see, that makes sense to me.

Nao · « Reply #**209**, on June 24th, 2013, 11:29 PM »

Quote from Arantor on June 22nd, 2013, 05:33 PM

It is, under certain conditions. Firstly, the parent class must be loaded (or invokable through an autoloader) before the child class, and secondly whatever invokes the child class will need the full class name and that seems a bit long for the current system.

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..? :-/
Wouldn't it be simpler to, I don't know... Force Notification::issue to require getNotifiers('Likes likes') or 'Likes:likes', or whatever structure that calls for the file to be loaded if not already done..?
Same for any situations where the 'likes' notifier is requested, anyway.

Quote

Autoloading is currently tricky because the class names don't really match the filenames but we could do something with it if we wanted.

Hmm, I think they do..? (Once you ucfirst them, of course...)

Quote

See, there's more potential for likes than we're currently using; there's no reason why we couldn't attach likes to media items, for example. I know media items have ranking, but I can see the use for likes on media items as an additional option.

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... :P

Quote

Quote
(BTW, I'm surprised that id_object doesn't even get its own key..?!)
It'd be better served as a key on (notifier (6), id_object) really from what I remember.

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

Posted: June 24th, 2013, 11:14 PM

Quote from Dragooon on June 22nd, 2013, 06:13 PM

I want it to be impartial, plus you got a huge freaking gallery inside the forum so I'd doubt that it's just a forum :P.

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

Quote

And it was a good idea, and it should be extensible. Maybe a crazy plugin author gets a good idea and implements something else for their notifier, it doesn't hurt to keep that option open.

And a good idea from John, I think..!

Quote

Notifications.php is the "controller" here, it handles the notifications logic. If you have a better name go ahead, but I've thought a lot about my code and to me it makes perfect sense.

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?

Quote

Hm, maybe. Notification::issue kind of made sense to me since Notification class is sort of the "model" for doing the DB interactions with individual Notification rows. Although it can be made different, not sure.

issue_notification, really, and everything that's in the issue method...

Code: [Select]

Notification::issue($id_author, WeNotif::getNotifiers('likes'), $_REQUEST['msg'], array(
	'topic' => $topic,
	'subject' => $subject,
	'member' => array(
		'id' => we::$id,
		'name' => we::$user['name'],
	),
));

Possible change...

Code: [Select]

issue_notification($id_author, 'Likes likes post', $_REQUEST['msg'], array(
	'topic' => $topic,
	'subject' => $subject,
	'member' => array(
		'id' => we::$id,
		'name' => we::$user['name'],
	),
));

With the second member meaning: Likes.php notifier file, 'likes' notifier name, 'post' type.
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.
After that, in the database, either store as 'likes_thought', or as 'likes' with an extra field called 'type', as I suggested... Thus, id_object would immediately reflect the type of item it relates to; really, if you want to get all notifications related to post #123, you can't simply search for anything with id_object=123, since this would also return all notifications for thought #123 or media item #123, wouldn't it..?

Ah well...
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.)

(I made it work on mobile devices, though... Which is an achievement of sorts, but it needs more love :P)

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

1.	Suggesting we should go for 'wenot', although it carries a bit of negativity... :lol:

Wedge

Home

Login

Register

Nao

[Plugin] Re: Notifications system (1.0)

spoogs

[Plugin] Re: Notifications system (1.0)

Nao

[Plugin] Re: Notifications system (1.0)

Dragooon

[Plugin] Re: Notifications system (1.0)

Arantor

[Plugin] Re: Notifications system (1.0)

Nao

[Plugin] Re: Notifications system (1.0)

Dragooon

[Plugin] Re: Notifications system (1.0)

Nao

[Plugin] Re: Notifications system (1.0)

Arantor

[Plugin] Re: Notifications system (1.0)

Dragooon

[Plugin] Re: Notifications system (1.0)

live627

[Plugin] Re: Notifications system (1.0)

Arantor

[Plugin] Re: Notifications system (1.0)

live627

[Plugin] Re: Notifications system (1.0)

Arantor

[Plugin] Re: Notifications system (1.0)

Nao

[Plugin] Re: Notifications system (1.0)