|
History
| Log In
|
|
|
|
[
Permlink
| « Hide
]
Pierre Pichet - 04/Apr/08 09:33 PM
some of the improvments have been done already on the item analysis report i.e. adding a HTML output and adding columns so that the data can be more useful for other software analysis
I attach a patch to tablelib :
+ /** + * Add a row of data to the table. This function takes an array with + * column names as keys. + * It ignores any elements with keys that are not defined as columns. It + * puts in empty strings into the row when there is no element in the passed + * array corresponding to a column in the table. It puts the row elements in + * the proper order. + * @param $rowwithkeys array + * + */ The basic idea is to have a way to add a row to a table without having to worry about getting the elements in the right order. All tables have an array $this->columns defined with unique names for each column in the table, this method takes an array with these column names as keys. This is the first part of my proposed additions to tablelib to make it a little easier to use and reduce code duplication. Comments please, do people agree that this can be added to HEAD and 1.9 branch? It is an addition to the API and won't affect any existing code. My second proposal to improve tablelib is to have a third way to add data to a table.
Most tables are constructed from an array of data returned by a sql query. I propose that we have a method on flexible_table, get_data(); get_data can be called after the columns array is set. It will call a virtual private method on itself to construct the sql and fetch any data needed from the db or elsewhere. Then it will iterate through the data and : * For each column in the array get_data will call a method on the object passed to it 'get_data_{$columnname}' with parameters : ** $data - a row from the returned sql and ** $download - a parameter indicating whether we are building data for download or for display. The parameters are only for convenience. The get_data_{$columnname} can also pull data from properties on $this. If no method get_data_{$columnname} is found then the column content is assumed to be $data->$columname. So for a simple table with just text you don't need to define get_data_{$columnname} methods at all. Or you can define them one by one after checking that the correct data is being fetched from the db. Then people who want to use this new api for building table data need to create a child class of flexible_table overriding the virtual function to fetch data and optionally add get_data_{$columnname} methods. My third proposal is to move the code for producing downloads into tablelib.
* we would filter download page parameter at the top of the page on which the table is printed and pass it to flexible_table to activate the download mode of tablelib. * headers and adding data to rows in the correct way is handled by flexible_table internals. * see above get_data_{$columnname} is passed a parameter $download which is set to 0 for no download or contains a string 'CSV', 'ODS' etc. * we would add methods to print download buttons. Buttons would be included for CSV, ODS, Excel and, if the table is paged, optionally to output full table as one XHTML file. These tablelib functions can be usefull for general uses and if correclty designed (as you always do :-) ) will be a + .
The different tables from the reports are a good way to test and code all the necessary features. Take care that flexible_table remains sufficiently flexible... I worry about "flexible_table internals." in "we would filter download page parameter at the top of the page on which the table is printed and pass it to flexible_table to activate the download mode of tablelib. * headers and adding data to rows in the correct way is handled by flexible_table internals. " I have to struggle with the internals of moodle_forms to obtain specific displays so ... I just added some of the usual suspects from Moodle HQ, so they can give their opinion on this.
I have long thought it silly that there is almost-duplicate code in the quiz reports for producing the HTML table and the various download formats. So I certainly support doing something. On a quick read through, Jamie's proposals sound plausible. But I want to think more before I comment. I started a docs page that documents how the new additions to the api might work, rather presumptuously here http://docs.moodle.org/en/Development:lib/tablelib.php
Here is my suggested patch to add to the tablelib api. It won't affect existing code using the tablelib api as it is. The patch is for the latest Moodle 1.9 stable branch.
test.php is a complete script with a basic usage example as explained in the docs page whose link is above.
overviewreportusingnewtablelib.txt is a patch to apply to 1.9 stable branch that shows the new api in use in the quiz overview report.
Hi Jamie,
I've been looking docs and patch. And here there are some comments: - Architectural: I think it's better to make all the "download" stuff to be supported within flexitable itself, so any flexitable should be able to: - flexitable->downloadable(array(TABLE_D_ODS, TABLE_D_XLS, TABLE_D_XML, TABLE_D_TXT) (setting the "is_downloadeable" attribute). Defaults to null or empty array. - optionaly: flexitable->show_download_buttons_at(array(TABLE_P_TOP, TABLE_P_BOTTOM), to specify where the buttons should be showed, default to TABLE_P_TOP IMO. - flexitable->print_html() should show dynamically the buttons (hopefully one div, no table) respecting previous settings. - flexitable->is_downloading(optional_param('download', '', PARAM_ALPHA)) can be used to set the downloading status, and also to request current status if called without param. - flexitable->download() should perform all the download, calling download_XXX() methods, one for each supported format. And then, with download support in flexitable... I'd implement your ultra-cool simple_table, perhaps calling it sql_table or database_table, only with the bits necessary to work against one query. I know it's one radical change from your proposed patch, but I think we should separate downloads (core function) and "suppliers of information" (array of data, sql...). Just my opinion. Ciao :-) Of course, I missed that, you ideas (and the implementation) looks quite interesting! Thanks!
(in fact that's the cause I proposed the alternative way in my previous message, because both downloads and sql are two ultra-cool extensions for the tablelib stuff). Re-thanks! Ciao :-) Thanks for your comments Eloy. I like your ideas. Am willing to refactor the code I've written when we have some agreement of what the api should be like.
Refactoring tablelib.php so much of it's functionality like paging, sorting by column etc. work with data sources other than one sql query will be quite a lot of work but I agree that putting the download code into the flexible_table class makes sense. Then at some later date someone else might agree to refactor tablelib so that it could work well with other data sources. Thought more about Eloy's comments and think that :
* it would be inefficient to implement a method "download()" which would be an alternative to print_html. The problem is that this would entail always keeping a copy of all data added to the table. There would necessarily be a method add_data as there is now. The data added to the table would have to be stored in an array. If download is called after adding all the data to the table then another for loop would have to write these values into the data structure the export plug in would need. * we have already told the table we are downloading by calling is_downloading(). Why do we have to call the download method to tell the table again that we are downloading. Instead add_data should do the correct thing with the data depending on what format we set the is_downloading flag to. So I think that we should probably do a download as follows : //start_output is always called. If this is not a download, it does nothing. $table->start_output($filename, $sheettitle); //add data to table/export here with a for loop for ($data as $datarow){ //process data here //then : $table->add_data_keyed($datatoaddtotable); } $table->finish_output(); In the case of the export formats to spreadsheet or to CSV we will probably output the data as we go along, instead of keeping a copy in memory. The old table class currently keeps a copy of the data in memory and then constructs the html from the internal array when you call print_html. We could call print_html from the finish_output function. I would propose we have a new plugin folder called lib/tableexport/{tableexportformats}/ these would contain folders with files of a sub class of table_export_format that would have methods add_data, start_output and finish_output Instead of is_downloading() calls to check whether to output html or non-html when formatting rows of data we would call $table->wants_html() there would be a wants_html() method on the tableexport plug in classes which would be set to return false on the default parent class. Uploaded new test.php which works with new tablelib.php patch which I'm about to upload.
My new patch for tablelib takes into account Eloy's comments above. Seperated out functionality download export types into seperate classes.
Data is now added straight to either the spreadsheet or output to the buffer rather than keeping a redundant copy of data in an array inside the table object as this might consume significant amounts of extra memory for large downloadable tables. The existing flexible_table API is still supported. This is a patch for the overview report that uses the new tablelib.
Looks pretty cool to me and fairly easy to understand. My +1 for HEAD iff Eloy agrees.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Sub-task
Resolved
Minor
