From: Lincoln M. <ljm...@ho...> - 2004-02-04 18:10:01
|
Jeroen Latour wrote: > On Tue, 03 Feb 2004 15:21:33 -0700, Lincoln Maskey > <ljm...@ho...> wrote: > >> The following patch replaces the filtering code in >> print_all_bug_page.php, >> print_all_bug_page_excel.php and print_all_bug_page_word.php with a >> call to filter_get_bug_rows(...) >> like the view_all_bug_page.php code. It removes a lot of redundant code. >> >> It also moves the code that draws the filter section in the view bug >> and print bug pages into a >> separate function so that they both can be updated from the one spot. >> >> Apologies if the patch isn't in the correct format: I haven't made >> many before... > > > I can't get it to apply properly even after quite some editing, probably > because I'm copy/pasting from my e-mail client. Did you paste the patch > in your mail, or did my client/SF convert it? If you pasted it, could > you send it again as an attachment? If not, could you try some other > means of sending it, for example by opening an issue on the BTS? > > Apart from that: > - Could you rename draw_filter_selection_area to filter_draw_selection_area > - I'm not sure about so many presentation code being in the APIs, > instead of an include file. Any opinions? > - We prefix function parameters with p_, so $p_for_screen > - You're using a $f_ form parameter inside the function. I'd rather have > you passing that as a parameter > - Watch the code formatting: it's function( arg ), not function(arg) > > As I said, I'm not so sure about moving all that presentation code to > filter_api. Anyone? > > Jeroen I originally just pasted it into the body of the text, but this time have attached it separately. Hopefully this will work! I've done all of the suggested changes (well in theory: it's possible I missed one or two). Just out of curiosity, I (now) understand that p_ means "parameter" and f_ means "form", but what does t_ mean? The only thing I can see / think of is "touched"? I agree that filter_api is not necessarily the best spot for that function to end up but at the time I wrote it I couldn't think of a better existing file (or new file name) so I figured at least by putting it there it would make it easier for me to remember where it was. Thanks, Lincoln. |