solved Reordering Threaded Comments

TiG
TiG
@tig
7 years ago
184 posts
We plan to provide a hybrid comment structure - where nodes at level 'n' and beyond are simply treated as a linear chronologically ordered list. This provides the benefit of the threaded comment structure as the major organization and then (when too deep in the hierarchy to be coherent) renders simply as a list of chronologically ordered comments. The attached png shows the results we are after (our actual n will be one less). We understand (and currently use) the flattened hierarchic presentation JR uses when the max level is reached. Our challenge is with the actual data structure and -in consequence- the true hierarchic numbers of the nodes (e.g. 3.1.7.4).

That established, to pull this off we would ideally replace the
jrComment_thread_comments
(recursive ordering) function. Conceptually, that is very clean and superbly future-proofed - equivalent to overriding a method.

Since that is not possible, any ideas on the next best (in terms of future proofing) approach to get as close to the above ideal as possible within the JR architecture?


--
TiG

updated by @tig: 03/27/18 08:18:37PM
michael
@michael
7 years ago
7,746 posts
your question is how to change jrComment_thread_comments to that structure?

If it is, I would say dont bother, replace that template call with your own modules comment system. Take whatever you need from jrComment and create a new module tgHybridThreadedComments_comments() or whatever.

That would give you more freedom to build as you like. You can always import the existing comments into your own modules datastore, or write to jrComments datastore direct, whichever suits.
TiG
TiG
@tig
7 years ago
184 posts
Quote: your question is how to change jrComment_thread_comments to that structure?

@michael

I know (algorithmically) how to effect the hybrid tree reordering (easy enough) so that is not my question. My question was how I could best simulate an override of this single function in the jrComment module.

Nothing else in jrComment would be affected so overriding the entire module and changing references in the balance of the code is not an appealing notion.

Regardless, you have answered my question so thanks for that. I will look for something akin to overriding the
jrComment_db_search_items_listener
which is the only function that invokes
jrComment_thread_comment
. That I know I can do with module priorities, but alas I cannot prevent it from executing. I would have to play games, I suppose, and temporarily turn off threading for the life of the function to avoid unnecessary and wasteful functionality in the original function. No doubt you appreciate why I was probing for other ideas.

Thanks, as always, for the advice Michael.


--
TiG
michael
@michael
7 years ago
7,746 posts
looks to me like one angle would be to find a way to remove the jrComments_db_search_items_listener from firing, then fire your own in its place.
instead.jpg
instead.jpg  •  752KB

TiG
TiG
@tig
7 years ago
184 posts
@michael

Interesting. If this could be done safely then we would have a general purpose mechanism for overriding listeners. This could significantly increase the flexibility of the platform IMO.

Immediately, from your suggestion, this is the function for me to investigate:

function jrCore_register_event_listener($module, $event, $function)
{ // We can register 1 of 3 events: // a specific event from a specific module - i.e. 'jrUser','get_info_by_id' // all events from a specific module - i.e. 'jrUser','all_events' // all events for the whole system - i.e. 'jrCore','all_events' if (!isset($GLOBALS['__JR_FLAGS']['jrcore_event_listeners'])) { $GLOBALS['__JR_FLAGS']['jrcore_event_listeners'] = array(); } $ename = "{$module}_{$event}"; if (!isset($GLOBALS['__JR_FLAGS']['jrcore_event_listeners'][$ename])) { $GLOBALS['__JR_FLAGS']['jrcore_event_listeners'][$ename] = array(); } $GLOBALS['__JR_FLAGS']['jrcore_event_listeners'][$ename][] = $function; return true; }

I could construct a function that simply removes a registered event listener from
$GLOBALS['__JR_FLAGS']['jrcore_event_listeners']
and allow for its replacement listener (registered normally) to provide all the needed functionality.

Seems a bit pathological to me, but considering the options this looks to be a reasonable course to at least investigate. Thanks for the tip!


--
TiG
michael
@michael
7 years ago
7,746 posts
The other option is for us to adjust the jrComment_db_search_items_listener to add a way to tell it to not fire. Which we could do if there was a clean place to do it.

You could also fire your listener after that listener and give it a lower priority so you know it fires last, but that would mean the comments listener fires and does all its processing, so you'd be doing it twice.

Your goal is to have a way for that listener NOT to fire (as I understand it).

Having a module working, but turning off part of it via another module might need a bit of creative thinking to get you there. :)

The above is not a recommendation, but just a suggestion of a possible angle that might work.

See what you come up with.
TiG
TiG
@tig
7 years ago
184 posts
Quote: Your goal is to have a way for that listener NOT to fire (as I understand it).

@michael

Yes. My goal is actually to override a single function but sans that ability, the next best thing is to prevent the jrComment db search listener from firing and totally replace it with my own listener.

