Add a way to search all from wwwlisting

Add a way to search all from wwwlisting

[Konstantin Ryabitsev]

Hello:

I switched the configuration to return wwwlisting as toplevel view (instead of
redirecting / to /all/), but there's some discontent, because the easy
interface to "search everything" is gone and unless someone knows about /all/,
they wouldn't find out about it from the wwwlisting view.

How about, if there is an extindex "all" defined, the wwwlisting adds an extra
search box:


[_________________] [search all] help

* 2021-08-26 16:54 - https://x-lore.kernel.org/all/
  All of lore.kernel.org

------------------------------------------------------

[_________________] [locate inbox]

* 2021-08-26 16:54 - https://x-lore.kernel.org/lkml/
  [description]
  [infourl]

* etc

* etc


-K

[PATCH 0/2] wwwlisting shows /all/

[Eric Wong]

Konstantin Ryabitsev wrote:
> Hello:
> 
> I switched the configuration to return wwwlisting as toplevel view (instead of
> redirecting / to /all/), but there's some discontent, because the easy
> interface to "search everything" is gone and unless someone knows about /all/,
> they wouldn't find out about it from the wwwlisting view.
> 
> How about, if there is an extindex "all" defined, the wwwlisting adds an extra
> search box:
> 
> [_________________] [search all] help
> 
> * 2021-08-26 16:54 - https://x-lore.kernel.org/all/
>   All of lore.kernel.org
> 
> ------------------------------------------------------
> 
> [_________________] [locate inbox]
> 
> * 2021-08-26 16:54 - https://x-lore.kernel.org/lkml/
>   [description]
>   [infourl]

I think that's too much vertical whitespace at the top of the
page, and multiple <form>s or <input> boxes at the top can get
confusing.

Just making /all/ show up at the top like a normal inbox (and
letting the admin decide on description) seems sufficient.  If
users can get to /all/ then they can search /all/ as normal.

I tried moving the <input>s next to infourl, but mixing <form>
and <pre> introduces a lot of vertical whitespace.


I also tried adding radio button (or drop-down):

	[_________________] [*] inboxes [ ] /all/   [search]

But extra <form> elements always confuse me whenever I encounter
them in any browser, so I decided to just leave things alone.

Eric Wong (2):
  www_listing: show ->ALL at top of HTML listing
  www_listing: fix odd "locate inbox" cases

 lib/PublicInbox/WwwListing.pm | 50 +++++++++++++++++++++++------------
 t/extindex-psgi.t             | 12 +++++++++
 2 files changed, 45 insertions(+), 17 deletions(-)

[PATCH 1/2] www_listing: show ->ALL at top of HTML listing

[Eric Wong]

It's a special case and we can show it in the HTML display
without affecting manifest.js.gz generation.
---
 lib/PublicInbox/WwwListing.pm | 45 ++++++++++++++++++++++-------------
 t/extindex-psgi.t             |  6 +++++
 2 files changed, 34 insertions(+), 17 deletions(-)

diff --git a/lib/PublicInbox/WwwListing.pm b/lib/PublicInbox/WwwListing.pm
index 8b54d724..eabda98a 100644
--- a/lib/PublicInbox/WwwListing.pm
+++ b/lib/PublicInbox/WwwListing.pm
@@ -7,22 +7,29 @@ package PublicInbox::WwwListing;
 use strict;
 use v5.10.1;
 use PublicInbox::Hval qw(prurl fmt_ts ascii_html);
-use PublicInbox::Linkify;
 use PublicInbox::GzipFilter qw(gzf_maybe);
 use PublicInbox::ConfigIter;
 use PublicInbox::WwwStream;
