[RFC] ui-repolist: Allow sections to be collapsible

[RFC] ui-repolist: Allow sections to be collapsible

[andy.doan]

This is a rough work-in-progress, but I wanted to get our take on whether
or not you'd be interested in this type of functionality:

The index page can be difficult to navigate for really large git
servers. This change allows a configuration like:

 section-collapse=people
 section-collapse=tests

And an index page would only display the "people" and "test" section
headers entries (not their repos) with a hyperlink that can be used to
drill down into each section.

Signed-off-by: Andy Doan <andy.doan at linaro.org>
---
 cgit.c        | 2 ++
 cgit.h        | 1 +
 cgitrc.5.txt  | 5 +++++
 ui-repolist.c | 4 +++-
 4 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/cgit.c b/cgit.c
index 9427c4a..abecc64 100644
--- a/cgit.c
+++ b/cgit.c
@@ -238,6 +238,8 @@ static void config_cb(const char *name, const char *value)
 			scan_tree(expand_macros(value), repo_config);
 	else if (!strcmp(name, "scan-hidden-path"))
 		ctx.cfg.scan_hidden_path = atoi(value);
+	else if (!strcmp(name, "section-collapse"))
+		string_list_insert(&ctx.cfg.section_collapse, xstrdup(value));
 	else if (!strcmp(name, "section-from-path"))
 		ctx.cfg.section_from_path = atoi(value);
 	else if (!strcmp(name, "repository-sort"))
diff --git a/cgit.h b/cgit.h
index 325432b..5c654be 100644
--- a/cgit.h
+++ b/cgit.h
@@ -251,6 +251,7 @@ struct cgit_config {
 	int renamelimit;
 	int remove_suffix;
 	int scan_hidden_path;
+	struct string_list section_collapse;
 	int section_from_path;
 	int snapshots;
 	int section_sort;
diff --git a/cgitrc.5.txt b/cgitrc.5.txt
index 9fcf445..2762657 100644
--- a/cgitrc.5.txt
+++ b/cgitrc.5.txt
@@ -404,6 +404,11 @@ section::
 	after this option will inherit the current section name. Default value:
 	none.

+section-collapse::
+	Name of a section to "collapse" and not display on the index page.
+	Multiple config entries can be specified and each one will be
+	collapsed.
+
 section-sort::
 	Flag which, when set to "1", will sort the sections on the repository
 	listing by name. Set this flag to "0" if the order in the cgitrc file should
diff --git a/ui-repolist.c b/ui-repolist.c
index 30915df..330f749 100644
--- a/ui-repolist.c
+++ b/ui-repolist.c
@@ -322,10 +322,12 @@ void cgit_print_repolist(void)
 		     strcmp(section, last_section)))) {
 			htmlf("<tr class='nohover'><td colspan='%d' class='reposection'>",
 			      columns);
-			html_txt(section);
+			htmlf("<a href='%s'>%s</a>", section, section);
 			html("</td></tr>");
 			last_section = section;
 		}
+		if (section && string_list_has_string(&ctx.cfg.section_collapse, section))
+			continue;
 		htmlf("<tr><td class='%s'>",
 		      !sorted && section ? "sublevel-repo" : "toplevel-repo");
 		cgit_summary_link(ctx.repo->name, ctx.repo->name, NULL, NULL);
--
2.7.4

[RFC] ui-repolist: Allow sections to be collapsible

[john]

On Sun, Aug 07, 2016 at 02:33:41PM -0500, Andy Doan wrote:
> This is a rough work-in-progress, but I wanted to get our take on whether
> or not you'd be interested in this type of functionality:
> 
> The index page can be difficult to navigate for really large git
> servers. This change allows a configuration like:
> 
>  section-collapse=people
>  section-collapse=tests
> 
> And an index page would only display the "people" and "test" section
> headers entries (not their repos) with a hyperlink that can be used to
> drill down into each section.

My initial thought is that the name is wrong, when I saw the summary I
assumed it would be about adding script to allow sections to be
dynamically expanded/collapsed.  I can't immediately think of a better
name, though.

I also wonder why this takes a list of sections rather than a global
boolean that applies to all sections.  That may be because I tend to use
section-from-path though.

Overall, the implementation seems remarkably simple (and the change to
make section titles into links should probably be a separate patch) and
I think it would be reasonable to do something like this.  But I do
wonder if a global boolean would make more sense given the range of ways
it is possible to configure sections.  Do you have a use case that
requires only some sections to have their contents hidden in the main
index?

