New revs - Public comments

Nao

  • Dadman with a boy
  • Posts: 16,082
Re: New revs - Public comments
« Reply #240, on October 18th, 2011, 09:13 AM »
Quote from live627 on October 18th, 2011, 12:54 AM
That looks familiar... :whistle:
I suspect that means something you offered before, sorry I can't remember.
I just looked at the huge query, remembered the hassle it was to plug some of AeMe's code into it (one of the biggest incompatibility generators out there), and wanted it gone, like, now.

Also, btw, I'm not sure wesqlQuery allows passing extra parameters not related to the query itself...?

Like, new wesqlQuery('hook_name', (query stuff), array($set, &$users, &$is_name))...?

TE

  • Posts: 286
Re: New revs - Public comments
« Reply #241, on October 18th, 2011, 09:15 AM »
Quote from Arantor on October 18th, 2011, 08:56 AM
The former is one place where I can see the logic, though surely all that's needed in that case is to turn over authentication over to the separate backend and if authenticated through that method, silently turn on 'disable admin security' (but only for the life of that page)?
yep, that's a perfect solution regarding admin security but there would still be an issue with the profile account data (can't be updated because I don't know my password). Maybe I can set that field as a hidden form field with a pre-filled password or something similar.. I currently have no clue to work around that issue with a proper solution..  Any ideas?
Thorsten "TE" Eurich - Former SMF Developer & Converters Guru

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re: New revs - Public comments
« Reply #242, on October 18th, 2011, 09:25 AM »
Quote
Well, if you and Dragooon are into it, then you should do it... I don't have the perfect answer to everything
It's on my todo list :P
Quote
Or, as I said: select m.id_member, g.online_color from members as m, membergroups as g where m.id_group = g.id_group
Something like that... (?)
Yes, but it means doing that treatment everywhere where names are pulled directly. The alternatives are converting everything to a case of loadMemberData (minimal) or doing it in the post-replace phase, either way that's an extra query.
Quote
Then get the entire list of people, push it into the cache for a few minutes, and use that cache internally to associate people with a color. It might represent a large cache file but if we ensure the option can be disabled, it's okay I guess... (?)
Hmm. I'm thinking that we could do it better by querying and caching the member group colours on a longer term basis, and fetching user+group instead, as querying every user with their group is going to be quite expensive.
Quote
Why? I can't think of an example where adding a class to an anchor would be detrimental to XML functionality...?
It depends how fussy the receiver is. A receiver expecting Atom and gets not-strictly-valid Atom might throw a wobbly. Besides, in XML modes, there's no point running that operation, might as well save a few CPU cycles...
Quote
Hmm yeah, I suppose... Although it's unlikely to happen.
But we could look through the entire anchor tag and reject it if it has bbc_ in it or something...
Works for me. Just something I wanted to be careful of, really.
Quote
Definitely cleaner. But easier to understand? I don't know, I've never been much into 'nested' output buffers, they never made much of a sense to me, I'd rather have it all in the same place...
It works like any other nesting, and for the most part it's pretty transparent. The buffer's passed in with each successive nesting, callback run when the buffer's done, then it's passed back up the nesting.
Quote
Well, we could simply turn these into sub-functions and call them all from ob_sessrewrite...
That would work and would probably be faster than using true buffers to pass back and forth.
Quote
Or 'custom', in my mind.
If you're sending out a feed of new members, there's really only so much you can put into it before you break Atom format, no?
Quote
Definitely a 'custom' candidate then...
Memberlist is definitely a custom candidate. We need to decide what columns are actually needed.
Quote
It only uses lMD when you're setting PM options in your profile...
That seems... illogical somehow.
Quote
I'll let you check the files independently... Yeah IIRC it's to find usernames for recipients that didn't go through the auto-suggest.
Ahhhh, loadMemberData with second parameter as true, that makes sense now.
Quote
I think that's the only place you get it apart from the info center... A real bummer.
Yup, it either should be throughout or not at all.
Quote
It's technically valid only in Linux. It doesn't work in Windows and Mac OS.
That'll be why I didn't think of it.
Quote
You know, you COULD revert this and no one would blame you for that...
I could but it would feel wrong. The whole solution seems hacky and I don't like it but I just don't know a better one.
Quote
I suspect that means something you offered before, sorry I can't remember.
I just looked at the huge query, remembered the hassle it was to plug some of AeMe's code into it (one of the biggest incompatibility generators out there), and wanted it gone, like, now.

