Themes 2.0: YUI, Caching, Performance, Consistency

can not attach patch here - it is too big and too many changes in binary files:
test site at http://ferda.cdv.tul.cz/moodle20t/
you can downlaod the cvs checkout from http://ferda.cdv.tul.cz/moodle20t.zip

Missing bits:

• theme switching page needs improvements - all diagnostics and problems should be showed there, not inline; add button for purging of caches
• all themes except standard and sample theme removed
• JS not supported in themes - do we need it?
• no examples of template use
• grade edit tree.css need to be fixed and moved into standard theme
• theme compression

Does IE use some custom css - other browsers (FF,Opera,Safari,Chrome) catch the color settings (of samplecolor theme) for css but IE does not... still IE does get the width of left and right colums from settings?

I mean that it looks like IE does not read theme/samplecolor/styles at all???

background-image:url([[themesetting:gradient]]) etc

oh, I did not test IE yet, going to fix it today!

found the problem, the sloppy IE thing has some weird limit on number of things in CSS - going to chop the large CSS into smaller chunks just for IE

IE can never stop surprising - was it CSS File Size Limit (288kb for IE) , maximum of 32 @import statements or some other limit?

4096 selectors limit

What is the current state of http://docs.moodle.org/en/Development:Theme_changes_proposal Petr?

Is it already approved and going to be implemented to moodle 2.0 HEAD ?

latest test site at http://ferda.cdv.tul.cz/moodle20t/
complete cvs checkout at http://ferda.cdv.tul.cz/moodle20t.zip

unfortunately normal patch has problems with binary files

adding tim as watcher here, if you find some time please have a loot at the list of subtasks, thanks

Go Petr go! +100

I'll try to look at this today, if you can hold off a bit after Martin's enthusiasm

I am sorry Tim, I can not wait much longer, we have to move forward. I am going to start committing changes later today.

Yes. Sorry Petr. I was working on my own code yesterday. Can you give me until lunchtime today (1:00 your time?)

OK, I have now commented on all the subtasks (that I feel the need to comment on). Basically I think the plan is great, apart from a few minor questions/concerns I raise.

Now I want to look at the code, but I am running out of time on my self-imposed deadline. Please could I have a little bit longer. I promise not to work on anything else until I have finished. (However, I will have a break for lunch )

I have to say, this would be much easier to review if the changes were in git.

I'm still struggling to get Eclipse to show me the diffs. It keeps asking me for your password, even though I have changed the sharing info after downloading your zip.

Also, CVS thinks that every file has been modified, even though there are no changes in the file. I guess that is a timestamp thing.

Right. CVS finally beaten into submission. I am now in a position to start reviewing code. Please give me a few hours.

I think 3. is the most important of these points:

1. admin/appearance.php. I can't work out, do the settings for all themes appear on one page, or a separate page for each theme?

2. backup/restorelib.php has commented out code. Why?
Same code commented out, rather than deleted, in other places too. Why?

3. I note you are removing all separate plugin styles.php files. That is one approach. The other option would be to move as many styles as possible into styles.php files in each plugin. That might be easier to maintain, and would reduce the difference between contrib and standard plugins.

4. course/lib.php, and other places, are still calling $OUTPUT->old_icon_url. Why?

5. coures/view.php. Aren't you going to rename $PAGE->set_generaltype('course'); to $PAGE->set_layouttype?

6. You should create a subtask for
//TODO: find some better way to disable blocks and minimise footer - pagetype just for this does not seem like a good solution
//$PAGE->set_generaltype('maxcontent');
in mod/imscp/view.php

7. login/index.php does $PAGE->set_generaltype('form'); Surely that should be login?

8. mod/resource/pix/icon.gif seems to be missing.

OK, I have now reviewed all the trivial changes (The ones outside lib and theme dirs). I have to go to an orchestra rehearsal now. I will review the rest later.

More.

9. I do not see any logic to the division

$THEME->sheets = array(
    'pagelayout',
    'core',
    'blocks',
    'course',
    'block_calendar_month',
);

10. I do no understand
'options' => array('nofooter', 'nonavbar'),
in $THEME->layouts. Surely you can control this in layout.php.

Doh! No. I see. It lets you reuse general.php in more cases.

11. Does $THEME need to be a global? Surely we can get rid of it and just use $PAGE->theme to access it. In which case you can delete $THEME-> everywhere in config.php.

12. This bit of code:

$regionsinfo = 'pagelayout';
if ($PAGE->blocks->region_has_content('side-pre', $OUTPUT)) {
    $regionsinfo .= '-pre';
}
if ($PAGE->blocks->region_has_content('side-post', $OUTPUT)) {
    $regionsinfo .= '-post';
}

