[Abook-devel] New feature

[l***@mailoo.org]

Hello everyone,
I started using abook a while ago and I felt, while viewing the index,
that I was loosing of my terminal width. So I made a patch that scale
the size of the fields of the index to fill the whole terminal width,
while keeping their respective size.
The patch should apply on master : if that is not the right branch to
develop on, please tell me.
Once the patch applied, you need to add the option "extend_index" to
true to see its effects.

Here is the patch (you can also find the code at my github [1], in the
percents branch) :
---
diff --git a/abook.c b/abook.c
index 3749561..4f7cf94 100644
--- a/abook.c
+++ b/abook.c
@@ -138,11 +138,11 @@ init_abook()

signal(SIGTERM, quit_abook_sig);

- init_index();
-
if(init_ui())
exit(EXIT_FAILURE);

+ init_index();
+
umask(DEFAULT_UMASK);

if(!datafile_writeable()) {
diff --git a/list.c b/list.c
index 1f0af79..04c1fd2 100644
--- a/list.c
+++ b/list.c
@@ -34,6 +34,32 @@ static WINDOW *list = NULL;


static void
+extend_index()
+{
+ if(!index_elements)
+ return;
+
+ struct index_elem* cur;
+ int total = 0, size = COLS;
+ float fact, nlen;
+
+ for(cur = index_elements; cur; cur = cur->next) {
+ if(cur->type == INDEX_TEXT)
+ size -= strlen(cur->d.text);
+ else
+ total += cur->d.field.len;
+ }
+
+ fact = (float)size / (float)total;
+ for(cur = index_elements; cur; cur = cur->next) {
+ if(cur->type != INDEX_TEXT) {
+ nlen = (float)cur->d.field.len * fact;
+ cur->d.field.len = (int)nlen;
+ }
+ }
+}
+
+static void
index_elem_add(int type, char *a, char *b)
{
struct index_elem *tmp = NULL, *cur, *cur2;
@@ -82,7 +108,7 @@ index_elem_add(int type, char *a, char *b)
}

static void
-parse_index_format(char *s)
+parse_index_format(char *s, bool extend)
{
char *p, *start, *lstart = NULL;
int in_field = 0, in_alternate = 0, in_length = 0, type;
@@ -117,13 +143,16 @@ parse_index_format(char *s)
}
if(!in_field)
index_elem_add(INDEX_TEXT, start, NULL);
+
+ if(extend == TRUE)
+ extend_index();
}

void
init_index()
{
assert(!index_elements);
- parse_index_format(opt_get_str(STR_INDEX_FORMAT));
+ parse_index_format(opt_get_str(STR_INDEX_FORMAT), opt_get_bool(BOOL_EXTEND_INDEX));
}

void
@@ -132,6 +161,8 @@ init_list()
list = newwin(LIST_LINES, LIST_COLS, LIST_TOP, 0);
scrollok(list, TRUE);
scroll_speed = abs(opt_get_int(INT_SCROLL_SPEED));
+ if(opt_get_bool(BOOL_EXTEND_INDEX))
+ extend_index();
}

void
diff --git a/options.c b/options.c
index 6f75a06..08cab63 100644
--- a/options.c
+++ b/options.c
@@ -51,6 +51,7 @@ static struct option abook_vars[] = {

{ "show_all_emails", OT_BOOL, BOOL_SHOW_ALL_EMAILS, TRUE },
{ "index_format", OT_STR, STR_INDEX_FORMAT, UL " {name:22} {email:40} {phone:12|workphone|mobile}" },
+ { "extend_index", OT_BOOL, BOOL_EXTEND_INDEX, FALSE},
{ "mutt_command", OT_STR, STR_MUTT_COMMAND, UL "mutt" },
{ "mutt_return_all_emails", OT_BOOL, BOOL_MUTT_RETURN_ALL_EMAILS,
TRUE },
diff --git a/options.h b/options.h
index 78515a9..6b0dddd 100644
--- a/options.h
+++ b/options.h
@@ -28,6 +28,7 @@ enum bool_opts {
BOOL_SHOW_CURSOR,
BOOL_USE_COLORS,
BOOL_USE_MOUSE,
+ BOOL_EXTEND_INDEX,
BOOL_MAX
};
---

If the patch is too big, or not in the right format, please tell me.

[1] https://github.com/lucas8/abook_extended_index

[Raphaël Droz]

Post by l***@mailoo.org
I started using abook a while ago and I felt, while viewing the index,
that I was loosing of my terminal width. So I made a patch that scale
the size of the fields of the index to fill the whole terminal width,
while keeping their respective size.

Hi Luc, thank you for the contribution.
Indeed, this is perfectly relevant...

[...]

Post by l***@mailoo.org
you need to add the option "extend_index" to true to see its effects.

but I feel uncomfortable with this because the consequence is a
directive contradicting another one.
extend_index said to abook "do not (strictly) respect index_format".
Eg: If I set index_format="{email:40}", extend_index=true tells to abook
to not (strictly) respect it and that paves the way to "configuration hell".


IHMO if at least one width was defined in index_format we can *not*
auto-expand "blindly" (= at the "line" level), that is to say that field
expansion would probably better fit as a hint on the field level.

The issue here is that a numeric width says 2 distinct things:
- fill texts with space to align the fields in columns of equal widths
- give these columns the specific fixed widths
Columning without user-defined widths would imply to be compute them
and that would bring us to the complexity of presentation rendering.
But abook is not a ncurses-based <table> renderer.


I see (at least) three points of view and thus three possibilities
(all three far from perfect if compared to a true flexible presentation engine)


option 1:
- index_format isn't negotiable. Auto-expansion (extended index) would
be implicit *only if* no width has been specified in none of the fields
specified in index_format.
That exclude any unwanted edge-case related to index_format to
arise... but is there many people really using a presentation without
columns (no width = no column alignment) ?


option 2:
- There is not point in respecting an apparently restrictive
user-configured option about width if we are really more intelligent.
Thus, if:
1) we can consistently guess COLS (linux-term, *term, screen, ...)
*and*
2) COLS give us space
then apply the auto-expansion. This is what the provided patch is the
nearest from (excluding the introduction of the "extend_index" directive
itself).

