Make selfoss navigable #834

niol · 2016-12-28T18:13:59Z

These patches make selfoss navigable using the url hash. This makes the url reflect whats displayed in selfoss. This also makes it possible to reload the item that was open before a browser crash or when the tab containing selfoss was offloaded from memory on a mobile browser.

This pages modifies the client ui to make heavy use of location.hash: hash changes are now responsible for loading the entry list. Another big change is that the server side template is basically empty of any data. It is loaded via the same client side code responsible for updating item lists, tags, stats, etc.

jtojnar · 2016-12-29T04:32:54Z

For fixing #774, would not it be cleaner to use where id < :id_of_last_loaded_item instead of offset + extra_ids? It could also be slightly faster as we can have index on id. I like the rest of 2f87dd1, though.

jtojnar · 2016-12-29T05:00:40Z

Regarding the second part, I discovered some edge cases:

Tag names are URL-encoded twice, causing tags using Unicode not to work. For example, 新闻国内 turns into
/selfoss/?offset=0&itemsPerPage=50&search=&type=unread&tag=%25E6%2596%25B0%25E9%2597%25BB%25E5%259B%25BD%25E5%2586%2585&source=&sourcesNav=false&ajax=true – notice the %25 instead of %.
Visiting item on one of the lower pages does not expand it. We might show the visited item on top, as loading all the preceding items when visiting an older item might not be feasible.
Now I also see why you choose the extra_ids, though I still think that using the id < :id_of_last_loaded_item might be better than sending the list of all the read items when loading next page.
Visiting source listing does not expand the sources in the sidebar like clicking the item source does.
Item from a source not in a certain tag can be visited through the tag’s URL. This is probably not worthy of worrying about.

I did not check the code in 8b7f8d2 very deeply yet.

bendem · 2016-12-29T09:08:15Z

Agreed with @jtojnar, here is some explanation in regard to why offset/limit pagination is considered bad.
https://blog.jooq.org/2016/08/10/why-most-programmers-get-pagination-wrong/

niol · 2016-12-29T11:30:04Z

Point 1, I cannot reproduce with multiple unicode strings using Chromium as browser.
Regarding the cleaner and id comparison, this would not work as we order by datetime and not id and that may not be the same order (a source crawled after but with an older item will have a higher id). I'll run some basic tests to see if seek pagination is worth it regarding performance. It looks very interesting but is not related to fixing that bug.

jtojnar · 2016-12-29T13:43:18Z

Indeed, it seems to work in Chromium as document.location.hash === "#unread/tag-新闻国内". In Firefox it is "#unread/tag-%E6%96%B0%E9%97%BB%E5%9B%BD%E5%86%85".

As for the seek pagination, my bad. The items would have to be sorted by datetime DESC, id DESC and then filtered using (datetime, id) < (:last_datetime, last_id); index could be created for this column pair.

Although I think the performance difference will be negligible in most installation, as selfoss, by default, collects garbage after 30 days, it would result in code that is much cleaner and thus more prone to bugs, which IMHO is more important value; any performance increase would be a nice bonus.

The problem is explained there : http://stackoverflow.com/questions/1703552/encoding-of-window-location-hash As a summary, the hash exposed in JS in Firefox is already URL-encoded, which is not the case in Chrome for instance.

jtojnar · 2016-12-29T14:53:32Z

Apparently the Firefox behaviour is correct one, since Location.href ought to return URL’s fragment, which is described as an ASCII string.

Interestingly enough, until 2015 Firefox behaved the same way as Chromium. On the other hand, Safari, IE and old Opera are correctly returning encoded hash, so this should probably be reported as Blink interop bug.

Since the location.href should be encoded everywhere, the proper fix is obtaining the hash using decodeURIComponent(location.href.split('#').splice(1).join('#')) (source). It should be correct as, if I am reading the RFC correctly, URL can only contain one number sign.

Decoding the hash as in d02a159 would fail in non-compliant browsers like chromium when the tag contains a percent sign.

niol · 2016-12-30T16:48:55Z

Regarding seek pagination, I do not understand why id should be included in the ORDER clause and in the comparison. Regarding performance, it seems about 5-10% better.

bendem · 2016-12-30T17:07:42Z

Because multiple rows could have the same timestamp, you only want the ones after the last one you got.

jtojnar · 2016-12-30T18:31:42Z

Indeed.

Paging requires a deterministic sort order.

See a more detailed writeup about the seek pagination:

http://use-the-index-luke.com/sql/partial-results/fetch-next-page

jtojnar · 2016-12-30T19:00:01Z

public/js/selfoss-events-navigation.js

