GTK patches - fully merged, stable version

Mark markieb.lists.20090330 at gmail.com
Mon Apr 20 22:55:18 BST 2009


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 <netsurfbrowser.org.MarkBenjamin at dfgh.net>
>
> 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




More information about the netsurf-dev mailing list