> Signed-off-by: Andy Doan <andy.doan at linaro.org>
> ---
>  cgit.c        | 2 ++
>  cgit.h        | 1 +
>  cgitrc.5.txt  | 5 +++++
>  ui-repolist.c | 4 +++-
>  4 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/cgit.c b/cgit.c
> index 9427c4a..abecc64 100644
> --- a/cgit.c
> +++ b/cgit.c
> @@ -238,6 +238,8 @@ static void config_cb(const char *name, const char *value)
>  			scan_tree(expand_macros(value), repo_config);
>  	else if (!strcmp(name, "scan-hidden-path"))
>  		ctx.cfg.scan_hidden_path = atoi(value);
> +	else if (!strcmp(name, "section-collapse"))
> +		string_list_insert(&ctx.cfg.section_collapse, xstrdup(value));
>  	else if (!strcmp(name, "section-from-path"))
>  		ctx.cfg.section_from_path = atoi(value);
>  	else if (!strcmp(name, "repository-sort"))
> diff --git a/cgit.h b/cgit.h
> index 325432b..5c654be 100644
> --- a/cgit.h
> +++ b/cgit.h
> @@ -251,6 +251,7 @@ struct cgit_config {
>  	int renamelimit;
>  	int remove_suffix;
>  	int scan_hidden_path;
> +	struct string_list section_collapse;
>  	int section_from_path;
>  	int snapshots;
>  	int section_sort;
> diff --git a/cgitrc.5.txt b/cgitrc.5.txt
> index 9fcf445..2762657 100644
> --- a/cgitrc.5.txt
> +++ b/cgitrc.5.txt
> @@ -404,6 +404,11 @@ section::
>  	after this option will inherit the current section name. Default value:
>  	none.
> 
> +section-collapse::
> +	Name of a section to "collapse" and not display on the index page.
> +	Multiple config entries can be specified and each one will be
> +	collapsed.
> +
>  section-sort::
>  	Flag which, when set to "1", will sort the sections on the repository
>  	listing by name. Set this flag to "0" if the order in the cgitrc file should
> diff --git a/ui-repolist.c b/ui-repolist.c
> index 30915df..330f749 100644
> --- a/ui-repolist.c
> +++ b/ui-repolist.c
> @@ -322,10 +322,12 @@ void cgit_print_repolist(void)
>  		     strcmp(section, last_section)))) {
>  			htmlf("<tr class='nohover'><td colspan='%d' class='reposection'>",
>  			      columns);
> -			html_txt(section);
> +			htmlf("<a href='%s'>%s</a>", section, section);
>  			html("</td></tr>");
>  			last_section = section;
>  		}
> +		if (section && string_list_has_string(&ctx.cfg.section_collapse, section))
> +			continue;
>  		htmlf("<tr><td class='%s'>",
>  		      !sorted && section ? "sublevel-repo" : "toplevel-repo");
>  		cgit_summary_link(ctx.repo->name, ctx.repo->name, NULL, NULL);
> --
> 2.7.4
> 
> _______________________________________________
> CGit mailing list
> CGit at lists.zx2c4.com
> http://lists.zx2c4.com/mailman/listinfo/cgit

[RFC] ui-repolist: Allow sections to be collapsible

[andy.doan]

On 08/07/2016 02:57 PM, John Keeping wrote:
> On Sun, Aug 07, 2016 at 02:33:41PM -0500, Andy Doan wrote:
>> This is a rough work-in-progress, but I wanted to get our take on whether
>> or not you'd be interested in this type of functionality:
>>
>> The index page can be difficult to navigate for really large git
>> servers. This change allows a configuration like:
>>
>>  section-collapse=people
>>  section-collapse=tests
>>
>> And an index page would only display the "people" and "test" section
>> headers entries (not their repos) with a hyperlink that can be used to
>> drill down into each section.
>
> My initial thought is that the name is wrong, when I saw the summary I
> assumed it would be about adding script to allow sections to be
> dynamically expanded/collapsed.  I can't immediately think of a better
> name, though.

I originally thought to just send the idea, and then I realized w/o code 
it would be too confusing. I considered using "section-folds" instead, 
but it didn't seem much better.

> I also wonder why this takes a list of sections rather than a global
> boolean that applies to all sections.  That may be because I tend to use
> section-from-path though.
>
> Overall, the implementation seems remarkably simple (and the change to
> make section titles into links should probably be a separate patch) and
> I think it would be reasonable to do something like this.  But I do
> wonder if a global boolean would make more sense given the range of ways
> it is possible to configure sections.

