Return status of filtering choices #268

ghost · 2017-12-29T13:33:45Z

... there is an issue here with aborted filtering on a large set of choices: sometimes "outdated choices" remain on the screen if filtering is rapidly aborted (by holding the BackSpace-Key to erase the query) ...

Erasing the query by slowly pressing BackSpace:

Erasing the query by holding BackSpace:

Erasing the query by holding BackSpace (fix applied):

codecov-io · 2017-12-29T13:39:42Z

Codecov Report

Merging #268 into master will decrease coverage by 0.08%.
The diff coverage is 84.61%.

@@            Coverage Diff             @@
##           master     #268      +/-   ##
==========================================
- Coverage   90.74%   90.66%   -0.09%     
==========================================
  Files           1        1              
  Lines         508      514       +6     
==========================================
+ Hits          461      466       +5     
- Misses         47       48       +1

Impacted Files	Coverage Δ
pick.c	`90.66% <84.61%> (-0.09%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f553d4a...59ddc0f. Read the comment docs.

ghost · 2017-12-29T18:15:48Z

Updated fix that works flawlessly even on fast typing (despite the red banner above ... ;-)

mptre · 2017-12-30T11:34:32Z

How about the following diff?

diff --git a/pick.c b/pick.c
index 16d27df..7cff7a5 100644
--- a/pick.c
+++ b/pick.c
@@ -71,7 +71,7 @@ struct choice {
 static int			 choicecmp(const void *, const void *);
 static void			 delete_between(char *, size_t, size_t, size_t);
 static char			*eager_strpbrk(const char *, const char *);
-static void			 filter_choices(size_t);
+static void			 filter_choices(size_t, int);
 static char			*get_choices(void);
 static enum key			 get_key(const char **);
 static void			 handle_sigwinch(int);
@@ -325,7 +325,7 @@ selected_choice(void)
 			choices_count = choices.length;
 		query_grew = 0;
 		if (dofilter) {
-			filter_choices(choices_count);
+			filter_choices(choices_count, query_length > 0);
 			dofilter = selection = yscroll = 0;
 		}
 
@@ -530,12 +530,12 @@ selected_choice(void)
 }
 
 /*
- * Filter choices using the current query while there is no new user input
- * available.
- * The first nchoices number of choices will be filtered.
+ * Filter the first nchoices number of choices using the current query.
+ * If intr is non-zero, regularly check for new user input and abort the
+ * filtering.
  */
 void
-filter_choices(size_t nchoices)
+filter_choices(size_t nchoices, int intr)
 {
 	struct choice	*c;
 	struct pollfd	 pfd;
@@ -561,7 +561,7 @@ filter_choices(size_t nchoices)
 		 * outdated. This improves the performance when the cardinality
 		 * of the choices is large.
 		 */
-		if (i > 0 && i % 50 == 0) {
+		if (intr && i > 0 && i % 50 == 0) {
 			pfd.fd = fileno(tty_in);
 			pfd.events = POLLIN;
 			if ((nready = poll(&pfd, 1, 0)) == -1)

ghost · 2017-12-30T13:03:06Z

How about the following diff?

Thanks, this fixes the issue with BackSpace, but on fast typing the query it gives still different results:

mptre · 2017-12-30T14:16:23Z

Having a hard time to reproduce this, maybe my input is too small(?).
Could run the fast typing example again with this diff applied and share
the contents of pick.out please.

$ some-command | pick 2>pick.out; xsel <pick.out

diff --git a/pick.c b/pick.c
index 16d27df..76c4c25 100644
--- a/pick.c
+++ b/pick.c
@@ -71,7 +71,7 @@ struct choice {
 static int			 choicecmp(const void *, const void *);
 static void			 delete_between(char *, size_t, size_t, size_t);
 static char			*eager_strpbrk(const char *, const char *);
-static void			 filter_choices(size_t);
+static void			 filter_choices(size_t, int);
 static char			*get_choices(void);
 static enum key			 get_key(const char **);
 static void			 handle_sigwinch(int);
@@ -325,7 +325,7 @@ selected_choice(void)
 			choices_count = choices.length;
 		query_grew = 0;
 		if (dofilter) {
-			filter_choices(choices_count);
+			filter_choices(choices_count, query_length > 0);
 			dofilter = selection = yscroll = 0;
 		}
 
@@ -530,18 +530,19 @@ selected_choice(void)
 }
 
 /*
- * Filter choices using the current query while there is no new user input
- * available.
- * The first nchoices number of choices will be filtered.
+ * Filter the first nchoices number of choices using the current query.
+ * If intr is non-zero, regularly check for new user input and abort the
+ * filtering.
  */
 void
-filter_choices(size_t nchoices)
+filter_choices(size_t nchoices, int intr)
 {
 	struct choice	*c;
 	struct pollfd	 pfd;
 	size_t		 i, match_length;
 	int		 nready;
 
+	fprintf(stderr, "%s: intr=%d, query=%s\n", __func__, intr, query);
 	for (i = 0; i < nchoices; i++) {
 		c = &choices.v[i];
 		if (min_match(c->string, 0,
@@ -561,15 +562,18 @@ filter_choices(size_t nchoices)
 		 * outdated. This improves the performance when the cardinality
 		 * of the choices is large.
 		 */
-		if (i > 0 && i % 50 == 0) {
+		if (intr && i > 0 && i % 50 == 0) {
 			pfd.fd = fileno(tty_in);
 			pfd.events = POLLIN;
 			if ((nready = poll(&pfd, 1, 0)) == -1)
 				err(1, "poll");
-			if (nready == 1 && pfd.revents & (POLLIN | POLLHUP))
+			if (nready == 1 && pfd.revents & (POLLIN | POLLHUP)) {
+				fprintf(stderr, "%s: poll break\n", __func__);
 				break;
+			}
 		}
 	}
+	fprintf(stderr, "%s: filtered: %ld/%ld\n", __func__, i, nchoices);
 
 	qsort(choices.v, nchoices, sizeof(struct choice), choicecmp);
 }

ghost · 2017-12-30T15:37:53Z

Fast typing:

filter_choices: intr=0, query=
filter_choices: filtered: 74440/74440
filter_choices: intr=1, query=m
filter_choices: poll break
filter_choices: filtered: 30900/74440
filter_choices: intr=1, query=mo
filter_choices: poll break
filter_choices: filtered: 3200/20516
filter_choices: intr=1, query=mod
filter_choices: filtered: 18085/18085
filter_choices: intr=1, query=modc
filter_choices: filtered: 2547/2547
filter_choices: intr=1, query=modcg
filter_choices: filtered: 1599/1599
filter_choices: intr=1, query=modcgi
filter_choices: filtered: 351/351

Slow typing:

filter_choices: intr=0, query=
filter_choices: filtered: 74440/74440
filter_choices: intr=1, query=m
filter_choices: filtered: 74440/74440
filter_choices: intr=1, query=mo
filter_choices: filtered: 50730/50730
filter_choices: intr=1, query=mod
filter_choices: filtered: 29253/29253
filter_choices: intr=1, query=modc
filter_choices: filtered: 11438/11438
filter_choices: intr=1, query=modcg
filter_choices: filtered: 7017/7017
filter_choices: intr=1, query=modcgi
filter_choices: filtered: 2713/2713

mptre · 2017-12-30T16:09:32Z

Thanks, try this diff:

diff --git a/pick.c b/pick.c
index 16d27df..7eac5aa 100644
--- a/pick.c
+++ b/pick.c
@@ -71,7 +71,7 @@ struct choice {
 static int			 choicecmp(const void *, const void *);
 static void			 delete_between(char *, size_t, size_t, size_t);
 static char			*eager_strpbrk(const char *, const char *);
-static void			 filter_choices(size_t);
+static int			 filter_choices(size_t, int);
 static char			*get_choices(void);
 static enum key			 get_key(const char **);
 static void			 handle_sigwinch(int);
@@ -325,8 +325,11 @@ selected_choice(void)
 			choices_count = choices.length;
 		query_grew = 0;
 		if (dofilter) {
-			filter_choices(choices_count);
-			dofilter = selection = yscroll = 0;
+			dofilter = 0;
+			if (filter_choices(choices_count, query_length > 0))
+				selection = yscroll = 0;
+			else
+				dochoices = 0;
 		}
 
 		tty_putp(cursor_invisible, 0);
@@ -530,12 +533,13 @@ selected_choice(void)
 }
 
 /*
- * Filter choices using the current query while there is no new user input
- * available.
- * The first nchoices number of choices will be filtered.
+ * Filter the first nchoices number of choices using the current query.
+ * If intr is non-zero, regularly check for new user input and abort the
+ * filtering.
+ * Returns non-zero if the filtering was not interrupted by new user input.
  */
-void
-filter_choices(size_t nchoices)
+int
+filter_choices(size_t nchoices, int intr)
 {
 	struct choice	*c;
 	struct pollfd	 pfd;
@@ -561,17 +565,18 @@ filter_choices(size_t nchoices)
 		 * outdated. This improves the performance when the cardinality
 		 * of the choices is large.
 		 */
-		if (i > 0 && i % 50 == 0) {
+		if (intr && i > 0 && i % 50 == 0) {
 			pfd.fd = fileno(tty_in);
 			pfd.events = POLLIN;
 			if ((nready = poll(&pfd, 1, 0)) == -1)
 				err(1, "poll");
 			if (nready == 1 && pfd.revents & (POLLIN | POLLHUP))
-				break;
+				return 0;
 		}
 	}
 
 	qsort(choices.v, nchoices, sizeof(struct choice), choicecmp);
+	return 1;
 }
 
 int

Typing a query fast enough to trigger the poll check in filter_choices() could yield a wrong set of choices. Since filter_choices() is interrupted, all choices have yet not been examined and potential matches could still be left unexamined. Calling print_choices() in this state causes the unexamined choices to never be reconsidered as potential matches since they will have a score = 0. The solution is to never call print_choices() if filter_choices() was interrupted. Problem reported and partial solution provided by Jenz Guenther in GitHub issue #268.

ghost · 2017-12-30T18:37:31Z

Closing this for the fix in #270 ...

Typing a query fast enough to trigger the poll check in filter_choices() could yield a wrong set of choices. Since filter_choices() is interrupted, all choices have yet not been examined and potential matches could still be left unexamined. Calling print_choices() in this state causes the unexamined choices to never be reconsidered as potential matches since they will have a score = 0. The solution is to never call print_choices() if filter_choices() was interrupted. Problem reported and partial solution provided by Jenz Guenther in GitHub issue #268.

Return status of filtering choices

59ddc0f

mptre mentioned this pull request Dec 30, 2017

Ensure correctness while typing a query fast #270

Merged

ghost closed this Dec 30, 2017

ghost deleted the filtered branch December 30, 2017 18:37

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return status of filtering choices #268

Return status of filtering choices #268

ghost commented Dec 29, 2017 •

edited by ghost

Loading

codecov-io commented Dec 29, 2017 •

edited

Loading

ghost commented Dec 29, 2017 •

edited by ghost

Loading

mptre commented Dec 30, 2017

ghost commented Dec 30, 2017

mptre commented Dec 30, 2017

ghost commented Dec 30, 2017

mptre commented Dec 30, 2017

ghost commented Dec 30, 2017

Return status of filtering choices #268

Return status of filtering choices #268

Conversation

ghost commented Dec 29, 2017 • edited by ghost Loading

codecov-io commented Dec 29, 2017 • edited Loading

Codecov Report

ghost commented Dec 29, 2017 • edited by ghost Loading

mptre commented Dec 30, 2017

ghost commented Dec 30, 2017

mptre commented Dec 30, 2017

ghost commented Dec 30, 2017

mptre commented Dec 30, 2017

ghost commented Dec 30, 2017

ghost commented Dec 29, 2017 •

edited by ghost

Loading

codecov-io commented Dec 29, 2017 •

edited

Loading

ghost commented Dec 29, 2017 •

edited by ghost

Loading