I can see at least one "strange" actual consequence:
If one specify widths for all *but* the last field, extend_index will
expand one of the previous (fixed-width) fields but strip characters
from the last one.
= the problem of "choosing which field to give space to".


option 3:
- index_format syntax is expanded. auto-expansion, if preferred by the
user, must be explicitly stated.
Eg: index_format={name:40+}, where the "+" allows *explicitly* the
expansion of this *specific* field (if sufficient width available)

This auto-expansion could apply:
- either only and fully for the first "+"-suffixed field found in index_format
- as fairly as possible among all "+"-suffixed fields from index_format
Eg "name" and "email" for index_format="{name:30+} {phone:12} {email:40+}"

Given the parser, this could be a concern for backward compatibility
of newer abookrc making use of this syntax with older abook.
Could it be worth the change depends on whether many people share an
unique abookrc among several distributions (= several abook versions)



Please note that 0.6.0pre2 deprecated 4 presentation-related directives
in favor of index_format. I see this as a direction to follow and as
such I'd rather favor option 3.
But I expect other subscribed users to express their insightful opinions.


I hope we could first tackle this before going any further with the
implementation proposed.



thank you for putting this whole thing to the light
best regards

Post by l***@mailoo.org
The patch should apply on master : if that is not the right branch to
develop on, please tell me.

This is the right branch to work on.

Post by l***@mailoo.org
If the patch is too big, or not in the right format, please tell me.

`git am` refused it.
I guess this is caused by =20 characters present all
along the inlined diff. Your MUA or your webmail may be to blame.

[l***@mailoo.org]

Post by Raphaël Droz
- index_format syntax is expanded. auto-expansion, if preferred by the
user, must be explicitly stated.
Eg: index_format={name:40+}, where the "+" allows *explicitly* the
expansion of this *specific* field (if sufficient width available)
- either only and fully for the first "+"-suffixed field found in index_format
- as fairly as possible among all "+"-suffixed fields from index_format
Eg "name" and "email" for index_format="{name:30+} {phone:12} {email:40+}"

I implemented this approach to see how hard and/or complex it would be.
It is in fact quite complex, as it needs to change the index_elem
structure. The parser with the modified structure is in the parser.patch
file. The new implementation of extend_index is in the extend.patch
file.

Now, if the width of the terminal is smaller than the sum of the width
setted, nothing resizing will be done. Furthermore, it will only resize
the columns with a + symbol on their width (as you described). The
resizing still care about ratio of the columns to resize, but I could
change that depending on your (or other) propositions.

Post by Raphaël Droz
Given the parser, this could be a concern for backward compatibility
of newer abookrc making use of this syntax with older abook.
Could it be worth the change depends on whether many people share an
unique abookrc among several distributions (= several abook versions)

I tried it with an abook version without the path : it still launch, but
there is no column presentation. The index_format is simply ignored
because of its wrong format.

Post by Raphaël Droz

Post by l***@mailoo.org
If the patch is too big, or not in the right format, please tell me.

`git am` refused it.
I guess this is caused by =20 characters present all
along the inlined diff. Your MUA or your webmail may be to blame.

Is it alright now ?

[Raphaël]

patch that scale the size of the fields of the index to fill the whole
terminal width, while keeping their respective size.

All the additional complexity lies in "keeping their respective size".


You'll find attached my suggestion with aims to help in one case
(related to the initial report).

If we have space available and that index_format ends with a field,
then expand the field up to COLS - 1.

... but does not try anything about "respective size": the Bonus space
goes to the last field.

