From 30a486d62462ad2bf34ac7af48547053f7167441 Mon Sep 17 00:00:00 2001 From: Thomas Martitz Date: Fri, 16 Aug 2019 14:58:09 +0200 Subject: [PATCH] utils: fix suboptimal elipsis substitution by utils_strv_shorten_file_list() Because utils_strv_find_lcs() didn't consisider path component boundaries it have have found substrings that are longest in itself but not so ideal when utils_strv_shorten_file_list() applied path boundaries. utils_strv_find_lcs() now can optionally restrict the substring between delimiters (i.e. dir separators). In that mode it will find the longest substring that is also sorrounded by the delimiters (there may be more delimiters inside the string). The unit test that demonstrated the deficient is fixed since now the expected substitution takes place. --- src/utils.c | 14 +++++- src/utils.h | 2 +- tests/test_utils.c | 116 +++++++++++++++++++++++++++++++++------------ 3 files changed, 98 insertions(+), 34 deletions(-) diff --git a/src/utils.c b/src/utils.c index 95099658..96187102 100644 --- a/src/utils.c +++ b/src/utils.c @@ -2086,7 +2086,7 @@ gchar *utils_strv_find_common_prefix(gchar **strv, gssize strv_len) * @return The common prefix that is part of all strings. */ GEANY_EXPORT_SYMBOL -gchar *utils_strv_find_lcs(gchar **strv, gssize strv_len) +gchar *utils_strv_find_lcs(gchar **strv, gssize strv_len, const gchar *delim) { gchar *first, *_sub, *sub; gsize num; @@ -2112,8 +2112,18 @@ gchar *utils_strv_find_lcs(gchar **strv, gssize strv_len) /* No point in continuing if the remainder is too short */ if (max > chars_left) break; + /* If delimiters are given, we only need to compare substrings which start and + * end with one of them, so skip any non-delim chars at front ... */ + if (NZV(delim) && (strchr(delim, _sub[0]) == NULL)) + continue; for (n_chars = 1; n_chars <= chars_left; n_chars++) { + if (NZV(delim)) + { /* ... and advance to the next delim char at the end, if any */ + if (!_sub[n_chars] || strchr(delim, _sub[n_chars]) == NULL) + continue; + n_chars += 1; + } g_strlcpy(sub, _sub, n_chars+1); found = 1; for (gsize i = 1; i < num; i++) @@ -2199,7 +2209,7 @@ gchar **utils_strv_shorten_file_list(gchar **file_names, gssize file_names_len) } lcs_len = 0; - substring = (i == num) ? utils_strv_find_lcs(names, num) : NULL; + substring = (i == num) ? utils_strv_find_lcs(names, num, G_DIR_SEPARATOR_S"/") : NULL; if (NZV(substring)) { /* Strip leading component. */ diff --git a/src/utils.h b/src/utils.h index 7fd02d59..3888f275 100644 --- a/src/utils.h +++ b/src/utils.h @@ -302,7 +302,7 @@ gchar **utils_strv_join(gchar **first, gchar **second) G_GNUC_WARN_UNUSED_RESULT gchar *utils_strv_find_common_prefix(gchar **strv, gssize strv_len) G_GNUC_WARN_UNUSED_RESULT; -gchar *utils_strv_find_lcs(gchar **strv, gssize strv_len) G_GNUC_WARN_UNUSED_RESULT; +gchar *utils_strv_find_lcs(gchar **strv, gssize strv_len, const gchar *delim) G_GNUC_WARN_UNUSED_RESULT; gchar **utils_strv_shorten_file_list(gchar **file_names, gssize file_names_len); diff --git a/tests/test_utils.c b/tests/test_utils.c index 44b53aa8..ea9a307f 100644 --- a/tests/test_utils.c +++ b/tests/test_utils.c @@ -106,51 +106,86 @@ static void test_utils_strv_find_common_prefix(void) g_strfreev(data); } +#define DIR_SEP "\\/" void test_utils_strv_find_lcs(void) { gchar **data, *s; - s = utils_strv_find_lcs(NULL, 0); + s = utils_strv_find_lcs(NULL, 0, ""); g_assert_null(s); data = utils_strv_new("", NULL); - s = utils_strv_find_lcs(data, -1); + s = utils_strv_find_lcs(data, -1, ""); g_assert_nonnull(s); g_assert_cmpstr(s, ==, ""); g_free(s); g_strfreev(data); data = utils_strv_new("1", "2", "3", NULL); - s = utils_strv_find_lcs(data, -1); + s = utils_strv_find_lcs(data, -1, ""); + g_assert_nonnull(s); + g_assert_cmpstr(s, ==, ""); + g_free(s); + s = utils_strv_find_lcs(data, -1, DIR_SEP); g_assert_nonnull(s); g_assert_cmpstr(s, ==, ""); g_free(s); g_strfreev(data); data = utils_strv_new("abc", "amn", "axy", NULL); - s = utils_strv_find_lcs(data, -1); + s = utils_strv_find_lcs(data, -1, ""); g_assert_nonnull(s); g_assert_cmpstr(s, ==, "a"); g_free(s); + s = utils_strv_find_lcs(data, -1, DIR_SEP); + g_assert_nonnull(s); + g_assert_cmpstr(s, ==, ""); + g_free(s); + g_strfreev(data); + + data = utils_strv_new("bca", "mna", "xya", NULL); + s = utils_strv_find_lcs(data, -1, ""); + g_assert_nonnull(s); + g_assert_cmpstr(s, ==, "a"); + g_free(s); + s = utils_strv_find_lcs(data, -1, DIR_SEP); + g_assert_nonnull(s); + g_assert_cmpstr(s, ==, ""); + g_free(s); g_strfreev(data); data = utils_strv_new("abc", "", "axy", NULL); - s = utils_strv_find_lcs(data, -1); + s = utils_strv_find_lcs(data, -1, ""); + g_assert_nonnull(s); + g_assert_cmpstr(s, ==, ""); + g_free(s); + s = utils_strv_find_lcs(data, -1, DIR_SEP); + g_assert_nonnull(s); + g_assert_cmpstr(s, ==, ""); + g_free(s); + g_strfreev(data); + + data = utils_strv_new("a123b", "b123c", "c123d", NULL); + s = utils_strv_find_lcs(data, -1, ""); + g_assert_nonnull(s); + g_assert_cmpstr(s, ==, "123"); + g_free(s); + s = utils_strv_find_lcs(data, -1, DIR_SEP); g_assert_nonnull(s); g_assert_cmpstr(s, ==, ""); g_free(s); g_strfreev(data); data = utils_strv_new("22", "23", "33", NULL); - s = utils_strv_find_lcs(data, 1); + s = utils_strv_find_lcs(data, 1, ""); g_assert_nonnull(s); g_assert_cmpstr(s, ==, "22"); g_free(s); - s = utils_strv_find_lcs(data, 2); + s = utils_strv_find_lcs(data, 2, ""); g_assert_nonnull(s); g_assert_cmpstr(s, ==, "2"); g_free(s); - s = utils_strv_find_lcs(data, 3); + s = utils_strv_find_lcs(data, 3, ""); g_assert_nonnull(s); g_assert_cmpstr(s, ==, ""); g_free(s); @@ -163,48 +198,77 @@ void test_utils_strv_find_lcs(void) "/home/user/src/geany/src/main.c", "/home/user/src/geany-plugins/addons/src/addons.c", NULL); - s = utils_strv_find_lcs(data, 4); + s = utils_strv_find_lcs(data, 4, ""); g_assert_nonnull(s); g_assert_cmpstr(s, ==, "/home/user/src/geany/src/s"); g_free(s); - s = utils_strv_find_lcs(data, 5); + s = utils_strv_find_lcs(data, 4, DIR_SEP); g_assert_nonnull(s); g_assert_cmpstr(s, ==, "/home/user/src/geany/src/"); g_free(s); - s = utils_strv_find_lcs(data, -1); + s = utils_strv_find_lcs(data, 5, ""); + g_assert_nonnull(s); + g_assert_cmpstr(s, ==, "/home/user/src/geany/src/"); + g_free(s); + s = utils_strv_find_lcs(data, 5, DIR_SEP); + g_assert_nonnull(s); + g_assert_cmpstr(s, ==, "/home/user/src/geany/src/"); + g_free(s); + s = utils_strv_find_lcs(data, -1, ""); g_assert_nonnull(s); g_assert_cmpstr(s, ==, "/home/user/src/geany"); g_free(s); + s = utils_strv_find_lcs(data, -1, DIR_SEP); + g_assert_nonnull(s); + g_assert_cmpstr(s, ==, "/home/user/src/"); + g_free(s); g_strfreev(data); data = utils_strv_new("/src/a/app-1.2.3/src/lib/module/source.c", "/src/b/app-2.2.3/src/module/source.c", NULL); - s = utils_strv_find_lcs(data, -1); + s = utils_strv_find_lcs(data, -1, ""); g_assert_nonnull(s); g_assert_cmpstr(s, ==, "/module/source.c"); g_free(s); + s = utils_strv_find_lcs(data, -1, DIR_SEP); + g_assert_nonnull(s); + g_assert_cmpstr(s, ==, "/module/"); + g_free(s); + g_strfreev(data); + + data = utils_strv_new("/src/a/app-1.2.3/src/lib/module\\source.c", + "/src/b/app-2.2.3/src/module\\source.c", NULL); + s = utils_strv_find_lcs(data, -1, ""); + g_assert_nonnull(s); + g_assert_cmpstr(s, ==, "/module\\source.c"); + g_free(s); + s = utils_strv_find_lcs(data, -1, DIR_SEP); + g_assert_nonnull(s); + g_assert_cmpstr(s, ==, "/module\\"); + g_free(s); g_strfreev(data); data = utils_strv_new("/src/a/app-1.2.3/src/lib/module/", "/src/b/app-2.2.3/src/module/", NULL); - s = utils_strv_find_lcs(data, -1); + s = utils_strv_find_lcs(data, -1, ""); g_assert_nonnull(s); g_assert_cmpstr(s, ==, ".2.3/src/"); g_free(s); - g_strfreev(data); - - data = utils_strv_new("a123b", "b123c", "c123d", NULL); - s = utils_strv_find_lcs(data, -1); + s = utils_strv_find_lcs(data, -1, DIR_SEP); g_assert_nonnull(s); - g_assert_cmpstr(s, ==, "123"); + g_assert_cmpstr(s, ==, "/module/"); g_free(s); g_strfreev(data); data = utils_strv_new("/usr/local/bin/geany", "/usr/bin/geany", "/home/user/src/geany/src/geany", NULL); - s = utils_strv_find_lcs(data, -1); + s = utils_strv_find_lcs(data, -1, ""); g_assert_nonnull(s); g_assert_cmpstr(s, ==, "/geany"); g_free(s); + s = utils_strv_find_lcs(data, -1, DIR_SEP); + g_assert_nonnull(s); + g_assert_cmpstr(s, ==, ""); + g_free(s); g_strfreev(data); } @@ -333,21 +397,11 @@ void test_utils_strv_shorten_file_list(void) g_strfreev(result); g_strfreev(data); - /* This case shows a weakness. It would be better to elipsize "module". Instead, - * nothing is elipsized as the code determines ".2.3/src/" to be the - * longest common substring. Then it reduces it to directory components, so "src" remains. - * This is shorter than the threshold of 5 chars and consequently not elipsized - * - * Plain utils_strv_find_lcs() actually finds /module/source.c, - * but utils_strv_shorten_file_list() calls it without the file name part, so - * ".2.3/src/" is longer than "/module/". This is illustrated in the test - * test_utils_strv_find_lcs() - */ data = utils_strv_new("/src/a/app-1.2.3/src/lib/module/source.c", "/src/b/app-2.2.3/src/module/source.c", NULL); result = utils_strv_shorten_file_list(data, -1); - expected = utils_strv_new("a/app-1.2.3/src/lib/module/source.c", - "b/app-2.2.3/src/module/source.c", NULL); + expected = utils_strv_new("a/app-1.2.3/src/lib/.../source.c", + "b/app-2.2.3/src/.../source.c", NULL); g_assert_true(strv_eq(result, expected)); g_strfreev(expected); g_strfreev(result);