Hi guys,
Sorry for the delay in this - it's been hectic recently. This review
overs up to a couple of revisions ago. The current head of Mike's
branch does not build - I suspect due to him forgetting to check some
stuff in from elsewhere in his tree, as there are function parameter
type mismatches.
My comments are proceeded with "*RJEK*" in order to aid your searching
for them. In general, great work. Most of my comments refer to source
code style (easily fixed) and possible memory leaks. I'd like to get
this functionality merged ASAP - it's generally excellent, but I'd like
to get the mentioned issues fixed first.
Additionally, the text selection changes are entirely over my head - can
somebody who knows that code look at what Mike's changed and comment?
Mike: In general, files that you have had a lot of input to or have
entirely rewritten lack your copyright at the top. You should do this.
If you'd like a @netsurf-browser.org address, poke me and I'll set up
a forwarder for you.
B.
| Index: gtk/options.h
| ===================================================================
This file all look fine.
| Index: gtk/gtk_window.c
| ===================================================================
All fine.
| Index: gtk/gtk_download.c
| ===================================================================
| --- gtk/gtk_download.c (revision 4636)
| +++ gtk/gtk_download.c (working copy)
| @@ -17,113 +17,452 @@
| */
|
| #include <gtk/gtk.h>
| +#include <glib/gstdio.h>
|
| #include "utils/log.h"
| -
| +#include "utils/utils.h"
| +#include "utils/url.h"
| +#include "utils/messages.h"
| +#include "content/fetch.h"
| +#include "desktop/gui.h"
| #include "gtk/gtk_gui.h"
| +#include "gtk/options.h"
| #include "gtk/gtk_download.h"
|
| -static GtkWindow *wndDownload;
| -static GtkTreeView *treeDownloads;
| -static GtkListStore *list_downloads;
| +#define GLADE_NAME "downloads.glade"
|
| -enum {
| - COLUMN_URL = 0,
| - COLUMN_DESTINATION,
| - COLUMN_PERCENTAGE,
| - COLUMN_DESCRIPTION,
| +static GtkWindow *window, *parent;
|
| - N_COLUMNS
| -};
| +static GtkTreeView *tree;
| +static GtkListStore *store;
| +static GtkTreeSelection *selection;
| +static GtkTreeIter iter;
| +static GList *buttons;
*RJEK* Things like "window", "parent" etc are quite common local
variable names; perhaps put a gtk_download_ prefix on the front of the
above statics to prevent confusion?
| -void nsgtk_downloadPause_clicked(GtkToolButton *button, gpointer data);
| +static gboolean nsgtk_download_hide (GtkWidget *window);
|
| -static void nsgtk_download_add(const char *url, const char *dest, int prog,
const char *desc)
| -{
| - GtkTreeIter iter;
| +static GtkTreeView *tree_view_new(GladeXML *gladeFile);
| +static void tree_view_row_activated(GtkTreeView *tree, GtkTreePath *path,
| + GtkTreeViewColumn *column, gpointer data);
| +
| +static void list_store_update_item(struct gui_download_window *dl);
| +static void list_store_create_item (struct gui_download_window *dl);
| +static void list_store_clear_item (struct gui_download_window *dl);
| +static void list_store_cancel_item (struct gui_download_window *dl);
| +
| +static void selection_do(GtkWidget *button, selection_action action);
| +
| +static void sensitivity_set(struct gui_download_window *dl,
| + Actions sensitivity);
| +static void sensitivity_selection_changed (GtkTreeSelection *selection);
| +static void sensitivity_update_buttons (Actions sensitivity);
| +
| +static gchar* dialog_show (gchar *filename, gchar *domain, gchar *size);
| +static gchar* url_parse_domain (const gchar *url);
| +static gchar* info_to_string (struct gui_download_window *dl);
| +static gboolean handle_error (GError *error);
| +
*RJEK* All of these static functions should certainly have a
gtk_download_ prefix on them: it helps locate the function when
bad/incomplete debugging data comes back to us.
| - gtk_list_store_append(list_downloads, &iter);
| - gtk_list_store_set(list_downloads, &iter,
| - COLUMN_URL, url,
| - COLUMN_DESTINATION, dest,
| - COLUMN_PERCENTAGE, prog,
| - COLUMN_DESCRIPTION, desc,
| - -1);
| -}
|
| -void nsgtk_download_initialise(void)
| +void nsgtk_download_init(GtkWindow *parent_window)
| {
| - wndDownload = GTK_WINDOW(glade_xml_get_widget(gladeWindows,
| - "wndDownloads"));
| - treeDownloads = GTK_TREE_VIEW(glade_xml_get_widget(gladeWindows,
| - "treeDownloads"));
| + char *glade_location = g_strconcat(res_dir_location, GLADE_NAME, NULL);
*RJEK* Is there a reason for using this rather than the usual strcat?
If so, shouldn't glade_location be a gchar *? Also, where is the memory
that g_strconcat allocate freed? (I don't think it is needed after this
function has returned)
| + GladeXML *gladeFile = glade_xml_new(glade_location, NULL, NULL);
|
| - gtk_tool_item_set_expand(GTK_TOOL_ITEM(
| - glade_xml_get_widget(gladeWindows, "toolProgress")), TRUE);
| + window = GTK_WINDOW(glade_xml_get_widget(gladeFile, "wndDownloads"));
| + parent = parent_window;
| + gtk_window_set_transient_for(GTK_WINDOW(window), parent);
| +
| + tree = tree_view_new(gladeFile);
|
| - list_downloads = gtk_list_store_new(N_COLUMNS,
| - G_TYPE_STRING, /* URL */
| - G_TYPE_STRING, /* Destination */
| + store = gtk_list_store_new(N_COLUMNS,
| + G_TYPE_STRING, /* Description */
| G_TYPE_INT, /* % complete */
| - G_TYPE_STRING /* time left */
| + G_TYPE_STRING, /* Status */
| + G_TYPE_STRING, /* Speed */
| + G_TYPE_POINTER /* Download structure */
| );
|
| - gtk_tree_view_set_model(treeDownloads, GTK_TREE_MODEL(list_downloads));
| + gtk_tree_view_set_model(tree, GTK_TREE_MODEL(store));
| + g_object_unref(store);
|
| - gtk_tree_view_insert_column_with_attributes(treeDownloads, -1,
| - "URL",
| - gtk_cell_renderer_text_new(),
| - "ellipsize",
PANGO_ELLIPSIZE_START,
| - "text",
| - COLUMN_URL,
| - NULL);
| + selection = gtk_tree_view_get_selection(tree);
| + gtk_tree_selection_set_mode(selection, GTK_SELECTION_MULTIPLE);
|
| -/* gtk_tree_view_insert_column_with_attributes(treeDownloads, -1,
| - "Destination",
| - gtk_cell_renderer_text_new(),
| - "text",
| - COLUMN_DESTINATION,
| - NULL);*/
*RJEK* If this commented-out code is unneeded, can we remove it? I'd
rather not have comments like this hanging around with no explanation.
If we need it back, we have it in Subversion :)
| + buttons = glade_xml_get_widget_prefix(gladeFile, "button");
| + g_signal_connect(G_OBJECT(selection), "changed",
| + G_CALLBACK(sensitivity_selection_changed), NULL);
| + g_signal_connect(G_OBJECT(window), "delete-event",
G_CALLBACK(nsgtk_download_hide),
| + NULL);
| + g_signal_connect(glade_xml_get_widget(gladeFile, "buttonClear"),
| + "clicked", G_CALLBACK(selection_do),
| + list_store_clear_item);
| + g_signal_connect(glade_xml_get_widget(gladeFile, "buttonCancel"),
| + "clicked", G_CALLBACK(selection_do),
| + list_store_cancel_item);
| + g_signal_connect(tree, "row-activated",
| + G_CALLBACK(tree_view_row_activated), NULL);
| +}
| +
| +void nsgtk_download_show()
| +{
| + gtk_widget_show_all(GTK_WIDGET(window));
| +}
| +
| +gboolean nsgtk_download_hide (GtkWidget *window){
| + gtk_widget_hide(window);
| + return TRUE;
| +}
*RJEK* Style. The open brace should be on a new line.
| +struct gui_download_window *gui_download_window_create(const char *url,
| + const char *mime_type, struct fetch *fetch,
| + unsigned int total_size)
| +{
| + gchar *filename;
| + gchar *domain = url_parse_domain(url);
| + gchar *size = human_friendly_bytesize(total_size);
| + gchar *destination;
| + struct gui_download_window *download;
|
| - gtk_tree_view_insert_column_with_attributes(treeDownloads, -1,
| - "Progress",
| -
gtk_cell_renderer_progress_new(),
| - "value",
| - COLUMN_PERCENTAGE,
| - NULL);
| -
| - gtk_tree_view_insert_column_with_attributes(treeDownloads, -1,
| - "Details",
| - gtk_cell_renderer_text_new(),
| - "text",
| - COLUMN_DESCRIPTION,
| - NULL);
*RJEK* The strings "Progress" and "Details" need to be internationalised
here. Look in the Messages file to see if there's anything suitable?
We may need to add a namespace in the Messages file for thigns
specific to the GTK port, I fear.
| + if (url_nice(url, &filename, false) != URL_FUNC_OK)
| + strcpy(filename, messages_get("SaveObject"));
| +
| + destination = dialog_show(filename, domain, size);
| + if (destination == NULL)
| + return NULL;
| +
| + download = malloc(sizeof *download);
| + download->fetch = fetch;
| + download->name = g_string_new(filename);
| + download->time_left = g_string_new("");
| + download->size_total = total_size;
| + download->size_downloaded = 0;
| + download->speed = 0;
| + download->filename = destination;
| + download->status = NULL;
| + download->progress = 0;
| + download->timer = g_timer_new();
| + download->error = NULL;
| + download->write = g_io_channel_new_file(destination, "w",
| + &download->error);
*RJEK* Does the open mode require a 'b' for binary mode? The GLib
documentation is quite ambiguous. (After all, it's essential that
you open with 'b' under Windows, for example.)
| + if (handle_error(download->error))
| + return NULL;
*RJEK* Memory leak: download is not freed on this error condition.
| +
| + if (!g_str_has_prefix(mime_type, "text"))
| + g_io_channel_set_encoding(download->write, NULL,
&download->error);
*RJEK* I prefer explicit if conditions. Perhaps drop ! and add
a "== FALSE" ?
| +
| + /* Add the new row and store the reference to it (which keeps track of
| + * the tree changes) */
| + gtk_list_store_append (store, &iter);
*RJEK* Style. Remove the space between function name and option bracket.
| + download->row = gtk_tree_row_reference_new(GTK_TREE_MODEL(store),
| + gtk_tree_model_get_path(GTK_TREE_MODEL(store), &iter));
| +
| + sensitivity_set(download, CANCEL);
| +
| + list_store_create_item(download);
| + nsgtk_download_show();
| +
| + return download;
| +}
|
| - /* add some fake entries to play about with */
| -
nsgtk_download_add("http://www.netsurf-browser.org/downloads/netsurf-1.0.zip";,
| - "/home/rjek/Downloads/netsurf-1.0.zip",
| - 23,
| - "500kB of 2MB, 120kB/sec, 12 seconds");
| -
| - nsgtk_download_add("http://www.rjek.com/gniggle.zip";,
| - "/home/rjek/Downloads/gniggle.zip",
| - 68,
| - "20kB of 90kB, 63kB/sec, 2 seconds");
| +
| +void gui_download_window_data(struct gui_download_window *dw, const char
*data,
| + unsigned int size)
| +{
| + g_io_channel_write_chars(dw->write, data, size, NULL, &dw->error);
| + if (dw->error != NULL) {
| + dw->status = "Error";
*RJEK* i18n.
| + dw->speed = 0;
| + dw->sensitivity = CLEAR;
| + list_store_update_item(dw);
| + fetch_abort(dw->fetch);
| +
| + gtk_window_present(window);
| + return;
| + }
| +
| + dw->size_downloaded += size;
| + gdouble elapsed = g_timer_elapsed(dw->timer, NULL);
|
| - nsgtk_download_add("http://www.whopper.com/biggy.iso";,
| - "/home/rjek/Downlaods/biggy.iso",
| - 2,
| - "2MB of 1923MB, 3kB/sec, 20 hours");
| + /* If enough time has gone by, update the window */
| + if (elapsed - dw->last_update > .5 && GTK_WIDGET_VISIBLE(window))
| + {
| + dw->speed = (gdouble)dw->size_downloaded/elapsed;
| + dw->progress = (double)(dw->size_downloaded)/
| + (double)(dw->size_total)*100;
| + list_store_update_item(dw);
| +
| + dw->last_update = elapsed;
| + }
| +}
*RJEK* Do we need to use doubles, here? Floating point can be quite
pricey on small systems.
| +
| +void gui_download_window_error(struct gui_download_window *dw,
| + const char *error_msg)
| +{
| }
*RJEK* Should this function do something?
| -void nsgtk_download_show(void)
| +
| +void gui_download_window_done(struct gui_download_window *dw)
| {
| - gtk_widget_show_all(GTK_WIDGET(wndDownload));
| - gdk_window_raise(GTK_WIDGET(wndDownload)->window);
| + g_io_channel_shutdown(dw->write, TRUE, &dw->error);
| + g_io_channel_unref(dw->write);
| +
| + dw->speed = 0;
| + dw->progress = 100;
| + sensitivity_set(dw, CLEAR);
| + dw->status = "Complete";
| +
| + if (option_downloads_clear)
| + list_store_clear_item(dw);
| + else
| + list_store_update_item(dw);
| }
*RJEK* Who frees dw once it is finished with?
| -void nsgtk_downloadPause_clicked(GtkToolButton *button, gpointer data)
| +
| +GtkTreeView* tree_view_new(GladeXML *gladeFile)
| +{
| + GtkTreeView *treeview = GTK_TREE_VIEW(glade_xml_get_widget(gladeFile,
| + "treeDownloads"));
| + GtkTreeViewColumn *column;
| + GtkCellRenderer *renderer;
| +
| + /* Progress column */
| + renderer = gtk_cell_renderer_progress_new();
| + gtk_tree_view_insert_column_with_attributes (treeview, 0, "Progress",
| + renderer, "value", PROGRESS, "text", STATUS, NULL);
| +
| + /* Information column */
| + renderer = gtk_cell_renderer_text_new();
| + g_object_set(G_OBJECT(renderer), "wrap-mode", PANGO_WRAP_WORD_CHAR,
| + "wrap-width", 300, NULL);
| + gtk_tree_view_insert_column_with_attributes (treeview, 1, "Information",
| + renderer, "text", INFO, NULL);
| +
| + /* Speed column */
| + renderer = gtk_cell_renderer_text_new();
| + gtk_tree_view_insert_column_with_attributes (treeview, 2, "Speed",
| + renderer, "text", SPEED, NULL);
| +
| + return treeview;
| +}
*RJEK* i18n needed.
| +void tree_view_row_activated(GtkTreeView *tree, GtkTreePath *path,
| + GtkTreeViewColumn *column, gpointer data)
| {
| + GtkTreeModel *model;
| + GtkTreeIter iter;
|
| + model = gtk_tree_view_get_model(tree);
| +
| + if (gtk_tree_model_get_iter(model, &iter, path)){
| + /* TODO: This will be a context action (pause, start, clear) */
| + selection_do(NULL, list_store_clear_item);
| + }
*RJEK* Style: space before the open brace.
| }
| +
| +void list_store_update_item (struct gui_download_window *dl)
| +{
| + gchar *info = info_to_string(dl);
| + gchar *speed = strcat(human_friendly_bytesize(dl->speed), "/s");
*RJEK* This gives me the willies! Can you take a copy of the string
returned by human_friendly_bytesize and then concatenate onto that?
| + /* Updates iter (which is needed to set and get data) with the dl row */
| + gtk_tree_model_get_iter(GTK_TREE_MODEL(store), &iter,
| + gtk_tree_row_reference_get_path(dl->row));
| +
| + gtk_list_store_set (store, &iter, PROGRESS, dl->progress, INFO, info,
| + SPEED, dl->speed == 0 ? "-" : speed, -1);
*RJEK* Perhaps "stalled" instead of "-" ?
| + if (dl->status)
| + gtk_list_store_set (store, &iter, STATUS, dl->status);
*RJEK* Style: space after function name.
| +}
| +
| +void list_store_create_item (struct gui_download_window *dl)
| +{
| + list_store_update_item(dl);
| + /* The iter has already been updated to this row */
| + gtk_list_store_set (store, &iter, DOWNLOAD, dl, -1);
| +}
| +
| +void list_store_clear_item (struct gui_download_window *dl)
| +{
| + if (dl->sensitivity & CLEAR) {
| + gtk_tree_model_get_iter(GTK_TREE_MODEL(store), &iter,
| + gtk_tree_row_reference_get_path(dl->row));
| + gtk_list_store_remove(store, &iter);
| + g_free(dl);
| + }
| +}
| +
| +void list_store_cancel_item (struct gui_download_window *dl)
| +{
| + if (dl->sensitivity & CANCEL) {
| + dl->status = "Canceled";
*RJEK* i18n (and spelling :)
| + dl->speed = 0;
| + dl->progress = 0;
| + sensitivity_set(dl, CLEAR);
| +
| + if (dl->fetch)
| + fetch_abort(dl->fetch);
| +
| + g_unlink(dl->filename);
| +
| + list_store_update_item(dl);
| + }
| +}
| +
| +void selection_do(GtkWidget *button, selection_action action)
| +{
| + GList *rows, *dls = NULL;
| + GtkTreeModel *model = GTK_TREE_MODEL(store);
| +
| + rows = gtk_tree_selection_get_selected_rows(selection, &model);
| + while (rows != NULL) {
| + struct gui_download_window *dl;
| + gtk_tree_model_get_iter(GTK_TREE_MODEL(store), &iter,
| + (GtkTreePath*)rows->data);
| + gtk_tree_model_get(GTK_TREE_MODEL(store), &iter, DOWNLOAD,
| + &dl, -1);
| + dls = g_list_prepend(dls, dl);
| +
| + rows = rows->next;
| + }
| + g_list_foreach(dls, (GFunc)action, NULL);
| +
| + g_list_foreach (rows, (GFunc)gtk_tree_path_free, NULL);
| + g_list_foreach (rows, (GFunc)g_free, NULL);
| + g_list_free (rows);
| + g_list_free (dls);
*RJEK* Style. Remove spaces after function names.
| +}
| +
| +void sensitivity_set(struct gui_download_window *dl, Actions sensitivity)
| +{
| + dl->sensitivity = sensitivity;
| + if (gtk_tree_selection_path_is_selected(selection,
| + gtk_tree_row_reference_get_path(dl->row)))
| + sensitivity_update_buttons(sensitivity);
| +
| +}
| +
| +void sensitivity_selection_changed (GtkTreeSelection *selection)
| +{
| + GtkTreeModel *model = GTK_TREE_MODEL(store);
| + GtkTreeIter iter;
| + GList *rows;
| + Actions sensitivity = 0;
| +
| + rows = gtk_tree_selection_get_selected_rows(selection, &model);
| + while (rows != NULL){
*RJEK* Style. Space before brace.
| + struct gui_download_window *dl;
| + gtk_tree_model_get_iter(model, &iter, (GtkTreePath*)rows->data);
| + gtk_tree_model_get(model, &iter, DOWNLOAD, &dl, -1);
| + sensitivity |= dl->sensitivity;
| + rows = rows->next;
| + }
| +
| + sensitivity_update_buttons(sensitivity);
| +}
| +
| +void sensitivity_update_buttons(Actions sensitivity)
| +{
| + /* Glade seems to pack the buttons in an arbitrary order */
| + enum { PAUSE_BUTTON, CLEAR_BUTTON, CANCEL_BUTTON, RESUME_BUTTON };
| +
| + gtk_widget_set_sensitive(g_list_nth_data(buttons, PAUSE_BUTTON),
| + sensitivity & PAUSE);
| + gtk_widget_set_sensitive(g_list_nth_data(buttons, CLEAR_BUTTON),
| + sensitivity & CLEAR);
| + gtk_widget_set_sensitive(g_list_nth_data(buttons, CANCEL_BUTTON),
| + sensitivity & CANCEL);
| + gtk_widget_set_sensitive(g_list_nth_data(buttons, RESUME_BUTTON),
| + sensitivity & RESUME);
| +}
| +
| +gchar* dialog_show (gchar *filename, gchar *domain, gchar *size)
| +{
| + enum { DOWNLOAD, SAVE_AS };
*RJEK* Perhaps namespace these? Or at least directly specify their values.
| + GtkWidget *dialog;
| + gchar* destination = NULL;
| + dialog = gtk_message_dialog_new_with_markup(parent,
| + GTK_DIALOG_DESTROY_WITH_PARENT,
| + GTK_MESSAGE_QUESTION, GTK_BUTTONS_NONE,
| + "<span size=\"x-large\" weight=\"ultrabold\">Download
file?</span>\n\n<small>%s from %s is %s in size</small>",
| + filename, domain, size);
*RJEK* i18n.
| + gtk_dialog_add_buttons(GTK_DIALOG(dialog), GTK_STOCK_SAVE, DOWNLOAD,
| + GTK_STOCK_CANCEL, GTK_RESPONSE_CANCEL,
| + GTK_STOCK_SAVE_AS, SAVE_AS, NULL);
| +
| + gint result = gtk_dialog_run(GTK_DIALOG(dialog));
| + gtk_widget_destroy(dialog);
| +
| + switch (result) {
| + case SAVE_AS: {
| + GtkWidget *dialog = gtk_file_chooser_dialog_new
| + ("Save file as...", parent,
| + GTK_FILE_CHOOSER_ACTION_SAVE,
| + GTK_STOCK_CANCEL, GTK_RESPONSE_CANCEL,
| + GTK_STOCK_SAVE, GTK_RESPONSE_ACCEPT,
| + NULL);
*RJEK* i18n.
| +
gtk_file_chooser_set_current_name(GTK_FILE_CHOOSER(dialog),
| + filename);
| + gtk_file_chooser_set_do_overwrite_confirmation
| + (GTK_FILE_CHOOSER(dialog),
| + option_request_overwrite);
| +
| + gint result = gtk_dialog_run(GTK_DIALOG(dialog));
| + if (result == GTK_RESPONSE_ACCEPT)
| + destination = gtk_file_chooser_get_filename
| + (GTK_FILE_CHOOSER(dialog));
| + gtk_widget_destroy(dialog);
| + break;
| + }
| + case DOWNLOAD: {
| + destination = g_strconcat(option_downloads_directory,
| + "/", filename, NULL);
| + break;
| + }
| + }
| + return destination;
*RJEK* Whose responsibility is it to free destination? Does
gtk_file_chooser_get_filename() return something with the same
responsibilities as g_strconcat()?
| +}
| +
| +gchar* info_to_string (struct gui_download_window *dl)
| +{
| + GString *info = g_string_new("");
| +
| + if (dl->status == NULL)
| + g_string_printf(info, "%s\n%s of %s completed",
| + dl->name->str,
human_friendly_bytesize(dl->size_downloaded),
| + human_friendly_bytesize(dl->size_total));
*RJEK* i18n.
| + else if (dl->status == "Error")
| + g_string_printf(info, "%s\n%s", dl->name->str,
| + dl->error->message);
| +
| + return info->str;
*RJEK* Is this freed safely?
| +}
| +
| +gchar* url_parse_domain (const gchar *url)
| +{
| + /* the + 2 is to account for both '/' after the protocol (ftp://) */
| + gchar *domain = strchr(url, '/') + 2;
*RJEK* Does this cope gracefully with data: URLs and file:// URLs with
unusual numbers of slashes? (ie, any number from one up)
| + return g_strndup(domain, strcspn(domain, "/"));
*RJEK* Where is this freed?
| +}
| +
| +gboolean handle_error (GError *error)
*RJEK* Style. No space here.
| +{
| + if (error != NULL) {
| + GtkWidget*dialog;
*RJEK* Style. But a space here...
| + dialog = gtk_message_dialog_new_with_markup(parent,
| + GTK_DIALOG_MODAL, GTK_MESSAGE_ERROR,
| + GTK_BUTTONS_OK,
| + "<big><b>Download failed</b></big>\n\n"
| + "<small>File error: %s</small>",
| + error->message);
*RJEK* i18n.
| + gtk_dialog_run(GTK_DIALOG(dialog));
| + gtk_widget_destroy(dialog);
| + return TRUE;
| + }
| + return FALSE;
| +}
| +
| +
| Index: gtk/gtk_download.h
| ===================================================================
| --- gtk/gtk_download.h (revision 4636)
| +++ gtk/gtk_download.h (working copy)
| @@ -21,7 +21,47 @@
|
| #include <gtk/gtk.h>
|
| -void nsgtk_download_initialise(void);
| +enum {
| + INFO,
| + PROGRESS,
| + STATUS,
| + SPEED,
| + DOWNLOAD,
| +
| + N_COLUMNS
| +};
| +
| +typedef enum {
| + PAUSE = 1 << 0,
| + RESUME = 1 << 1,
| + CANCEL = 1 << 2,
| + CLEAR = 1 << 3
| +} Actions;
*RJEK* Both these enums need to have namespacing on them (ie, have
NSGTK_DOWNLOAD_INFO or similar) or they'll likely clash. Also, the
Actions name needs namespacing in a similar way.
| +struct gui_download_window {
| + struct fetch *fetch;
| + Actions sensitivity;
| +
| + GString *name;
| + GString *time_left;
| + gint size_total;
| + gint size_downloaded;
| + gint progress;
| + gdouble last_update;
| + gdouble speed;
| + gchar *filename;
| + gchar *status;
| +
| + GtkTreeRowReference *row;
| + GTimer *timer;
| + GIOChannel *write;
| + GError *error;
| +};
| +
| +typedef void (*selection_action)(struct gui_download_window *dl);
*RJEK* Again, namespace this typename.
| +void nsgtk_download_init(GtkWindow *parent_window);
| void nsgtk_download_show(void);
| +void nsgtk_download_add(gchar *url, gchar *destination);
|
| #endif
| Index: gtk/gtk_scaffolding.c
| ===================================================================
Fine.
| Index: gtk/dialogs/gtk_options.c
| ===================================================================
Fine.
| Index: gtk/gtk_gui.c
| ===================================================================
| --- gtk/gtk_gui.c (revision 4636)
| +++ gtk/gtk_gui.c (working copy)
| @@ -301,7 +301,6 @@
| wndWarning = GTK_WINDOW(glade_xml_get_widget(gladeWindows,
"wndWarning"));
|
| nsgtk_history_init();
| - nsgtk_download_initialise();
| }
|
|
| @@ -401,31 +400,6 @@
| }
|
|
| -
| -struct gui_download_window *gui_download_window_create(const char *url,
| - const char *mime_type, struct fetch *fetch,
| - unsigned int total_size)
| -{
| - return 0;
| -}
| -
| -
| -void gui_download_window_data(struct gui_download_window *dw, const char
*data,
| - unsigned int size)
| -{
| -}
| -
| -
| -void gui_download_window_error(struct gui_download_window *dw,
| - const char *error_msg)
| -{
| -}
| -
| -
| -void gui_download_window_done(struct gui_download_window *dw)
| -{
| -}
| -
| static void nsgtk_select_menu_clicked(GtkCheckMenuItem *checkmenuitem,
| gpointer user_data)
| {
| Index: gtk/gtk_selection.c
| ===================================================================
| --- gtk/gtk_selection.c (revision 4636)
| +++ gtk/gtk_selection.c (working copy)
| @@ -25,9 +25,10 @@
| #include "desktop/browser.h"
| #include "gtk/gtk_selection.h"
| #include "gtk/gtk_window.h"
| +#include "utils/utf8.h"
|
| -GString *current_selection;
| -GtkClipboard *clipboard;
| +static GString *current_selection;
| +static GtkClipboard *clipboard;
|
| static bool copy_handler(const char *text, size_t length, struct box *box,
| void *handle, const char *whitespace_text,
| @@ -63,15 +64,19 @@
|
| bool gui_copy_to_clipboard(struct selection *s)
| {
| - current_selection = g_string_new(NULL);
| clipboard = gtk_clipboard_get (GDK_SELECTION_CLIPBOARD);
| if (s->defined && selection_traverse(s, copy_handler, NULL))
| - gtk_clipboard_set_text (clipboard, current_selection->str, -1);
| + gui_commit_clipboard();
| return TRUE;
| }
|
| void gui_start_selection(struct gui_window *g)
| {
| + if (!current_selection)
*RJEK* Can we get rid of the ! and check it explicitly?
| + current_selection = g_string_new(NULL);
| + else
| + g_string_set_size(current_selection, 0);
| +
| gtk_widget_grab_focus(GTK_WIDGET(g->drawing_area));
| }
|
| @@ -80,18 +85,33 @@
| char *text;
| clipboard = gtk_clipboard_get (GDK_SELECTION_CLIPBOARD);
| text = gtk_clipboard_wait_for_text (clipboard);
| - if (text)
| - browser_window_paste_text(g->bw, text,
| - strlen(text), true);
| + if (text){
*RJEK* Style. Space before brace. Also, what are we checking about
text?
| + char *utf8;
| + utf8_convert_ret ret;
| + /* Clipboard is in local encoding so
| + * convert to UTF8 */
| + ret = utf8_from_local_encoding(text,
| + strlen(text), &utf8);
| +
| + browser_window_paste_text(g->bw, utf8,
| + strlen(utf8), true);
| + }
| }
|
| bool gui_empty_clipboard(void)
| {
| + if (!current_selection)
*RJEK* Style.
| + current_selection = g_string_new(NULL);
*RJEK* Is this ever freed?
| + else
| + g_string_set_size(current_selection, 0);
| +
| return true;
| }
|
| bool gui_commit_clipboard(void)
| {
| + clipboard = gtk_clipboard_get (GDK_SELECTION_CLIPBOARD);
| + gtk_clipboard_set_text (clipboard, current_selection->str, -1);
| return true;
| }
|
| Index: riscos/window.c
| ===================================================================
| --- riscos/window.c (revision 4636)
| +++ riscos/window.c (working copy)
Fine.
| Index: desktop/textinput.c
| ===================================================================
| --- desktop/textinput.c (revision 4636)
| +++ desktop/textinput.c (working copy)
*RJEK* There's a lot of stuff in here I haven't a clue about. Can
somebody else have a look over this?
| Index: desktop/browser.c
| ===================================================================
| --- desktop/browser.c (revision 4636)
| +++ desktop/browser.c (working copy)
| @@ -2194,7 +2192,7 @@
| void browser_redraw_box(struct content *c, struct box *box)
| {
| int x, y;
| - union content_msg_data data;
| + union content_msg_data data;
*RJEK* What's changed here? I assume indentation.
| box_coords(box, &x, &y);
|
| Index: desktop/selection.c
| ===================================================================
| --- desktop/selection.c (revision 4636)
| +++ desktop/selection.c (working copy)
| @@ -50,7 +50,9 @@
| */
|
| #define IS_INPUT(box) ((box) && (box)->gadget && \
| - ((box)->gadget->type == GADGET_TEXTAREA || (box)->gadget->type ==
GADGET_TEXTBOX))
| + ((box)->gadget->type == GADGET_TEXTAREA || \
| + (box)->gadget->type == GADGET_TEXTBOX || \
| + (box)->gadget->type == GADGET_PASSWORD))
|
| /** check whether the given text box is in the same number space as the
| current selection; number spaces are identified by their uppermost
nybble */
| @@ -69,7 +71,6 @@
| size_t whitespace_length);
| static void selection_redraw(struct selection *s, unsigned start_idx,
| unsigned end_idx);
| -static unsigned selection_label_subtree(struct box *box, unsigned idx);
| static bool save_handler(const char *text, size_t length, struct box *box,
| void *handle, const char *whitespace_text,
| size_t whitespace_length);
| @@ -79,9 +80,8 @@
| unsigned int num_space, seln_traverse_handler handler,
| void *handle, save_text_whitespace *before, bool *first,
| bool do_marker);
| -static struct box *get_box(struct box *b, unsigned offset, int *pidx);
| +static struct box *get_box(struct box *b, unsigned offset, size_t *pidx);
|
| -
| /**
| * Creates a new selection object associated with a browser window.
| *
| @@ -261,11 +261,14 @@
| gui_drag_save_selection(s, s->bw->window);
| }
| else if (!modkeys) {
| - if (mouse & BROWSER_MOUSE_DRAG_1) {
| -
| + if (pos && (mouse & BROWSER_MOUSE_PRESS_1))
| + /* Clear the selection if mouse is pressed outside the
selection,
| + * Otherwise clear on release (to allow for drags) */
| + selection_clear(s, true);
| + else if (mouse & BROWSER_MOUSE_DRAG_1) {
| /* start new selection drag */
| selection_clear(s, true);
| -
| +
| selection_set_start(s, idx);
| selection_set_end(s, idx);
|
| @@ -291,12 +294,13 @@
| }
| gui_start_selection(s->bw->window);
| }
| - else if (pos && (mouse & BROWSER_MOUSE_CLICK_1)) {
| -
| - /* clear selection */
| - selection_clear(s, true);
| - s->drag_state = DRAG_NONE;
| - }
| + /* Selection should be cleared when button is released but in
| + * the RO interface click is the same as press */
| +// else if (!pos && (mouse & BROWSER_MOUSE_CLICK_1)) {
| +// /* clear selection */
| +// selection_clear(s, true);
| +// s->drag_state = DRAG_NONE;
| +// }
*RJEK* Can we be rid of this comment?
| else if (mouse & BROWSER_MOUSE_CLICK_2) {
|
| /* ignore Adjust clicks when there's no selection */
| @@ -699,15 +703,12 @@
| old_end = s->end_idx;
|
| s->defined = true;
| - s->start_idx = 0;
| - s->end_idx = s->max_idx;
| -
| - if (was_defined) {
| - selection_redraw(s, 0, old_start);
| - selection_redraw(s, old_end, s->end_idx);
| - }
| +
| + if (IS_INPUT(s->root))
| + selection_set_start(s,
s->root->children->children->byte_offset);
| else
| - selection_redraw(s, 0, s->max_idx);
| + selection_set_start(s, 0);
| + selection_set_end(s, s->max_idx);
| }
|
|
| @@ -722,9 +723,17 @@
| {
| bool was_defined = selection_defined(s);
| unsigned old_start = s->start_idx;
| -
| +
| s->start_idx = offset;
| s->defined = (s->start_idx < s->end_idx);
| +
| + if (s->root->gadget && s->defined){
*RJEK* Style. Space before brace.
| + /* update the caret text_box and offset so that it stays at the
| + * beginning of the selection */
| + s->root->gadget->caret_text_box = selection_get_start(s,
| + &s->root->gadget->caret_box_offset);
| + assert(s->root->gadget->caret_text_box != NULL);
| + }
|
| if (was_defined) {
| if (offset < old_start)
| @@ -773,14 +782,14 @@
| * \return ptr to box, or NULL if no selection defined
| */
|
| -struct box *get_box(struct box *b, unsigned offset, int *pidx)
| +struct box *get_box(struct box *b, unsigned offset, size_t *pidx)
*RJEK* What was the rationale for changing this to size_t?
| {
| struct box *child = b->children;
|
| if (b->text) {
|
| if (offset >= b->byte_offset &&
| - offset < b->byte_offset + b->length + b->space) {
| + offset <= b->byte_offset + b->length + b->space) {
|
| /* it's in this box */
| *pidx = offset - b->byte_offset;
| @@ -810,7 +819,7 @@
| * \return ptr to box, or NULL if no selection defined
| */
|
| -struct box *selection_get_start(struct selection *s, int *pidx)
| +struct box *selection_get_start(struct selection *s, size_t *pidx)
| {
| return (s->defined ? get_box(s->root, s->start_idx, pidx) : NULL);
| }
| @@ -824,7 +833,7 @@
| * \return ptr to box, or NULL if no selection defined.
| */
|
| -struct box *selection_get_end(struct selection *s, int *pidx)
| +struct box *selection_get_end(struct selection *s, size_t *pidx)
| {
| return (s->defined ? get_box(s->root, s->end_idx, pidx) : NULL);
| }
*RJEK* Same for these two, too.
| Index: desktop/selection.h
| ===================================================================
| --- desktop/selection.h (revision 4636)
| +++ desktop/selection.h (working copy)
| @@ -84,8 +84,8 @@
| void selection_set_start(struct selection *s, unsigned idx);
| void selection_set_end(struct selection *s, unsigned idx);
|
| -struct box *selection_get_start(struct selection *s, int *pidx);
| -struct box *selection_get_end(struct selection *s, int *pidx);
| +struct box *selection_get_start(struct selection *s, size_t *pidx);
| +struct box *selection_get_end(struct selection *s, size_t *pidx);
*RJEK* And here.
| bool selection_click(struct selection *s, browser_mouse_state mouse,
| unsigned idx);
| @@ -105,6 +105,8 @@
| bool selection_save_text(struct selection *s, const char *path);
|
| void selection_update(struct selection *s, size_t byte_offset, int change,
| - bool redraw);
| + bool redraw);
| +
| +unsigned selection_label_subtree(struct box *box, unsigned idx);
|
| #endif
