New revs - Public comments

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re: New revs - Public comments
« Reply #465, on October 8th, 2012, 06:06 PM »
Quote
I don't understand... Why can't we simply change these variables to use arrays instead, and have them go through the code I suggested?
So we go through the few hundred variables, testing for { and unserialize every page load. Sounds nice and efficient. How many rows is that actually applied to? You need to consider around 400-500 rows for a typical install.

But more than that, I guarantee it WILL introduce issues because it's not just the loading code that needs to be fixed, all of the saving code also all needs to be fixed. Which means potentially affecting everywhere in the admin panel that uses the generic saving code, because that won't cause bugs or anything.
Quote
And if you're writing a plugin that accesses a serialized array in the settings, you don't want to have to unserialize it yourself, because when are you supposed to do that..? As a plugin author I'd expect Wedge to do it for me. (But then again that's just me...)
Topic Solved doesn't seem to worry about it too much, seeing how certain types of setting can themselves be arrays, which are already serialised, such as multi-select or boards settings.

Do it if you really want but I'm really not keen on it because I'm not convinced it actually saves anything. The problem isn't loading, it's saving. That, and all the fun code related to editing settings in the admin panel, which was crappy enough to write the first time.
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: New revs - Public comments
« Reply #466, on October 8th, 2012, 06:10 PM »
Another test...
- Disabled caching for $settings
- Added unserialize directly to the $settings[$row[0]] = $row[1] line
- Result is about 40% slower, i.e. normally the array would be filled in about 1 millisecond (as well), and the extra code adds between 0.3 and 0.8 milliseconds, on average 0.4ms.

Also of note: because the entire thing is cached to begin with, *we don't need to unserialize data on every page load*... As it's already unserialized through the cache. It's already the case in the current codebase.

That means, it only adds half a millisecond to performance *before* the cache enters into account. When caching is enabled (even at the default level, which is on by default), there is absolutely no difference in performance when retrieving arrays instead of variables.

I would thus strongly recommend adding support for arrays into updateSettings!
Re: New revs - Public comments
« Reply #467, on October 8th, 2012, 06:13 PM »
Quote from Arantor on October 8th, 2012, 06:06 PM
So we go through the few hundred variables, testing for { and unserialize every page load. Sounds nice and efficient.
(See above... Sometimes, just doing some performance tests is faster than discussing performance issues ;))
Quote
How many rows is that actually applied to? You need to consider around 400-500 rows for a typical install.
It's on my local install. Which dates back to last year or so... So, I only added data to it, not removed any.
Quote
But more than that, I guarantee it WILL introduce issues because it's not just the loading code that needs to be fixed, all of the saving code also all needs to be fixed.
If you supply a string, which is always the case right now, it won't make a difference whether you add support for arrays or not... Whether it be at saving time, or at loading time...
Quote
Topic Solved doesn't seem to worry about it too much, seeing how certain types of setting can themselves be arrays, which are already serialised, such as multi-select or boards settings.
And AFAIK, serializing a serialized string will simply generate an escaped serialized string, won't it...? So you can keep using your custom decoding code on it later.
Quote
Do it if you really want but I'm really not keen on it because I'm not convinced it actually saves anything. The problem isn't loading, it's saving. That, and all the fun code related to editing settings in the admin panel, which was crappy enough to write the first time.
:^^;:

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re: New revs - Public comments
« Reply #468, on October 8th, 2012, 06:16 PM »
Firstly, serializing a serialized string gives you a doubly serialized string. Yes, no kidding, there was a bug in earlier releases of Project Tools where this was happening. So you have to unserialise it before unserialising it.

Secondly, the point I have been making is that certain things in prepareDBSettingsContext already have arrays, and it serialises them before sending to updateSettings.

