GNOME Bugzilla – Bug 721415
Design update for history dialog
Last modified: 2014-02-07 14:04:01 UTC
Here is a design update for the history dialog that more closely matches the one used in the passwords and cookies dialogs. This doesn't preclude us from adding history to the new overview but nice to have an improved dialog in the meanwhile or even in addition to it.
Created attachment 265237 [details] [review] Redesign the history dialog
Created attachment 265238 [details] [review] Make bookmark properties dialog modal
Created attachment 265239 [details] [review] Remove unused widgets After redesign of history dialog.
*** Bug 699519 has been marked as a duplicate of this bug. ***
Great! I see just one problem - it is very slow. Maybe to add a back-forward buttons, same to the user-history in gnome-control-center?
Btw, I see some warning, and also crashes: (epiphany:10645): Gtk-CRITICAL **: _gtk_tree_view_column_autosize: assertion 'GTK_IS_TREE_VIEW (tree_view)' failed (epiphany:20996): GLib-GObject-CRITICAL **: g_object_ref: assertion 'G_IS_OBJECT (object)' failed (epiphany:20996): GLib-GObject-CRITICAL **: g_object_unref: assertion 'G_IS_OBJECT (object)' failed Crashes: (epiphany:10573): Gtk-CRITICAL **: _gtk_tree_view_column_autosize: assertion 'GTK_IS_TREE_VIEW (tree_view)' failed Segmentation fault (core dumped)
Review of attachment 265237 [details] [review]: ::: src/ephy-history-window.c @@ +142,3 @@ + substrings = substrings_filter (self); + +} It is very slow, as Yosef pointed out. We might not want to do this in one pass -- perhaps first filter the history for the last month and when that's done, do the rest? Some kind of partial fetching until there is nothing left to add.
Created attachment 265566 [details] [review] Redesign the history dialog
ping? :)
By the way, there is a $250 bounty open on this issue: https://www.bountysource.com/issues/1372360-design-update-for-history-dialog
To be more exact, that bounty was created on bug 699519 (sort history entries by title/address/date), which was marked as a duplicate of this one so the bounty transferred over. I'll be happy to pay out the bounty if this new design allows history entries to be sorted on all those fields.
Review of attachment 265566 [details] [review]: Partial review. Needs update, but I'll provide an initial patch you can apply on top of this one. ::: src/ephy-history-window.c @@ +38,3 @@ #include <time.h> +#define NUM_RESULTS_LIMIT 500 We don't need to limit this if we remove the model sort (which we should). @@ +149,3 @@ + NUM_RESULTS_LIMIT, 0, + substrings, self->priv->cancellable, +static GList * I'm modifying this method so that you can pass the desired sorting method to the service. @@ +730,2 @@ + gtk_tree_sortable_set_sort_column_id (GTK_TREE_SORTABLE (self->priv->liststore), 0, + GTK_SORT_DESCENDING); This can go away. We should ask the history service to sort the results as we would like, so that sqlite does it internally. ::: src/resources/history-dialog.ui @@ +15,3 @@ + <object class="GtkTreeModelSort" id="treemodelsort"> + <property name="model">liststore</property> + </object> This makes the dialog terribly slow. Please remove the model sort.
Created attachment 268091 [details] [review] ephy-history-window: updates Feel free to squeeze this into your patch
Created attachment 268097 [details] [review] Redesign the history dialog Squashed and rebased
Review of attachment 268097 [details] [review]: A few other comments. ::: src/ephy-history-window.c @@ +129,3 @@ + substrings = g_list_prepend (substrings, *p++); + }; + if (success != TRUE) I think the order of the strings is irrelevant, so this you could remove. @@ +673,3 @@ } + gtk_widget_destroy (GTK_WIDGET (self)); I'm getting crashers when closing this dialog, irregularly. I suspect that something goes wrong because of destroying the dialog in a handler to a signal of the dialog itself.
Review of attachment 265238 [details] [review]: OK
Review of attachment 265239 [details] [review]: OK
Created attachment 268194 [details] [review] Redesign the history dialog
(In reply to comment #15) > I think the order of the strings is irrelevant, so this you could remove. OK > @@ +673,3 @@ > } > > + gtk_widget_destroy (GTK_WIDGET (self)); > > I'm getting crashers when closing this dialog, irregularly. I suspect that > something goes wrong because of destroying the dialog in a handler to a signal > of the dialog itself. We do that all the time though. I'm not seeing the crash can you get a backtrace?
> We do that all the time though. I'm not seeing the crash can you get a > backtrace? Unsurprisingly enough I can't reproduce it now and I lost the stacktrace. But I do remember it was send_delete_event () in gtkwindow.c calling gtk_widget_get_window () with a pointer that was not a window anymore. My guess is that the dialog might be triggering the gtk_window_close() call that schedules send_delete_event() after the gtk_widget_destroy() has been called on the it, but well, it doesn't seem to happen reliably :/
btw, escape is not closing the dialog.
Now it crashed:
+ Trace 233131
Attachment 265238 [details] pushed as 2d77a8e - Make bookmark properties dialog modal Attachment 265239 [details] pushed as bd6dd7c - Remove unused widgets Attachment 268194 [details] pushed as 865db79 - Redesign the history dialog
The new history dialog looks nice. Currently it allows sorting history entries by date or name (i.e. page title). Could we also enable sorting by location (i.e. URL)? It seems arbitrary to disallow this, and it could be useful if I want to see a list of domains I've visited or a list of URLs on a particular site.
Please file new or additional issues as new bugs.
Actually, I am going to remove sorting by page title, because it is currently implemented by running a sorting algorithm at the model and that's very inefficient and it locks the UI for several seconds for large history. The proper way to do this would be to get the history already sorted from the history backend in the way desired. File a new bug and we can discuss that.
OK - I'll simply reopen bug 699519 (sort history entries by title/address/date), which was previously marked as a duplicate of this one.