GNOME Bugzilla – Bug 699519
sort history entries by title/address/date
Last modified: 2014-07-10 13:15:20 UTC
It would be nice if I could sort the entries in the History window by title, address or date by clicking on the corresponding column header.
There's a bounty on this feature at Bountysource: https://www.bountysource.com/issues/1337703-sort-history-entries-by-title-address-date On that page a user named "bbbbb" said they started working on a solution on Dec 2. bbbbb, are you still working on this? Do you have any progress to report?
*** This bug has been marked as a duplicate of bug 721415 ***
I'm reopening this and unmarking as a duplicate, since the new history dialog (bug 721415) does not fully allow sorting on these fields: 1. The dialog does not allow 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. 2. Sorting by page title needs to be reimplemented: Claudio wrote 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.
Can you please explain your use case for sorting?
Use cases: for sorting by date: There's some cool web page I visited an hour ago but I can't remember its name. If I can sort by date I can scan back to find it. for sorting by title: I recently visited a web page for a restaurant whose name I know begins with T, but I can't remember the exact name: was it Tripoli? Tupelo? Torro? If I can sort by title I'll be able to browse through and find it easily. for sorting by URL: I recently visited a cool site whose name I can't remember, and whose page titles don't happen to include the site name (which certainly happens sometimes). If I can sort by URL, I can scan down the list and quickly see all the domains I've visited to find the one I want.
It is possible to implement this efficiently if the history backend is extended to allow retrieving search results sorted by url or title. This is easy to do and it should be pretty fast too, but it requires a few small changes here and there, including manually handling the "clicked" event in the treeview columns.
Created attachment 269679 [details] [review] Allow sorting by date, title or url in the history dialog. https://bugzilla.gnome.org/show_bug.cgi?id=699519 Implemented by: * adding the proper sorting enum values in the history service * using the order by in query statements based on the enum values * overriding the column header clicked event to change the sorting order and/or direction * reloading the data from the history service on column header clicks
Robert, I tested now your patch. it really awesome and work very well, thanks!
It mostly works and the implementation is on the right track, but it sometimes causes the UI to get stuck. I think it'd be good to research this.
It very very faster (I have history from 11 July 2013) than before the patch. The sort it done in a moment, each type of sort. btw, maybe this so fast because now I on MATE?
@Claudio Saavedra: any hints on how to make the UI to get stuck? How old is your history? I have been trying to reproduce it, but the refresh is fast every time, doesn't lag at all, and Yosef Or Boczko also reported it working fast. I would be willing to look into it, but I have to reproduce it. @Yosef Or Boczko: it shouldn't have much to do with the desktop environment you are using, the sort is done on the database side, and both Gnome Shell and MATE have to build up the tree row elements based on the results each time.
Robert, I run now epiphany with your patch on gnome-shell, and the histyory dialog is really slow, and also for the sorting I need to wait 1sec or 2sec. In MATE it happen immediately.
Created attachment 269849 [details] [review] Fill the history list store asynchronously on sorting to avoid blocking the user interface. To avoid the UI thread freezing because of building the tree model: * start a GTask when we have the results from the history service * create a new liststore instead of clearing the old one, as it is faster in case of large histories * fill the treemodel in the task * when the task finishes, set the fresh-built treemodel as the model of the treeview * set the sort indicator and sort column again, as changing the model will clear the old settings.
This second patch, applied after the first one solves what I did see as a problem, building the tree model on the UI thread after getting the results from the history service. That required clearing the model, adding the new items to the model, and freeing the list received from the history service. As these actions could take long, I have moved these to an async task. I have tested this with up to 100000 history entries, and it works quite ok, I didn't see the UI freeze.
Review of attachment 269849 [details] [review]: I got a few warnings when closing the window while the search was still being updated: (epiphany:17051): Gtk-CRITICAL **: gtk_tree_view_set_model: assertion 'GTK_IS_TREE_VIEW (tree_view)' failed (epiphany:17051): Gtk-CRITICAL **: gtk_tree_view_get_column: assertion 'GTK_IS_TREE_VIEW (tree_view)' failed (epiphany:17051): Gtk-CRITICAL **: gtk_tree_view_column_set_sort_order: assertion 'GTK_IS_TREE_VIEW_COLUMN (tree_column)' failed (epiphany:17051): Gtk-CRITICAL **: gtk_tree_view_column_set_sort_indicator: assertion 'GTK_IS_TREE_VIEW_COLUMN (tree_column)' failed ::: src/ephy-history-window.c @@ +87,3 @@ static void add_urls (GtkListStore *store, + GList *urls) Remove this change. @@ +107,3 @@ + EphyHistoryWindow *self, + GList *urls, + GCancellable *cancellable) Use spaces, not tabs. @@ +130,3 @@ + GtkTreeViewColumn *column; + + } Spacing is wrong. @@ +133,3 @@ + return; + + g_list_free_full (urls, (GDestroyNotify) ephy_history_url_free); Spacing is wrong. @@ +165,3 @@ + g_task_set_task_data (fill_task, urls, NULL); + g_task_set_return_on_cancel (fill_task, TRUE); + g_task_run_in_thread (fill_task, (GTaskThreadFunc) async_store_setup_thread); I think you can't really do GTK+ calls from a different thread, so this is a bit dangerous. Maybe modifying a model that has not been set to a view is safe, though, I don't know. @@ +539,3 @@ + g_cancellable_cancel (self->priv->cancellable); + g_clear_object (&self->priv->cancellable); + } This won't cancel the cancellable when the user types something.
Created attachment 270571 [details] [review] History list store filled on the main thread when idling to avoid blocking the user interface Based on Bastien Nocera's tips, I have revised (rewrote) the patch for asynchronously filling the list store, by doing the following: In case the history window contents are resorted, to avoid freezing the user interface, do the following: * register a source to process the url list received from the service one url at a time, by adding it to the model, and freeing the url * the source is added to the main loop with idle_add * in case it is finished or another sort is done, remove the source, (and free the list it was processing, if resort happened, otherwise the list is already empty) * as clearing the liststore while it is set to a treeview emits the row deleted signal for each row, the liststore is set to null before clearing, it is cleared, and then set again for the treeview * as setting the model again resets the sort column and indicator on the treeview, those attributes must be set again.
Review of attachment 270571 [details] [review]: Didn't try it yet, but commented on a few things. ::: src/ephy-history-window.c @@ +61,3 @@ + GList *urls; + guint sorter_source; Indentation is wrong. @@ +97,2 @@ + if (element) + { Should use K&R indentation (braces up). I know the file has mistakes on that, sorry. @@ +105,3 @@ -1); + self->priv->urls = element->next; + ephy_history_url_free (url); I think you're freeing the urls here but not the list. @@ +135,3 @@ + + if (self->priv->urls != NULL) + g_list_free_full (self->priv->urls, (GDestroyNotify)ephy_history_url_free); Here you run a destroy notify function on items that probably were freed already above. @@ +665,3 @@ + if (self->priv->urls != NULL) + { + g_list_free_full (self->priv->urls, (GDestroyNotify)ephy_history_url_free); Same.
Created attachment 270572 [details] [review] Updated patch to asynchronously fill the store based on review Thanks for the quick review. I have updated the patch based on your request, with the following (remade the patch): * fixed indentation in the private declaration (I hope that's OK now :) ) * removed the link of the element from the list, freed the data, and freed the list element, that should be OK now * the destroynotify shouldn't be called on something that has already been freed, as those cases are added for cleanup: in case the source is canceled (by resorting the whole list before the source finished processing all the urls) or in case we are closing the window while the urls list is still not empty. In case the source finishes processing all the URLs before resort/close, the urls list is NULL, so due to the if not null checks, the destroynotify won't be called again. If it would be called, it would clearly crash the application, but it doesn't.
@Claudio Saveedra: Could you please review the latest patches (and my comments on your review) to see if they are ok for you? (now that the 3.12 stress is over :) )
*** Bug 727715 has been marked as a duplicate of this bug. ***
ping, could anyone please review the patches here? They really make using the history dialog bearable both performance and usability-wide.
ping: Could anyone please review and test the patches on this bug, and provide some feedback?
I just can to say I use the patches from this bug already some month on my local build (in my PKGBUILD I have 'git bz apply 699519 etc`), and it works very well.
Sorting is noticeably slow, but without these patches, the history dialog just hangs the whole browser (Bug #682981) so I think these should go into gnome-3-12.
Thanks for the feedback, hopefully the maintainers will have the time to review this before 3.12.3 or 3.14 gets out.
Review of attachment 270572 [details] [review]: ::: src/ephy-history-window.c @@ +97,1 @@ + if (element) { This is too naive an approach. You're adding elements one-by-one, each time going through the main loop even though you might have had time to process more items. Look for GTK_TREE_VIEW_TIME_MS_PER_IDLE in gtk/gtktreeview.c for a better approach (you'd basically process all the items you can within a certain period of time, ensuring that you don't trample on the next frame drawing).
Review of attachment 269679 [details] [review]: A couple of niggles. ::: src/ephy-history-window.c @@ +145,3 @@ gint64 from, to; GList *substrings; + EphyHistorySortType type = EPHY_HISTORY_SORT_MOST_RECENTLY_VISITED; Add this assignment in the default switch statement instead. @@ +488,3 @@ + GtkTreeViewColumn *previous_sortby; + + if (column == self->priv->date_column) Use: g_object_set_data (self->priv->date_column, "column", GINT_TO_POINTER (COLUMN_DATE)); in _init() (and for each column), so you can run: new_sort_column = GPOINTER_TO_INT (g_object_get_data (column, "column")); here instead of a big switch statement.
Created attachment 276991 [details] [review] Complete patch for a sortable history dialog Thank you for the review Bastien. I have merged the two patches into one, and additionally fixed the three issues you have pointed out: * in one idle cycle process as many items as possible (based on the gtktreeview code) * added the default sorting option to the default case in the switch * store the column id in the column GObject to avoid multiple comparisons on resorting. If you find anything else to fix, let me know, I'll resubmit the patch.
@csaveedra: Sorry for bugging you again, could you please review this last complete patch, to see if there's anything else I could fix for this to get merged?
Review of attachment 276991 [details] [review]: Sorry for not having reviewed this. I think the patch is looking fine for the most part. However, I'd prefer to see the history service and types changes in a split patch. ::: src/ephy-history-window.c @@ +112,3 @@ + self->priv->urls = g_list_remove_link (self->priv->urls, element); + ephy_history_url_free (url); + g_list_free (element); I think you could use here g_list_free_1() to spare some cycles. @@ +146,3 @@ + + if (self->priv->urls != NULL) + g_list_free_full (self->priv->urls, (GDestroyNotify)ephy_history_url_free); I wonder whether you shouldn't be doing this before calling the history service instead. Also, there is repeated code here and in ephy_history_window_dispose(). You could factor it out to a method.
Created attachment 278252 [details] [review] History service patch First part of the split patch, for patching the history service to support querying history urls ordered by url or title, ascending or descending.
Created attachment 278253 [details] [review] History dialog patch Second part of the split patch, consisting of the patch for the history dialog to allow ascending/descending sort by any of the columns. The requested changes have been applied based on the review, namely: * use g_list_free_1 * free the url list before invoking the history service instead of freeing the existing list only in the callback * factor out the g_list_free_full for freeing the url results to a separate method
Created attachment 278254 [details] [review] History dialog patch Second part of the split patch, consisting of the patch for the history dialog to allow ascending/descending sort by any of the columns. The requested changes have been applied based on the review, namely: * use g_list_free_1 * free the url list before invoking the history service instead of freeing the existing list only in the callback * factor out the g_list_free_full for freeing the url results to a separate method
Created attachment 278255 [details] [review] History dialog patch Second part of the split patch, consisting of the patch for the history dialog to allow ascending/descending sort by any of the columns. The requested changes have been applied based on the review, namely: * use g_list_free_1 * free the url list before invoking the history service instead of freeing the existing list only in the callback * factor out the g_list_free_full for freeing the url results to a separate method * additionally fixed crash when resorting (caused by moving the url list free before invoking the history service), by removing the add_urls source before freeing the URLs.
@Claudio Saavedra: I have finished the implementation based on the review, and proposed a split patch (although the second part took some iterations), let me know if there's anything else you would like to see changed in the patches.
Review of attachment 278252 [details] [review]: Looks good.
Review of attachment 278255 [details] [review]: Looks good now, with one comment. Please fix and push. ::: src/ephy-history-window.c @@ +174,2 @@ static void +free_urls (EphyHistoryWindow *self) { I don't think free_urls() is descriptive enough of what this method does. remove_pending_sorter_source() or something like that.
Thanks for the review, updated method name from free_urls to remove_pending_sorter_source, and pushed to master. This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.