openerp-dev-web team mailing list archive

["Xavier \(Open ERP\)" <xmo@xxxxxxxxxxx>]

Review: Needs Fixing
== Looks and behavior of feature
* Big borders in Webkit for custom columns box (fieldset has borders)

* Why are [SEARCH] and [CLEAR] gone off to the right? They're the most important buttons in the filter view

* Style of custom columns group completely different than other groups (group by, extended filters, filters)

* Why don't the FILTERS group use the same implementation as the other groups (group by & extended filters)?

* Too much space between the [-] button and the field's label, fix (colspans?)

== Potential improvements (to discuss)
* Rename [Custom Columns] to [Hide Columns]

* Why not a button on the column itself (like a [x] to close the column) and replace [CUSTOM COLUMNS] by [RESET COLUMNS] or something like that, and it would re-enable all columns?

== Peripheral stuff to maybe fix
* What's with the [Actions] in the dropdown? Can we have actions there still? Why not remove it?

== Tech
I'm sure it took you some time to develop this, you can create more than one big commit. More smaller commits is usually (though not always) better: better commit messages, more self-contained, easier to review, ... (that means you don't have to create a single commit with fixes to all remarks you think are worth fixing for instance, though you don't have to create a commit for each either). Commits are cheap.

revid:sma@xxxxxxxxxxx-20100818103751-047wau16bd23qr1f, search.py
* All_values does not match naming conventions. `all` should be lowercase

* stray whitespace at end of `for key, val in v.items():` line (and in a bunch of others places in that commit and the next)

* get_domain extraction is nice, but there is  no need to accumulate: all cases are exclusive, just return the values already, there is no need for the tempdomain temporary variable

* Within get_domain's len == 3 and len == 4, tuple unpacking would probably make function more readable. Also, string formats and double quotes (to avoid having to escape single quote in value being created).

* the tmp_domain accumulator should be a list which is joined at the end.

* surely there is a better way than creating sequences of [stuff] and then doing string replacement of ][, [ and ] no?

revid:sma@xxxxxxxxxxx-20100818103751-047wau16bd23qr1f, search.js
* Wasn't always done before, but try to prefix names of variables containing jQuery objects with $ to differentiate from raw DOM nodes or nodelists (gotten from native DOM methods or MochiKit): filter_opt_tbl or the old filter_table should be $filter_opt_tbl and $filter_table. Several instances of this in the code/patch

* Chain jquery methods (maybe not too much, but it's usually clear and readable) for example lines
    jQuery('label#filterlabel').text(jQuery('select.filter_fields_and option:selected').text())
    jQuery('label#filterlabel').attr('value', jQuery(elem).val())
  can be chained, no need to fetch 'label#filterlabel' twice. Same under with new_tr manipulations. Just be careful with line breaks.

* If you have IDs, you shouldn't need element name prefixes. IDs should be unique in the page. So `#filterlabel` not `label#filterlabel`

* don't define variables twice (`position_tr` has a second `var` in condition, also it should be $-prefixed as it's a jQuery object)

* Also try to use consistent indentation (all spaces, 4 spaces per level), to remove stray line-ending whitespace (use bzr cdiff --check-style to verify) and don't forget the `;` at the end of JS lines (in $curr_body.find.each callback, there are at least 4 lines with no `;`)

* Try to avoid nesting too much, and use return guards: 
    function(i, v) {
        var theValue = jQuery(v).text();
        if (theValue == jQuery('select.filter_fields_and option:selected').text()){                          
            index_row = i
            new_tr.find('select.expr').hide()
            new_tr.find('label#filterlabel').hide()	           
            new_tr.find('label.and_or').remove()
	            
            var select_andor = jQuery('<label>', {'class': 'and_or'}).text('OR');               
            select_andor.insertBefore(new_tr.find('input.qstring'));              
	        }
        }
    should be:

    function(i, v) {
        var theValue = jQuery(v).text();
        if (theValue != jQuery('select.filter_fields_and option:selected').text()) { return; }

        index_row = i
        new_tr.find('select.expr').hide()
        new_tr.find('label#filterlabel').hide()	           
        new_tr.find('label.and_or').remove()
	            
        var select_andor = jQuery('<label>', {'class': 'and_or'}).text('OR');               
        select_andor.insertBefore(new_tr.find('input.qstring'));              
	        }
        }
    and there are unneeded temporary variables again.

* Be careful with globals, check if your editor/IDE can check them for you, also maybe enable Firefox's Strict mode in firebug (in switch_searchView there is a `i` used without `var` in a loop, so it's a global)

* Why isn't tbodys a jQuery object?

* In e.g. remove_filter_row you unwrap jquery objects, select one and rewrap immediately. Should be no need to e.g. `jQuery($paren.children('tbody')[t.index()-1])`  can be written `$paren.children('tbody').eq(t.index() - 1)` no?

* Use the right methods e.g. rather than `jQuery(this).find('> .filter_row_class');` is it not possible to use ` jQuery(this).children('.filter_row_class');`? That would do the same thing no? It's more readable and probably jQuery is better optimized to boot.

* Also don't split selectors when no need to, a few lines lower there is `var children = jQuery('#filter_option_table tbody').find('> .filter_row_class')` why is it not `var children = jQuery('#filter_option_table tbody > .filter_row_class')`?

* Don't needlessly rewrap in jQuery either: if $curr_body is a jQuery object, $curr_body.find('> .filter_row_class').eq(trs_keys[index]) is a jQuery object, no need to call jQuery() on it

revid:sma@xxxxxxxxxxx-20100819050036-vy8vfq5pygqfm6ux, search.js
* Use litteral collections, `[]` not `new Array()`

* Also why is custom_columns a global variable?

* jQuery objects have a length, no need for `jQuery('#custom_columns input:not(:checked)').get().length`

* For creating c_columns, why not use jQuery().map().get()?

* Also why does first condition return custom_columns as a string and second one as an array?

* And why initialize custom_columns above function when that initialization is never used?

same revision, listgrid.py
* Why reindent the whole function (and add one level) instead of just using a guard and returning early?

* Also use `not in` operator, `a not in b` is clearer than `not a in b`.

* Not new code, but dict.get returns None by default, so in attrs.get('widget', False) the False is redundant since we're just using for boolean test. Just `attrs.get('widget')` is enough

Same revision, viewform.mako

* 
        % if screen.view_type == 'tree':
        % if screen.widget:
    why two different tests, why not just one with an `and` between?
-- 
https://code.launchpad.net/~openerp-dev/openobject-client-web/improved-custom-filters/+merge/32975
Your team OpenERP R&D Team is subscribed to branch lp:~openerp-dev/openobject-client-web/improved-custom-filters.