Also, btw, I'm not sure wesqlQuery allows passing extra parameters not related to the query itself...?

Like, new wesqlQuery('hook_name', (query stuff), array($set, &$users, &$is_name))...?
There was a discussion on sm.org where someone offered a patch that did almost the exact same thing.

Also, why would you want to pass in parameters not related to the query itself, exactly?
Quote
yep, that's a perfect solution regarding admin security but there would still be an issue with the profile account data (can't be updated because I don't know my password). Maybe I can set that field as a hidden form field with a pre-filled password or something similar.. I currently have no clue to work around that issue with a proper solution..  Any ideas?
There was some stuff in the profile code relating to this for OpenID purposes, actually. There's a parameter 'password' that's needed to be set if password is going to be required for changing, IIRC, which was set to false if OpenID was used. But OpenID wasn't capable of taking any case where validatePassword was called (which meant you couldn't go into the admin panel, or change someone else's profile, though you could change your own because of the above)

It wouldn't be too bad to provide somewhere that that value can be reset through a hook, and optionally a hook in the validatePassword subsystem to allow authentication plugins to safely handle admin-type security checks.
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
Re: New revs - Public comments
« Reply #243, on October 18th, 2011, 09:37 AM »
Quote from Nao on October 18th, 2011, 09:13 AM
I suspect that means something you offered before, sorry I can't remember..
He provided a very similar patch on SM.org, plus it's very similar to how I pass parameters to wesqlQuery.
Quote from Nao on October 18th, 2011, 09:13 AM
Also, btw, I'm not sure wesqlQuery allows passing extra parameters not related to the query itself...?

Like, new wesqlQuery('hook_name', (query stuff), array($set, &$users, &$is_name))...?
It does via the fetch function, the parameters are passed to result callbacks.
Quote
Also, why would you want to pass in parameters not related to the query itself, exactly?
They can be required for processing the result.
The way it's meant to be

Nao

  • Dadman with a boy
  • Posts: 16,082
