ameyp / CscopeSublime

A Cscope plugin for Sublime Text 2 and 3
89 stars 37 forks source link

Add symbol confirmation #18

Closed vanrijn closed 11 years ago

vanrijn commented 11 years ago

I have this working, I think!

I've tested with prompt_before_searching set to both true and false and got correct results in both cases. I searched for different single symbols that were at the cursor point 5 times and made sure that the search criteria and results were correct. I also changed the symbol via the input panel and verified that the search criteria and results were correct.

As noted at https://github.com/ameyp/CscopeSublime/issues/8, it looks like multiple-symbol search is broken on master and it continues to be broken.

I hope I'm doing this pull request correctly. My goal is to let you review the changes before we merge this to master. =:)

Thanks Amey!

vanrijn commented 11 years ago

I had one thought... doing multiple searches at the same time will be harder if we're confirming each one, since we won't know when to do self.update_status(self.workers). One possible solution would be for run() to call on_search_confirmed (or whatever we call it) one additional time, passing in a string token like "CSCOPE_END_OF_SYMBOL_LIST" or something that on_search_confirmed would know to look for to indicate that the searches are all done and that it should now do self.update_status(self.workers) and return immediately instead of kicking off another worker thread. on_search_confirmed can only take a single string argument, according to the ST API, which is why this is a little more challenging. I can make that change as part of this branch if you like. I wanted to hold off on doing so until I could get your thoughts on the problem.

vanrijn commented 11 years ago

Hey Amey! Have you had a chance to look at this yet? Any thoughts?

ameyp commented 11 years ago

I'm sorry, I haven't checked Github over the past week, was busy with some other stuff.

I've replied to https://github.com/ameyp/CscopeSublime/issues/8 regarding searching for multiple symbols, but essentially if we decide to not have multi-symbol searches, the code looks good enough to merge to master.

If we do decide to have multi-symbol search, I'm thinking we not show the prompt at all. If someone has selected multiple symbols and then invoked symbol lookup, he/she clearly knows exactly what they're looking for. The input prompt is for users who want to look for symbols that don't happen to be under their cursor/in the current file, yes?

ameyp commented 11 years ago

Also @vanrijn, if it's not too much trouble, tag me with an @ twitter-style in future comments (the way I've tagged you in this one). That way Github will immediately send me an email notification, otherwise I'll find out that something's happening only when I manually visit Github :)

vanrijn commented 11 years ago

@ameyp Hey there! No problem! Okay, I will try to remember to tag you to help facilitate communication, =:) I did not realize that github didn't email you automatically without the tag. And yes, I completely agree that this should be only a single look up. I will update the branch and make that clearer. That doesn't make sense to me at all why anybody would want to try to do multiple lookups anyway, and I don't think that any of our users would actually intentionally try to. =:)

ameyp commented 11 years ago

Github sends automatic emails once someone is participating in a thread (e.g. by commenting), so future replies to this thread will automatically be emailed to me, but the first time a thread is created, someone has to tag me. This is because Github has only two options "Email me for everything" and "Email me when I'm mentioned or involved". Everything would be too much, because I would also get emails for change to every repository I'm watching, hence the second option :)

Cool, so I should hold off on merging this pull request till you've removed the multi-lookup option, right? Or would you like me to make those changes separately and pull this request as-is?

vanrijn commented 11 years ago

Ahh, okay, I understand. =:)

Hm. Actually, if you're okay with it, I'd rather merge this as is and then I'll do another branch with the multi-lookup cleanup. If that's okay with you?

ameyp commented 11 years ago

Roger that