I agree. I don't want to change default behavior of production sites. 
I'll split this out in my next attempt.

> Do you have a use case that
> requires only some sections to have their contents hidden in the main
> index?

Yes. I manage Linaro's git server[1]. Its sort of grown into something 
where we have important "advertised" type repos and tons of "personal" 
repos. I want to keep some of our most important ones visible on the 
front page, but "collapse" things such as the personal repos. This 
seemed like the least invasive way to make something like that happen.

I'll start up a new revision this week with the assumption you aren't 
totally against the list of sections.

1: https://git.linaro.org/

>> Signed-off-by: Andy Doan <andy.doan at linaro.org>
>> ---
>>  cgit.c        | 2 ++
>>  cgit.h        | 1 +
>>  cgitrc.5.txt  | 5 +++++
>>  ui-repolist.c | 4 +++-
>>  4 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/cgit.c b/cgit.c
>> index 9427c4a..abecc64 100644
>> --- a/cgit.c
>> +++ b/cgit.c
>> @@ -238,6 +238,8 @@ static void config_cb(const char *name, const char *value)
>>  			scan_tree(expand_macros(value), repo_config);
>>  	else if (!strcmp(name, "scan-hidden-path"))
>>  		ctx.cfg.scan_hidden_path = atoi(value);
>> +	else if (!strcmp(name, "section-collapse"))
>> +		string_list_insert(&ctx.cfg.section_collapse, xstrdup(value));
>>  	else if (!strcmp(name, "section-from-path"))
>>  		ctx.cfg.section_from_path = atoi(value);
>>  	else if (!strcmp(name, "repository-sort"))
>> diff --git a/cgit.h b/cgit.h
>> index 325432b..5c654be 100644
>> --- a/cgit.h
>> +++ b/cgit.h
>> @@ -251,6 +251,7 @@ struct cgit_config {
>>  	int renamelimit;
>>  	int remove_suffix;
>>  	int scan_hidden_path;
>> +	struct string_list section_collapse;
>>  	int section_from_path;
>>  	int snapshots;
>>  	int section_sort;
>> diff --git a/cgitrc.5.txt b/cgitrc.5.txt
>> index 9fcf445..2762657 100644
>> --- a/cgitrc.5.txt
>> +++ b/cgitrc.5.txt
>> @@ -404,6 +404,11 @@ section::
>>  	after this option will inherit the current section name. Default value:
>>  	none.
>>
>> +section-collapse::
>> +	Name of a section to "collapse" and not display on the index page.
>> +	Multiple config entries can be specified and each one will be
>> +	collapsed.
>> +
>>  section-sort::
>>  	Flag which, when set to "1", will sort the sections on the repository
>>  	listing by name. Set this flag to "0" if the order in the cgitrc file should
>> diff --git a/ui-repolist.c b/ui-repolist.c
>> index 30915df..330f749 100644
>> --- a/ui-repolist.c
>> +++ b/ui-repolist.c
>> @@ -322,10 +322,12 @@ void cgit_print_repolist(void)
>>  		     strcmp(section, last_section)))) {
>>  			htmlf("<tr class='nohover'><td colspan='%d' class='reposection'>",
>>  			      columns);
>> -			html_txt(section);
>> +			htmlf("<a href='%s'>%s</a>", section, section);
>>  			html("</td></tr>");
>>  			last_section = section;
>>  		}
>> +		if (section && string_list_has_string(&ctx.cfg.section_collapse, section))
>> +			continue;
>>  		htmlf("<tr><td class='%s'>",
>>  		      !sorted && section ? "sublevel-repo" : "toplevel-repo");
>>  		cgit_summary_link(ctx.repo->name, ctx.repo->name, NULL, NULL);
>> --
>> 2.7.4
>>
>> _______________________________________________
>> CGit mailing list
>> CGit at lists.zx2c4.com
>> http://lists.zx2c4.com/mailman/listinfo/cgit

[RFC] ui-repolist: Allow sections to be collapsible

[john]

On Sun, Aug 07, 2016 at 10:02:34PM -0500, Andy Doan wrote:
> On 08/07/2016 02:57 PM, John Keeping wrote:
> > On Sun, Aug 07, 2016 at 02:33:41PM -0500, Andy Doan wrote:
> >> This is a rough work-in-progress, but I wanted to get our take on whether
> >> or not you'd be interested in this type of functionality:
> >>
> >> The index page can be difficult to navigate for really large git
> >> servers. This change allows a configuration like:
> >>
> >>  section-collapse=people
> >>  section-collapse=tests
> >>
> >> And an index page would only display the "people" and "test" section
> >> headers entries (not their repos) with a hyperlink that can be used to
> >> drill down into each section.
> >
> > My initial thought is that the name is wrong, when I saw the summary I
> > assumed it would be about adding script to allow sections to be
> > dynamically expanded/collapsed.  I can't immediately think of a better
> > name, though.
> 
> I originally thought to just send the idea, and then I realized w/o code 
> it would be too confusing. I considered using "section-folds" instead, 
> but it didn't seem much better.