Re: New revs - Public comments
« Reply #244, on October 18th, 2011, 03:04 PM »
Quote from Arantor on October 18th, 2011, 09:25 AM
It's on my todo list :P
Then should I revert my changes...? Just to be sure. What about passing params? (I suspect you discuss it below...)
Quote
Yes, but it means doing that treatment everywhere where names are pulled directly. The alternatives are converting everything to a case of loadMemberData (minimal) or doing it in the post-replace phase, either way that's an extra query.
And lMD is going to be slower...
I have to admit I'm a sucker for dynamic replacements. Even on very text-heavy pages, it's still always super fast, and as long as it's done correctly... Well, there's nothing better :P
Right now, my color injection code, as slow and unoptimized as it is (preg_replace_callback...), only takes a millisecond to execute, and that includes loading the cached data. And with recaching, it takes 3ms, but OTOH I only have one custom group on my test site (admin), so it's bound to be fast overall...
Quote
Hmm. I'm thinking that we could do it better by querying and caching the member group colours on a longer term basis, and fetching user+group instead, as querying every user with their group is going to be quite expensive.
Can you elaborate on this...?
I've rewritten my code to store members+groups followed with a list of colors per group, to avoid repetition and thus generate a smaller cache file for bigger arrays. But I'm not sure what you're suggesting...?
The result is something like this: (let's say users 1, 2 and 5 are admins and user 3 is from group 7)
$member_colors = array(
  'group' => array(
    1 => 1,
    2 => 1,
    3 => 7,
    5 => 1,
  ),
  'color' => array(
    1 => '#FF0000',
    7 => '#FFCC99',
  ),
);

So, basically, we retrieve the color with $member_colors['color'][$member_colors['group'][$USER_ID]].

Of course, I'm also testing for 'bbc_link' inside the match, and it works well.
Quote
It depends how fussy the receiver is. A receiver expecting Atom and gets not-strictly-valid Atom might throw a wobbly. Besides, in XML modes, there's no point running that operation, might as well save a few CPU cycles...
I'm not sure... For instance, I quick-edited a post and the result had the proper group color, which was fine (and wouldn't have happened if XML didn't run it.) Obviously I've disabled it now, but basically I don't really see a reason to prevent any XML that shows a profile link, to actually show it in the proper color...
Quote
Works for me. Just something I wanted to be careful of, really.
I'm not entirely convinced it's the best way though. I mean, to me as a user, it would make sense to have my link show in red if I'm linking inside my post to an admin's profile... I don't know.
Of course, if I'm using PURLs, the link will show with the regular color because then Wedge has no way to (easily) figure out the target's group color...
Quote
It works like any other nesting, and for the most part it's pretty transparent.
Sorry, I don't mean that I don't understand them, I just mean that I don't get the point. To me, output buffers are nice if you're trying to 'catch' data output by something else, etc, but if you know the exact execution order, it's best to just put everything into the same OB...
For instance, link replacements have to happen after <URL> is transformed, but before pretty URLs are applied. I was thinking of using my replacement code right inside the purl filter file, to be able to both transform profile URLs and add a color at the same time, but... Err, one millisecond...? Who cares. Might as well make the code clearer and put everything in the same place.
Quote
Quote
Well, we could simply turn these into sub-functions and call them all from ob_sessrewrite...
That would work and would probably be faster than using true buffers to pass back and forth.
Definitely... But I'm not in a hurry to do that either, though :lol:
Quote
Quote
Or 'custom', in my mind.
If you're sending out a feed of new members, there's really only so much you can put into it before you break Atom format, no?
What do you mean?
Quote
Memberlist is definitely a custom candidate. We need to decide what columns are actually needed.
Sure.......................... Who's up for it? :P
Quote
Quote
I think that's the only place you get it apart from the info center... A real bummer.
Yup, it either should be throughout or not at all.
(With the technicality of big user names (e.g. userbox authors) being potentially a problem. I'm thinking of fixing it by changing the default colors to pastel ones, like the group colors on wedge.org... #d2653a is definitely nicer than red for admins!)
Quote
Quote
You know, you COULD revert this and no one would blame you for that...
I could but it would feel wrong. The whole solution seems hacky and I don't like it but I just don't know a better one.
Looking at the SMF2 code, it only checks for %p (lowercase), which generates uppercase AM and PM, and manually transforms them to time_am and time_pm, but they're not dealing with %P... Just so you know-- if you add a %P on a server that doesn't support it, it'll return an empty string... Not an empty am/pm; but an entirely empty string. (At least under Windows.) I don't think 'wrong' values are being filtered out if the user enters a custom time format...

(Heck, if it wasn't for the custom time format thing, I wouldn't have had to do the whole preg_match stuff to find the year.)
Quote
Also, why would you want to pass in parameters not related to the query itself, exactly?
<evil grin> "I don't know, but it's going to be a lot of fun finding out!" (Lister, epilogue, Red Dwarf 2x06)
Okay, forget it, once the query is converted, plugins will still have access to $users from within the query anyway... ;)

Oh... I just found out that my replacement code was also changing the color for links inside the profile area (showposts, etc.), ah ah... Well, easy to fix (just match against URLs with just a profile ID and nothing else...)
I could make the fix 'perfect' by storing user names in addition to groups in the cache. Then I would simply need to determine if the visible link matches the real_name, but... That would make for a bigger cache. I don't like the idea too much.
Quote
It wouldn't be too bad to provide somewhere that that value can be reset through a hook, and optionally a hook in the validatePassword subsystem to allow authentication plugins to safely handle admin-type security checks.
I'm definitely leaving that one up to you ;)

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re: New revs - Public comments
« Reply #245, on October 18th, 2011, 03:19 PM »
Quote
Then should I revert my changes...? Just to be sure. What about passing params? (I suspect you discuss it below...)
The discussion on wesqlQuery showed examples of doing that, no?
Quote
Can you elaborate on this...?
I've rewritten my code to store members+groups followed with a list of colors per group, to avoid repetition and thus generate a smaller cache file for bigger arrays. But I'm not sure what you're suggesting...?
I figured we might as well cache the membergroups and their colours generally (including post count groups), and in places we already do a join to the members table to get the member's name, add the id_group and id_post_group parameters from there into the query.

The idea is to save either the extra join or the extra query, since if we're already doing the join to members, there's no need to do any further joining or querying if we already have the list of membergroups and their colours available to us.
Quote
Sorry, I don't mean that I don't understand them, I just mean that I don't get the point. To me, output buffers are nice if you're trying to 'catch' data output by something else, etc, but if you know the exact execution order, it's best to just put everything into the same OB...
For instance, link replacements have to happen after <URL> is transformed, but before pretty URLs are applied. I was thinking of using my replacement code right inside the purl filter file, to be able to both transform profile URLs and add a color at the same time, but... Err, one millisecond...? Who cares. Might as well make the code clearer and put everything in the same place.
The argument I'm suggesting - but not that heavily - is encapsulating functionality. Right now the post-replace phase is pretty complex, and is made up of a series of stages. Wouldn't it be better to make each stage a separate function, so that if ever you need to alter behaviour, firstly it's localised into a single function, and secondly if you need to rearrange them, you're rearranging a few lines rather than bulk copy/paste?
Quote
Sure.......................... Who's up for it?
I kind of like the columns on here/Noisen actually, it's an improvement over the default. I also hear that what Antechinus made for the 'dynamic memberlist' isn't too bad either, might be worth looking at the visual side to see if it isn't hideous and see whether something vaguely like that wouldn't be a bad idea.[1]
Quote
What do you mean?
Atom is XML. If you add an attribute to that XML, it's still valid XML - but it stops being a valid Atom file. Remember, XML formats aren't just randomly extensible, they have schemas to direct what elements there are and what the valid list of attributes is.

Some XML consumers won't care and will totally ignore that attribute. Some will care a great deal and write the XML feed off as invalid. So yeah, it's kind of important not to just randomly mess with set XML formats unless you're the only one using it and can afford not to be too picky about it come consumption time. For example, the plugin manager doesn't ever expressly validate the plugin-info.xml files against the schema. It *can* but it doesn't. It only validates the content to make sure it makes sense and is understandable, not that it's strictly legal. (I figure we'll do that on plugin upload, you see)
Quote
(With the technicality of big user names (e.g. userbox authors) being potentially a problem. I'm thinking of fixing it by changing the default colors to pastel ones, like the group colors on wedge.org... #d2653a is definitely nicer than red for admins!)
*nods* I don't have a problem with that, I think it would be an improvement.
Quote
Looking at the SMF2 code, it only checks for %p (lowercase), which generates uppercase AM and PM, and manually transforms them to time_am and time_pm, but they're not dealing with %P... Just so you know-- if you add a %P on a server that doesn't support it, it'll return an empty string... Not an empty am/pm; but an entirely empty string. (At least under Windows.) I don't think 'wrong' values are being filtered out if the user enters a custom time format...

(Heck, if it wasn't for the custom time format thing, I wouldn't have had to do the whole preg_match stuff to find the year.)
No, Windows doesn't do any filtering, which is a bit poor really. (It should at least return them as literals.)
Quote
<evil grin> "I don't know, but it's going to be a lot of fun finding out!"
Actually, Dragooon answered that one, and wesqlQuery provides for it.

 1. But I'd code it from scratch anyway, not only to avoid claims of copying but simply because we'd have to do it differently.

Nao

  • Dadman with a boy
  • Posts: 16,082
Re: New revs - Public comments
« Reply #246, on October 18th, 2011, 06:43 PM »
Quote from Arantor on October 18th, 2011, 03:19 PM
I figured we might as well cache the membergroups and their colours generally (including post count groups),
Oh, I forgot about post count groups... I never enable colors for these.

Okay, done... (Half an hour later :P)
I also managed to catch the place where postgroups are updated so I can clear the cache, but I wouldn't mind some help spotting other areas where regular groups are updated.

Also, looked into rev 1118, just a note: there are plenty of multiple spaces (not tabs) in the file, most notably several distinct lines often find themselves on the same line separated by many spaces... Odd!
And the ManagePlugins template -- you committed an empty function, is this as intended?

Oh, and before I forget... There are dozens (hundreds?) of &quot; in the language files. Shouldn't we remove them...? I don't think they serve a point these days?
Quote
and in places we already do a join to the members table to get the member's name, add the id_group and id_post_group parameters from there into the query.
I think it's better (for now) to harmonize the queries, so I started removing online_color when not required by something else...
Well, I'm a bit lost in the amount of changes I made today so I'll just commit what I have now and you can look into it.
Quote
The idea is to save either the extra join or the extra query, since if we're already doing the join to members, there's no need to do any further joining or querying if we already have the list of membergroups and their colours available to us.
That's the idea... I think.
Quote
The argument I'm suggesting - but not that heavily - is encapsulating functionality. Right now the post-replace phase is pretty complex, and is made up of a series of stages. Wouldn't it be better to make each stage a separate function, so that if ever you need to alter behaviour, firstly it's localised into a single function, and secondly if you need to rearrange them, you're rearranging a few lines rather than bulk copy/paste?
I haven't been moving a lot of code honestly, only recently to straighten it up but I don't see myself adding to that function on a daily basis anyway..... :^^;:
Quote
I kind of like the columns on here/Noisen actually, it's an improvement over the default.
I just took out what I didn't care about, and added the stuff I wanted to have (avatars and last login)...
Quote
I also hear that what Antechinus made for the 'dynamic memberlist' isn't too bad either, might be worth looking at the visual side to see if it isn't hideous
Isn't there a 'super memberlist' mod around? The one where members are listed in boxes -avatar, then some info below... I'm not a big fan of the way it's shown, but it's probably a way to do it that's logical to most people.
Quote
Atom is XML. If you add an attribute to that XML, it's still valid XML - but it stops being a valid Atom file. Remember, XML formats aren't just randomly extensible, they have schemas to direct what elements there are and what the valid list of attributes is.
Hmm... I see. Well, now that the code is committed (I wanted to post this before, but I had a browser crash -- thank you for drafts! -- and committed while waiting for my tab list to reload), you can look into it and test the XML maybe...?

And please generally tell me what you think about the overall building of the feature ;)
Quote
Actually, Dragooon answered that one, and wesqlQuery provides for it.
Then all is good... Just promise me that wesqlQuery will be in Wedge before we release an alpha :P

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re: New revs - Public comments
« Reply #247, on October 18th, 2011, 09:18 PM »
Quote from Nao on October 18th, 2011, 06:43 PM
Oh, I forgot about post count groups... I never enable colors for these.
We might not but I know sites that do.
Quote
I also managed to catch the place where postgroups are updated so I can clear the cache, but I wouldn't mind some help spotting other areas where regular groups are updated.
Profile area is the main one, but it's not the only one. Group management (Groups.php) is the other main one for assigned groups. Theory says updateMemberData() should be called.
Quote
Also, looked into rev 1118, just a note: there are plenty of multiple spaces (not tabs) in the file, most notably several distinct lines often find themselves on the same line separated by many spaces... Odd!
Class-SFTP's formatting is... special. I thought I'd fixed the worst offenders.
Quote
And the ManagePlugins template -- you committed an empty function, is this as intended?
Yes, that was intended; I am going to populate that function but after fighting with the Class-FileWritable and Subs-Plugins code I was a bit fed up, so committed it as was, raw and unfinished.
Quote
Oh, and before I forget... There are dozens (hundreds?) of &quot; in the language files. Shouldn't we remove them...? I don't think they serve a point these days?
Be very careful with those. There are cases where mismatching goes on - when I created the Birthdays plugin out of the email templates, I made use of a little known quirk of SMF's language editor.

Specifically, you can actually do this in the language files and expect it to work properly in the language editor for both loading and saving:
Code: [Select]
$txt['string'] = 'line 1' . "\n\n" . 'line 2';

I don't know how ManageLanguages does that, so without testing to understand whether there are interesting side effects or not.
Quote
I think it's better (for now) to harmonize the queries, so I started removing online_color when not required by something else...
Well, I'm a bit lost in the amount of changes I made today so I'll just commit what I have now and you can look into it.
Harmonisation is good :) And yeah, I understand exactly what you mean where lots of changes happen, I had points during the early plugin manager dev where it was doing that to me.
Quote
I haven't been moving a lot of code honestly, only recently to straighten it up but I don't see myself adding to that function on a daily basis anyway..... :^^;:
I'm just thinking of the case of having to come back to it in 6 months time.
Quote
I just took out what I didn't care about, and added the stuff I wanted to have (avatars and last login)...
I like it.
Quote
Isn't there a 'super memberlist' mod around? The one where members are listed in boxes -avatar, then some info below... I'm not a big fan of the way it's shown, but it's probably a way to do it that's logical to most people.
Yes, that's the one.
Quote
Hmm... I see. Well, now that the code is committed (I wanted to post this before, but I had a browser crash -- thank you for drafts! -- and committed while waiting for my tab list to reload), you can look into it and test the XML maybe...?
Well, I'll take a look but we might as well not run that particular process if we're outputting XML, I can't see any case in XML (valid or otherwise) where we'd want the colour to be added.
Quote
And please generally tell me what you think about the overall building of the feature ;)
I like the idea but will look at the code later on.
Quote
Then all is good... Just promise me that wesqlQuery will be in Wedge before we release an alpha :P
Of course, I'm going to need it before long ;)

