Skip to main content ↑ ↓
loadJavascriptFile() Started by TestMonkey · · Read 21258 times 0 Members and 1 Guest are viewing this topic. previous topic - next topic

loadJavascriptFile()

For your attention.
https://github.com/emanuele45/Dialogo/blob/master/Sources/Load.php#L1967
Function loadJavascriptFile(), its parameters.

IMHO these params ('default_theme', 'force_current', 'validate') don't work together well... 'default_theme' and 'force_current' seem redundant to me... 'validate' seems implied by 'force_current' (sayz Ema), it's a possible interpretation...

Either way, IMHO we have too many params lol, with overlapping meanings, or I don't get them or their (real) usefulness. :D  :o

How about only: 'validate' and perhaps 'fallback' = true. (for 'local' files). By default fallback to default theme. Don't echo a thing for validate=true if the last possible file ain't there. What am I missing here?

 TestMonkey is sleeping already, tis late...
The best moment for testing your PR is right after you merge it. Can't miss with that one.

Re: loadJavascriptFile()

Reply #1 –

I'd agree that validate should be true once force_current is set ... the rest meh

Re: loadJavascriptFile()

Reply #2 –

A different view ... https://github.com/Spuds/Dialogo/tree/CryBabies

This changes (yet again) how that function reacts  :-X

In this one it uses fallback true/false in place of default_theme and removes a couple of the other options since they were quite confusing. It does not use validate since it has to validate in order to fallback I think?

Right now it is not set up to force default theme only, meaning that fallback = false it will only look in the current theme, fallback = true it will look first in the current and then if not found in will look in the default.

So for a lot of files (js) in custom themes it will do a look fail look with those associated file_exists's on every page load.   You could change fallback= false to mean default_theme only but that seems odd. So this provides theme flexibility with some associated overhead.

You could cache results to help since continuing to do file checks for static data seems unnecessary, or simply not check for the file existence one you are in the default theme since at that point if its not found your hosed anyway.

Re: loadJavascriptFile()

Reply #3 –

Quote from: Spuds – A different view ... https://github.com/Spuds/Dialogo/tree/CryBabies

This changes (yet again) how that function reacts :-X

Oops. My bad! I'm sorry, my fault for not looking into it properly, I should have.  :(

TBH I'm not sure about several bits in all this.

Indeed, I think there's almost always one file_exists() (you're right, we can avoid the second call, if that fails then there's not much one can do). And some caching can help us.