I understand that you are brainstorming and that you are not suggesting this as the Jamroom recommended approach. But if you guys ever pursue anything along these lines, one nice function would be an unregister (i.e. jrCore_unregister_event_listener). This function would only be allowed for modules that are defined as extensions so there would be a need for another function to link an extension module to its parent (a subclass relationship). If that were in place then JR would have a controlled mechanism to fully override event handlers and thus gain a bit of helpful polymorphism. Another approach (more consistent with the JR style) would be for any function defined in an extension (the subclass) to automatically fully override similarly named functions up the lineage. That would be outstanding. If that were in place today, I would simply create my own ntComment_thread_comment function (in ntComment) to fully override the jrComment_thread_comment in jrComment. Clean and future-proof.

Back to reality ...

When I have a chance to experiment, I will first, in effect, build a simple unregister function. I will try to locate it within the extension module, but I suspect my first roadblock will be timing - ensuring the unregister is placed at the right time so that it accomplishes the desired result. And, indeed, there may be no way to do this reliably.


--
TiG

updated by @tig: 12/23/17 05:11:11PM
michael
@michael
7 years ago
7,746 posts
How about this. Put this into a module's include file. (I called it xxCommentOverride, but anything)
<?php
/**
 * @copyright 2018 noone
 */

// make sure we are not being called directly
defined('APP_DIR') or exit();

/**
 * meta
 */
function xxCommentOverride_meta(){
    $_tmp = array(
        'name'        => 'Comment Override',
        'url'         => 'commentoverdrive',
        'version'     => '1.0.0',
        'developer'   => 'noone, ©' . strftime('%Y'),
        'description' => 'Stops the jrComment_db_search_items_listener listener from firing',
        'category'    => 'custom',
        'license'     => 'mpl',
        'priority'    => 255
    );
    return $_tmp;
}

/**
 * init
 */
function xxCommentOverride_init(){
    jrCore_register_event_listener('jrCore', 'parse_url', 'xxCommentOverride_parse_url_listener');

    return true;
}

function xxCommentOverride_parse_url_listener($_data, $_user, $_conf, $_args, $event)
{ if (isset($GLOBALS['__JR_FLAGS']['jrcore_event_listeners']['jrCore_db_search_items'])) { if ($pos = array_search('jrComment_db_search_items_listener', $GLOBALS['__JR_FLAGS']['jrcore_event_listeners']['jrCore_db_search_items'])) { unset($GLOBALS['__JR_FLAGS']['jrcore_event_listeners']['jrCore_db_search_items'][$pos]); } } return $_data; }

Turns off the comment modules listener.
updated by @michael: 12/24/17 12:44:30PM
TiG
TiG
@tig
7 years ago
184 posts
@michael

My solution pretty much did the same thing - my code was placed within my existing ntComment override module:

My module's init starts with this:

    ntComment_unregister_event_listener('jrCore', 'db_search_items', 'jrComment_db_search_items_listener');   

... to disable the jrComment listener. I then register my replacement listener normally.

The unregister function looks remarkably like your code:

function ntComment_unregister_event_listener($module, $event, $function)
{ $ename = "{$module}_{$event}"; if (($key = array_search($function, $GLOBALS['__JR_FLAGS']['jrcore_event_listeners'][$ename])) !== false) { unset($GLOBALS['__JR_FLAGS']['jrcore_event_listeners'][$ename][$key]); } return true; }

Given we used the same approach this is a good solution by definition, right? :)

A good pattern to have in inventory. Thanks.


--
TiG
michael
@michael
7 years ago
7,746 posts
my first idea was to put that unregister code directly in the _init() function, but the issue I ran into was that some modules were firing their _init() function AFTER mine, so I couldn't unset them because they were't set yet. so I moved it to the next firing listener after init which was 'parse_url'.
TiG
TiG
@tig
7 years ago
184 posts
@michael

Makes sense. The module I used was set at a priority lower than jrComment for historical reasons so I figured I would be okay in terms of the registration order.


--
TiG
michael
@michael
7 years ago
7,746 posts
I tried fiddling with the priority too, but for whatever reason when my _init() was firing the jrComment_db_search_items_listener hadn't been set yet.

Just something to remember if any future ones dont fire.
brian
@brian
7 years ago
10,148 posts
Just to add - yes - you can do this in the module init function if the module priority is lower than the module's event listener you want to unset (like you guys point out). We do this in the Media URL Scanner module.


--
Brian Johnson
Founder and Lead Developer - Jamroom
https://www.jamroom.net
TiG
TiG
@tig
7 years ago
184 posts
@brian

Great to know that! It is one thing to have a mechanism to effect this kind of a change, but it is another to have one that is recognized and supported by JR architecture going forward. Thanks for increasing my comfort because this is core to an important new feature for NT.


--
TiG
michael
@michael
7 years ago
7,746 posts
fire it from 'parse_url' event and you wont have to worry about priority.

Tags