Nao

  • Dadman with a boy
  • Posts: 16,082
Re: New revs - Public comments
« Reply #248, on October 19th, 2011, 12:46 AM »
I'm looking at Settings.template.php and only one question comes to mind... Why this? I can understand some themes might wanna add options, but... This makes it very hard to actually find them (they're neither accessible from Profile > Look & Layout, not from the regular admin area... You have to go to Admin > Current Theme, which I don't find very intuitive.)
And because they're not in the admin area per se, they're not searchable, even they they DO show up in the admin.
Even things like show_group_key... Why would we need to offer to ability to disable it...? To save space?! Why is it a theme option only? It should be in Admin > Membergroups > Settings...

I've been changing show_group_key btw. Now it will also show postgroups, if they have a custom color. Is this okay with everyone? I couldn't think of any strong reason not to do that.
Quote from Arantor on October 18th, 2011, 09:18 PM
We might not but I know sites that do.
Which is why I added a default color for the highest post group in Wedge... :)
Quote
Profile area is the main one, but it's not the only one. Group management (Groups.php) is the other main one for assigned groups. Theory says updateMemberData() should be called.
Uncaching on updateMemberData() means doing it on many, many user actions... Might as well not cache anything ;)
Quote
Class-SFTP's formatting is... special. I thought I'd fixed the worst offenders.
It only matters if you're not going to update the file in the future.
Quote
Be very careful with those. There are cases where mismatching goes on - when I created the Birthdays plugin out of the email templates, I made use of a little known quirk of SMF's language editor.
Well, we still have a lot of " in the language files, which we used naturally this year AFAIK. So it's pretty much a mixup...
Quote
Harmonisation is good :)
Feel free to delete these on your side, too. For instance it's bed time for me... :P

