Nnn: Questions regarding SEL_PLUG* variable manipulation

[0xACE]

Hey, I'm putting this discussion as an issue here, as it's difficult to track the conversation over all the commits/code that is relevant to this discussion. Keep this conversation going as low priority, as it's mostly about understanding the code, and may potentially add improvements or documentation to our beloved browse();

Image of nnn developer debugging browse();. The various scopes were used to handle the wide width of indentations and an valiant effort on keeping track of all the variables.

CONVERSATION SO FAR
inside run_selected_plugin():
https://github.com/jarun/nnn/blob/3c28d1f1d7fac7c4d8764cfce0749ec22dbbbaa3/src/nnn.c#L3410-L3416
@0xACE wrote:

Can someone explain why we are clearing runfile and rundir? What is the purpose of clearing them?

@jarun wrote:

They hold the name of the file to be passed as argument to plugin and the path in which the plugin has to be run respectively.

We clear them so next time the flow doesn't go and run the plugin.

CONTINUATION OF THREAD (my response)

Hmm, I'm trying to understand the code, but the variables are being used left and right and it's very confusing to understand what the true purpose is for them.

I encountered an issue with:
inside browse() under SEL_PLUGKEY:
https://github.com/jarun/nnn/blob/3c28d1f1d7fac7c4d8764cfce0749ec22dbbbaa3/src/nnn.c#L4969-L4974
https://github.com/jarun/nnn/commit/53a2e4c56058db01dbe7bcbaeb37a9352fc73959#diff-fc9d3b040de26a559b433d730b0c01baR4975, where runfile[0] = '\0' is actually clearing dents[cur].name

And not only that, but on the commit I linked, path gets overwritten to plugindir. What is the purpose of path?

I'm very confused...

And is this: inside browse() under SEL_PLUGIN:https://github.com/jarun/nnn/blob/3c28d1f1d7fac7c4d8764cfce0749ec22dbbbaa3/src/nnn.c#L4979-L4987 functioning correctly?

EDIT: included direct previews of the code so you don't have to click each link.

[jarun]

I see the issues now. This has been introduced by my hasty incorporation of https://github.com/jarun/nnn/commit/53a2e4c56058db01dbe7bcbaeb37a9352fc73959. I will take a look and fix these.

@annagrram the next time you think you wouldn't be able to follow up on a piece of code for sometime please let us know in advance. Fixing these things are terribly painful and I would have to be much more careful when merging stuff next time.

[0xACE]

I see the issues now. This has been introduced by my hasty incorporation of 53a2e4c. I will take a look and fix these.

Thank you my friend. But remember you are not alone in this. And don't stress about it. I started this to mostly get an understanding of browse();

And not only that, but on the commit I linked, path gets overwritten to plugindir. What is the purpose of path?

I'm still curious about this one

[jarun]

There are 2 cases;

• user navigates to the plugin dir and chooses the plugin. In that case path is automatically the pugin dir
• the other case where user runs the plugin directly. this is where the code is goofed up. we could have done without trying to use the same function in both the cases and that's what I am trying to fix now.

[jarun]

OK, having the pipe code makes it even more complex now because if I want to use a separate API I'll have to duplicate a lot of code.

[0xACE]

There are 2 cases;

• user navigates to the plugin dir and chooses the plugin. In that case path is automatically the pugin dir

Hmm, I guess I understand this. I interpret this as in browse();: path should always represent the current directory you are browsing.

• the other case where user runs the plugin directly. this is where the code is goofed up. we could have done without trying to use the same function in both the cases and that's what I am trying to fix now.

OK, having the pipe code makes it even more complex now because if I want to use a separate API I'll have to duplicate a lot of code.

Yeah, I wrestled with it a bit, I ended up adding a bool argument to run_selected_plugin() input parameters to get some features I was missing.

But another limitation I see is the limited/strict usage of variables in browse(); that makes you overwrite variables... Maybe I'm just not familiar with browse();

(My presence may be flakey this weekend)

[annagrram]

I see the issues now. This has been introduced by my hasty incorporation of 53a2e4c. I will take a look and fix these.

@annagrram the next time you think you wouldn't be able to follow up on a piece of code for sometime please let us know in advance. Fixing these things are terribly painful and I would have to be much more careful when merging stuff next time.

Yeah. Sorry! It wasn't planned.
You can also check out my approach to fixing this issue if you want to incorporate it.

[jarun]

@0xACE can you please test the patch thoroughly?

[0xACE]

@0xACE can you please test the patch thoroughly?

Yeah, but testing of this will take a lot of time, because it's a ongoing problem for me... But I'll give you a early indication of it ASAP (as in ill try it out now, and a thorough review will be a test of time...)

[jarun]

Yeah, but testing of this will take a lot of time

That's OK.

[0xACE]

Yeah, but testing of this will take a lot of time

That's OK.

I tried it out and it seems to work. I'll let you know if I find any problems with it...

[jarun]

Sure!