Enable setting and updating diagnostics on a single quickfix list(diagnostics) #974
Conversation
ajayvigneshk
changed the title
|
@ajayvigneshk sorry that it took this long to get an answer for you but if you are still interested in getting this merged and are willing to rebase this to |
|
@martskins This PR is raised against |
|
@ajayvigneshk You can just change the target branch of this PR if you click on the edit button next to the title. That, plus rebasing your branch to dev should do it. |
ajayvigneshk
force-pushed
the
origin/next
branch
from
b501767 to
c1bd434
Compare
|
@martskins Done. Thanks for the tip. |
| pub fn updateqflist(&self, list: &[QuickfixEntry], title: &str, id: i32) -> Result<()> { | ||
| info!("Begin updateqflist"); | ||
| let parms = json!([[], "r" ,{"title":title,"id":id, "items":list}]); | ||
| self.rpcclient.notify("setqflist", parms)?; | ||
| Ok(()) | ||
| } | ||
|
|
||
| pub fn addnewqflist(&self, list: &[QuickfixEntry], title: &str) -> Result<()> { | ||
| info!("Begin addnewqflist"); | ||
| let parms = json!([[], " ",{"title":title, "items":list}]); | ||
| self.rpcclient.notify("setqflist", parms)?; | ||
| Ok(()) | ||
| } | ||
|
|
||
| pub fn getqfid(&self, title: &str) -> Result<i32> { | ||
| info!("Begin getqfid"); | ||
| self.rpcclient.call("LSP#GetQfListIdForTitle", title) | ||
| } |
There was a problem hiding this comment.
I think it would be better if we moved all of this to a single vim function, say: UpsertQflist or something like that. I know this is used in a single place right now, but if it were to be used somewhere else it would require repeating that logic of check if there's an existing qflist, if there is update, if there isn't create. So I think it would be better to have this logic on the vimscript side. That would also mean that you can get rid of that self.update call you made for atomicity.
There was a problem hiding this comment.
self.update call seems to be required for locking. For instance, without it, I can see multiple concurrent calls on the log to qflist modification methods causing inconsistent behaviour.
There was a problem hiding this comment.
You don't really need to lock to get access to the vim rpcclient though. Or at least not through .update. I understand you wanted to make those 3 calls as atomic as possible with that though, but what I mean is, if we move that logic to vimscript you won't need to call self.update, cause it's a single call you'll be making, and you can get away with just self.vim()?.
So my suggestion is to basically create a function in vimscript that takes two arguments, lines and title. And have that function search for a qflist with that title and create one if it doesn't find it, and set the items on that list, all within the same function. Again that allows you to get away with a single call to vim from rust and also means that other bits of the code that need that logic won't have to repeat the search/create -> set lines logic again.
There was a problem hiding this comment.
I understand the motivation / intended change too. This section of code has to be locked across multiple textDocument/publishDiagnostics notification handlers which appear to be concurrent. Can you help me with that?
There was a problem hiding this comment.
Sorry it took so long to get back, I seemed to have not noticed this message. My point was that you don't really need to sync between concurrent publishDiagnostics. What makes you think you need to take care of that though? Have you experienced any issues with it?
There was a problem hiding this comment.
Hey, Yes I did face issues with it. For eg. something like this say there're two publishDiagnostics events. publishDiagnostics event -> check if named quickfix list exists -> if not, create qf list.
I faced issue with two qf lists getting created, because before creating qf list as a part of handling the first event, the second one checks if a qf list already exists, gets responded in the negative, effectively making both handlers create qf lists. This defeats the intended purpose of this change.
The code appears working. But I see the orginal code (left untouched) is also referenced by other parts. Need to know if those also need to be updated.
Fixes this: #951