Oh, there's a bug in the current version btw. It's related to createList() and the creation of the menu... Anyway... Just go to the info center, click one of the membergroups in the group key, and see the errors in the menu. Worked on it for 15 minutes and didn't find anything strange (except for a wetem::load() that can safely go in Groups.php...)
Quote
Well, I'll take a look but we might as well not run that particular process if we're outputting XML, I can't see any case in XML (valid or otherwise) where we'd want the colour to be added.
In that case, hmmm... How do we do that? Just a simple if (isset($_GET['xml'])) will do...?

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re: New revs - Public comments
« Reply #249, on October 19th, 2011, 03:17 AM »
Quote
I'm looking at Settings.template.php and only one question comes to mind... Why this? I can understand some themes might wanna add options, but... This makes it very hard to actually find them (they're neither accessible from Profile > Look & Layout, not from the regular admin area... You have to go to Admin > Current Theme, which I don't find very intuitive.)
It's to make it easier for themes to add options, that are under admin control. There's no way for themes to add user options AFAIK without replacing some/all of the profile template.

Admin > Current Theme makes some modicum of sense, but not a lot - though it's a world better than how unobvious theme settings are to find normally.
Quote
Even things like show_group_key... Why would we need to offer to ability to disable it...? To save space?! Why is it a theme option only? It should be in Admin > Membergroups > Settings...

