After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 699519 - sort history entries by title/address/date
sort history entries by title/address/date
Status: RESOLVED FIXED
Product: epiphany
Classification: Core
Component: History
git master
Other Linux
: Normal enhancement
: ---
Assigned To: Epiphany Maintainers
Epiphany Maintainers
: 727715 (view as bug list)
Depends on:
Blocks: 682981
 
 
Reported: 2013-05-02 19:56 UTC by Adam Dingle
Modified: 2014-07-10 13:15 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Allow sorting by date, title or url in the history dialog. (10.09 KB, patch)
2014-02-19 14:04 UTC, Robert Roth
needs-work Details | Review
Fill the history list store asynchronously on sorting to avoid blocking the user interface. (3.91 KB, patch)
2014-02-20 23:29 UTC, Robert Roth
reviewed Details | Review
History list store filled on the main thread when idling to avoid blocking the user interface (4.34 KB, patch)
2014-02-28 14:13 UTC, Robert Roth
reviewed Details | Review
Updated patch to asynchronously fill the store based on review (4.43 KB, patch)
2014-02-28 14:55 UTC, Robert Roth
needs-work Details | Review
Complete patch for a sortable history dialog (14.29 KB, patch)
2014-05-22 14:41 UTC, Robert Roth
needs-work Details | Review
History service patch (2.27 KB, patch)
2014-06-11 08:29 UTC, Robert Roth
committed Details | Review
History dialog patch (12.54 KB, patch)
2014-06-11 08:33 UTC, Robert Roth
none Details | Review
History dialog patch (12.49 KB, patch)
2014-06-11 08:37 UTC, Robert Roth
none Details | Review
History dialog patch (12.36 KB, patch)
2014-06-11 09:12 UTC, Robert Roth
committed Details | Review

Description Adam Dingle 2013-05-02 19:56:24 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.
Comment 1 Adam Dingle 2013-12-28 13:27:05 UTC
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?
Comment 2 William Jon McCann 2014-01-03 20:09:45 UTC

*** This bug has been marked as a duplicate of bug 721415 ***
Comment 3 Adam Dingle 2014-02-07 14:07:32 UTC
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.
Comment 4 William Jon McCann 2014-02-07 14:35:09 UTC
Can you please explain your use case for sorting?
Comment 5 Adam Dingle 2014-02-07 14:49:52 UTC
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.
Comment 6 Claudio Saavedra 2014-02-07 16:15:13 UTC
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.
Comment 7 Robert Roth 2014-02-19 14:04:40 UTC
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
Comment 8 Yosef Or Boczko 2014-02-19 21:30:50 UTC
Robert, I tested now your patch. it really awesome
and work very well, thanks!
Comment 9 Claudio Saavedra 2014-02-19 21:32:13 UTC
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.
Comment 10 Yosef Or Boczko 2014-02-19 21:58:21 UTC
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?
Comment 11 Robert Roth 2014-02-20 12:12:26 UTC
@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.
Comment 12 Yosef Or Boczko 2014-02-20 12:26:08 UTC
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.
Comment 13 Robert Roth 2014-02-20 23:29:38 UTC
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.
Comment 14 Robert Roth 2014-02-20 23:38:21 UTC
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.
Comment 15 Claudio Saavedra 2014-02-24 15:23:09 UTC
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.
Comment 16 Robert Roth 2014-02-28 14:13:05 UTC
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.
Comment 17 Claudio Saavedra 2014-02-28 14:35:48 UTC
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.
Comment 18 Robert Roth 2014-02-28 14:55:59 UTC
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.
Comment 19 Robert Roth 2014-03-28 07:48:47 UTC
@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 :) )
Comment 20 Yosef Or Boczko 2014-04-06 19:46:06 UTC
*** Bug 727715 has been marked as a duplicate of this bug. ***
Comment 21 Robert Roth 2014-04-09 05:15:43 UTC
ping, could anyone please review the patches here? They really make using the history dialog bearable both performance and usability-wide.
Comment 22 Robert Roth 2014-05-20 22:19:31 UTC
ping: Could anyone please review and test the patches on this bug, and provide some feedback?
Comment 23 Yosef Or Boczko 2014-05-20 22:27:58 UTC
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.
Comment 24 Michael Catanzaro 2014-05-21 02:07:31 UTC
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.
Comment 25 Robert Roth 2014-05-22 10:15:50 UTC
Thanks for the feedback, hopefully the maintainers will have the time to review this before 3.12.3 or 3.14 gets out.
Comment 26 Bastien Nocera 2014-05-22 10:53:16 UTC
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).
Comment 27 Bastien Nocera 2014-05-22 10:59:18 UTC
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.
Comment 28 Robert Roth 2014-05-22 14:41:13 UTC
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.
Comment 29 Robert Roth 2014-06-10 10:49:19 UTC
@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?
Comment 30 Claudio Saavedra 2014-06-11 07:51:04 UTC
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.
Comment 31 Robert Roth 2014-06-11 08:29:20 UTC
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.
Comment 32 Robert Roth 2014-06-11 08:33:03 UTC
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
Comment 33 Robert Roth 2014-06-11 08:37:55 UTC
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
Comment 34 Robert Roth 2014-06-11 09:12:50 UTC
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.
Comment 35 Robert Roth 2014-06-11 09:14:22 UTC
@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.
Comment 36 Claudio Saavedra 2014-06-11 12:37:32 UTC
Review of attachment 278252 [details] [review]:

Looks good.
Comment 37 Claudio Saavedra 2014-06-11 13:06:28 UTC
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.
Comment 38 Robert Roth 2014-06-11 13:59:10 UTC
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.