[#1804] Remember location after cider-inspector-pop

[ckoparkar]

We push our current location in the buffer i.e(point), onto a stack, before every cider-inspector-push operation. Then when we're navigation back using cider-inspector-pop,
we just goto-char to that location.

I've not used this feature of CIDER much. But does it make sense to do something like this for next-page and prev-page too ?

/cc @mallt

• The commits are consistent with our [contribution guidelines][1]
• You've added tests (if possible) to cover your change(s)
• All tests are passing (make test)
• The new code is not generating bytecode or M-x checkdoc warnings
• You've updated the changelog (if adding/changing user-visible functionality)
• You've updated the readme (if adding/changing user-visible functionality)
• You've updated the refcard (if you made changes to the commands listed there)

[Malabarba]

I don't understand why this is necessary. Can't we just (goto-char (pop cider-inspector-location-stack)) in the cider-inspector--value-handler function?

[ckoparkar]

We only have to goto-char if this handler is called by the pop operation. I just pushed a patch where we check this through the last-command. This was certainly not the best way to do this :-)

[bbatsov]

I've not used this feature of CIDER much. But does it make sense to do something like this for next-page and prev-page too ?

I'm not quite certain what you mean by this.

[Malabarba]

I think it might make sense to have it, but it would probably complicate the feature a lot (as the two trackings would interfere with each other), so maybe it's best to leave it like this.

[ckoparkar]

@bbatsov This PR, in its current state would only preserve cursor location across cider-inspector-push/pop operations in the inspector buffer. As these are the ones used to navigate down/up in a data structure, I think these would be most frequently used. But we also have cider-inspector-next-page/prev-page to handle paginated responses. So I was wondering if preserving cursor location across page navigation would be useful too :-)

@Malabarba You're right. It would complicate tracking method calls in the handler.

But lets keep this PR open for another day. I'll try this today, and if I'm able to get it to work and if we're happy with the added complexity vs usefulness of the feature, then lets merge it. Otherwise lets stick to only preserving cursor location across cider-inspector-push/pop :-)

[bbatsov]

So, what's the verdict here - are we extending this or merging it as it is?

[bbatsov]

I'd say this should be a buffer-local variable.

[ckoparkar]

I'm sorry I forgot to post an update :-) I have gotten it to work. We can just use 2 stacks to track the position. But I ran into something else. When I invoke any cider-* command through ido, tracking the method calls via last-command doesn't work. It sets the last command to something like ido-last-command. Need to look into this further.

I am AFK for the last two days. I will definitely finish this tomorrow :-)

[Malabarba]

It sets the last command to something like ido-last-command. Need to look into this further.

Yes, that's just how last-command works. The same happens if you invoke the command with regular M-x

[bbatsov]

Probably we shouldn't rely on last-command at all, but rather bake this into cider-inspector-pop. Using last-command is generally a recipe for trouble...

[ckoparkar]

Yes, that's just how last-command works. The same happens if you invoke the command with regular M-x

It is surprising I guess. I expected it to get the value of the command selected through M-x.

Probably we shouldn't rely on last-command at all, but rather bake this into cider-inspector-pop. Using last-command is generally a recipe for trouble...

Yep. It seems so. We're gonna have to track this via our own var I guess. Or through the stack like in the previous patch. I'll try and see we could get it done any other way.

On a side note, I have a doubt about this. Why have we used callback's here ? All the actions seem synchronous with the middleware response. What I guessed is that its because we have a good abstraction of the form nrepl-make-response-handler. Should we use the form cider-sync-request:* here ? Not necessarily in this feature.

[bbatsov]

On a side note, I have a doubt about this. Why have we used callback's here ? All the actions seem synchronous with the middleware response. What I guessed is that its because we have a good abstraction of the form nrepl-make-response-handler. Should we use the form cider-sync-request:* here ? Not necessarily in this feature.

We don't really need it. In the early days of the project we were trying to use callbacks everywhere as nREPL doesn't natively support sync evaluations and our emulation of those was poor. This code hasn't be updated in quite a while. Async handlers have a ton of room for improvement, this has been discussed in some older open tickets - unfortunately no one has had to the time to tackle them in years...

[bbatsov]

The discussions about callbacks is here #1099

[ckoparkar]

@bbatsov Ah ok. It makes sense now. Thanks! I'll read those tickets and we can start working on it next. I have some free time now :-)

For now, we can track this through our own global cider-inspector-last-command. It is not the cleanest solution, but it works. I just have to make all those vars buffer local. But please check if this approach is ok.

[bbatsov]

I just have to make all those vars buffer local. But please check if this approach is ok.

The solution looks good. Just use defvar-local and we should be good to go.

[bbatsov]

I think you've added this blank line by accident.

[bbatsov]

Thanks! I'll read those tickets and we can start working on it next.

That'd be great. This refactoring is way way overdue... Many of the older tickets are some essential changes that got stale mostly due to lack of time to work on them (comint is another such example).

[ckoparkar]

This is preventing us from creating a buffer-local var. It clears out all the state when creating a new buffer. I guess our this method could use an optional param which controls this behavior. But it would involve changes in a lot of places. Let's handle this in a separate PR ?

[bbatsov]

Ah, I forgot about this. Yeah, guess we can revisit this later. I'll merge the PR in its current state.

[ckoparkar]

Ill start reading the code and let's break the refactoring tasks down into smaller tickets we could handle. I'll start working on this from tomorrow 🔨 :-)