@@ -71,7 +71,7 @@ selfoss.events.navigation = function() {
        $(this).addClass('active');

        if($(this).hasClass('nav-tags-all')==false) {
-            location.hash = '#' + selfoss.filter.type + '/tag-' + $(this).find('span').html();
+            location.hash = '#' + selfoss.filter.type + '/tag-' + $(this).find('span').html().replace('%', '%25');


This should probably be encodeURIComponent($(this).find('span').html()). Works both in Chromium and Firefox.

Yes, but I had preferred the browsers to show a unencoded string in the address bar. I've also tested with both browers.

I am afraid that is Chromium issue, Firefox displays the hash decoded. It would be nice if someone with access to Safari could test how it behaves there.

It also works with the firefox I have (Mozilla/5.0 (X11; Linux x86_64; rv:50.0) Gecko/20100101 Firefox/50.0).

- slightly better performance - easier to read than extra explicitely requested entries for fixing fossar#774

jtojnar · 2017-01-02T18:22:38Z

In a brief inspection it looks really nice. However, 2999994 broke clicking the entry source to get to source listing. Two clicks are now needed.

…ossar#836)

jtojnar · 2017-01-03T15:24:50Z

Looks nice.

And while I am nitpicking, when you click the entry source, the source in the sidebar will be highlighted only after the listing finished loading – the other way around than when clicking the source in the sidebar.

niol · 2017-01-06T08:43:16Z

Yes, but the only way to fix this would be to make two AJAX requests: the first to load the source navigation sidebar items, the second to load the new filtered list of items. The current implementation loads both ressources using the same request, which should be a bit faster.

Hash is processed asynchroneously, which would cause ajax requests to be sent with an opened entry on slow devices. This change ensures the hash is processed in that case.

SSilence · 2017-02-04T19:43:36Z

This changes doesn't work with my sqlite database:

Ein Fehler ist aufgetreten: PDO: no such column: TRUE [selfoss/libs/f3/base.php:2032] Base->error(500,'PDO: no such column: TRUE') [selfoss/daos/mysql/Items.php:286] DB\SQL->exec('SELECT items.id FROM items AS items, sources AS sources WHERE items.source=sources.id AND TRUE LIMIT 1 OFFSET 50',array()) [selfoss/daos/Items.php:83] daos\mysql\Items->get(array('starred'=>false,'offset'=>'0','search'=>'','items'=>50,'offset_from_datetime'=>'','offset_from_id'=>'','itemsPerPage'=>'50','type'=>'newest','tag'=>'','source'=>'','sourcesNav'=>'false','ajax'=>'true')) [selfoss/controllers/Index.php:243] daos\Items->get(array('offset'=>'0','offset_from_datetime'=>'','offset_from_id'=>'','itemsPerPage'=>'50','search'=>'','type'=>'newest','tag'=>'','source'=>'','sourcesNav'=>'false','ajax'=>'true'))

niol added 4 commits December 22, 2016 15:58

make it possible to request specific extra items in item list

2f87dd1

ensure read items are counted for loading more items (fix fossar#774)

f6a0a0b

fix loosing tag or source filtering on mobile

4c7beac

assume the hash is encoded to handle tags with % sign in hash

12fedd2

jtojnar reviewed Dec 30, 2016

View reviewed changes

niol added 3 commits January 2, 2017 18:16

use seek pagination instead of offset

1aff08c

- slightly better performance - easier to read than extra explicitely requested entries for fixing fossar#774

ensure entry requested in location.hash is shown

3ffd36f

open source nav if source requested in location.hash

2999994

niol added 4 commits January 3, 2017 10:09

fix seek pagination on smartphone when last entry was marked read

aa96cbd

fix clicking the entry source to get to source listing

2df9d78

ensure .stream-error message events are set even on first load

d7cb2da

fix ‘Refresh this source’ button inserted also to subsequent pages (f…

d16491f

…ossar#836)

niol added 3 commits January 6, 2017 14:22

prevent unwanted scrolling to selected entry on stream more

1a9a2c4

prevent selected entry from being requested again upon stream more

b197e6e

ensure hash is processed after changing hash programmatically

d23174d

Hash is processed asynchroneously, which would cause ajax requests to be sent with an opened entry on slow devices. This change ensures the hash is processed in that case.

niol mentioned this pull request Jan 15, 2017

Sync client from server using last update timestamp #846

Merged

SSilence merged commit ce5fc91 into fossar:master Feb 4, 2017

SSilence mentioned this pull request Feb 4, 2017

Revert "Make selfoss navigable" #854

Merged

niol mentioned this pull request Feb 6, 2017

Make selfoss navigable #869

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make selfoss navigable #834

Make selfoss navigable #834

niol commented Dec 28, 2016

jtojnar commented Dec 29, 2016

jtojnar commented Dec 29, 2016

bendem commented Dec 29, 2016 •

edited

Loading

niol commented Dec 29, 2016

jtojnar commented Dec 29, 2016

jtojnar commented Dec 29, 2016

niol commented Dec 30, 2016

bendem commented Dec 30, 2016

jtojnar commented Dec 30, 2016

jtojnar Dec 30, 2016

niol Dec 30, 2016

jtojnar Dec 30, 2016

niol Dec 31, 2016

jtojnar commented Jan 2, 2017

jtojnar commented Jan 3, 2017

niol commented Jan 6, 2017

SSilence commented Feb 4, 2017

Make selfoss navigable #834

Make selfoss navigable #834

Conversation

niol commented Dec 28, 2016

jtojnar commented Dec 29, 2016

jtojnar commented Dec 29, 2016

bendem commented Dec 29, 2016 • edited Loading

niol commented Dec 29, 2016

jtojnar commented Dec 29, 2016

jtojnar commented Dec 29, 2016

niol commented Dec 30, 2016

bendem commented Dec 30, 2016

jtojnar commented Dec 30, 2016

jtojnar Dec 30, 2016

Choose a reason for hiding this comment

niol Dec 30, 2016

Choose a reason for hiding this comment

jtojnar Dec 30, 2016

Choose a reason for hiding this comment

niol Dec 31, 2016

Choose a reason for hiding this comment

jtojnar commented Jan 2, 2017

jtojnar commented Jan 3, 2017

niol commented Jan 6, 2017

SSilence commented Feb 4, 2017

bendem commented Dec 29, 2016 •

edited

Loading