If you want to add it, you go add it. I just want nothing to do with the debugging that I know will result. (You'll need to update Topic Solved and possibly other plugins too)

Nao

  • Dadman with a boy
  • Posts: 16,082
Re: New revs - Public comments
« Reply #469, on October 8th, 2012, 06:44 PM »
Okay, before I forget...
Regarding the setlocale thingy.
I hadn't looked into it before, but your change made me realize that my local install capitalizes month names in French, unlike what should be done. It works here on wedge.org, but not on my local install.
Where should I start looking to fix that...?
Quote from Arantor on October 8th, 2012, 06:16 PM
Firstly, serializing a serialized string gives you a doubly serialized string. Yes, no kidding, there was a bug in earlier releases of Project Tools where this was happening. So you have to unserialise it before unserialising it.
That's exactly as expected...

array(1,2,3,4,5,6,7,8,9,10)
Serializes to
a:10:{i:0;i:1;i:1;i:2;i:2;i:3;i:3;i:4;i:4;i:5;i:5;i:6;i:6;i:7;i:7;i:8;i:8;i:9;i:9;i:10;}
Which serializes to
s:88:"a:10:{i:0;i:1;i:1;i:2;i:2;i:3;i:3;i:4;i:4;i:5;i:5;i:6;i:6;i:7;i:7;i:8;i:8;i:9;i:9;i:10;}";
Which unserializes to the a:10 string, which then unserializes to the proper array.
Meaning that you can store serialized arrays in an array, and it won't screw them up.
I guess you'd only need to be careful not to send a serialized array to updateSettings, or maybe I should simply check at updateSettings time whether the variable is (1) an array (in which case, serialize it and store it serialized), (2) or a string, in which case, test whether it has { in it, in which case try to unserialize it, and if it works, then serialize it again so that the $settings loader will correctly unserialize that and store the serialized array (rather than its unserialized version) into the variable.

Do you know what I mean...? :^^;:
Quote from Arantor on October 8th, 2012, 06:16 PM
Secondly, the point I have been making is that certain things in prepareDBSettingsContext already have arrays, and it serialises them before sending to updateSettings.
In which case these arrays will be serialized twice, and then unserialized back into a serialized array, which should ensure that you don't even have to fix your code... (You can, of course.)
Quote
If you want to add it, you go add it. I just want nothing to do with the debugging that I know will result. (You'll need to update Topic Solved and possibly other plugins too)
Possibly not. :)
Posted: October 8th, 2012, 06:34 PM

Looked into my setlocale problem...

If I do this: (as suggested by http://www.php.net/manual/en/function.setlocale.php)

setlocale(LC_TIME, '');

It correctly uses French & lowercase on names. If I don't, it will actually return dates in capitalized English...?! (Using strftime.)
I'm not exactly sure where this needs fixing...

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re: New revs - Public comments
« Reply #470, on October 8th, 2012, 07:48 PM »
Quote
I hadn't looked into it before, but your change made me realize that my local install capitalizes month names in French, unlike what should be done. It works here on wedge.org, but not on my local install.
Then either the locale database is wrong or it's using the strings in the language file which are also capitalised.
Quote
In which case these arrays will be serialized twice, and then unserialized back into a serialized array, which should ensure that you don't even have to fix your code... (You can, of course.)
Why? Why serialize it twice? That's just overly complicating the matter (and is why I want nothing the hell to do with it) It's an array, not an array inside an array.
Quote
It correctly uses French & lowercase on names. If I don't, it will actually return dates in capitalized English...?! (Using strftime.)
I'm not exactly sure where this needs fixing...
Well, I tested it locally and it worked as expected for me :/

Refer back to http://wedge.org/pub/bugs/7158/smf-bug-4870-incorrectly-capitalised-month-names/

Nao

  • Dadman with a boy
  • Posts: 16,082
Re: New revs - Public comments
« Reply #471, on October 8th, 2012, 09:54 PM »
Quote from Arantor on October 8th, 2012, 07:48 PM
Quote
I hadn't looked into it before, but your change made me realize that my local install capitalizes month names in French, unlike what should be done. It works here on wedge.org, but not on my local install.
Then either the locale database is wrong or it's using the strings in the language file which are also capitalised.
Oh yeah, it's probably that... :-/
Problem is, I thought that these months were stored in there to be used in things like the calendar, where you show JUST the month, right..? And a lowercase month name looks silly, I guess, as a title...
Quote
Why? Why serialize it twice? That's just overly complicating the matter (and is why I want nothing the hell to do with it) It's an array, not an array inside an array.
We're really not serializing things twice, except if the submitted string is serialized, which means that the caller code is expecting to get a serialized string back when stuff is retrieved from the database. It's just a 'protection' against bugs... Other than that, everything's serialized just once. (Or never -- in the case of a non-array.)

The more I think about it, the less reasons I can think for this to fail... The only issue right now is with empty arrays. If they're not initialized, it's impossible for Wedge to tell that we should return an empty array. For instance, if 'js_lang' is not initialized, it's an empty string. Thus we're not turning it back into an empty array. Thus $settings['js_lang'] is an empty string, rather than an empty array, when js_lang hasn't been set yet. It might create some confusion/errors. The logical solution would be to either store ALL array-types variables in install.sql (with a default set to "a:0:{}" or whatever the content is to represent an empty array), or have fallback code for every occurrence...

Maybe I could/should simply do a quick commit at some point -- the bare minimum required to make it work, and if it fails to work correctly, then we can easily revert it.



In other news, I've just finished implementing block variables.
e.g. in the main skeleton, we have a <linktree> block, right? And it's called *again* but manually if in a topic page. I already worked with this before and gave up on it because I can't define two blocks with the same name. Then it occurred to me today that I could simply store the variable list inside the block name... Eh, why not? Then it works. I just need to extract the variable list at render time.

So, we have something like this...

<linktree />
<content />
<linktree:1 />

The second one calls template_linktree with the first parameter set to '1'. I've invertedt the two variables so that $on_bottom is set to 1 in that second call. It works perfectly... :)
Now people are finally going to be able to call a single block multiple times, either with multiple variables or simply entering dummy variables like:

<my-block:to-the-left />
<my-block:to-the-right />

If you have several variables to pass, just separate them with commas (<linktree:1,2,3 />)
There's just one weakness: you can't pass strings between quotes or anything with spaces in them. I decided against it because it would slow down the code for nothing IMHO. Really, if you're declaring a variable inside a skeleton, you're only telling the block what behavior to use in that position... No?
Of course it would be cool to have something like <title:"Hello, world!" /> and create a block that is a title saying that. But it doesn't sound too realistic to me.
This isn't something that's impossible to implement. It's just that it makes the regex longer, more complex, and I don't really trust long regexes, at least not too many... ;)

So, what do you think...?
Re: New revs - Public comments
« Reply #472, on October 9th, 2012, 06:10 PM »
Okay... I'm a bit lost in my work right now.

I'm working on several things at a time, and can't seem to make any progress.

1/ Moving linktrees into the container: this has proven to work well thanks to the post above, but I just realized that Wine causes a problem... (And Wuthering too, probably.) Namely, because the sidebar is a very stylish one and its style is applied to the containing table cell, I can not apply a margin-top above the sidebar to compensate for the absence of a linktree. Table cells only accept padding-top, and obviously the only effect is to make the sidebar's insides bigger rather than add padding above the sidebar.

Solutions:
a- Move the sidebar to a contained div, and give up on having it fill the entire height,
b- Move the sidebar to a contained div, apply an equal-height CSS hacks (basically: padding-bottom: 10000px and margin-bottom: -10000px or something like that), and give up on having the bottom of the sidebar rounded (instead, it will disappear behind the footer.) I can apply this solution to Weaving and Warm easily, BTW, this way I could even get rid of the table macros for IE6/7...
c- Move Warm to be a child of Weaving, and have Wine and Wuthering use solution (b),
d- Add margins above the sidebar's container's container, but this also adds space above the content area, and I can't apply a negative margin to it either, unless I do some positioning magic, which is sure to cause problems later in the process...
e- Maybe something else...?

2/ I'm working on an alternative way of modifying a skeleton through skin.xml, but I can't seem to get the hang of it... (Mostly in terms of writing standards...) Would it make sense or not? I'm thinking about doing something like this...

<move block="linktree:bottom" to="main" where="before" />

Which would allow blocks to be moved around without having to reproduce the original skeleton, essentially making it more solid when the skin is moved below another new default skin.
Posted: October 9th, 2012, 05:30 PM

I know I'm annoying, but a small bump is really needed here :P
Because the message is primarily for Pete and I'd rather have a 'I don't know, let me think about it' than nothing... :whistle:

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re: New revs - Public comments
« Reply #473, on October 9th, 2012, 06:15 PM »
Well, it might be primarily for me but it's not even a case of 'I don't know, let me think about it', but simply a 'I have no idea which is the best course of action'. There's simply so much stuff that it relates to, potentially, that I can't tell you which would be best.

What I will say is that I dislike a, either the sidebar should be full height or not at all. I will note that I am personally not averse to using tables to fulfil this, but that's because I don't give two hoots about good design :lol:

As far as 2 goes, that sounds very nice and flexible.

Nao

  • Dadman with a boy
  • Posts: 16,082
Re: New revs - Public comments
« Reply #474, on October 10th, 2012, 12:08 PM »
Quote from Arantor on October 9th, 2012, 06:15 PM
Well, it might be primarily for me but it's not even a case of 'I don't know, let me think about it', but simply a 'I have no idea which is the best course of action'. There's simply so much stuff that it relates to, potentially, that I can't tell you which would be best.
Hmm, so this is what I'm currently thinking of doing...

- Weaving: div system, linktree inside content, with margins and stuff. Sidebar is positioned through negative margin hacks.
-     Wine: table system, linktree above content, solves the margin problem because the sidebar is no longer stuck to the menu
-          Wuthering: inherits Wine
-          Warm: TBD (inherits Wine or Weaving?), because it changes the skeleton anyway...
Quote
What I will say is that I dislike a, either the sidebar should be full height or not at all.
If they're in a table, they're full height. If they're not, they're not full height. There's no alternative. Unless you mean the fact that the bottom can't have rounded corners in non-table mode..?
Quote
I will note that I am personally not averse to using tables to fulfil this, but that's because I don't give two hoots about good design :lol:
And it's a perfectly valid solution, of course, and one I'll keep using. After all, I wrote macros for a reason! And precisely for that one...
No, the fact is that I really want to have a 'simple' skin as default, and introduce more complex stuff in sub-skins and the likes. The main issue for me is to determine whether the table workaround is a simple or complex addition.
Quote
As far as 2 goes, that sounds very nice and flexible.
I've finished a first implementation, worked on that this morning... It's barely working. Well, it's already a good thing that it's working.

However, err... I've been thinking about something. Something silly really...
Currently, wetem takes the skeleton tags, turns them into a multi-dimensional array, and then we are allowed to apply various operations to it before it's rendered.
I was thinking, why exactly didn't I think of something simpler...? Or, at the very least, if I think of it, why did I give up on it? I don't remember...
The 'simpler' solution would be to apply all operations on the skeleton tags, and THEN at render time, build an array out of them...
Currently, because we're working on an array, doing simple changes to it can be quite hard. I'd even venture into saying, although I wrote the whole damn thing and I actually understand what the code is about (damn, I even commented a LOT of it!), I had a short WTF moment in front of one of the functions. Ever since, I've been wondering if it wouldn't be better to simply do the work on the tags themselves... It can't be much slower than working on the array...?! It's probably even faster, if I may say...

The only drawback of this approach, is that, ahem.... I actually have to rewrite all of the main manipulation functions.
Yeah. Well, I'd be ready to do just that... I'm sure it'll be no more than a few hours of work. (I'm not kidding.)
Re: New revs - Public comments
« Reply #475, on October 19th, 2012, 03:28 PM »
Okay, re: the above...

