Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes and refactorings to utils_strv_shorten_file_list() and friends #2262

Merged
merged 4 commits into from Sep 20, 2019

Conversation

@kugel-
Copy link
Member

commented Aug 16, 2019

When merging #1445 we found that the results could be improved.

I promised to fix the deficient results along with proper unit tests so here we go.

Basically utils_strv_shorten_file_list() can repaired and simplified at the same time by a relatively minor change to the supprting utils_strv_find_lcs(). This is all backed up by new unit tests which tests the related functions plus (now reborn) utils_strv_new().

@kugel- kugel- requested a review from b4n Aug 16, 2019
@kugel-

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2019

@elextr @b4n @ntrel @codebrainz ping

Since it's fixing bugs and not introducing a potentially controversial new feature, I don't want this to rot long on github. Please review or I'll go ahead and merge anyway in a few days/weeks

@kugel-

This comment has been minimized.

Copy link
Member Author

commented Sep 20, 2019

Almost 4 weeks passed, I'll go ahead.

kugel- added 4 commits Aug 14, 2019
We didn't use unit tests so far so I have picked up the glib testing
framework. While there are better frameworks out there glib's it gets the job
done and doesn't impose extra dependencies.

For upcoming fixes and refactorings to utils_strv_find_lcs and
utils_strv_shorten_file_list I would like to make sure
to not introduce regressions and unit tests are ideal for that.

A function to be tested must be exported by libgeany.so. Use
GEANY_EXPORT_SYMBOL for that. It's not the same as GEANY_API_SYMBOL
to avoid the impression that it's OK to use them in plugins. Also
no doxygen comments for those.

I resurrected utils_strv_new() because it's convinient to have in the tests.
The function has its own test suite since it's otherwise unused.
…_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.
Since utils_strv_find_lcs() respects path components by itself,
utils_strv_shorten_file_list() can be massively simplified. The path
list does not be prepared and post-processed around utils_strv_find_lcs()
anymore because it does the right thing already. This saves
a lot of complexity and also duplicating the string vector once.
utils_strv_shorten_file_list() is part of the plugin API so hiding it
inside GEANY_PRIVATE is plain wrong.
@kugel- kugel- force-pushed the kugel-:fix-1445 branch from c31e17c to 4b4a41e Sep 20, 2019
@kugel- kugel- merged commit 746697a into geany:master Sep 20, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kugel- kugel- deleted the kugel-:fix-1445 branch Sep 20, 2019
@ntrel

This comment has been minimized.

Copy link
Member

commented Sep 24, 2019

BTW with -Wextra I get:

utils.c: In function 'utils_strv_shorten_file_list':
utils.c:2192:2: warning: "/*" within comment [-Wcomment]
 2192 |  /* We only want to strip full path components, including the trailing slash.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.