Yeah, I thought about "nested" or "hidden" but those seem misleading.
Collapsed may be the best option.

> > I also wonder why this takes a list of sections rather than a global
> > boolean that applies to all sections.  That may be because I tend to use
> > section-from-path though.
> >
> > Overall, the implementation seems remarkably simple (and the change to
> > make section titles into links should probably be a separate patch) and
> > I think it would be reasonable to do something like this.  But I do
> > wonder if a global boolean would make more sense given the range of ways
> > it is possible to configure sections.
> 
> I agree. I don't want to change default behavior of production sites. 
> I'll split this out in my next attempt.

I don't think we need to worry about changing a section heading into a
link, it seems like a reasonable incremental improvement so I wouldn't
worry about making that configurable.  It just seemed like a logically
separate change to me.

> > Do you have a use case that
> > requires only some sections to have their contents hidden in the main
> > index?
> 
> Yes. I manage Linaro's git server[1]. Its sort of grown into something 
> where we have important "advertised" type repos and tons of "personal" 
> repos. I want to keep some of our most important ones visible on the 
> front page, but "collapse" things such as the personal repos. This 
> seemed like the least invasive way to make something like that happen.
> 
> I'll start up a new revision this week with the assumption you aren't 
> totally against the list of sections.

I thought about this a bit more and I wonder if it would be more natural
to configure this with something like:

	section.collapse = 1

We'd need to change the current "const char *section" into something
like:

	struct cgit_section {
		unsigned int collapse : 1;
		char name[];
	};

and I haven't thought too carefully about how exactly we parse the
"section.collapse" directive (e.g. does it apply to the current section
or does it apply to all future sections?  The former seems more natural
initially but the latter would make it useful with section-from-path).

What do you think?

[RFC] ui-repolist: Allow sections to be collapsible

[andy.doan]

On 08/08/2016 03:44 AM, John Keeping wrote:
> I thought about this a bit more and I wonder if it would be more natural
> to configure this with something like:
>
> 	section.collapse = 1
>
> We'd need to change the current "const char *section" into something
> like:
>
> 	struct cgit_section {
> 		unsigned int collapse : 1;
> 		char name[];
> 	};
>
> and I haven't thought too carefully about how exactly we parse the
> "section.collapse" directive (e.g. does it apply to the current section
> or does it apply to all future sections?  The former seems more natural
> initially but the latter would make it useful with section-from-path).
>
> What do you think?

I'm a little confused, but its probably my lack of experience with cgit 
configuration capabilities. I currently take advantage "scan-path" to 
find everything so I was using something like:

  section-from-path=1
  scan-path=/srv/repositories
  section-collapse=pkg
  section-collapse=people
  section-collapse=ubuntu

I'm not sure how I could accomplish that with your suggestion?

[RFC] ui-repolist: Allow sections to be collapsible

[john]

On Mon, Aug 08, 2016 at 10:28:17PM -0500, Andy Doan wrote:
> On 08/08/2016 03:44 AM, John Keeping wrote:
> > I thought about this a bit more and I wonder if it would be more natural
> > to configure this with something like:
> >
> > 	section.collapse = 1
> >
> > We'd need to change the current "const char *section" into something
> > like:
> >
> > 	struct cgit_section {
> > 		unsigned int collapse : 1;
> > 		char name[];
> > 	};
> >
> > and I haven't thought too carefully about how exactly we parse the
> > "section.collapse" directive (e.g. does it apply to the current section
> > or does it apply to all future sections?  The former seems more natural
> > initially but the latter would make it useful with section-from-path).
> >
> > What do you think?
> 
> I'm a little confused, but its probably my lack of experience with cgit 
> configuration capabilities. I currently take advantage "scan-path" to 
> find everything so I was using something like:
> 
>   section-from-path=1
>   scan-path=/srv/repositories
>   section-collapse=pkg
>   section-collapse=people
>   section-collapse=ubuntu
> 
> I'm not sure how I could accomplish that with your suggestion?