On the other hand. There is of course no problem if we define what we expect here in some other way than tbh I assumed... (namely, master branch redefines what I thought it's expected...). I just don't see how it would work.
In the current implementation (master branch). I'm trying to understand why we have calls to .css and .js from default theme 'forcefully' ('default_theme' => true, such as in Admin.php). What is a custom theme supposed to do for those, if it has css/js files of its own, in particular .css? (i.e. an admin.css)

Looking over the implementation and use of these functions, I'm almost wondering why we made them at all - in particular loadCSSFile(). Other than a small piece of code got together. loadCSSFile() is used in only one place, loadTemplate(), and it's used for index css and variants afaics... so its behavior there, IMHO, has to be to prefer the custom theme, and by default fall back to default theme... How else, what am I missing? If a custom theme is supposed to ever not have its index or index[variant] loaded by loadCSSFile() in loadTemplate(), then it is supposed to... add it itself in the template, even if default templates don't do that?
Please correct me with whatever bit I may be missing here.

On easiness to use (from custom themes or other extensions), just for the record, the alternative I had to this implementation was: https://github.com/Spuds/Dialogo/commit/252bc2e941b513f4751908bdccd449541159522e#commitcomment-2307214

A bit unorthodox... but it allows you to not have to specify 'true' every time when you call the function, since IMHO true is the most expected value. The most expected behavior of the function with optional params should be the one that happens with the least params possible, methinks... That's why I'd see (if we want this 'fallback' solution) the default behavior of the no-params call as: fallback to the default theme location.

QuoteIn this one it uses fallback true/false in place of default_theme and removes a couple of the other options since they were quite confusing. It does not use validate since it has to validate in order to fallback I think?
I think you're right... There was a 'validate' that also checks the second location (i.e. default theme location for a custom theme) and doesn't output at all if it's not there. The difference is between a file_exists() now or an extra request later. Choose your pick. :)

QuoteYou could cache results to help since continuing to do file checks for static data seems unnecessary, or simply not check for the file existence one you are in the default theme since at that point if its not found your hosed anyway.
Yes, exactly...
https://github.com/Spuds/Dialogo/blob/b3b6991d438bb14f10da2d60560c0d730bd0c089/Sources/Load.php#L2009
The best moment for testing your PR is right after you merge it. Can't miss with that one.

Re: loadJavascriptFile()

Reply #4 –

Quote from: TestMonkey – On the other hand. There is of course no problem if we define what we expect here in some other way than tbh I assumed... (namely, master branch redefines what I thought it's expected...). I just don't see how it would work.
In the current implementation (master branch). I'm trying to understand why we have calls to .css and .js from default theme 'forcefully' ('default_theme' => true, such as in Admin.php). What is a custom theme supposed to do for those, if it has css/js files of its own, in particular .css? (i.e. an admin.css)
Aha. Methinks I now know who TestMonkey is. That rambling, convoluted posting style that leaves the reader more confused than they were to start with is very Norvish. :D

Anyway, the quoted bit is definitely a bad idea. I've said it umpteen times and I'll say it again: having things forced on you by Sources is a themer's nightmare. If you really want to piss people off and restrict the range of themes, force default css files in Sources. That'll work. :P

Will anyone ever change admin.css? It might pay to remember how the 2.1 Alpha theme started out: when some crazy deranged got sick of debugging boring old Curve 2.1 and decided to theme admin independently of the main forum. Methinks that answers the question. :D

BTW, this also applies to things like the SCEditor css. If a themer is doing a really good job, they will probably want to adjust the display of quotes, etc in the editor. I did it for default (and it still needs a bit more work) so you can bet someone else will want to do it too. For instance, if doing a dark theme, having default editor css would be an eye burner.
Last Edit: June 11, 2013, 08:23:18 am by TestMonkey
Master of Expletives: Now with improved family f@&king friendliness! :D

Sources code: making easy front end changes difficult since 1873. :P

Re: loadJavascriptFile()

Reply #5 –

[off topic]
Buzz off, Aussie.
[/off topic]

On the rest: yeah, that's why I would have expected a clear 'fallback to default' behavior.
Although it sort of was... I guess I just don't see how those params were supposed to mix.

SpudsMan, thank you for the experiment! I see you already avoided the check on default. IMHO this solution is very good.

For me it remains to be seen tbh, if those functions (in particular loadCSSFile()) bring enough value... Does it make sense to be used by add-ons? Recommended for their use? Personally I'm taking for granted that the answer is yes, at least for js, but I don't think they're ready for it... Since I'm on the line for shooting at anyway, I'll dare add: they'd need custom paths for that.
Last Edit: December 14, 2012, 02:01:02 am by TestMonkey
The best moment for testing your PR is right after you merge it. Can't miss with that one.

Re: loadJavascriptFile()

Reply #6 –

As noted there are two functions of interest, the loadJavascritFile and loadCssFile.

With the JS the feeling (ok mine) was that themes really don't tend to include there own copy's, so the default_theme thing was used to always point to the base site files.  Of course since that's in the sources it does not allow an easy way to themers to load their own version of say script.js or whatever.   We have a lot of js on some pages now, so really checking if 6 or 7 files exist on a page load when 99.9% of the time they are only going to be in the default area seemed unnecessary, does that sound biased :P

That code was then copied to loadCss nd as noted the only css that was loaded was via loadtemplate, however those were also pointing to the site files only, bit of a mistake, as the intention for css was to be current theme then default. 

Both loadCss and loadJs are what feed the combiner as well, if the load, or intention to load, is not done until it hits the templates, its to late, or ugly to combine really.

So now both js and css will first look in current theme for files and then default, a venerable theme nerdvana , most flexibility and most overhead.  I'll look into using the cache to stash where things were located, and use a relatively long cache life since once found those file don't move.

Re: loadJavascriptFile()

Reply #7 –

Javascript is probably fairly safe, although I can think of times when I might want to modify it. However, that can usually be done in the template by inserting the relevant values where you call the js. It's not quite as neat as modifying the source, but it will work in a pinch. So, if that can be done, as a compromise for overhead I'd be fine with calling js from default.

One thing to be wary of is where the js files contain presentation. That should defintiely be theme-dependent. If it's all structural/functionality then default is fine.

Of course, anyone running their own site can still do what they like with the source, and probably will.
Last Edit: December 14, 2012, 03:14:15 pm by Antechinus
Master of Expletives: Now with improved family f@&king friendliness! :D

Sources code: making easy front end changes difficult since 1873. :P

Re: loadJavascriptFile()

Reply #8 –

 Feature Cat loves the quandaries between performance freaks and extensibility freaks.

Quote from: Spuds – With the JS the feeling (ok mine) was that themes really don't tend to include there own copy's, so the default_theme thing was used to always point to the base site files. Of course since that's in the sources it does not allow an easy way to themers to load their own version of say script.js or whatever. We have a lot of js on some pages now, so really checking if 6 or 7 files exist on a page load when 99.9% of the time they are only going to be in the default area seemed unnecessary, does that sound biased :P

 Feature Cat jumps up and down.

I know, I know! A theme setting for js. :D  :o

 Feature Cat runs before getting slapped banned into non-existence.

Re: loadJavascriptFile()

Reply #9 –

The best option is simply to remove presentation from js files, and put it in the css where it belongs. :)
Master of Expletives: Now with improved family f@&king friendliness! :D

Sources code: making easy front end changes difficult since 1873. :P

Re: loadJavascriptFile()

Reply #10 –

Quote* Feature Cat jumps up and down.
From the looks of feature cat I can't see that happening :D ... looks like breathing might be as much exercise as she gets ... plus lots of features to consume nom nom nom
Last Edit: December 16, 2012, 04:47:54 pm by Spuds

Re: loadJavascriptFile()

Reply #11 –

lol
Bugs creator.
Features destroyer.
Template killer.

Re: loadJavascriptFile()

Reply #12 –

I would like to see these just become part of a Theme class where you'd standardize some of these settings.
 

Re: loadJavascriptFile()

Reply #14 –

Good start. I prefer protected over private but I am glad to see progress.