review reply part three, this concludes the core review
> > Index: desktop/tree.h
> > +/* Tree flags */
> > +#define TREE_NO_FLAGS 0
> > +#define TREE_NO_DRAGS 1
> > +#define TREE_NO_FURNITURE 2
> > +#define TREE_SINGLE_SELECT 4
> > +#define TREE_NO_SELECT 8
> > +#define TREE_MOVABLE 16
> > +/* if the last child of a directory is deleted the directory will be
> > deleted
> > + too */
> > +#define TREE_DELETE_EMPTY_DIRS 16
>
> It'd be nice if these flags were an enum too.
/* Tree flags */
enum tree_flags {
TREE_NO_FLAGS = 0,
TREE_NO_DRAGS = 1,
TREE_NO_FURNITURE = 2,
TREE_SINGLE_SELECT = 4,
TREE_NO_SELECT = 8,
TREE_MOVABLE = 16,
TREE_DELETE_EMPTY_DIRS = 32, /**< if the last child of a
* directory is deleted the
* directory will be deleted
* too.
*/
};
>
> > +/* the flag for the first node_element in every node, all other flags
> > should
> > + be different than this one */
> > +#define TREE_ELEMENT_TITLE 0x00
>
> How can zero be a flag?!
/** A "flag" value to indicate the element data contains title
* text. This value should be the first node_element in every
* node. All other values should be different than this one. The term
* flag is misused as it is actually a value used by the API consumer
* to indicate teh type of data a node element contains.
*/
#define TREE_ELEMENT_TITLE 0x00
>
> > +/* these should be defined in front end code */
> > +extern const char tree_directory_icon_name[];
> > +extern const char tree_content_icon_name[];
>
> Let's have a function for the frontend:
>
sorry I dont get what you mean?
> > typedef enum {
> > + NODE_ELEMENT_TEXT, /* Text only */
> > + NODE_ELEMENT_TEXT_PLUS_ICON, /* Text and icon */
> > + NODE_ELEMENT_BITMAP /* Bitmap only */
> > } node_element_type;
>
> Not doccommented?
>
changed
> > +typedef enum {
> > + NODE_DELETE_ELEMENT_TXT, /* The text of an element of the node
> > + is being deleted */
> > + NODE_DELETE_ELEMENT_IMG, /* The bitmap or icon of a node is being
> > + deleted */
> > + NODE_LAUNCH, /* The node has been launched */
> > + NODE_ELEMENT_EDIT_FINISHING, /* New text has to be accepted or
> > + rejected */
> > + NODE_ELEMENT_EDIT_FINISHED /* Editing of a node_element has been
> > + finished */
> > +} node_msg;
>
> Ditto
done
>
> > +typedef enum {
> > + NODE_CALLBACK_HANDLED,
> > + NODE_CALLBACK_NOT_HANDLED,
> > + NODE_CALLBACK_REJECT, /* reject new text for node element and
> > + leave editing mode */
> > + NODE_CALLBACK_CONTINUE /* don't leave editig mode */
> > +} node_callback_resp;
>
> Ditto
>
ditto
> > +struct node_msg_data {
> > + node_msg msg;
> > + unsigned int flag;
> > + struct node *node;
> > + union {
> > + char *text;
> > + void *bitmap;
> > + } data;
> > };
>
> Ditto.
>
done
> > +struct treeview_table {
> > + void (*redraw_request)(int x, int y, int width, int height,
> > + void *data);
> > + void (*resized)(struct tree *tree, int width, int height,
> > + void *data);
> > + void (*scroll_visible)(int y, int height, void *data);
> > + void (*get_window_dimensions)(int *width, int *height, void *data);
> > };
>
> Ditto
>
done
> > Index: desktop/options.c
> > + { "tree_icons_dir", OPTION_STRING, &option_tree_icons_dir },
>
> Why is this an option?! Surely it's the job of the frontend to decide where
> these resources are?
>
dont know, require futher discussion/decision as to how to change it
> > Index: content/urldb.c
> > +void urldb_iterate_cookies(bool (*callback)(const struct cookie_data
> > *data))
>
> Typedef for that callback type *please*
>
> > + bool (*cookie_callback)(const struct cookie_data *data))
>
> Although I see it's not the style in this file :-(
>
it is not, urldb needs a serious refactor all on its own so i shall
refrain untill that happens.
> > // LOG(("%p: %s=%s", c, c->name, c->value));
>
> No C++ style comments please. This is core code.
>
all removed
--
Regards Vincent
http://www.kyllikki.org/
signature.asc
Description: Digital signature