- I'm not in the mood to rewrite wetem, at least not for now. It's something that can be done much later anyway...

- I'm thinking of adding some skin.xml commands to add/delete a layer or block. Is this a good idea...? Delete is nearly a given, but add is more problematic -- a layer or block would logically be associated with a function... And there's no way to define a custom function in a skin. But I don't see myself adding the 'delete' command without an 'add' somewhere..?!

- Also, I've resumed work on the sidebar thing... The longer I think about it, the stronger my feelings are for simplifying the display:table thing for IE6/7. That is, I'd rather have an unfinished sidebar column for them, than having the sidebar stuck to the side when the window is too small to show it. So, I want to go for divs everywhere.
However, there is still this minor problem: the negative margin hack requires for an intermediate div (or whatever) to be added between <div id="main"> and the main content. What should it be called? Should it be added for all browsers, or just for old IEs? Maybe designers will be glad I added that div if they want to do something special, but OTOH that's already what <div id="main"> was there for... Should it be a special unused tag, like <article>..? Hmm, too bad Ian Hickson said 'no' to a <content> tag... (See article in the latest .Net issue for his rebuttal.)

- Oh, and while I'm at it... The sidebar is the only element in all Wedge that uses the <aside> tag. I don't know if it's worth applying an ID to it..? Why not simply style the <aside> tag? (It's probably faster for the browser to target an ID, but whatever...)
Re: New revs - Public comments
« Reply #476, on October 22nd, 2012, 03:19 PM »
Shameless bump!

With that and my link tree issues, I have a success rate of 0% today. Sucks...
Posted: October 19th, 2012, 09:03 PM

And now for a totally deserved bump... :P
(Good thing I got the link tree thing covered. Except for multi-line versions.)

godboko71

  • Fence accomplished!
  • Hello
  • Posts: 361
Re: New revs - Public comments
« Reply #477, on October 22nd, 2012, 05:42 PM »
Personally I think you should do what you think is best. I enjoy both the simple link tree and the rounded one on withering ( blame apple tired of trying to fix haha )
Thank you,
Boko

Nao

  • Dadman with a boy
  • Posts: 16,082
Re: New revs - Public comments
« Reply #478, on October 22nd, 2012, 11:34 PM »
I'm mostly interested in comments about what I posted above, though... :P

Ah my. IE7 is real messy when it comes to this media query. After a lot of work I managed to make it work in Weaving, but then Wine comes in and breaks everything... I've also fixed Wine, but it adds even more lines of code. If I'm going to have to 'watch' my css everytime I change it, and apply similar changes to the IE code, I fear it's not going to be very fruitful... Hmm.
When does IE7 fall out of fashion, already..?

godboko71

  • Fence accomplished!
  • Hello
  • Posts: 361