Rob Kendrick wrote:
> OK, I've applied this, with a few modifications (nothing serious);
>
> Things I remain unsure about include these;
>
> + 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?
>
the idea is to produce a blank page that is properly drawn, rather than
'grey'/'beige', I'm looking to understand the core functions that could
achieve that, I'll put the comments in in future
> 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() ?
>
No reason, strdup() is better
> + 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.
>
the reason I create a combobox manually is that to set the selected
index according to its string value, as far as I know [unless I fit a
tree model into the combo box, that is even more involved/delicate!] I
need to create a gtk_combo_box_new_text() rather than a
gtk_combo_box_new(), then add the entries individually with
gtk_combo_box_append_text(), so I'm reading the entries from a file to
do so; glade won't let me do that, so in glade I simply make a vbox that
I know I'm going to add the combo box to; it's involved, you are right;
though it's simpler than the tree model. 58 is the place in the list for
"en" as a default unless a language was picked already, then matched
during the loading of the list into the combo box; 6 is the length of
the longest language string [including +1 for '\0'] as it stands: eg
"en-gb";
the parsing may be suboptimal, the objective is I basically need to read
the individual language strings, compare every language string with
option_accept_language, remember its place in the list in case of a
match, add every language string to the combo box in turn
> 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 :)
>
I could change it :-D an individual address for every purpose seems to
work well though
> 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 :)
>
It looks to me as though gtk_source.c, gtk_source.h need adding to the
repository? My repository installation won't compile now as there is a
reference to gtk_source.c in Makefile.sources
> B.
>
Best
Mark
http://www.halloit.com
Key ID 046B65CF
