|
History
| Log In
|
|
|
|
[
Permlink
| « Hide
]
Olli Savolainen - 01/Jul/08 11:35 PM
The current version, if you can't view the static mht file, read mht-readme.txt.
what was changed in question/ is in the diff, the other files are just to be added in mod/quiz/ for now.
diffs and separate files for current changes from moodle 1.9
This time, just the diff + one gif file + the lang files from moodledata
Drat! I can't get the patch to apply. Well, it is nearly time to go home here anyway, so I don't have time to look at the code now, but I was hoping to get the merge done now so I could start looking first thing in the morning.
It is not happy with weblib, I think because of whitespace differerences. More seriously, there are files like editlib_redesign.php where the patch contains changes, but 1.9 does not have a file of that name. Apart from that the patch works (once I have selected reverse). Could you try regenerating the patch, then I will try to have a look tomorrow. Thanks. Yes, there are some additional files. Since the original where I created the patch from does not contain the _redesign files, I figured patch would just create those files. But I'll create another package with also separate files.
Eclipse seems to want to forcibly add strange whitespace into weblib.php. I will look into that, too later today. (It's 13:20 here so plenty time). I am sorry indeed. It seems that revision 1 in my subversion repository (against witch the previous patch was made) was not clean moodle 1.9 after all. Now I have tested the new version 0.52 to patch against fresh 1.9, see notes.txt. I also made some fixes based on the usability tests already, now that I got started :) - more is still to come, though.
- Emphasized the question bank title, since in contrast with the earlier OOo prototype tests, now the whole right side of the screen often went completely unnoticed and frustrated users in tests - Made reorganize a subtab of edit - Made essay question icon resemble a checkbox less, since it confused a lot of users in tests Also, fixed IE 6 to display properly, just had to fix two things in CSS and no quirks were required. However, everything related to javascript is broken on IE 6 at the moment: IE6 only seems to show one error message: Line 67 Char 22: Expected identifier, string or number. So the "add question" dropdown does not work. When I remove the new YUI javascript however everything works, so there is a conflict of some kind (though not if used in firefox). One thing that might help is to move also the new javascript to <head>, it is now in <body>. This error seems to vary between page loads or something though; in any case, the YUI random question adding dialog does not work. On the other hand, it seems that Moodle does not support IE 5 at all: the login form does not appear on the login page so I could not test it. Just to make sure you can use something, Tim, here is everything I have done as separate files, too.
Some review comments:
1. Do you develop with Administration ► Server ► Debugging > Debug messages set to DEVELOPER, and Display debug messages turned on? You should. I do, and am seeing PHP notices displayed, and that is making it very hard to test. Also, some of the notices seem to be pointing out real bugs. 2. Please try to keep the code wrapped at about 80 colums wide. It makes it easier to read, and when you wrap a line, double-indent the following lines (8 spaces) to try to give some clue that it is a continuation. 3. Line 249 of mod/quiz/edit.php (an example of a line that is too long!) refers to $param->..., but I can't see where this is defined. Looking at the code being called, these parameters don't seem to be used. Oh ... 4. I did not know about the PHP compact function. It looks dangerous to me, because it makes it hard to see where a variable like $currentcat is used. I would stick to the more verbose array('currentcat' => $currentcat, 'contexts' => $contexts); 5. And it is this undefined variable that is giving me SQL errors like ERROR: syntax error at or near "AND" LINE 1: ...ies c1, s19_question_categories c2 WHERE c2.id = AND c1.con... ^ SELECT count(*) FROM s19_question_categories c1, s19_question_categories c2 WHERE c2.id = AND c1.contextid = c2.contextid I am on PHP 5.2.4 and PostgreSQL here. 6. <link> tags should use the short-tag syntax: <link ... />, instead of <link></link>. 7. The way to get extra stuff, like links to stylesheets, included in <head>, is to pass extra HTML as the 5th ($meta) parameter to print_header_simple. 8. You probably need to change your print_box_start(); call around line 570 of edit.php to print_box_start('generalbox clearfix'); 9. It still lets me add questions even though the "You cannot add or remove questions because there are attempts." message is displayed. Also, when that message is displayed, the stylesheet is not used so the layout is messed up. I'm going to stop reviewing there for now. Sorry that a lot of these points are trivial. The key ones to fix are 1. and 9. It would be a lot easier to carry on reviewing if these are fixed. Huh, I was trying to set debugging on all along on the server, and couldn't so far make ubuntu do the PHP logging to /var/log/php.log where I have gotten used to seeing it. Didn't come to think that it might be in the actual panel. Sorry to have you test at this point, you could've just said the first point I guess :o).
Anyway, thanks for the generous input. It isn't like I did not expect issues like this to pop up, however. As my schedule is tight and the focus is on user interaction, making everything solid on the backend can only be done once the interaction looks like it really matches the users' needs, since before that virtually anything can change. So, the question again is: How does the process of evaluating the entire project proceed? When would you expect to know positively if you can take my work into core? My current funding is about to end but I am committed to making this a finished package. However, I am a bit unease with the feeling that I have no guarantee at all from Moodle's side at this point that any of this stuff will be used, and as mentioned, www.coss.fi people would like to hear something, as well. Some replies: 7. Oh. I used to do that, but the parameter name kinda implies it's for <meta> tags so I thought it's not ok :). 9. Will fix. I'm happy you brought up the subject of "You cannot add or remove questions because there are attempts", since this is a real issue with multiple faces. This came up in the usability tests, too: teachers are used to try everything in student role to preview. However, this way of previewing normally locks the quiz up, and the undo - deleting all attempts - is everything but obvious. So this is quite a usability disaster. This is not really much of a change in the UI but something that could be automated or the user, as follows. I think this would be of tremendous benefit, as also various messages in the quiz forum show this is a real issue - do you think we could get something like the following done? 1. Always warn the user if the are about to do something that affects the set of choices they can make later (User control and freedom), and as the undo here is not obvious, tell the user about it 2. Make the user aware of the choices they have in the quiz edit screens, where their actions' consequences are the most obvious: "The quiz has been <a href="attempts listing">attempted</a>, so you cannot edit it. You can still edit the questions in categories, so if you have random questions you can change the contents of those. If you need to modify the quiz, you can: - <a href="do it, but confirm">Delete all the attempts of this quiz</a> - <a href="do it, but confirm">Export the attempts</a> from the quiz to a static (HTML? XML? separate db table?) file. This will mean that you will be able to view those attempts, but will not be able to grade them within the quiz. - <a href="do it, but confirm">Make a copy of the quiz</a>, after which you can delete all the attempts from one of the quizzes and still keep grading functional. http://www.useit.com/papers/heuristic/heuristic_list.html Yeah, I guess options to just - delete the attempts, or - if there are actual student attempts make a copy of the quiz without the attempts would suffice. Still, hiding the quiz contents when there are attempts is not an option, just editing needs to be disabled. Also, since understanding what kinds of attempts there are is key to making the decision of whether the attempts can be deleted, there is a chance to make the interaction really fluid here by providing that information exactly where the decision is to be made. During the idle hours of the evening I couldn't resist the temptation but to look at the notices. Everything else I found at a glance was easy to fix, but the SQL error puzzles me. Seems that I don't know how to use question_category_object and the commenting seems scarce. I will look into it later more, now just to let you know.
$qcobject = new question_category_object( $pagevars['cpage'], $thispageurl, $contexts->having_one_edit_tab_cap('categories'), null, /*this still causes an error, somewhere it is using this in SQL unquoted, so if this is "x", you get Unknown column 'x' in 'where clause' - SELECT count(*) FROM mdl_question_categories c1, mdl_question_categories c2 WHERE c2.id = x AND c1.contextid = c2.contextid*/ $pagevars['cat'], null, *// also changed this $contexts->having_cap('moodle/question:add')); Actually, I did not know about compact() before you introduced it to me, either. Ah yes, I seem to have copied the latter line from the earlier line here verbatim:
$this->catform = new question_category_edit_form($this->pageurl, compact('contexts', 'currentcat')); $this->catform_rand = new question_category_edit_form_randomquestion($this->pageurl, compact('contexts', 'currentcat')); I, too, think it is worth changing to standard array() call. I certainly did not write the compact call, since I had to look it up to find out what it did! CVS blames Jamie ;-) Anyway to more serious topics.
I have been using the interface some more this morning, and I really like it. There are a couple of parts I have questions about, so hopefully we can hook up on Skype some time today and talk about it. But the big question is, should this be included in Moodle 2.0? That is a big question, so we need to break it down a bit. 1. Is this the right direction for the Moodle quiz user interface to evolve? And do we have a consensus about the answer? 2. Is the code quality good enough to be included in Moodle 2.0? That is does it follow the coding guidelines, and does it look like it is maintainable? 2a. If not, about how much more work needs to be done? 3. Does it meet all the necessary accessibility standards? For example, does it work with JavaScript turned off? Is it all valid HTML? Is it all usable just with the keyboard, no mouse? 4. How much time to you have to commit to this project from now on? In particular from now until we think everything is finished, and then later when 2.0 is in beta? My opinions about the answers to these questions are: 1. Yes. It immediately improves the current interface, and it probably makes it easier to add some of the future enhancements that have been talked about. There are some parts that need to be improved further, for example doing the re-ordering using drag and drop, but we can release what we have now, it is good enough, and make further improvements later. On the consensus subject, I will start another thread in the quiz forum in a moment. Annoyingly, Martin D is still on holiday at the Olympics, so I won't be able to discuss this with him until Monday, which is bad timing. 2. I would like to review the code some more. I have mostly been focussing on using the functionality in my recent testing. For the remaining questions, your probably have a better idea than me, hence the fact it would be good to talk. The essay icon looks more finished now :). There is an implication of a "pretty darn long answers required" to that icon, as well as the name "essay", which may not be what teachers are looking for, though - the need that an essay q can serve may just as well be a question requiring a shorter answer (another small glimpse to the usability test results last week). I don't know what to do about that, though, except renaming the question to "Freeform text/Essay" perhaps. Also I am not sure if the contrast of the grey "text" is enough or could that hypothetically confuse someone. Just wondering :).
Initial thoughts: 1. The comments about the demo have all been mostly favourable. One thing that really needs to be settled is switching between quiz editing and question bank. More about that in my report, which I hope to publish tomorrow. 2. I have tried to stick to general PHP coding guidelines and Moodle guidelines. However, I have a bit of an paranoid feeling about having covered all Moodle's guidelines since at the beginning of the summer new docs documentation about development I had not seen, seemed to pop up every once in a while. 3. It is designed to work javascript turned off. Unfortunately the javascript currently seems to have some bugs so there are issues if javascript is turned on, on both IE and Opera. Not sure if it is a YUI issue or a YUI user issue :). Keyboard usage could probably be improved quite a bit from the current state - adding accesskey properties and setting tabbing order etc. > The HTML is valid, or very close at least, now that I have moved the styles to <head>. This will not be an issue. 4. I am writing my master's thesis about this project this next semester, and I believe it will motivate me to have something concrete to do with the subject, as well as analyzing usability work in an open source project. Besides that, I am learning French and building up a new social sphere since I don't know almost anyone from Metz, France yet. During the following two or three week it will probably be pretty crazy trying to arrange everything and settling in, but after that I expect to have lots of time and motivation for this. By the way, the new interface for adding a random question to the quiz does not work for me. The form appears, but I get an error when I click submit.
Some comments on the code: lang/en_utf8/quiz.php * the strings in lang files should be sorted alphabetically, although I understand why you wanted to keep all the new strings separate while this was a patch. * I might tweak the wording of some of the English strings, but then it is my native language. lang/en_utf8/help/quiz/editconcepts.html * Similarly, I would edit the wording slightly. lib/weblib.php * In popup_form, the line "$submitvalue=$go;" should be outside the inner if statement. * The hack in print_header needs to go. Acutally see (and vote for!) MDL-16151, which is something that has been needed for ages. * print_paging_bar - are these changes really relevant to the current project? If they are a separate thing, probably better to file a separate tracker bug and patch for these. question/type/essay/icon.gif question/type/random/icon.gif * I like the new icons, particularly the random one. question/category_class.php question/category_form_randomquestion.php * A lot of these changes are related to the quiz UI, although they are also related to random questions. Do they really belong here, or would they be better somewhere in the quiz? question/type/edit_question_form.php question/question.php question/editlib.php * No problems. Just listing here so I can track what I have reviewed. That is everything outside mod/quiz. Submitting this comment now, before it gets lost. More later. The usual follow-up question: What are you trying to use the random question dialog on?
print_paging_bar changes are separate I think. They depend on the CSS in the css file of this project, though - making them separate really might be a good idea. The random question icon is from the package gnome-tali. I am verifying that it is okay to use an icon from a GPL'd project in another one. http://library.gnome.org/users/gtali/2.21/playing.html.en (that particular icon is not visible in the url but just so you get an idea. I am using Firefox 3 on Linux. The console gives a strange error in the middle of a YUI library.
Forum thread about whether to include this in 2.0: http://moodle.org/mod/forum/discuss.php?d=103869 Copying from one GPL project to another is fine. Another question: Do we need to make any changes to the separate question bank interface? Probably yes, but only small visual changes to the stylesheet, to maintain consistency with the quiz interface. http://tracker.moodle.org/browse/CONTRIB-643
It is important to keep the question bank window and the separate question bank as consistent as possible, to avoid confusion. In addition to outlook, element order etc. should be as close as possible. Rather, I would like to think of this as having them only differ where absolutely necessary. Also, I am still wondering about whether the question bank window should have a separate "create new category" button. I think it should, but it's a bit tricky to add it anywhere so that it is easy enough to find. And the create new question might be simpler if it were a separate dialog to select the question type from, but then again, it is an additional step. More review comments:
mod/quiz/addrandom.php * Some todos in this file that need to be done. (Yes, you need to log all page views. I know that currently some pages are missed, but this is a bug.) * Innapropriate copy-and-pasted comment at the top of this file. * You have an unnecessary ?><?php near the end. mod/quiz/edit_redesign.css * If this code gets released as part of Moodle 2.0, these styles need to be put into theme/standard, with its annoying division of CSS into different files for layout, colours and fonts, and separate files for browser-specific hacks. * I note that you have comments in here for things that need to be tested. That would need to be done before 2.0 * You should normally be able to avoid using !important by using appropriate selectors. * You have some empty rules, which should be cleaned up. * You have some tabs for indent instead of spaces. mod/quiz/edit-js.css * instead of using a separate css file like this, I normally do a small script tag that does document.getElementById('repaginatedialog').style.display = 'none'; mod/quiz/quiz_edit.js * Contains commented out code that should probably just be deleted. * YAHOO.log("This is a simple log message."); looks like debug code that should be deleted to me. * Some lines have trailing whitespace. * You have some tabs for indent instead of spaces. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Improvement
Open
Major
Quiz UI redesign, based on usability research
In Progress
