[Github-comments] [geany/geany] Enable local variables for C/C++ and improve autocompletion (PR #3185)

[Jiří Techet]

> I can't follow. Yesterday I tried to review the whole thing for the first time. I was striked how large the change has become since my initial, very coarse, look (that wasnt a review) and have found that a lot of unrelated changes (w.r.t. to local tags) were added. GitHub even hides the diff for tm_workspace by default!! Since I also want this feature I'm trying to accept most of them in one PR although I don't feel truly confident. But the header feature a bit too much for my taste and should be dealt with separately.

For me personally, the problem is your attitude. You didn't indicate in #3175 that you are going to spend some more time with this PR and the conversation was mostly between me and @elextr. From the beginning of this PR you started bitching about "why a new PR", what you are "willing to accept" and about (in your opinion) "unrelated changes" that were introduced to the PR. The effect on me is significant reduction of willingness "to obey".

Those "unrelated changes" as you call them come from the testing of how autocompletion works with local variables enabled. It offers many more results than before and it's not sufficient to just simply enable local variables. I spent quite a bit of time testing how autocompletion works and the patches on the way are the result of the problems I (or @elextr ) run into (like the sorting of non-scope autocompletion tags was a very good idea Lex had and having the tags from the header means that in C++ you have members of classes - which you typically access frequently - at the top of the list). I normally try to separate patches as much as possible but at the same time I wanted to make the result useful and not annoying because you will see scope completion much more frequently and you will see more symbols in non-scope autocompletion. No need to remind me about making compact PRs, I try to do that with truly unrelated code but this isn't the case.

In any case, right now I suggest not to merge this PR until all problems are resolved and everything is discussed - I think there's no rush. You mention you "don't feel truly confident" so please take your time and post the things that are questionable in your opinion here. At the same time, I'd like to have the discussion about the header patch here too - I'm not going to spend time to remove it just because you happen not to like it - I'd like to see some real arguments (I tried to reply to your initial comments) and know what you suggest instead.

As I mentioned, I'd like to generalize it so symbols from included files are also sorted towards the top of the list - this would probably mean to change the implementation of the function checking whether the file is a header (or include) by having a hash table (simulating a set) with the included file names, going through TMSourceFile's and checking whether the file name is in the table.



-- 
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/3185#issuecomment-1114342322
You are receiving this because you are subscribed to this thread.

Message ID: <geany/geany/pull/3185/c1114342322 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.geany.org/pipermail/github-comments/attachments/20220501/d5d4f01a/attachment.htm>