« User configurable CC email accounts
New revs - Public comments
New revs »

Online Nao

  • Wedgeward
  • Posts: 10,275
  • Reality check: I just discovered that Wedge already has a 'Mark unread' button for PMs, a feature in my to-do. Uh. It just isn't in the 'right' place...
    • View profile
    • Cynacittà @ noisen
New revs - Public comments
« on July 27th, 2011, 12:20 PM »
Feel free to discuss anything interesting you see about the changelog updates!
  I say Wedge Wedge (in the butt)
« Everyone knows rock attained perfection in 1974. It's a scientific fact. » (Homer Simpson)

Online Arantor

  • Wedgeward
  • Posts: 10,364
  • The Twelve Tasks of Asterix is awesome.
    • View profile
    • Innovate, not Imitate
Re: New revs - Public comments
« Reply #1 on July 28th, 2011, 09:17 AM »
Quote
* encodeURIComponent() has been supported as far back as IE 5.5, so there's no reason to use SMF's way of emulating it
Do they behave differently? Specifically, the php_* functions in JS are not necessarily exact analogues of existing or later-defined JS structures, they are implemented specifically to replicate how PHP does things especially where character sets are concerned (since .toLowerCase() can behave rather differently to strtolower(), as a typical example)

Quote
* php_strtr uses charAt when it could use [] instead. It's a bit faster, which is good in long loops. (script.js)
Interesting, didn't realise JS supported that structure, will remember that for later.

Quote
I'm letting them be for now because they're testing for %u in the URL, which only happens in escape()'d strings
Is that all they're actually doing? That's one of the slightly maddening things about the codebase - stuff like that has been in there for years and it is a little tricky at times prising out the specific reason for it.
   computer-aided
   community building		             

Other stuff:

Online Nao

  • Wedgeward
  • Posts: 10,275
  • Reality check: I just discovered that Wedge already has a 'Mark unread' button for PMs, a feature in my to-do. Uh. It just isn't in the 'right' place...
    • View profile
    • Cynacittà @ noisen
Re: New revs - Public comments
« Reply #2 on July 28th, 2011, 06:39 PM »
Quote from Arantor on July 28th, 2011, 09:17 AM
Quote
* encodeURIComponent() has been supported as far back as IE 5.5, so there's no reason to use SMF's way of emulating it
Do they behave differently? Specifically, the php_* functions in JS are not necessarily exact analogues of existing or later-defined JS structures, they are implemented specifically to replicate how PHP does things especially where character sets are concerned (since .toLowerCase() can behave rather differently to strtolower(), as a typical example)
Come to think of it, I never bothered to look up the differences between toLowerCase and php_strtolower. I suppose the latter is UTF-aware or something.
Do you remember any reason for using php_strtolower?

As for php_urlencode, yes, encodeURIComponent gives results similar to PHP's urlencode(). I tried a silly combination of random chars and both gave me the same results, so I assume I could use it freely. (Which is why I committed my change without further research. If it's buggy, we'll fix. But I don't know how it will.)
From what you probably saw in my commit, SMF's implementation of the whole thing was kinda bastardized. They used different methods for encoding strings, and added useless crap here and there.

Quote
* php_strtr uses charAt when it could use [] instead. It's a bit faster, which is good in long loops. (script.js)
Interesting, didn't realise JS supported that structure, will remember that for later.[/quote]
Yeah, again as pointed out in the changelog-- it's only implemented in IE8+ and all other browsers. IE6/IE7 are a small share, but big enough to give up on that one. It's not like it's a big performance hog, since it's only used when encrypting passwords (sha1.js).

Quote
Quote
I'm letting them be for now because they're testing for %u in the URL, which only happens in escape()'d strings
Is that all they're actually doing?
Absolutely...

Quote
That's one of the slightly maddening things about the codebase - stuff like that has been in there for years and it is a little tricky at times prising out the specific reason for it.
Yeah, that's the problem when many developers work on the same project over the years. Each developer has their own quirks.
  I say Wedge Wedge (in the butt)
« Everyone knows rock attained perfection in 1974. It's a scientific fact. » (Homer Simpson)

Online Arantor

  • Wedgeward
  • Posts: 10,364
  • The Twelve Tasks of Asterix is awesome.
    • View profile
    • Innovate, not Imitate
Re: New revs - Public comments
« Reply #3 on July 29th, 2011, 01:27 AM »
The reason for using strtolower done like that is for accuracy. JavaScript's one is way more aware of things like UTF-8 than PHP's has been for a long time, and I suspect with PHP 5.4 this will have to be revisited anyway.

But ultimately the two have to be done exactly the same for password hashing to actually work, so JS has to then proceed to use the exact same case adjuster even if the browser could do a "better" job of it.

I suspect the different methods were used because browsers of the day couldn't be relied upon to be consistent. Heck, there's whole bushels of pre IE6 code even in 2.0 (like the popup crap for quoting posts)
Posted: July 28th, 2011, 07:10 PM

