On Mon, 20 Apr 2009 09:23:14 +0100 Rob Kendrick <[email protected]> wrote:
> On Mon, 20 Apr 2009 09:53:36 +0200 > Mark <[email protected]> wrote: > > > I did see talk of me 'cleaning up' the code so I've improved the > > presentation a bit, clearing the unreached functions, [I suppose > > there are core functions that should handle printing layout after > > all, it's simply a case of locating them] adding copyright notices > > to gtk_source.c, gtk_source.h, preparing I18N support for the save > > source messages, changing the wrapping in places as I had set my > > tabs to width 4, then noticed it makes a difference after all :-) > > Excellent. I'll try to take a look at this today. OK, I've applied this, with a few modifications (nothing serious); * Fixed glade additions to use capital letters where appropriate * Remove commented-out code. You should either remove this code before checking in/submitting, or leave a comment explaining why not. * We prefer /* C-style */ vs. // C++ style comments. While both are supported by C99, and we all love C99, we notionally aim towards C89-compliance as it's more available on more niche systems, although it's not essential for the GTK port. * Additionally on C89, avoid statements before local declarations (see MENUHANDLER open_link_in_background_tab, for example) * Our source code standard has the 'case' of 'switch' statements at the same level. Also, no space before the colon. * When setting a default value for an enum type (for example, GtkPrintOperationResult), don't just set it to zero; set it to one of the enum's members. (I've changed this to be set to its error state). * My personal taste is to avoid the use of ! for checking for zero or NULL, and to be explicit. ie, no "if (!foo)", use one of "(foo == 0)", "(foo == NULL)", or "(foo == false)" etc; this goes a long way to making your intentions clearer. Others may disagree with my taste, of course :) * Remember that GTK tends to use gboolean and not bool. The difference is subtle, but most importantly it's TRUE vs true. (nsgtk_scaffolding_update_link_operations_sensitivity; woah, a long one!) Things I remain unsure about include these; -void nsgtk_tab_add(struct gui_window *window) +void nsgtk_tab_add(struct gui_window *window, bool background) { - GtkNotebook *tabs = nsgtk_scaffolding_get_notebook(window); + GtkWidget *tabs = GTK_WIDGET(nsgtk_scaffolding_get_notebook(window)); GtkWidget *tabBox = nsgtk_tab_label_setup(window); - - gtk_notebook_append_page(tabs, - GTK_WIDGET(window->scrolledwindow), - tabBox); - + gint remember = gtk_notebook_get_current_page(GTK_NOTEBOOK(tabs)); + gtk_notebook_append_page(GTK_NOTEBOOK(tabs), + GTK_WIDGET(window->scrolledwindow), tabBox); + /*causes gtk errors can't set a parent + gtk_notebook_set_tab_reorderable(GTK_NOTEBOOK(tabs), + GTK_WIDGET(window->scrolledwindow), true); */ gtk_widget_show_all(GTK_WIDGET(window->scrolledwindow)); - - gtk_notebook_set_current_page(tabs, gtk_notebook_get_n_pages(tabs) - 1); + gtk_notebook_set_current_page(GTK_NOTEBOOK(tabs), + gtk_notebook_get_n_pages(GTK_NOTEBOOK(tabs)) - 1); + if (option_new_blank) { + /*char *blankpage = malloc(strlen(res_dir_location) + + SLEN("file:///blankpage") + 1); + blankpage = g_strconcat("file:///", res_dir_location, + "blankpage", NULL); */ + /* segfaults + struct browser_window *bw = nsgtk_get_browser_for_gui(window); + browser_window_go(bw, blankpage, 0, true); */ + /* free(blankpage); */ + } + if (background) + gtk_notebook_set_current_page(GTK_NOTEBOOK(tabs), remember); + gtk_widget_grab_focus(GTK_WIDGET(window->scaffold->url_bar)); } This function appears to be mostly commented-out and empty if; can we have comments about why in the sources? This is from ngtk_options_load(); + if (!option_accept_language) { + option_accept_language = (char *) malloc(3); + strcpy(option_accept_language, "en"); + } Any reason for not just using strdup() ? + GtkVBox *combolanguagevbox = GTK_VBOX(glade_xml_get_widget(gladeFile, "combolanguagevbox")); + comboLanguage = gtk_combo_box_new_text(); + gchar * languagefile = g_strconcat(res_dir_location, "languages", NULL); + FILE * f; + char * in = malloc(6); + char temp; + temp = 1; + int temprowcount = 0; + int final = 58; + f = fopen(languagefile, "r"); + int i = 0; + while ((temp != '\0') && (fread(&temp, 1, 1, f))) { + if (temp == '\n') { + in[i] = '\0'; + i = 0; + gtk_combo_box_append_text(GTK_COMBO_BOX(comboLanguage), in); + if (strcmp(in, option_accept_language) == 0) { + final = temprowcount; + } else { + temprowcount++; + } + } else { + in[i++] = temp; + } + } + fclose(f); + free(in); + gtk_combo_box_set_active(GTK_COMBO_BOX(comboLanguage), final); + gtk_widget_set_tooltip_text(GTK_WIDGET(comboLanguage), + "set preferred language for web pages"); + gtk_box_pack_start(GTK_BOX(combolanguagevbox), comboLanguage, FALSE, FALSE, 0); + gtk_widget_show(comboLanguage); I have no idea what's going on here; but it could do with refactoring, I feel! Firstly, trivialness: the * should be next to the variable name, and variables are defined mid-scope. It then does all sorts of weird parsing for a file format that doesn't appear documented. The magic numbers (6, 58) make me uneasy. temp should have a meaningful name. Also, I think having the combobox in the glade file to start with, rather than creating a new one and inserting it into an empty VBox would be all together less faff. I think this would be best all wrapped up into a function of its own, too. nsgtk_on_source_save_as_activate() looks like it could do with some sprintf love. + * Copyright 2009 Mark Benjamin <[email protected]> Interesting email address :) As for the local history toolbar icon, I'm not happy with its visual appearance the moment; it looks like it is trying to emulate other browser's hold-down functionality with the down arrow. Unfortunately, it doesn't pull it off. I've simply made the button invisible for now, until a more suitable icon can be found for it. (I'm sure GTK had a stock History icon, but now I can't find it.) If you could test that I've not broken anything when checking this in, that'd be good :) B.
