« Guess the Feature
Rewriting the skin file parser...
facepalm or headdesk smiley »

Nao

  • Dadman with a boy
  • Posts: 16,082
Re : Rewriting the skin file parser...
« Reply #16, on May 20th, 2012, 06:07 PM »
Tested here...
file_exists method: 0.15 to 0.35ms, average: 0.2ms.
glob method: 0.4 to 0.7ms, average: 0.5ms

It's still twice slower, but still under a millisecond...
What really scared me, though, is that right after I uploaded my file and refreshed, it sent me a benchmark 1.65 SECOND for that code block...! Then I refreshed, and it went under a millisecond.

I don't know what it means, but to me it means only one thing -- glob() is indeed cached by Apache across all page requests :P
Posted: May 20th, 2012, 06:03 PM

Hmm... The current version here is the file_exists one, and I *just* had a 1.15 second benchmark time for it. So I guess it's not limited to glob()... It's just that glob() is a bit slower in every task.
Re : Rewriting the skin file parser...
« Reply #17, on May 20th, 2012, 07:05 PM »
Okay, I'm having a logic problem here...

Let's say I have this main skin:

/skins/index.css
/skins/editor.css
/skins/editor.firefox.css

And a replace-type skin:

/skins/Foobar/index.css

Now, let's say that I don't have an editor.css file in the sub-skin, because I want to re-use the default one.
This was possible (and supported) in Wedge up until now.
Now, with the glob() rewrite, I have to make some adjustments, but I'm not even SURE I can remember how I dealt with it...
Basically: should I use the top-level editor.css? Yes. Should I use the top-level editor.firefox.css if I'm on Firefox? I have no idea... How do I determine that it's a generic fix file and not something related to the default skin...? What if I have an editor.css in my sub-skin, but not editor.firefox.css --- should I use the top-level's editor.firefox.css file? It's a lot of fun...

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re : Rewriting the skin file parser...
« Reply #18, on May 20th, 2012, 07:08 PM »
It should use the top-level's file. The child editor.css should only replace editor.css from the parent. That way if someone wants to replace both, they should state both (and likely will need both anyway)
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

Nao

  • Dadman with a boy
  • Posts: 16,082
Re : Rewriting the skin file parser...
« Reply #19, on May 20th, 2012, 07:11 PM »
"State both"?
Posted: May 20th, 2012, 07:09 PM

But if my sub-skin's editor.css has no need for Firefox hacks, why should I add an empty editor.firefox.css file then...?
Posted: May 20th, 2012, 07:10 PM

As a reminder, I'm talking about a replace-type skin, not an inherit-type (add-type) skin.
Which means that the only reason we should look into the default skin is if we can't find a *main* file such as editor.css or index.css...
But then it gets funny when you specify an editor.firefox.css file in your sub-skin, but no editor.css file... Probably meaning you want to include the top-level's editor.css, but modify it for Firefox in your sub-skin.
Lots of fun :)

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re : Rewriting the skin file parser...
« Reply #20, on May 20th, 2012, 07:18 PM »
Oh, if you're talking about replace, then editor.firefox.css should not be included. The idea then is that the child should replace the parent unless the parent doesn't have it.

If the child has no editor.css at all, then go to the parent (and use the parent's editor.firefox.css if appropriate). But if the child has editor.css and no editor.firefox.css, load the child's editor.css and ignore the parent's editor.firefox.css.

Nao

  • Dadman with a boy
  • Posts: 16,082
Re : Rewriting the skin file parser...
« Reply #21, on May 20th, 2012, 07:41 PM »
Agreed.
But:

/editor.css
/Foobar/editor.firefox.css

If only these files are there -- I can see the intent as "use the top-level editor.css, and modify it with my custom firefox file..."

Hmm, the change to glob() unfortunately makes it a tad harder to sort these files correctly.

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re : Rewriting the skin file parser...
« Reply #22, on May 20th, 2012, 07:57 PM »
Well... replace skins implies replacing everything, no?

Whatever the behaviour is, as long as it's documented and allows people to replace things as necessary, I don't really see it as a big problem.

Nao

  • Dadman with a boy
  • Posts: 16,082
Re : Rewriting the skin file parser...
« Reply #23, on May 20th, 2012, 07:59 PM »
I really only want for this all to just... make sense ;)
Intuition is what you rely on when you're too lazy to read a documentation. So everything I do, I try to make intuitive :P
Re : Rewriting the skin file parser...
« Reply #24, on May 20th, 2012, 11:44 PM »
Just wanted to say — going to bed with a half done function. CSS may fail here and there.
Heck. The new function adds something like 60 lines.... Adding support for comma separated suffixes is definitely more complicated than I originally thought ;)
Re : Rewriting the skin file parser...
« Reply #25, on May 21st, 2012, 09:16 AM »
Quote from Nao on May 20th, 2012, 11:44 PM
Just wanted to say — going to bed with a half done function. CSS may fail here and there.
Wasn't failed.... Just forgot to empty the cache ;)
Posted: May 21st, 2012, 09:13 AM
Quote from Nao on May 20th, 2012, 07:41 PM
Agreed.
But:

/editor.css
/Foobar/editor.firefox.css

If only these files are there -- I can see the intent as "use the top-level editor.css, and modify it with my custom firefox file..."
Haven't found a solution for this, btw...

Also:
/editor.css
/editor.firefox.css
/Foobar/(nothing)

Should firefox* be loaded in Foobar...?

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re : Rewriting the skin file parser...
« Reply #26, on May 21st, 2012, 03:36 PM »
Foobar doesn't provide editor.css so the root editor.css will be required, and I would assume you'd have to load editor.firefox.css for that too, regardless of replace or inherit.

Nao

  • Dadman with a boy
  • Posts: 16,082
Re : Rewriting the skin file parser...
« Reply #27, on May 21st, 2012, 11:31 PM »
I have this terrible feeling that I'm going to have to add even more lines of code...
I usually make my rewrites shorter than any earlier code. It would be a first ;)
Re : Rewriting the skin file parser...
« Reply #28, on May 22nd, 2012, 12:15 PM »
Hmm... Tried replacing the glob() call with scandir() and opendir(), hoping to get better performance...
Unfortunately, it's not any faster, maybe even a bit slower, although at least the performance is more stable across multiple refreshes.
I'll probably switch to scandir() because at the very least it's supported everywhere...
Re : Rewriting the skin file parser...
« Reply #29, on May 22nd, 2012, 04:38 PM »
Phew... Finally done rewriting the logic for replace-type skins.

I added a dummy skin with just two files (index.member.css and sections.css), and here's how it was included:

Array
(
   
  • => /Themes/default/skins/index.css
[1] => /Themes/default/skins/index.opera.css
    [2] => /Themes/default/skins/index.member.css
    [3] => /Themes/default/skins/Wutherings/index.member.css
    [4] => /Themes/default/skins/Wutherings/sections.css
    [5] => /Themes/default/skins/custom.css
    [6] => /Themes/default/skins/custom.admin.css
)

The absolute perfect order.
Meaning you can easily manipulate ordering just by removing files in your replace skin or adding empty files, or choosing adequate suffixes.

Meanwhile, I've switched to makinge use of scandir(), and caching the results in a static array, although I'm disappointed by the lack of performance improvements with the caching... (Well, it probably works better on external files like editor.css, so I won't be removing it.)
« Guess the Feature
facepalm or headdesk smiley »