must not be in the layout PHP file. Instead, make a $PAGE->pagedivclasses like $PAGE->bodyclasses to encapsulate that logic. Actually, how come you need that code in base, but not in standard? I don't understand.

13. I don't really like the logic

if (empty($PAGE->layout_options['nonavbar']) and $OUTPUT->has_navbar()) {

for two reasons. First, we should not have logic in layout PHP. so at most it should be

if (empty($PAGE->has_navbar()) {

Second. $OUTPUT is a renderer. Its sole job is to produce HTML. There is no way that a $OUTPUT->has_navbar() method should exist.

14. Do we really need that many divs in general.php?

15. Oh yuck! Just seen $THEME->sheets in theme/standard/config.php. That really convinces me that I am right in 3. above. We should have a styles.css in each plugin.

16. And I really do not see any point in having both base and standard themes. We should just have a base theme, whatever it is called.

17. what replaces the old rtl.css?

18. why is favicon.ico at the top level, not in pix?

19. I am very doubtful about deleting customcorners and anomaly from CVS. We want them (working) in 2.0. If you delete them now and add them back later, it will make CVS history even harder to follow. I think it is better to leave them there, but broken, until they are fixed.

20. Please commit the template renderer code somewhere in contrib, so it is easily browsable, rather than deleting it completely.

21. That is weird, the unit tests have been changed to say set_pagelayout, even though other code still refers to generaltype.

22. Why is the change in unction path_inaccessdata($path, $accessdata) { in this patch?

23. lib/javascript-static.js has tabs, not spaces.

Saving these before my browser crashes. More to come.

11. Correction. Of coures, we don't want to delete $THEME->, because it is an instance of theme_config, which gives you autocomplete and PHPdoc comments. But if it is no longer a global, we could change it to $theme. Or not bother.

24. I don't think the name minlib.php really explains what it is. I can't currently think of a better name though.

25. //TODO: add themedir support in moodlelib.php should have a subtask filed.

26. Coding guidelines say two blank lines between classes. http://docs.moodle.org/en/Development:Coding_style#Class_declarations - you have removed at least on of these in outputfactories.php

27. I don't understand why you want theme renderers to be in lib.php, while everywhere else they live in renderers.php. Is there a reason? Oh it is slightly more complicated than that. It is renderers.php compared to renderer.php. Hmm. theme_config constructor still mentions renderes.php as well as lib.php. Now I am really confused.

28. The theme_config properties that are not settable from config.php. Wouldn't it be better to make them protected, with getters, to avoid the possibility of abuse.

29. You have tabs in outputlib.php.

30. Theme diagnose($themename) function is a great idea! However, please create a subtask for the TODO.

31. theme_config constructor should be called __construct.

32. css_content method is very long, and looks to have a lot of repetitious code. Can't you refactor it?

33. Was it really right to delete the two existing css post-processing funcitions?

34. resolve_image_location also looks like it should be refactored to avoid duplication.

35. In lib/outputrenderers.php, line 327, I think you should use $this->output_empty_tag, to keep the coding style consistent.

36. Please commit the whitespace fixes in webdavlib.php as a separate commit.

I hope you don't feel too bad about the number of things I found to comment on. It is mostly excellent, but I think these points need to be addressed before it is ready for CVS.

2. backup/restore is dead

3. plugin stylesheets - this is a bit of misunderstanding - the styles will be in plugins + themes + once more as theme additions in plugins

4. I had problems with merging - sure, it will be removed everywhere

5. again merging problems - search replace is going to fix it

6. yep

7. yep - login layout

8. yes - merging trouble

9. is a migration process - I just moved everything to standard to make room for new stuff in plugins

10. this is internal implementation details of themes - it is allowed to do anything with those options; core code is not goign to access these

11. $THEME does not need to be global, BC only

12. yes

13. internal implementation detail - this is not part of API

14. IE6 needs tons of useless divs, grrr

15. no, the current themes in standard are just temporary - we need room to work on better sheets

16. this is not my idea, theme designer had huge problems with undoing stuff from standard theme, they need something very basic when doing new designs

17. rtl is replaced by direction class

18. no idea why is fav icon there, I was not sure - pix is better place

19. yes, custom corners shoudl be there in 2.0, the CVS history is not going to be problem because the style sheets will have different names - no more font+style+color

20. yes

21. merging problem -search replace fixes that

22. I had some problems in installation - the new themes load the pages in different order

23. grrrr, death to tabs

24. anything better than minlib.php is fine for me - can be changed later

25. yet, a lot more subtasks

26. who invented the two lines??? ok, ok

27. I just like lib, it could be placed into other file - can be changed later at any time

28. I like picking only what is needed - internal implementation detail, but my proposed way looks a bit safer

29. grrrrrr tabs die dir

30. -32. ok

33. one postprocessing looks enough for me - going to review it once more

34.-- ok

THANKS A LOT - GREAT FEEDBACK!

A few responses:

2. Yes, but that commented out code appears in many other places. That was just the first place I saw it. The other places are more important.

3. Please can you explain how you think it should work on http://docs.moodle.org/en/Development:Theme_changes_proposal

9. It is a migration process that makes CVS history unnecessarily difficult to track. Why not go directly to styles.css?

13. $OUTPUT->has_navbar is part of the API that should not be there. I'll file a bug on Sam H. MDL-20833

16. But surely no-one uses standard theme in production. My point is that we should have one base theme, which uses all the CSS that is in plugins and core outside the theme folder. And then a bunch of nice themes that people might actually want to use, one of which is the default.

19. Still better to reduce the number of renames etc. that you have to track when looking back through history. Also, if/when we move to git, it can cope well with renames, but it cannot cope with delete then add back later.

I think a better solution would be to leave these themes in core, but in their config.php do something hacky like set_config('theme', 'standard'); die;

24. The best I can do is configonlylib.php. That is a bit sucky, but clearer, and it is not a library that people will refer to often.

28. The point it to make the PHP docs as helpful as possible. I don't see the problem in making the other properties protected with get methods.

3. yes

9. no, I need to keep them separate for now so that I am able to work on this; each theme is allowed to do whatever it thinks is correct here

16. we do need the standard theme for now and MD wanted something similar to standard in 2.0, I think it is a good idea to keep it

19. Old and new themes will share only config.php, keeping the old unused code around breaks my grepping and sanity. I believe there will be very little shared history. The themes are something really new both inside and outside.

28. theme designers are not developers, I think it is better to not allow them to override the config directly and use it just as a simple object with properties - it is very hard to diagnose OOP problems in code that can not print directly to output. I think that theme developers need something easier to use than PHPDoc.

Petr, please revert your last commit immediately. The

removed problematic HTML_ATTR_EMPTY, simply use null instead + removed incorrect trimming of values (yes, leading/trailing spaces are sometimes valid attribute values too!)

one.

It is very, very useful that in many places you can do things like
html_writer::tag('div', array('class' => $this->get_class()));
or something, and know that that class attribute will be omitted if get_class returns something blank.

It saves about a million if statements all over the place in output code.

It is very rare to need an something="" attribute - alt="" is the only one I can think of right now, so it is right that that requires an special value, which should probably be html_writer::EMPTY_ATTRIBUTE.

sorry tim, please use real NULL as a default value, this HTML_ATTR_EMPTY was breaking other stuff because empty string is a valid value in cases like html select options. Please let's not do the same mistake like Oracle - empty string and NULL is something really different, so let's use it properly.

BTW while working on this I discovered wrong handling of disabled attributes and other similar issues.

also I discovered one scary trim($value) which I suppose had the same origin like the HTML_ATTR_EMPTY

please file any regression into tracker and I will fix them asap

Sorry Petr, how is it a good idea to force people to write code like:

$attributes = array(...);
$class = $this->get_class();
if ($class) {
$attributes ['class'] = $class;
}
html_writer::tag('div', $attributes);

When it could be

html_writer::tag('div', array(..., $this->get_class()));

Surely you agree that the second is a nicer API for developers.

I can understand why, using the Oracle parallel, you have some doubts, but I think that on balance, convenience 99% of the time is worth it.

you still can write html_writer::tag('div', array(..., $this->get_class())); if your get_class() returns null, alternatively we could intelligently omit some known empty attributes such as class, but redefining empty string to null and HTML_ATTR_EMPTY to empty string is definitely not a good technical solution. Your HTML_ATTR_EMPTY was breaking my code pretty badly and there was no workaround, I strongly disagree that it is useful in 99% of cases, I spent a lot of time in this area recently...

Actually, I worked out the answer is:

fuction opt($value) {
    if ($value !== '0' && !$value) {
        return null;
    }
    return $value;
}

Then

html_writer::tag('div', array(..., opt($this->get_class())));

That should make both of us happy.

I just ran into a big headache trying to do some theming stuff and finding that the css didn't change when I emptied the browser cache.

It took me a while to find the button to reset the theme cache because it doesn't show up when you search the admin block. It's also not where I expected it - I was looking in the performance section.

I think one or both of those things needs changing so that it's easier to find.