If n is -1, then the entire string will be added, up to the
maximum number of characters that will fit on the line, or until a
terminating null is reached.

(don't know how to interpret "line" then)


Note 2: using COLS (rather than COLS - 1), a superfluous newline is
added after the last entry.


Long-run solutions about reduced width seem way more around
horizontal-scrolling and/or live column hiding.


There's also another thing to consider.

Contrary to a table where width could be expressed in %, abook uses an
index format with fixed width only.
We set a number of characters for an email or for a phone number, but
*not* to one proportionally to the other

Eg: "{name:20} {phone:10} {nick:10}" is in no way the same (semantically) than
"{name:50%} {phone:25%} {nick:25%}"
Moreover many fields are useless without their last characters.
If {phone}, for example, is striped down to 9 characters, we get into
troubles. Better for us to get the nickname stripped.

I think that any complexity added by trying to preserve a ratio among
column' widths isn't relevant since this ratio isn't relevant in the
first place.

I even think that respecting ratio gives more chance to strip more fields
introducing potential errors in more fields. Striping the last seems by
far a preferable behavior.

I talked here about ratio preservation in case of restricted width while
OP is about extra width available, but I doubt we should or could
easily have two distinct way to compute column width according to the
way the terminal dimensions changed (width increase or decrease).

[l***@mailoo.org]

Post by Raphaël

patch that scale the size of the fields of the index to fill the whole
terminal width, while keeping their respective size.

All the additional complexity lies in "keeping their respective size".
You'll find attached my suggestion with aims to help in one case
(related to the initial report).

If we have space available and that index_format ends with a field,
then expand the field up to COLS - 1.

... but does not try anything about "respective size": the Bonus space
goes to the last field.

But for example, in my use case, the last field is a phone number : it
doesn't need more space. Furthermore, I may want to increase the size of
a few columns (for example mail and name). With your proposal, because
only the last field can use the width of the terminal, we need to
reorganize how we order fields not to waste space.

Post by Raphaël
Long-run solutions about reduced width seem way more around
horizontal-scrolling and/or live column hiding.

I think you are right about that, but my patch was more aimed for
terminal with more width than the one used. But I think allowing
horizontal scrolling is a better solution for reduced width.

Post by Raphaël
There's also another thing to consider.
Contrary to a table where width could be expressed in %, abook uses an
index format with fixed width only.
We set a number of characters for an email or for a phone number, but
*not* to one proportionally to the other
Eg: "{name:20} {phone:10} {nick:10}" is in no way the same (semantically) than
"{name:50%} {phone:25%} {nick:25%}"
Moreover many fields are useless without their last characters.
If {phone}, for example, is striped down to 9 characters, we get into
troubles. Better for us to get the nickname stripped.

I agree with you on that. What I wanted to do when I started coding was
to allow a syntax with percents, but it meant more complexity, and
needed to handle cases where the user mess its percents. My patch is
also better for backward compatibility, because the index_format wasn't
changed, and it could run on older version with just a warning about an
unrecognised option.
But I agree it isn't right semantically. Furthermore, you're right about
case where the length is stripped down. I think we could just just abort
resizing the columns when the width of the terminal is inferior of total
length of fields.

Post by Raphaël
I think that any complexity added by trying to preserve a ratio among
column' widths isn't relevant since this ratio isn't relevant in the
first place.

I don't agree with on this point. While ratio isn't relevant for column
like phone numbers, it is relevant for columns like email or name,
because both need to be expanded and giving them the same size does not
feel right (to me).

Post by Raphaël
I even think that respecting ratio gives more chance to strip more fields
introducing potential errors in more fields. Striping the last seems by
far a preferable behavior.
I talked here about ratio preservation in case of restricted width while
OP is about extra width available, but I doubt we should or could
easily have two distinct way to compute column width according to the
way the terminal dimensions changed (width increase or decrease).

We could just reset columns width to their value when the size of the
terminal is inferior to the total size, but it would need a few more
entries to the index_elem structure (like two sizes : the one of the
config file, and the one used when drawing).

I think the best solution is the one you proposed in your other mail,
with changing index_format syntax to something like :
"{name:20+} {email:30+} {phone:12}"
That way, only name and email are extended and phone keep its size. We
could also abort resizing when terminal width is inferior to, in that
example, 62. But it would need to change the index_elem structure and
won't be backward compatible. I have started an example implementation
of this approach at [1], but I'll wait for others opinions before
sending it.

[1] https://github.com/lucas8/abook_extended_index/tree/extend_index

[l***@mailoo.org]

Post by Raphaël
Long-run solutions about reduced width seem way more around
horizontal-scrolling and/or live column hiding.

About that, I implemented a basic column-based horizontal scrolling :
hit right and the first column won't be shown anymore, left to show it
again, etc... It is quite primitive : it will scroll even if the
terminal is wide enough to show all the columns, and it can hide all the
columns (it will scroll even if there is nothing left to show).

Hope it can be useful.