Quote
! If a timezone has no slash in it (such as 'UTC'), the timezone retrieval code will generate a warning. (ManageSettings.php)
I had actually fixed that locally, though I've not committed what I have because I'm not happy with it yet, concerning how users select timezones. Still not sure about that, maybe I should just commit what I have (which is to use a timezone column in the user's table and fallback to the offset for the time being)
   computer-aided
   community building		             

Other stuff:

Offline live627

  • Consultants
  • Posts: 1,014
  • Took me forever to restyle a menu. And now, it loooks wonderful.
    • View profile
Re: New revs - Public comments
« Reply #4 on July 30th, 2011, 02:44 AM »
Quote
I meant '30 lines instead of 40'. Changelog error, sorry.
You can go back and change it. The pre-revprop-change hook is present[1].
 1. Not all repos have that, though.

Online Nao

  • Wedgeward
  • Posts: 10,275
  • Reality check: I just discovered that Wedge already has a 'Mark unread' button for PMs, a feature in my to-do. Uh. It just isn't in the 'right' place...
    • View profile
    • Cynacittà @ noisen
Re: New revs - Public comments
« Reply #5 on August 1st, 2011, 10:05 AM »
Quote from Arantor on July 29th, 2011, 01:27 AM
The reason for using strtolower done like that is for accuracy. JavaScript's one is way more aware of things like UTF-8 than PHP's has been for a long time, and I suspect with PHP 5.4 this will have to be revisited anyway.

But ultimately the two have to be done exactly the same for password hashing to actually work, so JS has to then proceed to use the exact same case adjuster even if the browser could do a "better" job of it.
Yes, I suppose so.
Which is why it's only used in sha1.js... :)

Quote
I had actually fixed that locally, though I've not committed what I have because I'm not happy with it yet, concerning how users select timezones. Still not sure about that, maybe I should just commit what I have (which is to use a timezone column in the user's table and fallback to the offset for the time being)
Is my fix okay for you? Feel free to rewrite it... I only fixed the error, really.

Another question.
Your number convert function -- isn't he comma_format stuff going to kill performance if used on a larger scale...?
Do you plan to convert more strings into using that? Or should I do it? :P
  I say Wedge Wedge (in the butt)
« Everyone knows rock attained perfection in 1974. It's a scientific fact. » (Homer Simpson)

Online Arantor

  • Wedgeward
  • Posts: 10,364
  • The Twelve Tasks of Asterix is awesome.
    • View profile
    • Innovate, not Imitate
Re: New revs - Public comments
« Reply #6 on August 1st, 2011, 10:12 AM »
Quote
Is my fix okay for you? Feel free to rewrite it... I only fixed the error, really.
I haven't looked (been way too busy with stuff) but I suspect it's similar to the one I have on my local copy, which also has the ability to load the timezone from the members table as well.

Quote
Your number convert function -- isn't he comma_format stuff going to kill performance if used on a larger scale...?
Not really, no, because it's not actually used all that often in the scheme of things.

The biggest culprit is the board index but that ran comma_format anyway, so it's one function wrapper and an if+isset overhead on top per value per board on the board/message index (1 per redirect board, 2 per normal board), it'll be another 2 call+if+isset on the message index for the counts of views/replies, plus another for the view count inside a topic. The actual add-on load is really not worth being too concerned about, IMO.
   computer-aided
   community building		             

Other stuff:

Online Nao

  • Wedgeward
  • Posts: 10,275
  • Reality check: I just discovered that Wedge already has a 'Mark unread' button for PMs, a feature in my to-do. Uh. It just isn't in the 'right' place...
    • View profile
    • Cynacittà @ noisen
Re: New revs - Public comments
« Reply #7 on August 1st, 2011, 10:22 AM »
Quote from Arantor on August 1st, 2011, 10:12 AM
Quote
Is my fix okay for you? Feel free to rewrite it... I only fixed the error, really.
I haven't looked (been way too busy with stuff) but I suspect it's similar to the one I have on my local copy, which also has the ability to load the timezone from the members table as well.
Again, I didn't touch the timezone code. I only fixed the loop that retrieves them, to ensure it won't generate a warning on the list (,) code (since UTC doesn't have a comma and thus throws out an error when splitting it.)

Quote
Quote
Your number convert function -- isn't he comma_format stuff going to kill performance if used on a larger scale...?
Not really, no, because it's not actually used all that often in the scheme of things.
I suppose we could have used it more, though ;)
Posted: August 1st, 2011, 10:21 AM

So, who's going to convert the rest of the plural thingies?
  I say Wedge Wedge (in the butt)
« Everyone knows rock attained perfection in 1974. It's a scientific fact. » (Homer Simpson)

Online Arantor

  • Wedgeward
  • Posts: 10,364
  • The Twelve Tasks of Asterix is awesome.
    • View profile
    • Innovate, not Imitate
Re: New revs - Public comments
« Reply #8 on August 1st, 2011, 11:06 AM »
Quote
Again, I didn't touch the timezone code. I only fixed the loop that retrieves them, to ensure it won't generate a warning on the list (,) code (since UTC doesn't have a comma and thus throws out an error when splitting it.)
Yeah, that's what I meant; I've fixed the loop myself separately, but I'll fix it when I commit what I do have.

Quote
I suppose we could have used it more, though
Yes, it can be used more, because it's not heavy in itself, it's only heavy if comma_format wasn't being used already, but the places that occurs are not common.

Quote
So, who's going to convert the rest of the plural thingies?
That one's on my todo list.
   computer-aided
   community building		             

Other stuff:

Online Nao

  • Wedgeward
  • Posts: 10,275
  • Reality check: I just discovered that Wedge already has a 'Mark unread' button for PMs, a feature in my to-do. Uh. It just isn't in the 'right' place...
    • View profile
    • Cynacittà @ noisen
Re: New revs - Public comments
« Reply #9 on August 1st, 2011, 02:28 PM »
Quote from Arantor on August 1st, 2011, 11:06 AM
Yeah, that's what I meant; I've fixed the loop myself separately, but I'll fix it when I commit what I do have.
Okay. Don't forget to merge before, to avoid conflicts.

Quote
Yes, it can be used more, because it's not heavy in itself, it's only heavy if comma_format wasn't being used already, but the places that occurs are not common.
It's really not heavy in the end. I did some benchmarking and it's barely twice slower than calling strpos(), for instance. Well, after all it's just some hardware string manipulation if I may say. And usually on short strings... Still, I optimized it a bit more in rev 900.

Quote
Quote
So, who's going to convert the rest of the plural thingies?
That one's on my todo list.
I've dealt with the existing _n plurals in rev 900, too. And added one that was in the index language file. Should be a good start ;)

Note to self: merge aeva_utf2entities() into westr::cut().
  I say Wedge Wedge (in the butt)
« Everyone knows rock attained perfection in 1974. It's a scientific fact. » (Homer Simpson)

Online Arantor

  • Wedgeward
  • Posts: 10,364
  • The Twelve Tasks of Asterix is awesome.
    • View profile
    • Innovate, not Imitate
Re: New revs - Public comments
« Reply #10 on August 1st, 2011, 02:32 PM »
Quote
I've dealt with the existing _n plurals in rev 900, too. And added one that was in the index language file. Should be a good start
Looks good to me :) The other stuff that does need doing really is the rest of the message index and the display - these are the user visible stuff, the rest of the things that need doing aren't really user visible but admin stuff primarily.
   computer-aided
   community building		             

Other stuff:

Online Nao

  • Wedgeward
  • Posts: 10,275
  • Reality check: I just discovered that Wedge already has a 'Mark unread' button for PMs, a feature in my to-do. Uh. It just isn't in the 'right' place...
    • View profile
    • Cynacittà @ noisen
Re: New revs - Public comments
« Reply #11 on August 1st, 2011, 04:17 PM »
Yeah, that's what I'm thinking too. The admin area doesn't need as much attention as the front end, at least when it comes to these details.

Small remark on rev 901. ManageSettings.php got screwed up during your merge operation. You cancelled my changes in the timezone retrieval loop, but didn't cancel the line just before it that did an array_flip(). As a result, it's broken. I've restored my copy (which uses isset() instead of in_array() and should thus be faster), I'll commit it again when I have something else to commit (other than minor details in the ManageMedia template.)
  I say Wedge Wedge (in the butt)
« Everyone knows rock attained perfection in 1974. It's a scientific fact. » (Homer Simpson)

Online Arantor

  • Wedgeward
  • Posts: 10,364
  • The Twelve Tasks of Asterix is awesome.
    • View profile
    • Innovate, not Imitate
Re: New revs - Public comments
« Reply #12 on August 1st, 2011, 04:28 PM »
Bah, I missed the array_flip, thinking you were just doing the isset check on it at the end and that it wouldn't work as expected then...
   computer-aided
   community building		             

Other stuff:

Online Nao

  • Wedgeward
  • Posts: 10,275
  • Reality check: I just discovered that Wedge already has a 'Mark unread' button for PMs, a feature in my to-do. Uh. It just isn't in the 'right' place...
    • View profile
    • Cynacittà @ noisen
Re: New revs - Public comments
« Reply #13 on August 1st, 2011, 04:31 PM »
Quote from Arantor on August 1st, 2011, 04:28 PM
Bah, I missed the array_flip, thinking you were just doing the isset check on it at the end and that it wouldn't work as expected then...
So it was intentional? Why would you replace an isset with an in_array, generally speaking..?
  I say Wedge Wedge (in the butt)
« Everyone knows rock attained perfection in 1974. It's a scientific fact. » (Homer Simpson)

Online Arantor

  • Wedgeward
  • Posts: 10,364
  • The Twelve Tasks of Asterix is awesome.
    • View profile
    • Innovate, not Imitate
Re: New revs - Public comments
« Reply #14 on August 1st, 2011, 04:34 PM »
No... the original code had an in_array to select only the regions of interest, and IIRC there was an isset for something else at the end before actually adding it to the loop but off hand I can't remember what.

Right now am dealing with the fact that the imperative scheduled system is completely broken (and in fact I'm dubious about how it ever worked when I tested it originally because it shouldn't have done), will commit that when I've fixed it and finished the feature that actually uses it.
   computer-aided
   community building		             

Other stuff:
« User configurable CC email accounts
New revs »