Code review comment for lp:~donadigo/pantheon-files/filechooser-module

Revision history for this message
Jeremy Wootten (jeremywootten) wrote :

(1) On compiling the following warning was given:

"Access to static member `CustomFileChooserDialog.chooser' with an instance reference:
     dialog_new.chooser.set_current_folder dialog.get_current_folder ());"

(2) Both Scratch and Audience gave the same error message:

"[_LOG_LEVEL_FATAL 12:39:37.465513] gee_array_list_real_slice: assertion '_tmp3_ <= _tmp4_' failed"

when changing the directory with the pathbar while opening a file. Not sure whether it is due to this branch tho'.

(3) The variable MOULDES_LIBDIR is presumably a mispelling of MODULES_LIBDIR? or MODULE_LIBDIR? It occurs in diff lines 228 and 250.

(4) Is it necessary to duplicate code and filenames in the filechooser-module directory? There are now three files named LocationBar.vala two of which duplicate code. The filechooser-module/AbstractLocationBar.vala duplicates code from /libwidgets/LocationBar.vala. filechooser-module/BreadcrumbsElements.vala duplicates code from libwidgets/BreadcrumbsElements. If this can be avoided it would make maintenance easier.

(5) diff lines 379 & following are indented 8 spaces instead of 4.

(6) Try to avoid wrapping caused by too long lines by vertically aligning parameters (e.g. diff lines 106, 167,168,290,292) (some of these may have been pre-existing in the code duplicated). Elementary code style gives a hard maximum line length of 120 characters, preferably not more than 80.

(7) The deep nesting in remove_gtk_widgets should be avoided if possible. Above 4 or 5 levels becomes hard to follow and the lines become too long.

(8) Please remove the tabs in FileChooserDialog.vala - all indents should be spaces.

(9) The "Create folder" button needs a tooltip.

(10) Formatting of diff lines 897-899 looks odd.

(11) If you change directory with the pathbar then you have to click the back button twice before the directory changes back.

I note that the problem of variable filechooser dialogs is not completely solved by this branch - for example LibreOffice and Geany still show different filechoosers.

review: Needs Fixing

« Back to merge proposal