That might be covered by my "haven't thought too carefully", but I was
thinking of maybe setting "section.collapse = 1" and then any new
sections created by scan-path would have that bit set.

However, that would require you to have separate scan-path directives to
find the repositories that should collapse and those that shouldn't.

I think you've convinced me that listing section names in the config is
the way to go.  But I still think we should invent the cgit_section
structure to hold the configuration, presumably accessed via a function
like:

	struct cgit_section *get_or_create_section(const char *name)

[RFC] ui-repolist: Allow sections to be collapsible

[andy.doan]

On 08/09/2016 01:10 PM, John Keeping wrote:
> On Mon, Aug 08, 2016 at 10:28:17PM -0500, Andy Doan wrote:
>> On 08/08/2016 03:44 AM, John Keeping wrote:
>>> I thought about this a bit more and I wonder if it would be more natural
>>> to configure this with something like:
>>>
>>> 	section.collapse = 1
>>>
>>> We'd need to change the current "const char *section" into something
>>> like:
>>>
>>> 	struct cgit_section {
>>> 		unsigned int collapse : 1;
>>> 		char name[];
>>> 	};
>>>
>>> and I haven't thought too carefully about how exactly we parse the
>>> "section.collapse" directive (e.g. does it apply to the current section
>>> or does it apply to all future sections?  The former seems more natural
>>> initially but the latter would make it useful with section-from-path).
>>>
>>> What do you think?
>>
>> I'm a little confused, but its probably my lack of experience with cgit
>> configuration capabilities. I currently take advantage "scan-path" to
>> find everything so I was using something like:
>>
>>   section-from-path=1
>>   scan-path=/srv/repositories
>>   section-collapse=pkg
>>   section-collapse=people
>>   section-collapse=ubuntu
>>
>> I'm not sure how I could accomplish that with your suggestion?
>
> That might be covered by my "haven't thought too carefully", but I was
> thinking of maybe setting "section.collapse = 1" and then any new
> sections created by scan-path would have that bit set.
>
> However, that would require you to have separate scan-path directives to
> find the repositories that should collapse and those that shouldn't.
>
> I think you've convinced me that listing section names in the config is
> the way to go.  But I still think we should invent the cgit_section
> structure to hold the configuration, presumably accessed via a function
> like:
>
> 	struct cgit_section *get_or_create_section(const char *name)

Okay. I *think* have something inline with your thinking now. Its a bit 
more invasive. I'll send patches in separate email.

[RFC] ui-repolist: Allow sections to be collapsible

[andy.doan]

On 08/09/2016 01:10 PM, John Keeping wrote:
> On Mon, Aug 08, 2016 at 10:28:17PM -0500, Andy Doan wrote:
>> On 08/08/2016 03:44 AM, John Keeping wrote:
>>> I thought about this a bit more and I wonder if it would be more natural
>>> to configure this with something like:
>>>
>>> 	section.collapse = 1
>>>
>>> We'd need to change the current "const char *section" into something
>>> like:
>>>
>>> 	struct cgit_section {
>>> 		unsigned int collapse : 1;
>>> 		char name[];
>>> 	};
>>>
>>> and I haven't thought too carefully about how exactly we parse the
>>> "section.collapse" directive (e.g. does it apply to the current section
>>> or does it apply to all future sections?  The former seems more natural
>>> initially but the latter would make it useful with section-from-path).
>>>
>>> What do you think?
>>
>> I'm a little confused, but its probably my lack of experience with cgit
>> configuration capabilities. I currently take advantage "scan-path" to
>> find everything so I was using something like:
>>
>>   section-from-path=1
>>   scan-path=/srv/repositories
>>   section-collapse=pkg
>>   section-collapse=people
>>   section-collapse=ubuntu
>>
>> I'm not sure how I could accomplish that with your suggestion?
>
> That might be covered by my "haven't thought too carefully", but I was
> thinking of maybe setting "section.collapse = 1" and then any new
> sections created by scan-path would have that bit set.
>
> However, that would require you to have separate scan-path directives to
> find the repositories that should collapse and those that shouldn't.
>
> I think you've convinced me that listing section names in the config is
> the way to go.  But I still think we should invent the cgit_section
> structure to hold the configuration, presumably accessed via a function
> like:
>
> 	struct cgit_section *get_or_create_section(const char *name)

Hey John - I think I've addressed this in these two changes, but I think 
they may have gotten missed while you were on holiday:

  https://lists.zx2c4.com/pipermail/cgit/2016-August/003236.html
  https://lists.zx2c4.com/pipermail/cgit/2016-August/003237.html