[Github-comments] [geany/geany] Improve goto-symbols popup (#1445)

Colomban Wendling notifications at xxxxx
Fri Nov 16 11:05:41 UTC 2018


> Fixing this isn't trivial

That's why I suggested in an earlier comment that the fact `utils_strv_find_lcs()` looks generic is cute, but in practice this function does *not* do what `utils_strv_shorten_file_list()` needs.

> and without proper unit tests there is a risk to break previously working cases.

Yeah, we should probably add tests for this.  Fortunately, it's probably one of the few things that should be easy enough to add a test for, because `utils.c` don't usually have dependencies on internal Geany state, which means it can likely be tested by a minimal test program linking libgeany.

> I suggest to accept the behavior for now (and merge) and I'll follow up with a fix + unit tests. But I would hate if this doesn't make it into the release just because of select (edge) cases because the status quo is just plain unusable for me. In fact I've been using this patch at work for ages where I have real-world cases and I never came across issues like this.

Fair enough; I must admit I specifically crafted the test to fail, looking at the weakness of the implementation :)

This said, note that I have another implementation (see a message above) that doesn't have the issue.  It has arguably worse performance, and can probably use some improvement in other area, but I didn't find a bug in its results yet.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/1445#issuecomment-439360404
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.geany.org/pipermail/github-comments/attachments/20181116/a222bb37/attachment.html>


More information about the Github-comments mailing list