I've been changing show_group_key btw. Now it will also show postgroups, if they have a custom color. Is this okay with everyone? I couldn't think of any strong reason not to do that.
It's a theme option because not all themes provide it. Seriously tempted to move that back out into being a plugin anyway because partly not all themes provide it and partly because so many people want it 'ordered how they want, not just alphabetic', and I think it would be a stretch too far to make core with that in mind. It's not like it's tough to expand any more or anything.
Quote
Which is why I added a default color for the highest post group in Wedge...
Nice. I spent the rest of my evening trying to score the one remaining achievement I had for Monkey Island II on Steam (complete the game in under 3 hours, just finished with 30 mins to spare), so I haven't looked at the commit yet.
Quote
Uncaching on updateMemberData() means doing it on many, many user actions... Might as well not cache anything
Is there really a lot of places that call updateMemberData?
Quote
It only matters if you're not going to update the file in the future.
I'll update it if it becomes necessary but given how there haven't been updates for a while and it's one of those things that generally 'just works', it should be OK to fix up now.
Quote
Well, we still have a lot of " in the language files, which we used naturally this year AFAIK. So it's pretty much a mixup...
Hmm, we should probably test that. It's likely I'm worrying about nothing but I'd rather that and be proven wrong than to be bitten by it later.
Quote
Oh, there's a bug in the current version btw. It's related to createList() and the creation of the menu... Anyway... Just go to the info center, click one of the membergroups in the group key, and see the errors in the menu. Worked on it for 15 minutes and didn't find anything strange (except for a wetem::load() that can safely go in Groups.php...)
I'll do that in the morning to see what's going on.
Quote
In that case, hmmm... How do we do that? Just a simple if (isset($_GET['xml'])) will do...?
Testing for $_GET['xml'] is good but it doesn't automagically do that for the feeds IIRC. But if we had the feeds set that too (if they don't already), then it's a no-brainer.

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,670
Re: New revs - Public comments
« Reply #250, on October 29th, 2011, 05:48 AM »
ManagePlugins is missing a comma at the end of line 1408 casuing a fatal error.
A confident man keeps quiet.whereas a frightened man keeps talking, hiding his fear.