+use URI::Escape qw(uri_escape_utf8);
 
 sub ibx_entry {
 	my ($ctx, $ibx, $ce) = @_;
-	$ce->{description} //= $ibx->description;
+	my $desc = ascii_html($ce->{description} //= $ibx->description);
 	my $ts = fmt_ts($ce->{-modified} //= $ibx->modified);
-	my $url = prurl($ctx->{env}, $ibx->{url});
-	my $tmp = <<"";
-* $ts - $url
-  $ce->{description}
-
-	if (defined(my $info_url = $ibx->{infourl})) {
-		$tmp .= '  ' . prurl($ctx->{env}, $info_url) . "\n";
+	my ($url, $href);
+	if (defined($ibx->{url})) {
+		$url = $href = ascii_html(prurl($ctx->{env}, $ibx->{url}));
+	} else {
+		$href = ascii_html(uri_escape_utf8($ibx->{name})) . '/';
+		$url = ascii_html($ibx->{name});
+	}
+	my $tmp = <<EOM;
+* $ts - <a\nhref="$href">$url</a>
+  $desc
+EOM
+	if (defined($url = $ibx->{infourl})) {
+		$url = ascii_html(prurl($ctx->{env}, $url));
+		$tmp .= qq(  <a\nhref="$url">$url</a>\n);
 	}
 	push(@{$ctx->{-list}}, (scalar(@_) == 3 ? # $misc in use, already sorted
 				$tmp : [ $ce->{-modified}, $tmp ] ));
@@ -86,19 +93,24 @@ sub add_misc_ibx { # MiscSearch->retry_reopen callback
 		limit => $q->{l}
 	};
 	$qs .= ' type:inbox';
-	if (my $user_query = $q->{'q'}) {
+
+	delete $ctx->{-list}; # reset if retried
+	my $pi_cfg = $ctx->{www}->{pi_cfg};
+	if (defined(my $user_query = $q->{'q'})) {
 		$qs = "( $qs ) AND ( $user_query )";
+	} else { # special case for ALL
+		$ctx->ibx_entry($pi_cfg->ALL // die('BUG: ->ALL expected'), {});
 	}
 	my $mset = $misc->mset($qs, $opt); # sorts by $MODIFIED (mtime)
-	delete $ctx->{-list}; # reset if retried
-	my $pi_cfg = $ctx->{www}->{pi_cfg};
+	my $hide_key = $ctx->hide_key;
+
 	for my $mi ($mset->items) {
 		my $doc = $mi->get_document;
 		my ($eidx_key) = PublicInbox::Search::xap_terms('Q', $doc);
 		$eidx_key // next;
 		my $ibx = $pi_cfg->lookup_eidx_key($eidx_key) // next;
-		next if $ibx->{-hide}->{$ctx->hide_key};
-		grep(/$re/, @{$ibx->{url}}) or next;
+		next if $ibx->{-hide}->{$hide_key};
+		grep(/$re/, @{$ibx->{url} // []}) or next;
 		$ctx->ibx_entry($ibx, $misc->doc2ibx_cache_ent($doc));
 		if ($r) { # for descriptions in search_nav_bot
 			my $pct = PublicInbox::Search::get_pct($mi);
@@ -203,9 +215,8 @@ sub psgi_triple {
 			@$list = map { $_->[1] }
 				sort { $b->[0] <=> $a->[0] } @$list;
 		}
-		$list = join("\n", @$list);
-		my $l = PublicInbox::Linkify->new;
-		$gzf->zmore('<pre>'.$l->to_html($list));
+		$gzf->zmore('<pre>');
+		$gzf->zmore(join("\n", @$list));
 		$gzf->zmore(mset_footer($ctx, $mset)) if $mset;
 	} else {
 		$gzf->zmore('<pre>no inboxes, yet');
diff --git a/t/extindex-psgi.t b/t/extindex-psgi.t
index d4761641..31b04acd 100644
--- a/t/extindex-psgi.t
+++ b/t/extindex-psgi.t
@@ -28,6 +28,8 @@ run_script([qw(-extindex --all), "$tmpdir/eidx"], $env) or BAIL_OUT;
 [extindex "all"]
 	topdir = $tmpdir/eidx
 	url = http://bogus.example.com/all
+[publicinbox]
+	wwwlisting = all
 EOM
 }
 my $www = PublicInbox::WWW->new(PublicInbox::Config->new($pi_config));
@@ -55,6 +57,10 @@ my $client = sub {
 
 	$res = $cb->(GET('/all/all.mbox.gz'));
 	is($res->code, 200, 'all.mbox.gz');
+
+	$res = $cb->(GET('/'));
+	my $html = $res->content;
+	like($html, qr!\Qhttp://bogus.example.com/all\E!, 'html shows /all');
 };
 test_psgi(sub { $www->call(@_) }, $client);
 %$env = (%$env, TMPDIR => $tmpdir, PI_CONFIG => $pi_config);

[PATCH 2/2] www_listing: fix odd "locate inbox" cases

[Eric Wong]

Searching inboxes with an empty query no longer gives 500 errors
due to Xapian.  Also, improve the error message when no inboxes
match, since saying no inboxes exist yet is wrong.
---
 lib/PublicInbox/WwwListing.pm |  7 ++++++-
 t/extindex-psgi.t             | 10 ++++++++--
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/lib/PublicInbox/WwwListing.pm b/lib/PublicInbox/WwwListing.pm
index eabda98a..1bb5fbd0 100644
--- a/lib/PublicInbox/WwwListing.pm
+++ b/lib/PublicInbox/WwwListing.pm
@@ -96,7 +96,8 @@ sub add_misc_ibx { # MiscSearch->retry_reopen callback
 
 	delete $ctx->{-list}; # reset if retried
 	my $pi_cfg = $ctx->{www}->{pi_cfg};
-	if (defined(my $user_query = $q->{'q'})) {
+	my $user_query = $q->{'q'} // '';
+	if ($user_query =~ /\S/) {
 		$qs = "( $qs ) AND ( $user_query )";
 	} else { # special case for ALL
 		$ctx->ibx_entry($pi_cfg->ALL // die('BUG: ->ALL expected'), {});
@@ -218,6 +219,10 @@ sub psgi_triple {
 		$gzf->zmore('<pre>');
 		$gzf->zmore(join("\n", @$list));
 		$gzf->zmore(mset_footer($ctx, $mset)) if $mset;
+	} elsif (my $mset = delete $ctx->{-mset}) {
+		$gzf->zmore(mset_nav_top($ctx, $mset));
+		$gzf->zmore('<pre>no matching inboxes');
+		$gzf->zmore(mset_footer($ctx, $mset));
 	} else {
 		$gzf->zmore('<pre>no inboxes, yet');
 	}
diff --git a/t/extindex-psgi.t b/t/extindex-psgi.t
index 31b04acd..4e26962e 100644
--- a/t/extindex-psgi.t
+++ b/t/extindex-psgi.t
@@ -59,8 +59,14 @@ my $client = sub {
 	is($res->code, 200, 'all.mbox.gz');
 
 	$res = $cb->(GET('/'));
-	my $html = $res->content;
-	like($html, qr!\Qhttp://bogus.example.com/all\E!, 'html shows /all');
+	like($res->content, qr!\Qhttp://bogus.example.com/all\E!,
+		'/all listed');
+	$res = $cb->(GET('/?q='));
+	is($res->code, 200, 'no query means all inboxes');
+	$res = $cb->(GET('/?q=nonexistent'));
+	is($res->code, 404, 'no inboxes matched');
+	unlike($res->content, qr!no inboxes, yet!,
+		'we have inboxes, just no matches');
 };
 test_psgi(sub { $www->call(@_) }, $client);
 %$env = (%$env, TMPDIR => $tmpdir, PI_CONFIG => $pi_config);

Re: [PATCH 0/2] wwwlisting shows /all/

[Konstantin Ryabitsev]

On Fri, Aug 27, 2021 at 12:08:43PM +0000, Eric Wong wrote:
> I think that's too much vertical whitespace at the top of the
> page, and multiple <form>s or <input> boxes at the top can get
> confusing.
> 
> Just making /all/ show up at the top like a normal inbox (and
> letting the admin decide on description) seems sufficient.  If
> users can get to /all/ then they can search /all/ as normal.

Sure, this works for me, too, thanks!
https://x-lore.kernel.org/

One thing I noticed is that the "description" value for inbox entries isn't
coming from what is on disk in inboxname/description files but from
manifest.js entries. 

E.g. if you look at "lkml" you will notice that it has "LKML Archive on
lore.kernel.org" as description on the wwwlisting page, but if you click on
it, the description is "linux-kernel.vger.kernel.org archive mirror". In
theory, they shouldn't be different, but a mirror admin may choose to give
their own descriptions to mirrored lists.

Best regards,
-K

Re: [PATCH 0/2] wwwlisting shows /all/

[Konstantin Ryabitsev]

On Fri, 27 Aug 2021 at 09:03, Konstantin Ryabitsev
<konstantin@linuxfoundation.org> wrote:
> > Just making /all/ show up at the top like a normal inbox (and
> > letting the admin decide on description) seems sufficient.  If
> > users can get to /all/ then they can search /all/ as normal.
>
> Sure, this works for me, too, thanks!
> https://x-lore.kernel.org/

Hmm.. I just noticed that the "all" link has vanished. The source is showing:

<pre>* 2021-08-27 13:24 - <a
href=""></a>
  All of lore.kernel.org

Restarting public-inbox-httpd fixes it, but I wonder if it will happen again.

-K

Re: [PATCH 0/2] wwwlisting shows /all/

[Eric Wong]

Konstantin Ryabitsev <konstantin@linuxfoundation.org> wrote:
> On Fri, Aug 27, 2021 at 12:08:43PM +0000, Eric Wong wrote:
> > I think that's too much vertical whitespace at the top of the
> > page, and multiple <form>s or <input> boxes at the top can get
> > confusing.
> > 
> > Just making /all/ show up at the top like a normal inbox (and
> > letting the admin decide on description) seems sufficient.  If
> > users can get to /all/ then they can search /all/ as normal.
> 
> Sure, this works for me, too, thanks!
> https://x-lore.kernel.org/
> 
> One thing I noticed is that the "description" value for inbox entries isn't
> coming from what is on disk in inboxname/description files but from
> manifest.js entries. 

It's actually coming from $EXTINDEX_DIR/misc which is written
during -extindex invocations.

> E.g. if you look at "lkml" you will notice that it has "LKML Archive on
> lore.kernel.org" as description on the wwwlisting page, but if you click on
> it, the description is "linux-kernel.vger.kernel.org archive mirror". In
> theory, they shouldn't be different, but a mirror admin may choose to give
> their own descriptions to mirrored lists.

There might be a bug in -extindex invocations, since I'm trying
to avoid opening per-inbox description files in WWW/IMAP/NNTP
with thousands of inboxes on the horizon.

[PATCH] www: avoid potential auto-vivification on ibx->{url}

[Eric Wong]

Konstantin Ryabitsev <konstantin@linuxfoundation.org> wrote:
> On Fri, 27 Aug 2021 at 09:03, Konstantin Ryabitsev
> <konstantin@linuxfoundation.org> wrote:
> > > Just making /all/ show up at the top like a normal inbox (and
> > > letting the admin decide on description) seems sufficient.  If
> > > users can get to /all/ then they can search /all/ as normal.
> >
> > Sure, this works for me, too, thanks!
> > https://x-lore.kernel.org/
> 
> Hmm.. I just noticed that the "all" link has vanished. The source is showing:
> 
> <pre>* 2021-08-27 13:24 - <a
> href=""></a>
>   All of lore.kernel.org
> 
> Restarting public-inbox-httpd fixes it, but I wonder if it will happen again.

If it happens once, it'll happen again.  I'm not sure which code
path triggers it, but maybe this will fix it.  There's also a secondary
patch below which may be more complete, but may also be
unneeded.

--------------8<-------------
Subject: [PATCH] www: avoid potential auto-vivification on ibx->{url}

This may fix problems with the "all" link disappearing.

Link: https://public-inbox.org/meta/CAMwyc-Tw=v5yT1U1U66GSwwTK8OJXv8_YDu-=oXbZO3tHSnYWw@mail.gmail.com/
---
 lib/PublicInbox/Inbox.pm      | 3 ++-
 lib/PublicInbox/WwwListing.pm | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/PublicInbox/Inbox.pm b/lib/PublicInbox/Inbox.pm
index df140cac..b0bb9dcc 100644
--- a/lib/PublicInbox/Inbox.pm
+++ b/lib/PublicInbox/Inbox.pm
@@ -250,7 +250,8 @@ sub base_url {
 	}
 	# called from a non-PSGI environment (e.g. NNTP/POP3):
 	$self->{-base_url} ||= do {
-		my $url = $self->{url}->[0] or return undef;
+		my $url = $self->{url} // return undef;
+		$url = $url->[0] // return undef;
 		# expand protocol-relative URLs to HTTPS if we're
 		# not inside a web server
 		$url = "https:$url" if $url =~ m!\A//!;
diff --git a/lib/PublicInbox/WwwListing.pm b/lib/PublicInbox/WwwListing.pm
index 1bb5fbd0..fe5ee087 100644
--- a/lib/PublicInbox/WwwListing.pm
+++ b/lib/PublicInbox/WwwListing.pm
@@ -41,7 +41,7 @@ sub list_match_i { # ConfigIter callback
 		return if $section !~ m!\Apublicinbox\.([^/]+)\z!;
 		my $ibx = $cfg->lookup_name($1) or return;
 		if (!$ibx->{-hide}->{$ctx->hide_key} &&
-					grep(/$re/, @{$ibx->{url}})) {
+					grep(/$re/, @{$ibx->{url} // []})) {
 			$ctx->ibx_entry($ibx);
 		}
 	} else { # undef == "EOF"

--------8<------
Secondary patch in case extindex $ibx->{url} got vivified somewhere:

diff --git a/lib/PublicInbox/WwwListing.pm b/lib/PublicInbox/WwwListing.pm
index fe5ee087..ef9048b5 100644
--- a/lib/PublicInbox/WwwListing.pm
+++ b/lib/PublicInbox/WwwListing.pm
@@ -17,7 +17,7 @@ sub ibx_entry {
 	my $desc = ascii_html($ce->{description} //= $ibx->description);
 	my $ts = fmt_ts($ce->{-modified} //= $ibx->modified);
 	my ($url, $href);
-	if (defined($ibx->{url})) {
+	if (scalar(@{$ibx->{url} // []})) {
 		$url = $href = ascii_html(prurl($ctx->{env}, $ibx->{url}));
 	} else {
 		$href = ascii_html(uri_escape_utf8($ibx->{name})) . '/';

Re: [PATCH] www: avoid potential auto-vivification on ibx->{url}

[Konstantin Ryabitsev]

On Fri, Aug 27, 2021 at 10:03:02PM +0000, Eric Wong wrote:
> If it happens once, it'll happen again.  I'm not sure which code
> path triggers it, but maybe this will fix it.  There's also a secondary
> patch below which may be more complete, but may also be
> unneeded.

Applied what was between scissors to x-lore.kernel.org (Dallas replica only).
Will report how it goes.

Thanks,
-K

[PATCH] www_listing: support /all/ search as a 302 redirect

[Eric Wong]

On Fri, 27 Aug 2021 12:08:43 +0000, Eric Wong wrote:
> Konstantin Ryabitsev wrote:
> > I switched the configuration to return wwwlisting as toplevel view (instead of
> > redirecting / to /all/), but there's some discontent, because the easy
> > interface to "search everything" is gone and unless someone knows about /all/,
> > they wouldn't find out about it from the wwwlisting view.
> > 
> > How about, if there is an extindex "all" defined, the wwwlisting adds an extra
> > search box:
> > 
> > [_________________] [search all] help
> > 
> > * 2021-08-26 16:54 - https://x-lore.kernel.org/all/
> >   All of lore.kernel.org
> > 
> > ------------------------------------------------------
> > 
> > [_________________] [locate inbox]
> > 
> 
> I think that's too much vertical whitespace at the top of the
> page, and multiple <form>s or <input> boxes at the top can get
> confusing.
> 
> Just making /all/ show up at the top like a normal inbox (and
> letting the admin decide on description) seems sufficient.  If
> users can get to /all/ then they can search /all/ as normal.
> 
> I tried moving the <input>s next to infourl, but mixing <form>
> and <pre> introduces a lot of vertical whitespace.
> 
> 
> I also tried adding radio button (or drop-down):
> 
> 	[_________________] [*] inboxes [ ] /all/   [search]
> 
> But extra <form> elements always confuse me whenever I encounter
> them in any browser, so I decided to just leave things alone.

-------------8<------------
Subject: [PATCH] www_listing: support /all/ search as a 302 redirect

This allows users to search /all/ from the top-level WwwListing
without extra manual steps, although there's still extra network
roundtrips incurred.

No vertical whitespace is added, and there's no clumsy radio
buttons nor menus to deal with.  Users only have to use a
different <input type=submit /> button.  I forgot how to do this
until I realized we already do something similar with multiple
submit buttons for threaded vs non-threaded mboxrd.gz downloads.

Link: https://public-inbox.org/meta/20210827120845.29682-1-e@80x24.org/
---
 lib/PublicInbox/WwwListing.pm | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/lib/PublicInbox/WwwListing.pm b/lib/PublicInbox/WwwListing.pm
index a9290802..79c0a8ec 100644
--- a/lib/PublicInbox/WwwListing.pm
+++ b/lib/PublicInbox/WwwListing.pm
@@ -11,6 +11,7 @@ use PublicInbox::GzipFilter qw(gzf_maybe);
 use PublicInbox::ConfigIter;
 use PublicInbox::WwwStream;
 use URI::Escape qw(uri_escape_utf8);
+use PublicInbox::MID qw(mid_escape);
 
 sub ibx_entry {
 	my ($ctx, $ibx, $ce) = @_;
@@ -135,6 +136,13 @@ sub response {
 	my ($re, $qs) = $ctx->url_filter;
 	$re // return $ctx->psgi_triple;
 	if (my $ALL = $ctx->{www}->{pi_cfg}->ALL) { # fast path
+		if ($ctx->{qp}->{a} && # "search all inboxes"
+				$ctx->{qp}->{'q'}) {
+			my $u = 'all/?q='.mid_escape($ctx->{qp}->{'q'});
+			return [ 302, [ 'Location' => $u,
+				qw(Content-Type text/plain) ],
+				[ "Redirecting to $u\n" ] ];
+		}
 		# FIXME: test this in t/
 		$ALL->misc->reopen->retry_reopen(\&add_misc_ibx,
 						$ctx, $re, $qs);
@@ -166,11 +174,10 @@ sub mset_nav_top {
 	$qh = qq[\nvalue="$qh"] if $qh ne '';
 	my $rv = <<EOM;
 <form
-action="./"><pre><input
-name=q
-type=text$qh /><input
-type=submit
-value="locate inbox" /></pre></form><pre>
+action="./"><pre><input name=q type=text$qh
+/><input type=submit value="locate inbox"
+/><input type=submit name=a value="search all inboxes"
+/></pre></form><pre>
 EOM
 	chomp $rv;
 	if (defined($q->{'q'})) {