Comment 10 for bug 473205

Revision history for this message
Steven Sheehy (steven-sheehy) wrote :

Pretty good first attempt. Here's some gentle comments:

- The code looks pretty generalized, so it's not tied specifically to Hub which is good. This means this code could be abstracted out completely to TreeView. The only thing needed in Hub would be to add the copy sub menu to the regular popup menu (no glade needed). I would suggest calling some method like TreeView::addCopyMenu_gui(GtkWidget *menu) from Hub which would append a Copy menu item and its corresponding dynamically generated sub menu
- Moving to TreeView would also allow you to take advantage of the ColMap to iterate over
- I don't think you need TreeView::getFormattedText. TreeView::getString will automatically format the cell data into the correct text form. Unless you don't want the formatted text but you wanted the non-formatted text. For example, you don't want "3.36 GiB" you want the actual bytes 3608079887. Most of the times you want the formatted text.
- Instead of "Copy Row" it's probably better to put "All". Copy is implied by the menu title.
- The column separator should probably be configurable. Or at least a constant class variable so it's more easily changed.
- Use G_DIR_SEPARATOR for '\n'
- You might be able to abstract out the similar portions of onCopyDataItemClicked and onCopyRowClicked into a separate method.
- Use tabs only for indentation. I see some spaces mixed in.