Nao

  • Dadman with a boy
  • Posts: 16,082
Re: New revs - Public comments
« Reply #251, on October 29th, 2011, 10:52 AM »
I'll look into it. I shall be committing today anyway.

Oh and I need to answer the post above...

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re: New revs - Public comments
« Reply #252, on October 30th, 2011, 02:10 AM »
I fixed the comma issue in r1138.

The template layer issue I referred to (that you asked about in the notes on r1137) stems from what happens in the event that a template layer is empty. The post was in http://wedge.org/pub/off/6980/battlefield-3/msg269815/#msg269815

I don't seem to have a problem if the array is defined with some content up front, or with ::layer, but it gets upset if it's declared in a master wetem::load call as an empty array.


On a related note, I have a case where I'm defining a container layer, which has before and after content, but that there may or may not be content added to that layer, and because that container is designed to be hookable, I see two routes. Either the plugin (of a plugin) tests for layer existence and re-adds it if necessary, or I create the layer by default, let hooks run, then have some kind of is-empty test on the layer, and if so, remove the layer.

I'm not sure whether it's necessary to have the is-empty test because I do have the ability to test and add if necessary but it seems that it might be nicer to be able to check is-empty status to allow things to do cleanup. (I'm not really fussed if it isn't implemented)

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,670
Re: New revs - Public comments
« Reply #253, on October 30th, 2011, 06:03 AM »
Quote
! Provide a hook for altering the list of posts to be retrieved, as suggested by live627. (ManagePlugins.php, Display.php)
What is the time for? If it's what I think it is (plugin-side caching based on time) shouldn't it also include modified time? Whichever is higher? Or am I totally missing the point?

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re: New revs - Public comments
« Reply #254, on October 30th, 2011, 04:22 PM »
I added it because the time is expressly queried when fetching the list of messages (and posters, as a by product)

It's used in relation to the double post merging, I believe.